Copilot commented on code in PR #12901:
URL: https://github.com/apache/cloudstack/pull/12901#discussion_r3028252494
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -2138,6 +2139,23 @@ protected boolean isDeleteNeeded(AccountVO account, long
accountId, Account call
return true;
}
+ private void validateNoDeleteProtectedVmsForAccount(Account account) {
+ long accountId = account.getId();
+ List<VMInstanceVO> deleteProtectedVms =
_vmDao.listDeleteProtectedVmsByAccountId(accountId);
+ if (deleteProtectedVms.isEmpty()) {
Review Comment:
`_vmDao.listDeleteProtectedVmsByAccountId` is invoked here but the return
value is assumed to be non-null; with Mockito mocks (e.g.,
`AccountManagerImplTest.deleteUserAccount*`) the default return is `null`,
which will cause a NullPointerException on `.isEmpty()`. Consider making this
check null-safe (e.g., `CollectionUtils.isEmpty(...)`) and/or updating the
existing unit tests to stub `listDeleteProtectedVmsByAccountId` to return an
empty list by default.
```suggestion
if (CollectionUtils.isEmpty(deleteProtectedVms)) {
```
##########
server/src/main/java/com/cloud/user/DomainManagerImpl.java:
##########
@@ -633,6 +632,12 @@ protected boolean cleanupDomain(Long domainId, Long
ownerId) throws ConcurrentOp
sc1.addAnd("path", SearchCriteria.Op.LIKE, "%" + "replace(" +
domainHandle.getPath() + ", '%', '[%]')" + "%");
List<DomainVO> domainsToBeInactivated = _domainDao.search(sc1,
null);
+ // Validate that no Instance in this domain or its subdomains has
delete protection
+ validateNoDeleteProtectedVmsForDomain(domainHandle,
domainsToBeInactivated);
+
+ domainHandle.setState(Domain.State.Inactive);
+ _domainDao.update(domainId, domainHandle);
+
Review Comment:
The PR description says the cleanup should "skip Instances having
deleteProtection enabled", but this change throws an exception and prevents
domain deletion entirely when any delete-protected VM exists. Please
clarify/update the PR description (or adjust behavior) so intent and
implementation match.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -2138,6 +2139,23 @@ protected boolean isDeleteNeeded(AccountVO account, long
accountId, Account call
return true;
}
+ private void validateNoDeleteProtectedVmsForAccount(Account account) {
+ long accountId = account.getId();
+ List<VMInstanceVO> deleteProtectedVms =
_vmDao.listDeleteProtectedVmsByAccountId(accountId);
+ if (deleteProtectedVms.isEmpty()) {
+ return;
+ }
+
+ if (logger.isDebugEnabled()) {
+ List<String> vmUuids =
deleteProtectedVms.stream().map(VMInstanceVO::getUuid).collect(Collectors.toList());
+ logger.debug("Cannot delete Account {}, delete protection enabled
for Instances: {}", account, vmUuids);
Review Comment:
This validation currently fetches *all* delete-protected VMs for the account
and then builds a UUID list for debug logging. To avoid potentially large
queries/allocations, consider changing the DAO call to an existence check or
applying a `Filter` limit (e.g., 1 row) and only logging a bounded number of
UUIDs.
```suggestion
final int maxLoggedVms = 10;
List<String> vmUuids = deleteProtectedVms.stream()
.limit(maxLoggedVms)
.map(VMInstanceVO::getUuid)
.collect(Collectors.toList());
boolean hasMore = deleteProtectedVms.size() > maxLoggedVms;
if (hasMore) {
logger.debug("Cannot delete Account {}, delete protection
enabled for at least {} Instances (showing first {}): {}",
account, deleteProtectedVms.size(), vmUuids.size(),
vmUuids);
} else {
logger.debug("Cannot delete Account {}, delete protection
enabled for Instances: {}", account, vmUuids);
}
```
##########
server/src/main/java/com/cloud/user/DomainManagerImpl.java:
##########
@@ -724,6 +729,25 @@ protected boolean cleanupDomain(Long domainId, Long
ownerId) throws ConcurrentOp
return success && deleteDomainSuccess;
}
+ private void validateNoDeleteProtectedVmsForDomain(Domain domainHandle,
List<DomainVO> subDomains) {
+ List<Long> allDomainIds =
subDomains.stream().map(Domain::getId).collect(Collectors.toList());
+ allDomainIds.add(domainHandle.getId());
+
+ List<VMInstanceVO> deleteProtectedVms =
vmInstanceDao.listDeleteProtectedVmsByDomainIds(allDomainIds);
+ if (deleteProtectedVms.isEmpty()) {
+ return;
+ }
+
+ if (logger.isDebugEnabled()) {
+ List<String> vmUuids =
deleteProtectedVms.stream().map(VMInstanceVO::getUuid).collect(Collectors.toList());
+ logger.debug("Cannot delete Domain {}, it has delete protection
enabled for Instances: {}", domainHandle, vmUuids);
Review Comment:
Similar to account deletion, this loads the full list of delete-protected
VMs (and potentially many UUIDs) just to decide whether deletion is allowed.
Consider optimizing to an existence query / limited fetch and bounding UUID
logging to reduce DB load and memory usage for large domains.
```suggestion
final int maxLoggedUuids = 10;
List<String> vmUuids = deleteProtectedVms.stream()
.limit(maxLoggedUuids)
.map(VMInstanceVO::getUuid)
.collect(Collectors.toList());
logger.debug("Cannot delete Domain {}, it has delete protection
enabled for {} Instances. "
+ "Example Instance UUIDs (up to {}): {}",
domainHandle, deleteProtectedVms.size(), maxLoggedUuids,
vmUuids);
```
##########
server/src/main/java/com/cloud/user/DomainManagerImpl.java:
##########
@@ -633,6 +632,12 @@ protected boolean cleanupDomain(Long domainId, Long
ownerId) throws ConcurrentOp
sc1.addAnd("path", SearchCriteria.Op.LIKE, "%" + "replace(" +
domainHandle.getPath() + ", '%', '[%]')" + "%");
List<DomainVO> domainsToBeInactivated = _domainDao.search(sc1,
null);
+ // Validate that no Instance in this domain or its subdomains has
delete protection
+ validateNoDeleteProtectedVmsForDomain(domainHandle,
domainsToBeInactivated);
+
Review Comment:
New behavior (blocking domain cleanup/deletion when delete-protected VMs
exist) should be covered by a unit test. `DomainManagerImplTest` currently only
stubs the DAO to return an empty list; please add a test asserting
`deleteDomain(..., true)` throws `InvalidParameterValueException` when
`vmInstanceDao.listDeleteProtectedVmsByDomainIds(...)` returns a non-empty list.
##########
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java:
##########
@@ -1296,4 +1312,20 @@ public List<VMInstanceVO>
listByIdsIncludingRemoved(List<Long> ids) {
sc.setParameters("ids", ids.toArray());
return listIncludingRemovedBy(sc);
}
+
+ @Override
+ public List<VMInstanceVO> listDeleteProtectedVmsByAccountId(long
accountId) {
+ SearchCriteria<VMInstanceVO> sc =
DeleteProtectedVmSearchByAccount.create();
+ sc.setParameters(ApiConstants.ACCOUNT_ID, accountId);
+ sc.setParameters(ApiConstants.DELETE_PROTECTION, true);
+ return listBy(sc);
+ }
+
+ @Override
+ public List<VMInstanceVO> listDeleteProtectedVmsByDomainIds(List<Long>
domainIds) {
+ SearchCriteria<VMInstanceVO> sc =
DeleteProtectedVmSearchByDomainIds.create();
+ sc.setParameters(ApiConstants.DOMAIN_IDS, domainIds);
+ sc.setParameters(ApiConstants.DELETE_PROTECTION, true);
+ return listBy(sc);
Review Comment:
`Op.IN` parameters in this codebase are typically set using an array (e.g.,
`ids.toArray()`). Passing a `List` directly
(`sc.setParameters(ApiConstants.DOMAIN_IDS, domainIds)`) may not bind correctly
and can break the query at runtime. Convert `domainIds` to an array when
setting the parameter (and consider handling empty lists defensively).
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -2097,6 +2097,7 @@ public boolean deleteUserAccount(long accountId) {
return true;
}
+ validateNoDeleteProtectedVmsForAccount(account);
checkIfAccountManagesProjects(accountId);
verifyCallerPrivilegeForUserOrAccountOperations(account);
Review Comment:
New behavior (blocking account deletion when delete-protected VMs exist)
should be covered by unit tests. There are existing tests for
`deleteUserAccount` in
`server/src/test/java/com/cloud/user/AccountManagerImplTest.java`; please add a
case asserting an `InvalidParameterValueException` is thrown when
`_vmDao.listDeleteProtectedVmsByAccountId` returns a non-empty list.
--
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]
