address copilot review

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

Author: shwstppr

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

@@ -3885,6 +3885,8 @@ public boolean deleteVmGroup(long groupId) {
             sc.addAnd("instanceId", SearchCriteria.Op.EQ, groupMap.getInstanceId());
             _groupVMMapDao.expunge(sc);
         }
+        // don't leave a stale instance boot group member pointing at a group that no longer exists
+        instanceBootGroupMembershipGuard.removeInstanceGroupBootGroupMembershipIfPresent(groupId);
 
         if (_vmGroupDao.remove(groupId)) {
             return true;

server/src/main/java/org/apache/cloudstack/vm/bootgroup/InstanceBootGroupApiServiceImpl.java

@@ -261,6 +261,11 @@ public ListResponse<InstanceBootGroupResponse> listInstanceBootGroups(ListInstan
         final Account caller = ctx.getCallingAccount();
         final Long id = cmd.getId();
         final String keyword = cmd.getKeyword();
+        final Long virtualMachineId = cmd.getVirtualMachineId();
+        final Long instanceGroupId = cmd.getInstanceGroupId();
+        if (virtualMachineId != null && instanceGroupId != null) {
+            throw new InvalidParameterValueException("Only one of virtualmachineid or instancegroupid may be specified");
+        }
 
         List<InstanceBootGroupResponse> responsesList = new ArrayList<>();
         List<Long> permittedAccounts = new ArrayList<>();
@@ -272,12 +277,30 @@ public ListResponse<InstanceBootGroupResponse> listInstanceBootGroups(ListInstan
         Boolean isRecursive = domainIdRecursiveListProject.second();
         Project.ListProjectResourcesCriteria listProjectResourcesCriteria = domainIdRecursiveListProject.third();
 
+        // A VM or Instance Group belongs to at most one boot group (unique per member), so this filter
+        // resolves to either exactly one boot group id, or no results at all.
+        Long memberBootGroupId = null;
+        if (virtualMachineId != null || instanceGroupId != null) {
+            InstanceBootGroupMember.MemberType memberType = virtualMachineId != null
+                    ? InstanceBootGroupMember.MemberType.VirtualMachine
+                    : InstanceBootGroupMember.MemberType.InstanceGroup;
+            long memberId = virtualMachineId != null ? virtualMachineId : instanceGroupId;
+            InstanceBootGroupMemberVO member = instanceBootGroupMemberDao.findByMember(memberType, memberId);
+            if (member == null) {
+                ListResponse<InstanceBootGroupResponse> emptyResponse = new ListResponse<>();
+                emptyResponse.setResponses(new ArrayList<>(), 0);
+                return emptyResponse;
+            }
+            memberBootGroupId = member.getBootGroupId();
+        }
+
         Filter searchFilter = new Filter(InstanceBootGroupJoinVO.class, "id", true, cmd.getStartIndex(),
                 cmd.getPageSizeVal());
         SearchBuilder<InstanceBootGroupJoinVO> sb = instanceBootGroupJoinDao.createSearchBuilder();
         accountManager.buildACLSearchBuilder(sb, domainId, isRecursive, permittedAccounts,
                 listProjectResourcesCriteria);
         sb.and("id", sb.entity().getId(), SearchCriteria.Op.EQ);
+        sb.and("memberBootGroupId", sb.entity().getId(), SearchCriteria.Op.EQ);
         sb.and("name", sb.entity().getName(), SearchCriteria.Op.EQ);
         sb.and("keyword", sb.entity().getName(), SearchCriteria.Op.LIKE);
         SearchCriteria<InstanceBootGroupJoinVO> sc = sb.create();
@@ -289,6 +312,9 @@ public ListResponse<InstanceBootGroupResponse> listInstanceBootGroups(ListInstan
         if (id != null) {
             sc.setParameters("id", id);
         }
+        if (memberBootGroupId != null) {
+            sc.setParameters("memberBootGroupId", memberBootGroupId);
+        }
         Pair<List<InstanceBootGroupJoinVO>, Integer> bootGroupsAndCount = instanceBootGroupJoinDao.searchAndCount(sc, searchFilter);
         for (InstanceBootGroupJoinVO bootGroup : bootGroupsAndCount.first()) {
             InstanceBootGroupResponse response = createInstanceBootGroupResponse(bootGroup);

server/src/main/java/org/apache/cloudstack/vm/bootgroup/InstanceBootGroupManagerImpl.java

@@ -201,7 +201,7 @@ public void startInstanceBootGroup(InstanceBootGroupVO group) {
                         userVmService.startVirtualMachine(vm, null);
                     }
                     anchorInitialDelay(group, progressByVmId.get(vmId), vm, alreadyRunning);
-                });
+                }, true);
             } catch (CloudRuntimeException e) {
                 halt(group, "Failed to start a VM in tier " + tierOrder + ": " + e.getMessage());
                 throw e;
@@ -489,7 +489,7 @@ public void stopInstanceBootGroup(InstanceBootGroupVO group, boolean forced) {
                 if (vm != null && vm.getState() != com.cloud.vm.VirtualMachine.State.Stopped) {
                     userVmService.stopVirtualMachine(vmId, forced);
                 }
-            });
+            }, false);
         }
 
         logger.info("{} stop completed ({}ms)", group, System.currentTimeMillis() - groupStoppedAtMs);
@@ -503,12 +503,17 @@ public void rebootInstanceBootGroup(InstanceBootGroupVO group, boolean forced) {
     }
 
     /**
-     * Runs {@code action} for every VM in a tier concurrently and aborts on the first failure. Each
-     * thread gets a copied {@link CallContext} — without one, a VM lifecycle action routed through
-     * the job-queue path fails to submit its sub-job ("no lock found").
+     * Runs {@code action} for every VM in a tier concurrently. Each thread gets a copied
+     * {@link CallContext} — without one, a VM lifecycle action routed through the job-queue path
+     * fails to submit its sub-job ("no lock found").
+     *
+     * @param haltOnFailure when true (start), the first per-VM failure aborts immediately so the
+     *                      caller can halt the whole boot group; when false (stop, which continues
+     *                      through every tier regardless per its own documented contract), every
+     *                      VM's action is still attempted and failures are logged, not thrown.
      */
     private void runTierConcurrently(List<Long> vmIds, InstanceBootGroupVO group,
-                                     String actionName, VmAction action) {
+                                     String actionName, VmAction action, boolean haltOnFailure) {
         if (vmIds.isEmpty()) {
             return;
         }
@@ -517,7 +522,7 @@ private void runTierConcurrently(List<Long> vmIds, InstanceBootGroupVO group,
                 actionName, group, vmIds.size(), vmIds);
         long actionStartedAtMs = System.currentTimeMillis();
         CallContext callerContext = CallContext.current();
-        int threadCount = Math.min(vmIds.size(), ReadinessCheckConcurrency.value().intValue());
+        int threadCount = Math.max(1, Math.min(vmIds.size(), ReadinessCheckConcurrency.value().intValue()));
         ExecutorService executor = Executors.newFixedThreadPool(
                 threadCount, new NamedThreadFactory("InstanceBootGroup-" + actionName));
 
@@ -541,24 +546,34 @@ protected void runInContext() {
                 }));
             }
 
+            List<String> failures = new ArrayList<>();
             for (Future<?> future : futures) {
                 try {
                     future.get();
                 } catch (ExecutionException e) {
                     Throwable cause = e.getCause() != null ? e.getCause() : e;
-                    throw new CloudRuntimeException(
-                            String.format("Failed to %s a VM in boot group %s: %s",
-                                    actionName, group.getName(), cause.getMessage()),
-                            cause);
-
+                    String message = String.format("Failed to %s a VM in boot group %s: %s",
+                            actionName, group.getName(), cause.getMessage());
+                    if (haltOnFailure) {
+                        throw new CloudRuntimeException(message, cause);
+                    }
+                    logger.warn(message, cause);
+                    failures.add(message);
                 } catch (InterruptedException e) {
                     Thread.currentThread().interrupt();
-                    throw new CloudRuntimeException(
-                            String.format("Interrupted while waiting to %s VMs in boot group %s",
-                                    actionName, group.getName()),
-                            e);
+                    String message = String.format("Interrupted while waiting to %s VMs in boot group %s",
+                            actionName, group.getName());
+                    if (haltOnFailure) {
+                        throw new CloudRuntimeException(message, e);
+                    }
+                    logger.warn(message, e);
+                    failures.add(message);
                 }
             }
+            if (!failures.isEmpty()) {
+                logger.warn("'{}' action for a tier of {} completed with {} failure(s) out of {} VM(s)",
+                        actionName, group, failures.size(), vmIds.size());
+            }
         } finally {
             executor.shutdown();
         }

server/src/main/java/org/apache/cloudstack/vm/bootgroup/InstanceBootGroupMembershipGuard.java

@@ -135,4 +135,18 @@ public void validateInstanceGroupEligibleForBootGroupMembership(long instanceGro
             validateVmEligibleForGroupMembership(member.getInstanceId());
         }
     }
+
+    /**
+     * Removes the boot-group membership row for an Instance Group being deleted, if any, so the
+     * delete doesn't leave a stale member pointing at a group that no longer exists. Cascades
+     * silently rather than blocking the delete, since {@code UserVmManagerImpl.deleteVmGroup} is
+     * also invoked during account cleanup, where a hard failure here would be worse than the group
+     * simply dropping out of its boot group.
+     */
+    public void removeInstanceGroupBootGroupMembershipIfPresent(long instanceGroupId) {
+        InstanceBootGroupMemberVO member = instanceBootGroupMemberDao.findByMember(InstanceBootGroupMember.MemberType.InstanceGroup, instanceGroupId);
+        if (member != null) {
+            instanceBootGroupMemberDao.expunge(member.getId());
+        }
+    }
 }

server/src/main/java/org/apache/cloudstack/vm/bootgroup/readiness/InstanceBootGroupReadinessRuleManagerImpl.java

@@ -526,9 +526,15 @@ private void validateInstanceQuorumDetails(Map<String, String> details) {
         }
         try {
             if ("PERCENTAGE".equalsIgnoreCase(thresholdType)) {
-                Double.parseDouble(thresholdValue);
+                double percentage = Double.parseDouble(thresholdValue);
+                if (percentage < 0) {
+                    throw new InvalidParameterValueException(THRESHOLD_VALUE_KEY + " must not be negative: " + thresholdValue);
+                }
             } else {
-                Long.parseLong(thresholdValue);
+                long count = Long.parseLong(thresholdValue);
+                if (count < 0) {
+                    throw new InvalidParameterValueException(THRESHOLD_VALUE_KEY + " must not be negative: " + thresholdValue);
+                }
             }
         } catch (NumberFormatException e) {
             throw new InvalidParameterValueException("Invalid " + THRESHOLD_VALUE_KEY + ": " + thresholdValue);

server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

@@ -100,6 +100,8 @@
 import org.apache.cloudstack.storage.template.VnfTemplateManager;
 import org.apache.cloudstack.userdata.UserDataManager;
 import org.apache.cloudstack.vm.UnmanagedVMsManager;
+import org.apache.cloudstack.annotation.AnnotationService;
+import org.apache.cloudstack.annotation.dao.AnnotationDao;
 import org.apache.cloudstack.vm.bootgroup.InstanceBootGroupMembershipGuard;
 import org.apache.cloudstack.vm.lease.VMLeaseManager;
 import org.junit.After;
@@ -209,6 +211,8 @@
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.utils.exception.ExceptionProxyObject;
 import com.cloud.utils.fsm.NoTransitionException;
+import com.cloud.vm.dao.InstanceGroupDao;
+import com.cloud.vm.dao.InstanceGroupVMMapDao;
 import com.cloud.vm.dao.NicDao;
 import com.cloud.vm.dao.UserVmDao;
 import com.cloud.vm.dao.VMInstanceDetailsDao;
@@ -474,6 +478,15 @@ public class UserVmManagerImplTest {
     @Mock
     private InstanceBootGroupMembershipGuard instanceBootGroupMembershipGuard;
 
+    @Mock
+    private InstanceGroupDao _vmGroupDao;
+
+    @Mock
+    private InstanceGroupVMMapDao _groupVMMapDao;
+
+    @Mock
+    private AnnotationDao annotationDao;
+
     @Mock
     private UUIDManager uuidMgr;
 
@@ -3907,6 +3920,23 @@ public void testDestroyVmBlockedWhenPartOfBootGroup() {
         }
     }
 
+    @Test
+    public void testDeleteVmGroupCascadesBootGroupMembershipCleanup() {
+        long groupId = 55L;
+        InstanceGroupVO group = mock(InstanceGroupVO.class);
+        when(group.getUuid()).thenReturn("group-uuid");
+        when(_vmGroupDao.findById(groupId)).thenReturn(group);
+        when(_groupVMMapDao.listByGroupId(groupId)).thenReturn(new ArrayList<>());
+        when(_vmGroupDao.remove(groupId)).thenReturn(true);
+
+        boolean result = userVmManagerImpl.deleteVmGroup(groupId);
+
+        assertTrue(result);
+        Mockito.verify(instanceBootGroupMembershipGuard).removeInstanceGroupBootGroupMembershipIfPresent(groupId);
+        Mockito.verify(annotationDao).removeByEntityType(AnnotationService.EntityType.INSTANCE_GROUP.name(), "group-uuid");
+        Mockito.verify(_vmGroupDao).remove(groupId);
+    }
+
     @Test(expected = InvalidParameterValueException.class)
     public void testValidateLeasePropertiesInvalidDuration() {
         userVmManagerImpl.validateLeaseProperties(-2, VMLeaseManager.ExpiryAction.STOP);

server/src/test/java/org/apache/cloudstack/vm/bootgroup/InstanceBootGroupApiServiceImplTest.java

@@ -43,6 +43,7 @@
 import org.apache.cloudstack.api.command.user.bootgroup.DeleteInstanceBootGroupReadinessRuleCmd;
 import org.apache.cloudstack.api.command.user.bootgroup.ListInstanceBootGroupMembersCmd;
 import org.apache.cloudstack.api.command.user.bootgroup.ListInstanceBootGroupReadinessRulesCmd;
+import org.apache.cloudstack.api.command.user.bootgroup.ListInstanceBootGroupsCmd;
 import org.apache.cloudstack.api.command.user.bootgroup.RebootInstanceBootGroupCmd;
 import org.apache.cloudstack.api.command.user.bootgroup.RemoveInstanceBootGroupMemberCmd;
 import org.apache.cloudstack.api.command.user.bootgroup.StartInstanceBootGroupCmd;
@@ -51,9 +52,11 @@
 import org.apache.cloudstack.api.command.user.bootgroup.UpdateInstanceBootGroupMemberCmd;
 import org.apache.cloudstack.api.command.user.bootgroup.UpdateInstanceBootGroupReadinessRuleCmd;
 import org.apache.cloudstack.api.query.dao.InstanceBootGroupJoinDao;
+import org.apache.cloudstack.api.query.vo.InstanceBootGroupJoinVO;
 import org.apache.cloudstack.api.response.InstanceBootGroupMemberChildResponse;
 import org.apache.cloudstack.api.response.InstanceBootGroupMemberResponse;
 import org.apache.cloudstack.api.response.InstanceBootGroupReadinessRuleResponse;
+import org.apache.cloudstack.api.response.InstanceBootGroupResponse;
 import org.apache.cloudstack.api.response.ListResponse;
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.vm.bootgroup.readiness.InstanceBootGroupReadinessRule;
@@ -75,6 +78,8 @@
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
 import com.cloud.utils.db.Transaction;
 import com.cloud.utils.db.TransactionCallback;
 import com.cloud.vm.InstanceGroupVMMapVO;
@@ -374,6 +379,74 @@ public void testGetGroupAndCheckAccessSuccessDelegatesToAccountManager() {
         verify(accountManager).checkAccess(callerMock, null, true, group);
     }
 
+    // ---------------------------------------------------------------- listInstanceBootGroups
+
+    private ListInstanceBootGroupsCmd baseListGroupsCmd(Long virtualMachineId, Long instanceGroupId) {
+        ListInstanceBootGroupsCmd cmd = mock(ListInstanceBootGroupsCmd.class);
+        when(cmd.getVirtualMachineId()).thenReturn(virtualMachineId);
+        when(cmd.getInstanceGroupId()).thenReturn(instanceGroupId);
+        return cmd;
+    }
+
+    private SearchBuilder<InstanceBootGroupJoinVO> mockSearchBuilder() {
+        @SuppressWarnings("unchecked")
+        SearchBuilder<InstanceBootGroupJoinVO> sb = mock(SearchBuilder.class);
+        @SuppressWarnings("unchecked")
+        SearchCriteria<InstanceBootGroupJoinVO> sc = mock(SearchCriteria.class);
+        when(sb.entity()).thenReturn(new InstanceBootGroupJoinVO());
+        when(sb.create()).thenReturn(sc);
+        when(instanceBootGroupJoinDao.createSearchBuilder()).thenReturn(sb);
+        return sb;
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testListInstanceBootGroupsBothVmAndInstanceGroupIdThrows() {
+        ListInstanceBootGroupsCmd cmd = baseListGroupsCmd(VM_ID, INSTANCE_GROUP_ID);
+        service.listInstanceBootGroups(cmd);
+    }
+
+    @Test
+    public void testListInstanceBootGroupsByVirtualMachineIdNotAMemberReturnsEmpty() {
+        ListInstanceBootGroupsCmd cmd = baseListGroupsCmd(VM_ID, null);
+        when(instanceBootGroupMemberDao.findByMember(InstanceBootGroupMember.MemberType.VirtualMachine, VM_ID)).thenReturn(null);
+
+        ListResponse<InstanceBootGroupResponse> response = service.listInstanceBootGroups(cmd);
+
+        assertEquals(0, response.getCount().intValue());
+        assertTrue(response.getResponses().isEmpty());
+        verify(instanceBootGroupJoinDao, never()).createSearchBuilder();
+    }
+
+    @Test
+    public void testListInstanceBootGroupsByVirtualMachineIdFiltersToMemberBootGroup() {
+        ListInstanceBootGroupsCmd cmd = baseListGroupsCmd(VM_ID, null);
+        InstanceBootGroupMemberVO member = newMember(MEMBER_ID, GROUP_ID, InstanceBootGroupMember.MemberType.VirtualMachine, VM_ID, 0);
+        when(instanceBootGroupMemberDao.findByMember(InstanceBootGroupMember.MemberType.VirtualMachine, VM_ID)).thenReturn(member);
+
+        SearchBuilder<InstanceBootGroupJoinVO> sb = mockSearchBuilder();
+        InstanceBootGroupJoinVO joinVO = new InstanceBootGroupJoinVO();
+        ReflectionTestUtils.setField(joinVO, "id", GROUP_ID);
+        ReflectionTestUtils.setField(joinVO, "name", "group1");
+        when(instanceBootGroupJoinDao.searchAndCount(any(), any()))
+                .thenReturn(new com.cloud.utils.Pair<>(Collections.singletonList(joinVO), 1));
+
+        ListResponse<InstanceBootGroupResponse> response = service.listInstanceBootGroups(cmd);
+
+        assertEquals(1, response.getResponses().size());
+        verify(sb.create()).setParameters("memberBootGroupId", GROUP_ID);
+    }
+
+    @Test
+    public void testListInstanceBootGroupsByInstanceGroupIdNotAMemberReturnsEmpty() {
+        ListInstanceBootGroupsCmd cmd = baseListGroupsCmd(null, INSTANCE_GROUP_ID);
+        when(instanceBootGroupMemberDao.findByMember(InstanceBootGroupMember.MemberType.InstanceGroup, INSTANCE_GROUP_ID)).thenReturn(null);
+
+        ListResponse<InstanceBootGroupResponse> response = service.listInstanceBootGroups(cmd);
+
+        assertEquals(0, response.getCount().intValue());
+        verify(instanceBootGroupJoinDao, never()).createSearchBuilder();
+    }
+
     // ---------------------------------------------------------------- addMemberToInstanceBootGroup
 
     @Test