Repository navigation
extensions: ignore removed offerings when removing a network service - #14370
Open
DaanHoogland wants to merge 1 commit into
Open
DaanHoogland wants to merge 1 commit into
DaanHoogland wants to merge 1 commit into
Conversation
DaanHoogland
requested review from
weizhouapache
and
a balanced review from Copilot
October 8, 2026 16:28
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14370 +/- ##
============================================
- Coverage 19.91% 19.90% -0.01%
- Complexity 20198 20201 +3
============================================
Files 6373 6373
Lines 577234 577236 +2
Branches 70696 70696
============================================
- Hits 114945 114925 -20
- Misses 449724 449755 +31
+ Partials 12565 12556 -9
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:
|
Contributor
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.
Comment on lines
+1318
to
+1322
| boolean usedByNetworkOffering = networkOfferingServiceMapDao | ||
| .listOfferingIdsByServiceAndProvider(removedService, extension.getName()).stream() | ||
| .anyMatch(offeringId -> networkOfferingDao.findById(offeringId) != null); | ||
| boolean usedByVpcOffering = vpcOfferingServiceMapDao | ||
| .listOfferingIdsByServiceAndProvider(removedService, extension.getName()).stream() |
Comment on lines
+1318
to
+1323
| 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); |
| extensionsManager.updateNetworkExtensionServicesOnPhysicalNetworks(ext, | ||
| Set.of(Network.Service.CustomAction), Set.of(Network.Service.UserData)); | ||
|
|
||
| ArgumentCaptor<List<Network.Service>> captor = ArgumentCaptor.forClass(List.class); |
Copilot stopped reviewing on behalf of
DaanHoogland due to an error
October 8, 2026 17:20
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.82% |
| Branch coverage | 19.04% |
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
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?