Skip to content

Commit 1e69e3a

Browse files
nvazqueznvazquez
authored
Allow removing VMs from instance groups (#14395)
* Allow removing VMs from instance groups * Address review comments
1 parent c7667d8 commit 1e69e3a

4 files changed

Lines changed: 76 additions & 22 deletions

File tree

‎api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public class UpdateVMCmd extends BaseCustomIdCmd implements SecurityGroupAction,
6565
@Parameter(name = ApiConstants.DISPLAY_NAME, type = CommandType.STRING, description = "User generated name")
6666
private String displayName;
6767

68-
@Parameter(name = ApiConstants.GROUP, type = CommandType.STRING, description = "Group of the Instance")
68+
@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")
6969
private String group;
7070

7171
@Parameter(name = ApiConstants.HA_ENABLE, type = CommandType.BOOLEAN, description = "True if high-availability is enabled for the Instance, false otherwise")

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3208,8 +3208,11 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo
32083208
osTypeId = vm.getGuestOSId();
32093209
}
32103210

3211-
if (group != null) {
3211+
if (StringUtils.isNotBlank(group)) {
32123212
addInstanceToGroup(id, group);
3213+
} else if (group != null) {
3214+
// Allow removing an instance from its group when passing an empty group on update
3215+
removeInstanceFromInstanceGroup(vm.getId());
32133216
}
32143217

32153218
if (isDynamicallyScalable == null) {
@@ -3791,6 +3794,10 @@ public InstanceGroupVO getGroupForVm(long vmId) {
37913794
public void removeInstanceFromInstanceGroup(long vmId) {
37923795
try {
37933796
List<InstanceGroupVMMapVO> groupVmMaps = _groupVMMapDao.listByInstanceId(vmId);
3797+
if (CollectionUtils.isEmpty(groupVmMaps)) {
3798+
logger.debug("Vm id: {} does not belong to any group", vmId);
3799+
return;
3800+
}
37943801
for (InstanceGroupVMMapVO groupMap : groupVmMaps) {
37953802
SearchCriteria<InstanceGroupVMMapVO> sc = _groupVMMapDao.createSearchCriteria();
37963803
sc.addAnd("instanceId", SearchCriteria.Op.EQ, groupMap.getInstanceId());

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

Lines changed: 59 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@
6060
import java.util.TimeZone;
6161
import java.util.UUID;
6262

63+
import com.cloud.utils.db.SearchCriteria;
64+
import com.cloud.vm.dao.InstanceGroupVMMapDao;
6365
import org.apache.cloudstack.acl.ControlledEntity;
6466
import org.apache.cloudstack.acl.SecurityChecker;
6567
import org.apache.cloudstack.api.ApiCommandResourceType;
@@ -471,6 +473,9 @@ public class UserVmManagerImplTest {
471473
@Mock
472474
ServiceOfferingDetailsDao serviceOfferingDetailsDao;
473475

476+
@Mock
477+
InstanceGroupVMMapDao instanceGroupVMMapDao;
478+
474479
private static final long vmId = 1L;
475480
private static final long zoneId = 2L;
476481
private static final long accountId = 3L;
@@ -583,7 +588,7 @@ public void validateInputsAndPermissionForUpdateVirtualMachineCommandTest() {
583588

584589
@Test
585590
public void updateVirtualMachineTestDisplayChanged() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException {
586-
configureDoNothingForMethodsThatWeDoNotWantToTest();
591+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
587592
ServiceOffering offering = getSvcoffering(512);
588593
Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering);
589594
Mockito.when(userVmVoMock.isDisplay()).thenReturn(true);
@@ -598,7 +603,7 @@ public void updateVirtualMachineTestDisplayChanged() throws ResourceUnavailableE
598603

599604
@Test
600605
public void updateVirtualMachineTestCleanUpTrue() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException {
601-
configureDoNothingForMethodsThatWeDoNotWantToTest();
606+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
602607
ServiceOffering offering = getSvcoffering(512);
603608
Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering);
604609
Mockito.when(updateVmCommand.isCleanupDetails()).thenReturn(true);
@@ -615,6 +620,41 @@ public void updateVirtualMachineTestCleanUpTrue() throws ResourceUnavailableExce
615620
Mockito.verify(userVmManagerImpl, times(0)).updateDisplayVmFlag(false, vmId, userVmVoMock);
616621
}
617622

623+
@Test
624+
public void testUpdateVirtualMachineInstanceGroupNull() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException {
625+
configureDoNothingForMethodsThatWeDoNotWantToTest(false);
626+
ServiceOffering offering = getSvcoffering(512);
627+
Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering);
628+
Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null);
629+
630+
userVmManagerImpl.updateVirtualMachine(updateVmCommand);
631+
Mockito.verify(userVmManagerImpl, never()).removeInstanceFromInstanceGroup(Mockito.anyLong());
632+
}
633+
634+
@Test
635+
public void testUpdateVirtualMachineInstanceGroupEmptyValue() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException {
636+
configureDoNothingForMethodsThatWeDoNotWantToTest(false);
637+
ServiceOffering offering = getSvcoffering(512);
638+
Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering);
639+
Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null);
640+
641+
Mockito.when(userVmVoMock.getId()).thenReturn(vmId);
642+
InstanceGroupVMMapVO mapVO = Mockito.mock(InstanceGroupVMMapVO.class);
643+
Mockito.when(mapVO.getInstanceId()).thenReturn(vmId);
644+
Mockito.when(instanceGroupVMMapDao.listByInstanceId(vmId)).thenReturn(Collections.singletonList(mapVO));
645+
SearchCriteria<InstanceGroupVMMapVO> searchCriteria = Mockito.mock(SearchCriteria.class);
646+
Mockito.when(instanceGroupVMMapDao.createSearchCriteria()).thenReturn(searchCriteria);
647+
648+
// Set group = "" to reset/restore the instance group for a VM
649+
Mockito.when(updateVmCommand.getGroup()).thenReturn("");
650+
651+
userVmManagerImpl.updateVirtualMachine(updateVmCommand);
652+
653+
verifyMethodsThatAreAlwaysExecuted();
654+
Mockito.verify(userVmManagerImpl).removeInstanceFromInstanceGroup(vmId);
655+
Mockito.verify(instanceGroupVMMapDao).expunge(searchCriteria);
656+
}
657+
618658
@Test
619659
public void updateVirtualMachineTestCleanUpTrueAndDetailEmpty() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException {
620660
prepareAndExecuteMethodDealingWithDetails(true, true);
@@ -647,7 +687,7 @@ private List<VMInstanceDetailVO> prepareExistingDetails(Long vmId, String... exi
647687
}
648688

649689
private void prepareAndExecuteMethodDealingWithDetails(boolean cleanUpDetails, boolean isDetailsEmpty) throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException {
650-
configureDoNothingForMethodsThatWeDoNotWantToTest();
690+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
651691

652692
ServiceOffering offering = getSvcoffering(512);
653693
Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering);
@@ -701,18 +741,20 @@ private void verifyMethodsThatAreAlwaysExecuted() throws ResourceUnavailableExce
701741

702742
}
703743

704-
private void configureDoNothingForMethodsThatWeDoNotWantToTest() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException {
744+
private void configureDoNothingForMethodsThatWeDoNotWantToTest(boolean mockUpdateVM) throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException {
705745
Mockito.doNothing().when(userVmManagerImpl).validateInputsAndPermissionForUpdateVirtualMachineCommand(updateVmCommand);
706746
Mockito.doReturn(new ArrayList<Long>()).when(userVmManagerImpl).getSecurityGroupIdList(updateVmCommand);
707747

708-
Mockito.lenient().doReturn(Mockito.mock(UserVm.class)).when(userVmManagerImpl).updateVirtualMachine(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean(),
709-
Mockito.anyBoolean(), Mockito.anyBoolean(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyBoolean(), Mockito.any(HTTPMethod.class), Mockito.anyString(), Mockito.anyString(),
710-
Mockito.anyString(), anyList(), Mockito.any());
748+
if (mockUpdateVM) {
749+
Mockito.lenient().doReturn(Mockito.mock(UserVm.class)).when(userVmManagerImpl).updateVirtualMachine(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean(),
750+
Mockito.anyBoolean(), Mockito.anyBoolean(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyBoolean(), Mockito.any(HTTPMethod.class), Mockito.anyString(), Mockito.anyString(),
751+
Mockito.anyString(), anyList(), Mockito.any());
752+
Mockito.doNothing().when(userVmManagerImpl).removeInstanceFromInstanceGroup(Mockito.anyLong());
753+
}
711754

712755
Mockito.doNothing().when(userVmManagerImpl).validateIfVmSupportsMigration(Mockito.any(), Mockito.anyLong());
713756
Mockito.doNothing().when(userVmManagerImpl).validateOldAndNewAccounts(Mockito.nullable(Account.class), Mockito.nullable(Account.class), Mockito.anyLong(), Mockito.nullable(String.class), Mockito.nullable(Long.class));
714757
Mockito.doNothing().when(userVmManagerImpl).validateIfVmHasNoRules(Mockito.any(), Mockito.anyLong());
715-
Mockito.doNothing().when(userVmManagerImpl).removeInstanceFromInstanceGroup(Mockito.anyLong());
716758
Mockito.doNothing().when(userVmManagerImpl).validateIfNewOwnerHasAccessToTemplate(Mockito.any(), Mockito.any(), Mockito.any());
717759

718760
Mockito.doNothing().when(userVmManagerImpl).updateVmOwner(Mockito.any(), Mockito.any());
@@ -3199,7 +3241,7 @@ public void moveVmToUserTestMovesVmWhenProjectIdIsProvidedAndDomainIdIsNull() th
31993241
Mockito.doNothing().when(userVmManagerImpl).executeStepsToChangeOwnershipOfVm(Mockito.any(), Mockito.any(), Mockito.any(),
32003242
Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
32013243

3202-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3244+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
32033245

32043246
userVmManagerImpl.moveVmToUser(assignVmCmdMock);
32053247

@@ -3215,7 +3257,7 @@ public void moveVmToUserTestValidateIfVmHasNoRulesThrowsInvalidParameterValueExc
32153257
Mockito.doReturn(userVmVoMock).when(userVmDao).findById(Mockito.anyLong());
32163258
Mockito.doReturn(null).when(assignVmCmdMock).getProjectId();
32173259

3218-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3260+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
32193261

32203262
doThrow(InvalidParameterValueException.class).when(userVmManagerImpl).validateIfVmHasNoRules(Mockito.any(), Mockito.anyLong());
32213263

@@ -3234,7 +3276,7 @@ public void moveVmToUserTestSnapshotsForVolumeExistThrowsInvalidParameterValueEx
32343276
Mockito.doReturn(null).when(assignVmCmdMock).getProjectId();
32353277
Mockito.doReturn(volumes).when(volumeDaoMock).findByInstance(Mockito.anyLong());
32363278

3237-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3279+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
32383280

32393281
doThrow(InvalidParameterValueException.class).when(userVmManagerImpl).validateIfVolumesHaveNoSnapshots(Mockito.any());
32403282

@@ -3252,7 +3294,7 @@ public void moveVmToUserTestVerifyResourceLimitsForAccountAndStorageThrowsResour
32523294
Mockito.doReturn(null).when(assignVmCmdMock).getProjectId();
32533295
Mockito.doReturn(volumes).when(volumeDaoMock).findByInstance(Mockito.anyLong());
32543296

3255-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3297+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
32563298

32573299
doThrow(ResourceAllocationException.class).when(userVmManagerImpl).verifyResourceLimitsForAccountAndStorage(Mockito.any(), Mockito.any(),
32583300
Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
@@ -3271,7 +3313,7 @@ public void moveVmToUserTestVerifyValidateIfNewOwnerHasAccessToTemplateThrowsInv
32713313
Mockito.doReturn(null).when(assignVmCmdMock).getProjectId();
32723314
Mockito.doReturn(volumes).when(volumeDaoMock).findByInstance(Mockito.anyLong());
32733315

3274-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3316+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
32753317

32763318
doThrow(InvalidParameterValueException.class).when(userVmManagerImpl).validateIfNewOwnerHasAccessToTemplate(Mockito.any(), Mockito.any(), Mockito.any());
32773319

@@ -3287,7 +3329,7 @@ public void executeStepsToChangeOwnershipOfVmTestUpdateVmNetworkThrowsInsufficie
32873329
try (MockedStatic<UsageEventUtils> ignored = mockStatic(UsageEventUtils.class)) {
32883330
Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(userVmVoMock).getHypervisorType();
32893331

3290-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3332+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
32913333

32923334
doThrow(InsufficientAddressCapacityException.class).when(userVmManagerImpl).updateVmNetwork(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(),
32933335
Mockito.any());
@@ -3310,7 +3352,7 @@ public void executeStepsToChangeOwnershipOfVmTestUpdateVmNetworkThrowsResourceAl
33103352
try (MockedStatic<UsageEventUtils> ignored = mockStatic(UsageEventUtils.class)) {
33113353
Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(userVmVoMock).getHypervisorType();
33123354

3313-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3355+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
33143356

33153357
doThrow(ResourceAllocationException.class).when(userVmManagerImpl).updateVmNetwork(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(),
33163358
Mockito.any());
@@ -3335,7 +3377,7 @@ public void executeStepsToChangeOwnershipOfVmTestResourceCountRunningVmsOnlyEnab
33353377
Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(userVmVoMock).getHypervisorType();
33363378
Mockito.doReturn(false).when(userVmManagerImpl).isResourceCountRunningVmsOnlyEnabled();
33373379

3338-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3380+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
33393381

33403382
userVmManagerImpl.executeStepsToChangeOwnershipOfVm(assignVmCmdMock, callerAccount, accountMock, accountMock, userVmVoMock, serviceOfferingVoMock, volumes,
33413383
virtualMachineTemplateMock);
@@ -3358,7 +3400,7 @@ public void executeStepsToChangeOwnershipOfVmTestResourceCountRunningVmsOnlyEnab
33583400
Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(userVmVoMock).getHypervisorType();
33593401
Mockito.doReturn(true).when(userVmManagerImpl).isResourceCountRunningVmsOnlyEnabled();
33603402

3361-
configureDoNothingForMethodsThatWeDoNotWantToTest();
3403+
configureDoNothingForMethodsThatWeDoNotWantToTest(true);
33623404

33633405
userVmManagerImpl.executeStepsToChangeOwnershipOfVm(assignVmCmdMock, callerAccount, accountMock, accountMock, userVmVoMock, serviceOfferingVoMock, volumes,
33643406
virtualMachineTemplateMock);

‎ui/src/views/compute/EditVM.vue‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@
8080
<a-auto-complete
8181
v-model:value="form.group"
8282
:filterOption="(input, option) => {
83-
return option.value.toLowerCase().indexOf(input.toLowerCase()) >= 0
83+
return !option.value || !input || input === resource.group ||
84+
option.value.toLowerCase().indexOf(input.toLowerCase()) >= 0
8485
}"
8586
:options="groups.opts" />
8687
</a-form-item>
@@ -338,6 +339,10 @@ export default {
338339
fetchInstaceGroups () {
339340
this.groups.loading = true
340341
this.groups.opts = []
342+
this.groups.opts.push({
343+
id: '',
344+
value: ''
345+
})
341346
const params = {
342347
domainid: this.$store.getters.userInfo.domainid,
343348
listall: true
@@ -411,8 +416,8 @@ export default {
411416
if (values.haenable !== undefined) {
412417
params.haenable = values.haenable
413418
}
414-
if (values.group && values.group.length > 0) {
415-
params.group = values.group
419+
if ((values.group || '') !== (this.resource.group || '')) {
420+
params.group = values.group || ''
416421
}
417422
if (values.userdata && values.userdata.length > 0) {
418423
params.userdata = this.$toBase64AndURIEncoded(values.userdata)

0 commit comments

Comments
 (0)