-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Skip removal of offerings if in use during domain removal #11780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11780 +/- ##
============================================
+ Coverage 16.19% 16.30% +0.11%
- Complexity 13306 13427 +121
============================================
Files 5657 5657
Lines 498467 503719 +5252
Branches 60491 62373 +1882
============================================
+ Hits 80702 82117 +1415
- Misses 408783 412537 +3754
- Partials 8982 9065 +83
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shwstppr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sudo87 idea seems correct, implementation not
|
@blueorangutan package |
|
@sudo87 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15304 |
|
@blueorangutan package |
|
@sudo87 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
Outdated
Show resolved
Hide resolved
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15628 |
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm, some overly complexity remains in the removeXxxOffering methods in DomainManagerImpl that might not be needed. whitebox testing advised.
|
@blueorangutan package |
|
@rosi-shapeblue a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 15989 |
|
@blueorangutan package |
|
Thank you, @RosiKyu, for taking the time to review and for validating the functionality. On Black UI rendering issue, I will take a look at the code changes. Although that seems to be related to UI packaging issue. |
|
@blueorangutan package |
|
@RosiKyu a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16048 |
|
@sudo87 , @DaanHoogland - the rendering issue does not appear to be caused by the PR code. Rebuilding the packages with the PR code resolved it, so I believe this can be safely merged.
|
RosiKyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I have tested the PR and the core functionality is working as expected (below are the Test Restuls). All test cases for protecting offerings from deletion during domain removal have passed.
Test Results
| Test Case | Result |
|---|---|
| TC1: Prevent restricting service offering when VMs exist outside domain | PASSED |
| TC2: Allow restricting offering after VM destroyed | PASSED |
| TC3: Skip service offering removal during domain deletion when in use | PASSED |
| TC4: Disk Offering protection | PASSED |
| TC5: Network Offering protection | PASSED |
| TC6: VPC Offering protection | PASSED |
| TC7: Clean deletion (offering deleted when not in use) | PASSED |
Functionality Verified:
- Service offerings protected from deletion when VMs are using them
- Disk offerings protected from deletion when volumes are using them
- Network offerings protected from deletion when networks are using them
- VPC offerings protected from deletion when VPCs are using them
- Offerings correctly deleted when no resources are using them
- Domain restriction blocked when resources exist outside target domain
Detailed Test Results
Test Case 1 - Prevent restricting service offering when VMs exist outside target domain
Expected Result: Error message is triggered; Update should be blocked.
Actual Result: Error (530): "There are Instances associated to this service offering outside of the specified domains." - Update was blocked.
Status: PASSED
Test Case 2 - Allow Restricting Offering After VM is Destroyed
Expected Result: The update should succeed (no error).
Actual Result: Update is successfull
Test Case 3 - Skip Offering Removal During Domain Deletion
Expected Result: Domain deleted successfully, but should NOT be deleted because VMs were using it
Actual Result: Domain deleted. Offering test-offering-pr11780 still exists in Compute Offerings.
Status: PASSED
Test Case 4 - Disk Offering protection during domain deletion
Preconditions
- Disk offering
test-disk-offering-2created (public) - Volume
test-volume-2created using this offering (in ROOT) - Offering restricted to
test-domain-3
Steps: Delete test-domain-3 with cleanup
Expected Result Disk offering should NOT be deleted because a volume is using it
Actual Result Disk offering test-disk-offering-2 still exists after domain deletion
Status: PASSED
Test Case 5 - Network Offering protection during domain deletion
Preconditions
- Network offering
test-network-offering-pr11780created (public) - Network
test-network-pr11780created using this offering (in ROOT) - Offering restricted to
test-domain-4
Steps: Delete test-domain-4 with cleanup
Expected Result Network offering should NOT be deleted because a network is using it
Actual Result Network offering test-network-offering-pr11780 still exists after domain deletion
Status PASSED
Test Case 6 - VPC Offering protection during domain deletion
Preconditions:
- VPC offering
test-vpc-offering-pr11780created (public) - VPC
test-vpc-pr11780created using this offering (in ROOT) - Offering restricted to
test-domain-5
Steps: Delete test-domain-5 with cleanup
Expected Result: VPC offering should NOT be deleted because a VPC is using it
Actual Result VPC offeringtest-vpc-offering-pr11780still exists after domain deletion
Status: PASSED
Test Case 7 - Clean deletion (offering deleted when not in use)
**Preconditions: **
- Compute offering
test-delete-offeringcreated (public) - NO VMs created using this offering
- Offering restricted to
test-domain-6
Steps:
- Delete
test-domain-6with cleanup - Check if offering still exists
Expected Result: Offering SHOULD be deleted because no resources are using it
Actual Result: Offering test-delete-offering was deleted (no longer in list)
Status: PASSED
|
@shwstppr can you have another look? |
shwstppr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16265 |
|
@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-15125)
|

Description
This PR fixes #11502
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?