diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java index cea18f4e8241..ead3a21605ac 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java @@ -65,7 +65,7 @@ public class UpdateVMCmd extends BaseCustomIdCmd implements SecurityGroupAction, @Parameter(name = ApiConstants.DISPLAY_NAME, type = CommandType.STRING, description = "User generated name") private String displayName; - @Parameter(name = ApiConstants.GROUP, type = CommandType.STRING, description = "Group of the Instance") + @Parameter(name = ApiConstants.GROUP, type = CommandType.STRING, description = "Group of the Instance. To remove an Instance from its current Group, please pass an empty string as parameter") private String group; @Parameter(name = ApiConstants.HA_ENABLE, type = CommandType.BOOLEAN, description = "True if high-availability is enabled for the Instance, false otherwise") diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 7636ef6b152c..ac3236fd7b8f 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3208,8 +3208,11 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo osTypeId = vm.getGuestOSId(); } - if (group != null) { + if (StringUtils.isNotBlank(group)) { addInstanceToGroup(id, group); + } else if (group != null) { + // Allow removing an instance from its group when passing an empty group on update + removeInstanceFromInstanceGroup(vm.getId()); } if (isDynamicallyScalable == null) { @@ -3791,6 +3794,10 @@ public InstanceGroupVO getGroupForVm(long vmId) { public void removeInstanceFromInstanceGroup(long vmId) { try { List groupVmMaps = _groupVMMapDao.listByInstanceId(vmId); + if (CollectionUtils.isEmpty(groupVmMaps)) { + logger.debug("Vm id: {} does not belong to any group", vmId); + return; + } for (InstanceGroupVMMapVO groupMap : groupVmMaps) { SearchCriteria sc = _groupVMMapDao.createSearchCriteria(); sc.addAnd("instanceId", SearchCriteria.Op.EQ, groupMap.getInstanceId()); diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index cc2dc1cc9efa..13abf17ed5db 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -60,6 +60,8 @@ import java.util.TimeZone; import java.util.UUID; +import com.cloud.utils.db.SearchCriteria; +import com.cloud.vm.dao.InstanceGroupVMMapDao; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.api.ApiCommandResourceType; @@ -471,6 +473,9 @@ public class UserVmManagerImplTest { @Mock ServiceOfferingDetailsDao serviceOfferingDetailsDao; + @Mock + InstanceGroupVMMapDao instanceGroupVMMapDao; + private static final long vmId = 1L; private static final long zoneId = 2L; private static final long accountId = 3L; @@ -583,7 +588,7 @@ public void validateInputsAndPermissionForUpdateVirtualMachineCommandTest() { @Test public void updateVirtualMachineTestDisplayChanged() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); ServiceOffering offering = getSvcoffering(512); Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering); Mockito.when(userVmVoMock.isDisplay()).thenReturn(true); @@ -598,7 +603,7 @@ public void updateVirtualMachineTestDisplayChanged() throws ResourceUnavailableE @Test public void updateVirtualMachineTestCleanUpTrue() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); ServiceOffering offering = getSvcoffering(512); Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering); Mockito.when(updateVmCommand.isCleanupDetails()).thenReturn(true); @@ -615,6 +620,41 @@ public void updateVirtualMachineTestCleanUpTrue() throws ResourceUnavailableExce Mockito.verify(userVmManagerImpl, times(0)).updateDisplayVmFlag(false, vmId, userVmVoMock); } + @Test + public void testUpdateVirtualMachineInstanceGroupNull() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { + configureDoNothingForMethodsThatWeDoNotWantToTest(false); + ServiceOffering offering = getSvcoffering(512); + Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering); + Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null); + + userVmManagerImpl.updateVirtualMachine(updateVmCommand); + Mockito.verify(userVmManagerImpl, never()).removeInstanceFromInstanceGroup(Mockito.anyLong()); + } + + @Test + public void testUpdateVirtualMachineInstanceGroupEmptyValue() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { + configureDoNothingForMethodsThatWeDoNotWantToTest(false); + ServiceOffering offering = getSvcoffering(512); + Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering); + Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null); + + Mockito.when(userVmVoMock.getId()).thenReturn(vmId); + InstanceGroupVMMapVO mapVO = Mockito.mock(InstanceGroupVMMapVO.class); + Mockito.when(mapVO.getInstanceId()).thenReturn(vmId); + Mockito.when(instanceGroupVMMapDao.listByInstanceId(vmId)).thenReturn(Collections.singletonList(mapVO)); + SearchCriteria searchCriteria = Mockito.mock(SearchCriteria.class); + Mockito.when(instanceGroupVMMapDao.createSearchCriteria()).thenReturn(searchCriteria); + + // Set group = "" to reset/restore the instance group for a VM + Mockito.when(updateVmCommand.getGroup()).thenReturn(""); + + userVmManagerImpl.updateVirtualMachine(updateVmCommand); + + verifyMethodsThatAreAlwaysExecuted(); + Mockito.verify(userVmManagerImpl).removeInstanceFromInstanceGroup(vmId); + Mockito.verify(instanceGroupVMMapDao).expunge(searchCriteria); + } + @Test public void updateVirtualMachineTestCleanUpTrueAndDetailEmpty() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { prepareAndExecuteMethodDealingWithDetails(true, true); @@ -647,7 +687,7 @@ private List prepareExistingDetails(Long vmId, String... exi } private void prepareAndExecuteMethodDealingWithDetails(boolean cleanUpDetails, boolean isDetailsEmpty) throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); ServiceOffering offering = getSvcoffering(512); Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering); @@ -701,18 +741,20 @@ private void verifyMethodsThatAreAlwaysExecuted() throws ResourceUnavailableExce } - private void configureDoNothingForMethodsThatWeDoNotWantToTest() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { + private void configureDoNothingForMethodsThatWeDoNotWantToTest(boolean mockUpdateVM) throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { Mockito.doNothing().when(userVmManagerImpl).validateInputsAndPermissionForUpdateVirtualMachineCommand(updateVmCommand); Mockito.doReturn(new ArrayList()).when(userVmManagerImpl).getSecurityGroupIdList(updateVmCommand); - Mockito.lenient().doReturn(Mockito.mock(UserVm.class)).when(userVmManagerImpl).updateVirtualMachine(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean(), - Mockito.anyBoolean(), Mockito.anyBoolean(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyBoolean(), Mockito.any(HTTPMethod.class), Mockito.anyString(), Mockito.anyString(), - Mockito.anyString(), anyList(), Mockito.any()); + if (mockUpdateVM) { + Mockito.lenient().doReturn(Mockito.mock(UserVm.class)).when(userVmManagerImpl).updateVirtualMachine(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean(), + Mockito.anyBoolean(), Mockito.anyBoolean(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyBoolean(), Mockito.any(HTTPMethod.class), Mockito.anyString(), Mockito.anyString(), + Mockito.anyString(), anyList(), Mockito.any()); + Mockito.doNothing().when(userVmManagerImpl).removeInstanceFromInstanceGroup(Mockito.anyLong()); + } Mockito.doNothing().when(userVmManagerImpl).validateIfVmSupportsMigration(Mockito.any(), Mockito.anyLong()); Mockito.doNothing().when(userVmManagerImpl).validateOldAndNewAccounts(Mockito.nullable(Account.class), Mockito.nullable(Account.class), Mockito.anyLong(), Mockito.nullable(String.class), Mockito.nullable(Long.class)); Mockito.doNothing().when(userVmManagerImpl).validateIfVmHasNoRules(Mockito.any(), Mockito.anyLong()); - Mockito.doNothing().when(userVmManagerImpl).removeInstanceFromInstanceGroup(Mockito.anyLong()); Mockito.doNothing().when(userVmManagerImpl).validateIfNewOwnerHasAccessToTemplate(Mockito.any(), Mockito.any(), Mockito.any()); Mockito.doNothing().when(userVmManagerImpl).updateVmOwner(Mockito.any(), Mockito.any()); @@ -3199,7 +3241,7 @@ public void moveVmToUserTestMovesVmWhenProjectIdIsProvidedAndDomainIdIsNull() th Mockito.doNothing().when(userVmManagerImpl).executeStepsToChangeOwnershipOfVm(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); userVmManagerImpl.moveVmToUser(assignVmCmdMock); @@ -3215,7 +3257,7 @@ public void moveVmToUserTestValidateIfVmHasNoRulesThrowsInvalidParameterValueExc Mockito.doReturn(userVmVoMock).when(userVmDao).findById(Mockito.anyLong()); Mockito.doReturn(null).when(assignVmCmdMock).getProjectId(); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); doThrow(InvalidParameterValueException.class).when(userVmManagerImpl).validateIfVmHasNoRules(Mockito.any(), Mockito.anyLong()); @@ -3234,7 +3276,7 @@ public void moveVmToUserTestSnapshotsForVolumeExistThrowsInvalidParameterValueEx Mockito.doReturn(null).when(assignVmCmdMock).getProjectId(); Mockito.doReturn(volumes).when(volumeDaoMock).findByInstance(Mockito.anyLong()); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); doThrow(InvalidParameterValueException.class).when(userVmManagerImpl).validateIfVolumesHaveNoSnapshots(Mockito.any()); @@ -3252,7 +3294,7 @@ public void moveVmToUserTestVerifyResourceLimitsForAccountAndStorageThrowsResour Mockito.doReturn(null).when(assignVmCmdMock).getProjectId(); Mockito.doReturn(volumes).when(volumeDaoMock).findByInstance(Mockito.anyLong()); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); doThrow(ResourceAllocationException.class).when(userVmManagerImpl).verifyResourceLimitsForAccountAndStorage(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); @@ -3271,7 +3313,7 @@ public void moveVmToUserTestVerifyValidateIfNewOwnerHasAccessToTemplateThrowsInv Mockito.doReturn(null).when(assignVmCmdMock).getProjectId(); Mockito.doReturn(volumes).when(volumeDaoMock).findByInstance(Mockito.anyLong()); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); doThrow(InvalidParameterValueException.class).when(userVmManagerImpl).validateIfNewOwnerHasAccessToTemplate(Mockito.any(), Mockito.any(), Mockito.any()); @@ -3287,7 +3329,7 @@ public void executeStepsToChangeOwnershipOfVmTestUpdateVmNetworkThrowsInsufficie try (MockedStatic ignored = mockStatic(UsageEventUtils.class)) { Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(userVmVoMock).getHypervisorType(); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); doThrow(InsufficientAddressCapacityException.class).when(userVmManagerImpl).updateVmNetwork(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); @@ -3310,7 +3352,7 @@ public void executeStepsToChangeOwnershipOfVmTestUpdateVmNetworkThrowsResourceAl try (MockedStatic ignored = mockStatic(UsageEventUtils.class)) { Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(userVmVoMock).getHypervisorType(); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); doThrow(ResourceAllocationException.class).when(userVmManagerImpl).updateVmNetwork(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); @@ -3335,7 +3377,7 @@ public void executeStepsToChangeOwnershipOfVmTestResourceCountRunningVmsOnlyEnab Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(userVmVoMock).getHypervisorType(); Mockito.doReturn(false).when(userVmManagerImpl).isResourceCountRunningVmsOnlyEnabled(); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); userVmManagerImpl.executeStepsToChangeOwnershipOfVm(assignVmCmdMock, callerAccount, accountMock, accountMock, userVmVoMock, serviceOfferingVoMock, volumes, virtualMachineTemplateMock); @@ -3358,7 +3400,7 @@ public void executeStepsToChangeOwnershipOfVmTestResourceCountRunningVmsOnlyEnab Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(userVmVoMock).getHypervisorType(); Mockito.doReturn(true).when(userVmManagerImpl).isResourceCountRunningVmsOnlyEnabled(); - configureDoNothingForMethodsThatWeDoNotWantToTest(); + configureDoNothingForMethodsThatWeDoNotWantToTest(true); userVmManagerImpl.executeStepsToChangeOwnershipOfVm(assignVmCmdMock, callerAccount, accountMock, accountMock, userVmVoMock, serviceOfferingVoMock, volumes, virtualMachineTemplateMock); diff --git a/ui/src/views/compute/EditVM.vue b/ui/src/views/compute/EditVM.vue index 0763303b24a4..449304e205ab 100644 --- a/ui/src/views/compute/EditVM.vue +++ b/ui/src/views/compute/EditVM.vue @@ -80,7 +80,8 @@ @@ -338,6 +339,10 @@ export default { fetchInstaceGroups () { this.groups.loading = true this.groups.opts = [] + this.groups.opts.push({ + id: '', + value: '' + }) const params = { domainid: this.$store.getters.userInfo.domainid, listall: true @@ -411,8 +416,8 @@ export default { if (values.haenable !== undefined) { params.haenable = values.haenable } - if (values.group && values.group.length > 0) { - params.group = values.group + if ((values.group || '') !== (this.resource.group || '')) { + params.group = values.group || '' } if (values.userdata && values.userdata.length > 0) { params.userdata = this.$toBase64AndURIEncoded(values.userdata)