feat: Implement address space cleanup on workspace service uninstall - #5076
feat: Implement address space cleanup on workspace service uninstall#5076James Chapman (JC-wk) wants to merge 6 commits into
Conversation
Unit Test Results779 tests 779 ✅ 11s ⏱️ Results for commit 806d6ae. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup persistence and upgrade dispatch are not atomic or serialized, allowing lost or stale updates that leave Cosmos and Azure inconsistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Releases workspace-service address spaces after uninstall and prevents active allocations from being reused.
Changes:
- Removes the service CIDR and dispatches a workspace upgrade.
- Includes active service CIDRs during allocation validation.
- Adds cleanup/allocation tests, documentation, changelog, and version updates.
File summaries
| File | Description |
|---|---|
CHANGELOG.md |
Records the bug fix. |
azuretre-address-space-cleanup-plan.md |
Documents designs, risks, and recommendations. |
api_app/tests_ma/test_service_bus/test_deployment_status_update.py |
Tests cleanup and upgrade dispatch. |
api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py |
Tests service CIDR allocation checks. |
api_app/service_bus/deployment_status_updater.py |
Performs post-uninstall cleanup. |
api_app/db/repositories/workspaces.py |
Includes active service CIDRs when allocating. |
api_app/_version.py |
Bumps the API patch version. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…n CIDR allocation
2744b5d to
602252c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup currently reuses a stale Cosmos ETag, and lock enforcement can permit conflicting mutations or permanently block creation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Expired-lock handling breaks workspace PATCH requests, and the unrenewed lease permits overlapping long-running operations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
api_app/_version.py:1
- The PR description says this component should be bumped from 0.26.9 to 0.26.10, but this line publishes 0.27.0 instead. Align the version with the stated release plan (or update the PR description if the minor bump is intentional) so downstream release automation and consumers receive the intended version.
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
…d keep lock active during in-progress cleanup
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup locking can dead-letter competing work, retain stale locks indefinitely, and overwrite concurrent workspace metadata.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
| workspace.etag = fresh.etag | ||
| workspace.properties = fresh.properties |
| except AddressSpaceCleanupBusyError: | ||
| return False |
| operations_repo = getattr(self, "operations_repo", None) | ||
| if operations_repo: | ||
| return await operations_repo.is_address_space_cleanup_active(operation_id) | ||
| return lock.get("expires_when", 0) > time.time() |
Resolves #4727
What is being addressed
Workspace services don't release the address spaces they use when they are deallocated
How is this addressed