Copilot commented on code in PR #12901:
URL: https://github.com/apache/cloudstack/pull/12901#discussion_r2999713973
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -1000,6 +1000,12 @@ protected boolean cleanupAccount(AccountVO account, long
callerUserId, Account c
}
for (UserVmVO vm : vms) {
+ if (vm.isDeleteProtection()) {
+ logger.warn("Instance [id = {}, name = {}] has delete
protection enabled and cannot be deleted.",
+ vm.getUuid(), vm.getName());
+ continue;
+ }
Review 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).
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -1000,6 +1000,12 @@ protected boolean cleanupAccount(AccountVO account, long
callerUserId, Account c
}
for (UserVmVO vm : vms) {
+ if (vm.isDeleteProtection()) {
+ logger.warn("Instance [id = {}, name = {}] has delete
protection enabled and cannot be deleted.",
+ vm.getUuid(), vm.getName());
Review Comment:
When a VM has delete protection enabled, this loop now `continue`s 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.
```suggestion
vm.getUuid(), vm.getName());
accountCleanupNeeded = true;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
