Repository navigation
extensions: ignore removed offerings when removing a network service - #14370
DaanHoogland wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14370 +/- ##
============================================
+ Coverage 3.86% 20.73% +16.86%
- Complexity 0 20786 +20786
============================================
Files 494 6049 +5555
Lines 42875 547785 +504910
Branches 8136 73176 +65040
============================================
+ Hits 1657 113573 +111916
- Misses 40986 420893 +379907
- Partials 232 13319 +13087
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
CollectionUtils.isNotEmpty(...). If either… · NewList.classcaptor, which typically triggers unchecked warnings and can hide type… · New
What changed in this PR
Fixes updateExtension incorrectly blocking removal of a network service from a NetworkOrchestrator extension when only removed network/VPC offerings still reference the service via persisted service-map rows.
Changes:
- Update in-use checks to ignore deleted (removed) offerings by resolving offering IDs through DAOs.
- Add DAO injections for
NetworkOfferingDaoandVpcOfferingDao. - Extend unit tests to cover “service-map rows exist but offerings are removed” behavior.
| File | Description |
|---|---|
| framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java | Adjusts “service in use” logic to ignore removed offerings by checking offering existence via DAOs. |
| framework/extensions/src/test/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImplTest.java | Updates existing tests and adds a new test ensuring removed offerings don’t block service removal. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| boolean usedByNetworkOffering = networkOfferingServiceMapDao | ||
| .listOfferingIdsByServiceAndProvider(removedService, extension.getName()).stream() | ||
| .anyMatch(offeringId -> networkOfferingDao.findById(offeringId) != null); | ||
| boolean usedByVpcOffering = vpcOfferingServiceMapDao | ||
| .listOfferingIdsByServiceAndProvider(removedService, extension.getName()).stream() | ||
| .anyMatch(offeringId -> vpcOfferingDao.findById(offeringId) != null); |
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.64% |
| Branch coverage | 20.79% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
69183a9 to
1acbd8d
Compare
boring-cyborg
Bot
added
component:integration-test
Python
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 19971 |
|
rebuilding debian package |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17533)
|
11ca854 to
3d8d704
Compare
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 20024 |
Description
This PR... fixes an issue reported by claude code during extension development
It fixes
updateExtensionrefusing to remove a service from a NetworkOrchestrator extension'snetwork.servicesdetail after the only offering using it has been deleted.deleteNetworkOffering(and VPC offering deletion) only marks the offering as removed; its rows inntwk_offering_service_map/vpc_offering_service_mapremain. The in-use check inExtensionsManagerImpl.updateNetworkExtensionServicesOnPhysicalNetworkslooked only at those map rows, so it kept failing with:The check now resolves each mapped offering ID through
NetworkOfferingDao/VpcOfferingDaoand only counts offerings that are not removed.updateExtension refused to drop a service from a NetworkOrchestrator
extension's network.services detail if any ntwk_offering_service_map or
vpc_offering_service_map row referenced it for the extension provider.
Deleting an offering only sets its removed date and keeps those map rows,
so a service stayed "in use" forever once any offering had used it.
Only count offerings that still exist (findById excludes removed rows)
when checking whether a service can be removed.
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?