Implements address space cleanup when a workspace service is uninstalled

[James Chapman (JC-wk)]

Resolves #4727

PR

• unit testing
• I have noted an edge case of the address being released during a failed uninstall and then allocated elsewhere. I need to move the deallocation process to run after the main step has succeeded so it runs after the terraform uninstall. I am testing doing this in the service_bus now and it is working ok.

What is being addressed

Currently address spaces are not cleaned up when a workspace service is uninstalled from a workspace, this could lead to ip range exhaustion.
This PR adds the functionality to delete the address_space used by a workspace-service on uninstall of the service. The address range is then freed from the workspace and can be reused.

How is this addressed

• Adds a step to the api to delete the address space when a workspace service is uninstalled
• Updates templates to conduct a workspace upgrade after uninstall
• I have not added any tests but happy to accept advice on how to implement these
• Updated documentation
• Updated CHANGELOG.md
• Increment template versions

[github-actions]

Unit Test Results

764 tests   764 ✅  10s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 2711e28.

♻️ This comment has been updated with latest results.

[James Chapman (JC-wk)]

I have been testing this for a few days, I am not sure if unit tests are needed and how best to write them if anyone wants to assist.

[Copilot]

Pull request overview

This PR addresses IP range exhaustion risk by ensuring workspace address spaces allocated by workspace services are freed on successful uninstall, and by triggering a workspace upgrade so downstream infra reflects the removal.

Changes:

• Add post-uninstall cleanup in the service bus deployment status handler to remove a workspace-service address_space from the parent workspace’s address_spaces.
• Update AzureML and Databricks workspace-service templates to run a workspace upgrade step after uninstall.
• Bump API + template versions and add a changelog entry.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
templates/workspace_services/databricks/template_schema.json	Adds a workspace upgrade step after uninstall (and JSON formatting changes).
templates/workspace_services/databricks/porter.yaml	Patch version bump.
templates/workspace_services/azureml/template_schema.json	Adds a workspace upgrade step after uninstall.
templates/workspace_services/azureml/porter.yaml	Patch version bump.
api_app/service_bus/deployment_status_updater.py	Implements address space cleanup after successful uninstall main step.
api_app/_version.py	API patch version bump.
CHANGELOG.md	Adds an Unreleased entry describing the change.

[Copilot]

🟢 Ready to approve

Core behavior change (address space cleanup + template trailing upgrade) is implemented and covered by focused unit tests, with only a minor tracing-attribute typing improvement suggested.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

api_app/service_bus/deployment_status_updater.py:89

• message.operationId is a UUID4 (UUID) and is being passed directly to OpenTelemetry Span.set_attribute. OTEL span attributes are expected to be primitive types (e.g., str/int/bool); non-primitive values like UUIDs may be dropped (or logged as invalid), so the trace can lose the operation id. Consider stringifying UUIDs (and any enums) before setting attributes, and update the unit test accordingly.

                message = parse_obj_as(DeploymentStatusUpdateMessage, json.loads(str(msg)))

                current_span.set_attribute("step_id", message.stepId)
                current_span.set_attribute("operation_id", message.operationId)
                current_span.set_attribute("status", message.status)

api_app/tests_ma/test_service_bus/test_deployment_status_update.py:179

• This test currently asserts operation_id is set as a uuid.UUID instance. OpenTelemetry span attributes are expected to be primitives; if process_message is updated to set operation_id as a string (recommended), this assertion should be updated to match.

    mock_span.set_attribute.assert_any_call("step_id", test_sb_message["stepId"])
    mock_span.set_attribute.assert_any_call("operation_id", uuid.UUID(test_sb_message["operationId"]))
    mock_span.set_attribute.assert_any_call("status", test_sb_message["status"])

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

The address space cleanup is implemented in the appropriate post-uninstall path, templates are updated to reconcile workspace state, and the new behavior is covered by focused unit tests (including ETag-retry scenarios).

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

Cleanup failures are swallowed, allowing uninstall operations to succeed while address spaces remain allocated.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

api_app/service_bus/deployment_status_updater.py:55

• This substantially changes the receiver/client lifecycle, idle timeout, and complete-versus-abandon flow, but the added receive_messages test only exercises failure before a receiver is created. Add an async receiver test covering a processed message and asserting both settlement branches, plus the idle timeout path, so this long-running queue loop is protected from regressions.

                        async with ServiceBusClient(fully_qualified_namespace=config.SERVICE_BUS_FULLY_QUALIFIED_NAMESPACE, credential=credential) as service_bus_client:
                            logger.debug("Creating Deployment Status receiver session")
                            async with service_bus_client.get_queue_receiver(queue_name=config.SERVICE_BUS_DEPLOYMENT_STATUS_UPDATE_QUEUE, max_wait_time=config.SERVICE_BUS_MAX_WAIT_TIME, session_id=NEXT_AVAILABLE_SESSION) as receiver:

• Files reviewed: 13/13 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

api_app/service_bus/deployment_status_updater.py:220

• Creating a new WorkspaceRepository on every uninstall completion can add unnecessary connection/setup overhead in a hot path (and makes lifecycle management harder). Consider creating/storing self.workspace_repo in init_repos() (similar to the other repos) and reusing it within _free_workspace_address_space.

            workspace_repo = await WorkspaceRepository.create()

api_app/service_bus/deployment_status_updater.py:159

• The inline comment states this cleanup MUST happen before preparing/enqueuing any next step, but _free_workspace_address_space swallows all exceptions and only logs. If cleanup fails (e.g., persistent ETag conflicts), the code will still enqueue the next step with stale workspace address_spaces, which undermines the intended behavior. Consider making cleanup failure block progression (e.g., return False from update_status_in_database so the message is abandoned and retried) or otherwise prevent enqueuing subsequent steps until the workspace patch succeeds.

            if step_to_update.templateStepId == "main" and step_to_update.is_success() and operation.action == RequestAction.UnInstall:
                await self._free_workspace_address_space(resource_to_persist, operation)

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

api_app/service_bus/deployment_status_updater.py:168

• The address is removed only after the workspace upgrade has completed, but that upgrade payload was already built and sent from the workspace's old properties at lines 179-200. Consequently Terraform keeps the address in the Azure VNet; the subsequent patch_workspace(..., False) only changes Cosmos DB, allowing the allocator to reuse a prefix that Azure still holds. The address list must be patched before constructing the trailing upgrade payload while remaining reserved from new allocations until that upgrade succeeds (for example, via pending-release state).

                if not await self._free_workspace_address_space(operation):

api_app/service_bus/deployment_status_updater.py:167

• This condition matches every successful workspace-upgrade step in an uninstall, including steps before the service's main uninstall. The OHDSI pipeline has such a workspace upgrade before main (templates/workspace_services/ohdsi/template_schema.json:355-372), so its address is released while the service still exists—the exact allocation race this ordering is intended to prevent. Gate cleanup on the root service's main step having succeeded and the current step occurring after it.

            if (step_to_update.resourceType in (ResourceType.Workspace, ResourceType.Workspace.value)
                    and step_to_update.resourceAction == RequestAction.Upgrade
                    and step_to_update.is_success()
                    and operation.action == RequestAction.UnInstall):

api_app/tests_ma/test_service_bus/test_deployment_status_update.py:749

• The test name says cleanup occurs before the next step is enqueued, but its assertions explicitly require send_deployment_message first and patch_workspace only after the upgrade result. Rename it to describe the behavior actually under test.

async def test_workspace_service_uninstall_frees_address_space_before_enqueuing_next_step(

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

api_app/service_bus/deployment_status_updater.py:169

• When this cleanup fails, the last workspace-upgrade step and overall uninstall operation have already been persisted as successful at lines 133–137. Returning False only abandons the message; after repeated delivery and eventual dead-lettering, the operation still appears deleted while the address remains reserved. Defer persisting the final success until cleanup succeeds, or persist an explicit failure state before returning.

                if not await self._free_workspace_address_space(operation):
                    return False