From ce956b8053487aec025ac5bb486f5051efd92b88 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 5 Sep 2022 23:50:56 +0530 Subject: [PATCH 1/8] server: fix volume migration on user vm scale Fixes #6701 When volume migration is initiated by system account check is not needed. Signed-off-by: Abhishek Kumar --- .../com/cloud/server/ManagementService.java | 2 +- .../cloud/server/ManagementServerImpl.java | 21 ++++++++++++------- .../cloud/storage/VolumeApiServiceImpl.java | 2 +- .../vm/UnmanagedVMsManagerImpl.java | 2 +- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/com/cloud/server/ManagementService.java b/api/src/main/java/com/cloud/server/ManagementService.java index 5c385cc18d9b..6aea206c7968 100644 --- a/api/src/main/java/com/cloud/server/ManagementService.java +++ b/api/src/main/java/com/cloud/server/ManagementService.java @@ -405,7 +405,7 @@ public interface ManagementService { */ Pair, List> listStoragePoolsForMigrationOfVolume(Long volumeId); - Pair, List> listStoragePoolsForMigrationOfVolumeInternal(Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck); + Pair, List> listStoragePoolsForSystemMigrationOfVolume(Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck); String[] listEventTypes(); diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 074fff86be5a..540b2e391595 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -1526,7 +1526,7 @@ private boolean hasSuitablePoolsForVolume(final VolumeVO volume, final Host host @Override public Pair, List> listStoragePoolsForMigrationOfVolume(final Long volumeId) { - Pair, List> allPoolsAndSuitablePoolsPair = listStoragePoolsForMigrationOfVolumeInternal(volumeId, null, null, null, null, false, true); + Pair, List> allPoolsAndSuitablePoolsPair = listStoragePoolsForMigrationOfVolumeInternal(volumeId, null, null, null, null, false, true, false); List allPools = allPoolsAndSuitablePoolsPair.first(); List suitablePools = allPoolsAndSuitablePoolsPair.second(); List avoidPools = new ArrayList<>(); @@ -1542,13 +1542,20 @@ public Pair, List> listStorag return new Pair, List>(allPools, suitablePools); } - public Pair, List> listStoragePoolsForMigrationOfVolumeInternal(final Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck) { - final Account caller = getCaller(); - if (!_accountMgr.isRootAdmin(caller.getId())) { - if (s_logger.isDebugEnabled()) { - s_logger.debug("Caller is not a root admin, permission denied to migrate the volume"); + @Override + public Pair, List> listStoragePoolsForSystemMigrationOfVolume(final Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck) { + return listStoragePoolsForMigrationOfVolumeInternal(volumeId, newDiskOfferingId, newSize, newMinIops, newMaxIops, keepSourceStoragePool, bypassStorageTypeCheck, true); + } + + public Pair, List> listStoragePoolsForMigrationOfVolumeInternal(final Long volumeId, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean keepSourceStoragePool, boolean bypassStorageTypeCheck, boolean bypassAccountCheck) { + if (!bypassAccountCheck) { + final Account caller = getCaller(); + if (!_accountMgr.isRootAdmin(caller.getId())) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("Caller is not a root admin, permission denied to migrate the volume"); + } + throw new PermissionDeniedException("No permission to migrate volume, only root admin can migrate a volume"); } - throw new PermissionDeniedException("No permission to migrate volume, only root admin can migrate a volume"); } final VolumeVO volume = _volumeDao.findById(volumeId); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index e6726f6977b8..0c622f91452a 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1774,7 +1774,7 @@ private Volume changeDiskOfferingForVolumeInternal(VolumeVO volume, Long newDisk StoragePoolVO existingStoragePool = _storagePoolDao.findById(volume.getPoolId()); - Pair, List> poolsPair = managementService.listStoragePoolsForMigrationOfVolumeInternal(volume.getId(), newDiskOffering.getId(), newSize, newMinIops, newMaxIops, true, false); + Pair, List> poolsPair = managementService.listStoragePoolsForSystemMigrationOfVolume(volume.getId(), newDiskOffering.getId(), newSize, newMinIops, newMaxIops, true, false); List suitableStoragePools = poolsPair.second(); if (!suitableStoragePools.stream().anyMatch(p -> (p.getId() == existingStoragePool.getId()))) { diff --git a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java index 0a4599e1560c..38f57d1f0bb9 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -794,7 +794,7 @@ private UserVm migrateImportedVM(HostVO sourceHost, VirtualMachineTemplate templ continue; } LOGGER.debug(String.format("Volume %s needs to be migrated", volumeVO.getUuid())); - Pair, List> poolsPair = managementService.listStoragePoolsForMigrationOfVolumeInternal(profile.getVolumeId(), null, null, null, null, false, true); + Pair, List> poolsPair = managementService.listStoragePoolsForSystemMigrationOfVolume(profile.getVolumeId(), null, null, null, null, false, true); if (CollectionUtils.isEmpty(poolsPair.first()) && CollectionUtils.isEmpty(poolsPair.second())) { cleanupFailedImportVM(vm); throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("VM import failed for unmanaged vm: %s during volume ID: %s migration as no suitable pool(s) found", userVm.getInstanceName(), volumeVO.getUuid())); From b1fb6e8c893085f8b7932b708b0e8a464f47a85d Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Mon, 17 Oct 2022 08:40:59 +0530 Subject: [PATCH 2/8] Added Global/zone setting allow.diskOffering.change.during.scale.vm to allow or skip disk offering change upon scale VM operation. Added marvin tests to verify scale VM operation with user account and also to verify the new global/zone setting --- .../main/java/com/cloud/vm/UserVmManager.java | 3 + .../java/com/cloud/vm/UserVmManagerImpl.java | 15 +- test/integration/smoke/test_scale_vm.py | 365 +++++++++++++++++- 3 files changed, 376 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java b/server/src/main/java/com/cloud/vm/UserVmManager.java index cde2d04970db..8e2e87915de4 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManager.java +++ b/server/src/main/java/com/cloud/vm/UserVmManager.java @@ -47,9 +47,12 @@ */ public interface UserVmManager extends UserVmService { String EnableDynamicallyScaleVmCK = "enable.dynamic.scale.vm"; + String AllowDiskOfferingChangeDuringScaleVmCK = "allow.diskOffering.change.during.scale.vm"; String AllowUserExpungeRecoverVmCK ="allow.user.expunge.recover.vm"; ConfigKey EnableDynamicallyScaleVm = new ConfigKey("Advanced", Boolean.class, EnableDynamicallyScaleVmCK, "false", "Enables/Disables dynamically scaling a vm", true, ConfigKey.Scope.Zone); + ConfigKey AllowDiskOfferingChangeDuringScaleVm = new ConfigKey("Advanced", Boolean.class, AllowDiskOfferingChangeDuringScaleVmCK, "false", + "Determines whether to allow or disallow disk offering change for root volume during scaling of a stopped or running vm", true, ConfigKey.Scope.Zone); ConfigKey AllowUserExpungeRecoverVm = new ConfigKey("Advanced", Boolean.class, AllowUserExpungeRecoverVmCK, "false", "Determines whether users can expunge or recover their vm", true, ConfigKey.Scope.Account); ConfigKey DisplayVMOVFProperties = new ConfigKey("Advanced", Boolean.class, "vm.display.ovf.properties", "false", diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 748ee4c6e74e..74b76c685b9a 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -1241,7 +1241,7 @@ private UserVm upgradeStoppedVirtualMachine(Long vmId, Long svcOffId, Map customParameters) throws ResourceAllocationException { + private void changeDiskOfferingForRootVolume(Long vmId, DiskOfferingVO newDiskOffering, Map customParameters, Long zoneId) throws ResourceAllocationException { + + if (!AllowDiskOfferingChangeDuringScaleVm.valueIn(zoneId)) { + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Disk Offering change for the root volume is disabled during the compute offering change operation. Please check the setting %s", AllowDiskOfferingChangeDuringScaleVm.key())); + } + return; + } List vols = _volsDao.findReadyAndAllocatedRootVolumesByInstance(vmId); @@ -7791,7 +7798,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {EnableDynamicallyScaleVm, AllowUserExpungeRecoverVm, VmIpFetchWaitInterval, VmIpFetchTrialMax, + return new ConfigKey[] {EnableDynamicallyScaleVm, AllowDiskOfferingChangeDuringScaleVm, AllowUserExpungeRecoverVm, VmIpFetchWaitInterval, VmIpFetchTrialMax, VmIpFetchThreadPoolMax, VmIpFetchTaskWorkers, AllowDeployVmIfGivenHostFails, EnableAdditionalVmConfig, DisplayVMOVFProperties, KvmAdditionalConfigAllowList, XenServerAdditionalConfigAllowList, VmwareAdditionalConfigAllowList}; } diff --git a/test/integration/smoke/test_scale_vm.py b/test/integration/smoke/test_scale_vm.py index c9b1bf837ba3..ab913e1a887f 100644 --- a/test/integration/smoke/test_scale_vm.py +++ b/test/integration/smoke/test_scale_vm.py @@ -25,8 +25,10 @@ Host, VirtualMachine, ServiceOffering, + DiskOffering, Template, - Configurations) + Configurations, + Volume) from marvin.lib.common import (get_zone, get_template, get_domain) @@ -54,7 +56,7 @@ def setUpClass(cls): return # Get Zone, Domain and templates - domain = get_domain(cls.apiclient) + cls.domain = get_domain(cls.apiclient) cls.zone = get_zone(cls.apiclient, testClient.getZoneForTests()) cls.services['mode'] = cls.zone.networktype @@ -90,7 +92,7 @@ def setUpClass(cls): cls.account = Account.create( cls.apiclient, cls.services["account"], - domainid=domain.id + domainid=cls.domain.id ) cls._cleanup.append(cls.account) @@ -170,6 +172,11 @@ def tearDownClass(cls): name="enable.dynamic.scale.vm", value="false" ) + Configurations.update( + cls.apiclient, + name="allow.diskOffering.change.during.scale.vm", + value="false" + ) super(TestScaleVm,cls).tearDownClass() return @@ -177,6 +184,7 @@ def setUp(self): self.apiclient = self.testClient.getApiClient() self.dbclient = self.testClient.getDbConnection() self.cleanup = [] + self.services["disk_offering"]["disksize"] = 8 if self.unsupportedHypervisor: self.skipTest("Skipping test because unsupported hypervisor\ @@ -480,4 +488,355 @@ def test_03_scale_vm(self): else: self.fail("Expected an exception to be thrown, failing") + return + + @attr(hypervisor="xenserver") + @attr(tags=["advanced", "basic"], required_hardware="false") + def test_04_scale_vm_with_user_account(self): + """Test scale virtual machine under useraccount + """ + # Validate the following + # Create a user Account and create a VM in it. + # Scale up the vm and see if it scales to the new svc offering + + # Create an user account + self.userAccount = Account.create( + self.apiclient, + self.services["account"], + admin=False, + domainid=self.domain.id + ) + + self.cleanup.append(self.userAccount) + # Create user api client of the user account + self.userapiclient = self.testClient.getUserApiClient( + UserName=self.userAccount.name, + DomainName=self.userAccount.domain + ) + + # create a virtual machine + self.virtual_machine_in_user_account = VirtualMachine.create( + self.userapiclient, + self.services["small"], + accountid=self.userAccount.name, + domainid=self.userAccount.domainid, + serviceofferingid=self.small_offering.id, + mode=self.services["mode"] + ) + + if self.hypervisor.lower() == "vmware": + sshClient = self.virtual_machine_in_user_account.get_ssh_client() + result = str( + sshClient.execute("service vmware-tools status")).lower() + self.debug("and result is: %s" % result) + if not "running" in result: + self.skipTest("Skipping scale VM operation because\ + VMware tools are not installed on the VM") + if self.hypervisor.lower() != 'simulator': + list_vm_response = VirtualMachine.list( + self.apiclient, + id=self.virtual_machine_in_user_account.id + )[0] + hostid = list_vm_response.hostid + host = Host.list( + self.apiclient, + zoneid=self.zone.id, + hostid=hostid, + type='Routing' + )[0] + + try: + username = self.hostConfig["username"] + password = self.hostConfig["password"] + ssh_client = self.get_ssh_client(host.ipaddress, username, password) + res = ssh_client.execute("hostnamectl | grep 'Operating System' | cut -d':' -f2") + except Exception as e: + pass + + if 'XenServer' in res[0]: + self.skipTest("Skipping test for XenServer as it's License does not allow scaling") + + self.virtual_machine_in_user_account.update( + self.userapiclient, + isdynamicallyscalable='true') + + self.debug("Scaling VM-ID: %s to service offering: %s and state %s" % ( + self.virtual_machine_in_user_account.id, + self.big_offering.id, + self.virtual_machine_in_user_account.state + )) + + cmd = scaleVirtualMachine.scaleVirtualMachineCmd() + cmd.serviceofferingid = self.big_offering.id + cmd.id = self.virtual_machine_in_user_account.id + + try: + self.userapiclient.scaleVirtualMachine(cmd) + except Exception as e: + if "LicenceRestriction" in str(e): + self.skipTest("Your XenServer License does not allow scaling") + else: + self.fail("Scaling failed with the following exception: " + str(e)) + + list_vm_response = VirtualMachine.list( + self.userapiclient, + id=self.virtual_machine_in_user_account.id + ) + + vm_response = list_vm_response[0] + + self.debug( + "Scaling VM-ID: %s from service offering: %s to new service\ + offering %s and the response says %s" % + (self.virtual_machine_in_user_account.id, + self.virtual_machine_in_user_account.serviceofferingid, + self.big_offering.id, + vm_response.serviceofferingid)) + self.assertEqual( + vm_response.serviceofferingid, + self.big_offering.id, + "Check service offering of the VM" + ) + + self.assertEqual( + vm_response.state, + 'Running', + "Check the state of VM" + ) + return + + @attr(hypervisor="xenserver") + @attr(tags=["advanced", "basic"], required_hardware="false") + def test_05_scale_vm_dont_allow_disk_offering_change(self): + """Test scale virtual machine with disk offering changes + """ + # Validate the following two cases + + # 1 + # create serviceoffering1 with diskoffering1 + # create serviceoffering2 with diskoffering2 + # create a VM with serviceoffering1 + # Scale up the vm to serviceoffering2 + # Check disk offering of root volume to be diskoffering1 since setting allow.diskOffering.change.during.scale.vm is false + + # 2 + # create serviceoffering3 with diskoffering3 + # update setting allow.diskOffering.change.during.scale.vm to true + # scale up the VM to serviceoffering3 + # Check disk offering of root volume to be diskoffering3 since setting allow.diskOffering.change.during.scale.vm is true + + self.disk_offering1 = DiskOffering.create( + self.apiclient, + self.services["disk_offering"], + ) + self._cleanup.append(self.disk_offering1) + offering_data = { + 'displaytext': 'ServiceOffering1WithDiskOffering1', + 'cpuspeed': 500, + 'cpunumber': 1, + 'name': 'ServiceOffering1WithDiskOffering1', + 'memory': 512, + 'diskofferingid': self.disk_offering1.id + } + + self.ServiceOffering1WithDiskOffering1 = ServiceOffering.create( + self.apiclient, + offering_data, + ) + self._cleanup.append(self.ServiceOffering1WithDiskOffering1) + + # create a virtual machine + self.virtual_machine_test = VirtualMachine.create( + self.apiclient, + self.services["small"], + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.ServiceOffering1WithDiskOffering1.id, + mode=self.services["mode"] + ) + + if self.hypervisor.lower() == "vmware": + sshClient = self.virtual_machine_test.get_ssh_client() + result = str( + sshClient.execute("service vmware-tools status")).lower() + self.debug("and result is: %s" % result) + if not "running" in result: + self.skipTest("Skipping scale VM operation because\ + VMware tools are not installed on the VM") + if self.hypervisor.lower() != 'simulator': + hostid = self.virtual_machine_test.hostid + host = Host.list( + self.apiclient, + zoneid=self.zone.id, + hostid=hostid, + type='Routing' + )[0] + + try: + username = self.hostConfig["username"] + password = self.hostConfig["password"] + ssh_client = self.get_ssh_client(host.ipaddress, username, password) + res = ssh_client.execute("hostnamectl | grep 'Operating System' | cut -d':' -f2") + except Exception as e: + pass + + if 'XenServer' in res[0]: + self.skipTest("Skipping test for XenServer as it's License does not allow scaling") + + self.virtual_machine_test.update( + self.apiclient, + isdynamicallyscalable='true') + + self.disk_offering2 = DiskOffering.create( + self.apiclient, + self.services["disk_offering"], + ) + self._cleanup.append(self.disk_offering2) + offering_data = { + 'displaytext': 'ServiceOffering2WithDiskOffering2', + 'cpuspeed': 1000, + 'cpunumber': 2, + 'name': 'ServiceOffering2WithDiskOffering2', + 'memory': 1024, + 'diskofferingid': self.disk_offering2.id + } + + self.ServiceOffering2WithDiskOffering2 = ServiceOffering.create( + self.apiclient, + offering_data, + ) + self._cleanup.append(self.ServiceOffering2WithDiskOffering2) + + self.debug("Scaling VM-ID: %s to service offering: %s and state %s" % ( + self.virtual_machine_test.id, + self.ServiceOffering2WithDiskOffering2.id, + self.virtual_machine_test.state + )) + + cmd = scaleVirtualMachine.scaleVirtualMachineCmd() + cmd.serviceofferingid = self.ServiceOffering2WithDiskOffering2.id + cmd.id = self.virtual_machine_test.id + + try: + self.apiclient.scaleVirtualMachine(cmd) + except Exception as e: + if "LicenceRestriction" in str(e): + self.skipTest("Your XenServer License does not allow scaling") + else: + self.fail("Scaling failed with the following exception: " + str(e)) + + list_vm_response = VirtualMachine.list( + self.apiclient, + id=self.virtual_machine_test.id + ) + + vm_response = list_vm_response[0] + + self.debug( + "Scaling VM-ID: %s from service offering: %s to new service\ + offering %s and the response says %s" % + (self.virtual_machine_test.id, + self.virtual_machine_test.serviceofferingid, + self.ServiceOffering2WithDiskOffering2.id, + vm_response.serviceofferingid)) + + vm_response = list_vm_response[0] + + self.assertEqual( + vm_response.serviceofferingid, + self.ServiceOffering2WithDiskOffering2.id, + "Check service offering of the VM" + ) + + volume_response = Volume.list( + self.apiclient, + virtualmachineid=self.virtual_machine_test.id, + listall=True + )[0] + + self.assertEqual( + volume_response.diskofferingid, + self.disk_offering1.id, + "Check disk offering of the VM, this should not change since allow.diskOffering.change.during.scale.vm is false" + ) + + # Do same scale vm operation with allow.diskOffering.change.during.scale.vm value to true + + self.disk_offering3 = DiskOffering.create( + self.apiclient, + self.services["disk_offering"], + ) + self._cleanup.append(self.disk_offering3) + offering_data = { + 'displaytext': 'ServiceOffering3WithDiskOffering3', + 'cpuspeed': 1500, + 'cpunumber': 2, + 'name': 'ServiceOffering3WithDiskOffering3', + 'memory': 1024, + 'diskofferingid': self.disk_offering3.id + } + + self.ServiceOffering3WithDiskOffering3 = ServiceOffering.create( + self.apiclient, + offering_data, + ) + self._cleanup.append(self.ServiceOffering3WithDiskOffering3) + + Configurations.update( + self.apiclient, + name="allow.diskOffering.change.during.scale.vm", + value="true" + ) + + self.debug("Scaling VM-ID: %s to service offering: %s and state %s" % ( + self.virtual_machine_test.id, + self.ServiceOffering3WithDiskOffering3.id, + self.virtual_machine_test.state + )) + + cmd = scaleVirtualMachine.scaleVirtualMachineCmd() + cmd.serviceofferingid = self.ServiceOffering3WithDiskOffering3.id + cmd.id = self.virtual_machine_test.id + + try: + self.apiclient.scaleVirtualMachine(cmd) + except Exception as e: + if "LicenceRestriction" in str(e): + self.skipTest("Your XenServer License does not allow scaling") + else: + self.fail("Scaling failed with the following exception: " + str(e)) + + list_vm_response = VirtualMachine.list( + self.apiclient, + id=self.virtual_machine_test.id + ) + + vm_response = list_vm_response[0] + + self.debug( + "Scaling VM-ID: %s from service offering: %s to new service\ + offering %s and the response says %s" % + (self.virtual_machine_test.id, + self.virtual_machine_test.serviceofferingid, + self.ServiceOffering3WithDiskOffering3.id, + vm_response.serviceofferingid)) + + self.assertEqual( + vm_response.serviceofferingid, + self.ServiceOffering3WithDiskOffering3.id, + "Check service offering of the VM" + ) + + volume_response = Volume.list( + self.apiclient, + virtualmachineid=self.virtual_machine_test.id, + listall=True + )[0] + + self.assertEqual( + volume_response.diskofferingid, + self.disk_offering3.id, + "Check disk offering of the VM, this should change since allow.diskOffering.change.during.scale.vm is true" + ) + return \ No newline at end of file From c2bdbd32f3c92a0c1094070e6e28493c3d851b07 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 18 Oct 2022 14:28:34 +0530 Subject: [PATCH 3/8] Fixed marvin tests --- .../cloud/storage/VolumeApiServiceImpl.java | 22 ++++++++++++++++++- test/integration/smoke/test_scale_vm.py | 2 +- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 0c622f91452a..46e59ea04e3c 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1767,7 +1767,7 @@ private Volume changeDiskOfferingForVolumeInternal(VolumeVO volume, Long newDisk return volume; } - if (currentSize != newSize || newMaxIops != volume.getMaxIops() || newMinIops != volume.getMinIops()) { + if (currentSize != newSize || !compareEqualsIncludingNullOrZero(newMaxIops, volume.getMaxIops()) || !compareEqualsIncludingNullOrZero(newMinIops, volume.getMinIops())) { volumeResizeRequired = true; validateVolumeReadyStateAndHypervisorChecks(volume, currentSize, newSize); } @@ -1823,6 +1823,26 @@ private Volume changeDiskOfferingForVolumeInternal(VolumeVO volume, Long newDisk return volume; } + /** + * This method is to compare long values, in miniops and maxiops a or b can be null or 0. + * Use this method to treat 0 and null as same + * + * @param a + * @param b + * @return true if a and b are equal excluding 0 and null values. + */ + private boolean compareEqualsIncludingNullOrZero(Long a, Long b) { + if ((a != null && a == 0 && b == null) || (a == null && b != null && b == 0)) { + return true; + } + + if (a == b) { + return true; + } + + return false; + } + /** * Returns true if the new disk offering is the same than current offering, and the respective Service offering is a custom (constraint or unconstraint) offering. */ diff --git a/test/integration/smoke/test_scale_vm.py b/test/integration/smoke/test_scale_vm.py index ab913e1a887f..0815dc640d11 100644 --- a/test/integration/smoke/test_scale_vm.py +++ b/test/integration/smoke/test_scale_vm.py @@ -184,7 +184,7 @@ def setUp(self): self.apiclient = self.testClient.getApiClient() self.dbclient = self.testClient.getDbConnection() self.cleanup = [] - self.services["disk_offering"]["disksize"] = 8 + self.services["disk_offering"]["disksize"] = 2 if self.unsupportedHypervisor: self.skipTest("Skipping test because unsupported hypervisor\ From 2c7f27f70840b3da69cb1626d439e7e35137c110 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 22 Nov 2022 18:29:45 +0530 Subject: [PATCH 4/8] test fixes Signed-off-by: Abhishek Kumar --- test/integration/smoke/test_scale_vm.py | 168 ++++++++++++++++-------- 1 file changed, 113 insertions(+), 55 deletions(-) diff --git a/test/integration/smoke/test_scale_vm.py b/test/integration/smoke/test_scale_vm.py index 0815dc640d11..7f8b65b84656 100644 --- a/test/integration/smoke/test_scale_vm.py +++ b/test/integration/smoke/test_scale_vm.py @@ -31,6 +31,7 @@ Volume) from marvin.lib.common import (get_zone, get_template, + get_test_template, get_domain) from nose.plugins.attrib import attr from marvin.sshClient import SshClient @@ -75,14 +76,19 @@ def setUpClass(cls): isdynamicallyscalable='true' ) else: - cls.template = Template.register( - cls.apiclient, - cls.services["CentOS7template"], - zoneid=cls.zone.id - ) - cls._cleanup.append(cls.template) - cls.template.download(cls.apiclient) - time.sleep(60) + cls.template = get_test_template( + cls.apiclient, + cls.zone.id, + cls.hypervisor + ) + if cls.template == FAILED: + assert False, "get_test_template() failed to return template\ + with hypervisor %s" % cls.hypervisor + cls.template = Template.update( + cls.template, + cls.apiclient, + isdynamicallyscalable='true' + ) # Set Zones and disk offerings cls.services["small"]["zoneid"] = cls.zone.id @@ -126,37 +132,6 @@ def setUpClass(cls): dynamicscalingenabled=False ) - # create a virtual machine - cls.virtual_machine = VirtualMachine.create( - cls.apiclient, - cls.services["small"], - accountid=cls.account.name, - domainid=cls.account.domainid, - serviceofferingid=cls.small_offering.id, - mode=cls.services["mode"] - ) - - # create a virtual machine which cannot be dynamically scalable - cls.virtual_machine_with_service_offering_dynamic_scaling_disabled = VirtualMachine.create( - cls.apiclient, - cls.services["small"], - accountid=cls.account.name, - domainid=cls.account.domainid, - serviceofferingid=cls.small_offering_dynamic_scaling_disabled.id, - mode=cls.services["mode"] - ) - - # create a virtual machine which cannot be dynamically scalable - cls.virtual_machine_not_dynamically_scalable = VirtualMachine.create( - cls.apiclient, - cls.services["small"], - accountid=cls.account.name, - domainid=cls.account.domainid, - serviceofferingid=cls.small_offering.id, - mode=cls.services["mode"], - dynamicscalingenabled=False - ) - cls._cleanup = [ cls.small_offering, cls.big_offering, @@ -207,6 +182,9 @@ def get_ssh_client(self, ip, username, password, retries=10): return ssh_client + def is_host_xcpng8(self, hostname): + return type(hostname) == list and len(hostname) > 0 and 'XCP-ng 8' in hostname[0] + @attr(hypervisor="xenserver") @attr(tags=["advanced", "basic"], required_hardware="false") def test_01_scale_vm(self): @@ -224,6 +202,17 @@ def test_01_scale_vm(self): # scaling is not # guaranteed until tools are installed, vm rebooted + # create a virtual machine + self.virtual_machine = VirtualMachine.create( + self.apiclient, + self.services["small"], + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.small_offering.id, + mode=self.services["mode"] + ) + self.cleanup.append(self.virtual_machine) + # If hypervisor is Vmware, then check if # the vmware tools are installed and the process is running # Vmware tools are necessary for scale VM operation @@ -235,6 +224,7 @@ def test_01_scale_vm(self): if not "running" in result: self.skipTest("Skipping scale VM operation because\ VMware tools are not installed on the VM") + res = None if self.hypervisor.lower() != 'simulator': hostid = self.virtual_machine.hostid host = Host.list( @@ -259,14 +249,27 @@ def test_01_scale_vm(self): self.apiclient, isdynamicallyscalable='true') + if self.is_host_xcpng8(res): + self.debug("Only scaling for CPU for XCP-ng 8") + offering_data = self.services["service_offerings"]["big"] + offering_data["cpunumber"] = 2 + offering_data["memory"] = self.virtual_machine.memory + self.bigger_offering = ServiceOffering.create( + self.apiclient, + offering_data + ) + self.cleanup.append(self.bigger_offering) + else: + self.bigger_offering = self.big_offering + self.debug("Scaling VM-ID: %s to service offering: %s and state %s" % ( self.virtual_machine.id, - self.big_offering.id, + self.bigger_offering.id, self.virtual_machine.state )) cmd = scaleVirtualMachine.scaleVirtualMachineCmd() - cmd.serviceofferingid = self.big_offering.id + cmd.serviceofferingid = self.bigger_offering.id cmd.id = self.virtual_machine.id try: @@ -305,11 +308,11 @@ def test_01_scale_vm(self): offering %s and the response says %s" % (self.virtual_machine.id, self.virtual_machine.serviceofferingid, - self.big_offering.id, + self.bigger_offering.id, vm_response.serviceofferingid)) self.assertEqual( vm_response.serviceofferingid, - self.big_offering.id, + self.bigger_offering.id, "Check service offering of the VM" ) @@ -321,7 +324,7 @@ def test_01_scale_vm(self): return @attr(tags=["advanced", "basic"], required_hardware="false") - def test_02_scale_vm(self): + def test_02_scale_vm_negative_offering_disable_scaling(self): """Test scale virtual machine which is created from a service offering for which dynamicscalingenabled is false. Scaling operation should fail. """ @@ -333,6 +336,17 @@ def test_02_scale_vm(self): # scaling is not # guaranteed until tools are installed, vm rebooted + # create a virtual machine which cannot be dynamically scalable + self.virtual_machine_with_service_offering_dynamic_scaling_disabled = VirtualMachine.create( + self.apiclient, + self.services["small"], + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.small_offering_dynamic_scaling_disabled.id, + mode=self.services["mode"] + ) + self.cleanup.append(self.virtual_machine_with_service_offering_dynamic_scaling_disabled) + # If hypervisor is Vmware, then check if # the vmware tools are installed and the process is running # Vmware tools are necessary for scale VM operation @@ -376,7 +390,7 @@ def test_02_scale_vm(self): self.debug("Scaling VM-ID: %s to service offering: %s for which dynamic scaling is disabled and VM state %s" % ( self.virtual_machine_with_service_offering_dynamic_scaling_disabled.id, self.big_offering_dynamic_scaling_disabled.id, - self.virtual_machine.state + self.virtual_machine_with_service_offering_dynamic_scaling_disabled.state )) cmd = scaleVirtualMachine.scaleVirtualMachineCmd() @@ -396,7 +410,7 @@ def test_02_scale_vm(self): self.debug("Scaling VM-ID: %s to service offering: %s for which dynamic scaling is enabled and VM state %s" % ( self.virtual_machine_with_service_offering_dynamic_scaling_disabled.id, self.big_offering.id, - self.virtual_machine.state + self.virtual_machine_with_service_offering_dynamic_scaling_disabled.state )) cmd = scaleVirtualMachine.scaleVirtualMachineCmd() @@ -416,7 +430,7 @@ def test_02_scale_vm(self): return @attr(tags=["advanced", "basic"], required_hardware="false") - def test_03_scale_vm(self): + def test_03_scale_vm_negative_vm_disable_scaling(self): """Test scale virtual machine which is not dynamically scalable to a service offering. Scaling operation should fail. """ # Validate the following @@ -430,6 +444,18 @@ def test_03_scale_vm(self): # scaling is not # guaranteed until tools are installed, vm rebooted + # create a virtual machine which cannot be dynamically scalable + self.virtual_machine_not_dynamically_scalable = VirtualMachine.create( + self.apiclient, + self.services["small"], + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.small_offering.id, + mode=self.services["mode"], + dynamicscalingenabled=False + ) + self.cleanup.append(self.virtual_machine_not_dynamically_scalable) + # If hypervisor is Vmware, then check if # the vmware tools are installed and the process is running # Vmware tools are necessary for scale VM operation @@ -471,7 +497,7 @@ def test_03_scale_vm(self): self.debug("Scaling VM-ID: %s to service offering: %s for which dynamic scaling is enabled and VM state %s" % ( self.virtual_machine_not_dynamically_scalable.id, self.big_offering.id, - self.virtual_machine.state + self.virtual_machine_not_dynamically_scalable.state )) cmd = scaleVirtualMachine.scaleVirtualMachineCmd() @@ -532,6 +558,7 @@ def test_04_scale_vm_with_user_account(self): if not "running" in result: self.skipTest("Skipping scale VM operation because\ VMware tools are not installed on the VM") + res = None if self.hypervisor.lower() != 'simulator': list_vm_response = VirtualMachine.list( self.apiclient, @@ -560,14 +587,27 @@ def test_04_scale_vm_with_user_account(self): self.userapiclient, isdynamicallyscalable='true') + if self.is_host_xcpng8(res): + self.debug("Only scaling for CPU for XCP-ng 8") + offering_data = self.services["service_offerings"]["big"] + offering_data["cpunumber"] = 2 + offering_data["memory"] = self.virtual_machine_in_user_account.memory + self.bigger_offering = ServiceOffering.create( + self.apiclient, + offering_data + ) + self.cleanup.append(self.bigger_offering) + else: + self.bigger_offering = self.big_offering + self.debug("Scaling VM-ID: %s to service offering: %s and state %s" % ( self.virtual_machine_in_user_account.id, - self.big_offering.id, + self.bigger_offering.id, self.virtual_machine_in_user_account.state )) cmd = scaleVirtualMachine.scaleVirtualMachineCmd() - cmd.serviceofferingid = self.big_offering.id + cmd.serviceofferingid = self.bigger_offering.id cmd.id = self.virtual_machine_in_user_account.id try: @@ -590,11 +630,11 @@ def test_04_scale_vm_with_user_account(self): offering %s and the response says %s" % (self.virtual_machine_in_user_account.id, self.virtual_machine_in_user_account.serviceofferingid, - self.big_offering.id, + self.bigger_offering.id, vm_response.serviceofferingid)) self.assertEqual( vm_response.serviceofferingid, - self.big_offering.id, + self.bigger_offering.id, "Check service offering of the VM" ) @@ -663,6 +703,7 @@ def test_05_scale_vm_dont_allow_disk_offering_change(self): if not "running" in result: self.skipTest("Skipping scale VM operation because\ VMware tools are not installed on the VM") + res = None if self.hypervisor.lower() != 'simulator': hostid = self.virtual_machine_test.hostid host = Host.list( @@ -700,6 +741,9 @@ def test_05_scale_vm_dont_allow_disk_offering_change(self): 'memory': 1024, 'diskofferingid': self.disk_offering2.id } + if self.is_host_xcpng8(res): + self.debug("Only scaling for CPU for XCP-ng 8") + offering_data["memory"] = self.virtual_machine_test.memory self.ServiceOffering2WithDiskOffering2 = ServiceOffering.create( self.apiclient, @@ -761,10 +805,21 @@ def test_05_scale_vm_dont_allow_disk_offering_change(self): ) # Do same scale vm operation with allow.diskOffering.change.during.scale.vm value to true - + if self.hypervisor.lower() in ['simulator']: + self.debug("Simulator doesn't support changing disk offering, volume resize") + return + disk_offering_data = self.services["disk_offering"] + if self.hypervisor.lower() in ['xenserver']: + self.debug("For hypervisor %s, do not resize volume and just change try to change the disk offering") + volume_response = Volume.list( + self.apiclient, + virtualmachineid=self.virtual_machine_test.id, + listall=True + )[0] + disk_offering_data["disksize"] = int(volume_response.size / (1024 ** 3)) self.disk_offering3 = DiskOffering.create( self.apiclient, - self.services["disk_offering"], + disk_offering_data, ) self._cleanup.append(self.disk_offering3) offering_data = { @@ -775,6 +830,9 @@ def test_05_scale_vm_dont_allow_disk_offering_change(self): 'memory': 1024, 'diskofferingid': self.disk_offering3.id } + if self.is_host_xcpng8(res): + self.debug("Only scaling for CPU for XCP-ng 8") + offering_data["memory"] = vm_response.memory self.ServiceOffering3WithDiskOffering3 = ServiceOffering.create( self.apiclient, @@ -839,4 +897,4 @@ def test_05_scale_vm_dont_allow_disk_offering_change(self): "Check disk offering of the VM, this should change since allow.diskOffering.change.during.scale.vm is true" ) - return \ No newline at end of file + return From e75446e684601cdeab12cb015985d0af278f4808 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Tue, 29 Nov 2022 13:33:50 +0530 Subject: [PATCH 5/8] Update server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com> --- .../java/com/cloud/storage/VolumeApiServiceImpl.java | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 46e59ea04e3c..b6b452e9d4d3 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1832,15 +1832,10 @@ private Volume changeDiskOfferingForVolumeInternal(VolumeVO volume, Long newDisk * @return true if a and b are equal excluding 0 and null values. */ private boolean compareEqualsIncludingNullOrZero(Long a, Long b) { - if ((a != null && a == 0 && b == null) || (a == null && b != null && b == 0)) { - return true; - } - - if (a == b) { - return true; - } + a = ObjectUtils.defaultIfNull(a, 0L); + b = ObjectUtils.defaultIfNull(b, 0L); - return false; + return a.equals(b); } /** From 08a5769d7133c4033bbe26105eb621b9b6838928 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Tue, 29 Nov 2022 13:36:53 +0530 Subject: [PATCH 6/8] fix import Signed-off-by: Rohit Yadav --- server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index b6b452e9d4d3..bbebc7a2f3bb 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -96,6 +96,7 @@ import org.apache.cloudstack.utils.volume.VirtualMachineDiskInfo; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; +import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.builder.ToStringBuilder; import org.apache.commons.lang3.builder.ToStringStyle; From 1860068644c1935d7b1f554d99b82cd1181f222e Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Wed, 30 Nov 2022 12:47:27 +0530 Subject: [PATCH 7/8] Update server/src/main/java/com/cloud/vm/UserVmManager.java --- server/src/main/java/com/cloud/vm/UserVmManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java b/server/src/main/java/com/cloud/vm/UserVmManager.java index 8e2e87915de4..4449dc54860b 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManager.java +++ b/server/src/main/java/com/cloud/vm/UserVmManager.java @@ -47,7 +47,7 @@ */ public interface UserVmManager extends UserVmService { String EnableDynamicallyScaleVmCK = "enable.dynamic.scale.vm"; - String AllowDiskOfferingChangeDuringScaleVmCK = "allow.diskOffering.change.during.scale.vm"; + String AllowDiskOfferingChangeDuringScaleVmCK = "allow.diskoffering.change.during.scale.vm"; String AllowUserExpungeRecoverVmCK ="allow.user.expunge.recover.vm"; ConfigKey EnableDynamicallyScaleVm = new ConfigKey("Advanced", Boolean.class, EnableDynamicallyScaleVmCK, "false", "Enables/Disables dynamically scaling a vm", true, ConfigKey.Scope.Zone); From 24e7cb21b92a609b6466196495bcd56bbf746de5 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Wed, 30 Nov 2022 12:48:07 +0530 Subject: [PATCH 8/8] Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com> --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 74b76c685b9a..3e17cb75809a 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -2040,7 +2040,7 @@ private void changeDiskOfferingForRootVolume(Long vmId, DiskOfferingVO newDiskOf if (!AllowDiskOfferingChangeDuringScaleVm.valueIn(zoneId)) { if (s_logger.isDebugEnabled()) { - s_logger.debug(String.format("Disk Offering change for the root volume is disabled during the compute offering change operation. Please check the setting %s", AllowDiskOfferingChangeDuringScaleVm.key())); + s_logger.debug(String.format("Changing the disk offering of the root volume during the compute offering change operation is disabled. Please check the setting [%s].", AllowDiskOfferingChangeDuringScaleVm.key())); } return; }