Skip deleting instance with delete protection during account cleanup#12901
Skip deleting instance with delete protection during account cleanup#12901sudo87 wants to merge 1 commit intoapache:4.22from
Conversation
c3ab6eb to
6569709
Compare
There was a problem hiding this comment.
Pull request overview
Fixes account/domain cleanup behavior to honor VM “delete protection” by skipping destruction/expunge of protected instances during account cleanup (addresses issue #12895).
Changes:
- Skip destroy/expunge for instances that have
deleteProtectionenabled duringcleanupAccount.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (UserVmVO vm : vms) { | ||
| if (vm.isDeleteProtection()) { | ||
| logger.warn("Instance [id = {}, name = {}] has delete protection enabled and cannot be deleted.", | ||
| vm.getUuid(), vm.getName()); |
There was a problem hiding this comment.
When a VM has delete protection enabled, this loop now continues without destroying/expunging it, but it also leaves accountCleanupNeeded unchanged. That means the cleanup can still be marked as complete (needsCleanup=false) even though protected instances remain under a removed account, potentially leaving orphaned VMs/resources with no follow-up cleanup attempts (e.g., in basic/shared-network setups where later network cleanup may still succeed).
Consider treating this as an incomplete cleanup: set accountCleanupNeeded = true when delete-protected VMs are found and/or fail the account/domain deletion with a clear error so operators can remove protection first, rather than silently completing cleanup.
| vm.getUuid(), vm.getName()); | |
| vm.getUuid(), vm.getName()); | |
| accountCleanupNeeded = true; |
| if (vm.isDeleteProtection()) { | ||
| logger.warn("Instance [id = {}, name = {}] has delete protection enabled and cannot be deleted.", | ||
| vm.getUuid(), vm.getName()); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This change introduces new behavior for account cleanup when deleteProtection is enabled, but there is no unit test covering it. Please add a test (e.g., in server/src/test/java/com/cloud/user/AccountManagerImplTest) that provides a UserVmVO with isDeleteProtection()==true and asserts the VM is not expunged (and that the account is handled as expected, e.g., marked for cleanup or the delete operation fails).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12901 +/- ##
============================================
- Coverage 17.61% 17.60% -0.01%
+ Complexity 15676 15670 -6
============================================
Files 5917 5917
Lines 531537 531541 +4
Branches 64985 64986 +1
============================================
- Hits 93610 93567 -43
- Misses 427369 427421 +52
+ Partials 10558 10553 -5
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:
|
|
@sudo87 is it good to check any instances with delete protection enabled before proceeding with account deletion, and fail the deletion with relevant msg, eg. "cannot delete account, there are some instances with delete protection enabled"? the user/operator can disable the delete protection and retry the account deletion. Check if there is any provision to list delete protection enabled instances for the account. |
agree |
|
@sureshanaparti @weizhouapache Does this mean that account deletion should not be allowed if any VM/instance has delete protection enabled? If so, we can add an early validation check and return with an error. |
yes @sudo87 |
Description
This PR address #12895 by skipping Instances having deleteProtection enabled.
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?