From 56f0448f0d46e95f50047636000a73fc754fd0be Mon Sep 17 00:00:00 2001 From: Rene Peinthor Date: Thu, 8 Feb 2024 19:27:38 +0100 Subject: [PATCH 1/2] Linstor fix migration while node offline (#8610) * linstor: Add util method getBestErrorMessage from main * linstor: failed remove of allow-two-primaries is no fatal error * linstor: Fix failure if a Linstor node is down while migrating If a Linstor node is down while migrating resource, allow-two-primaries setting will fail because we can't reach the downed node. But it will still set the property on the other nodes and migration should work. We now just report an error instead of completely failing. --- .../kvm/storage/LinstorStorageAdaptor.java | 28 +++++++++++++------ .../storage/datastore/util/LinstorUtil.java | 11 ++++++++ 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java b/plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java index 3a703cdb426..426145d9dcc 100644 --- a/plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java +++ b/plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java @@ -269,27 +269,35 @@ public class LinstorStorageAdaptor implements StorageAdaptor { } final DevelopersApi api = getLinstorAPI(pool); + String rscName; try { - final String rscName = getLinstorRscName(volumePath); + rscName = getLinstorRscName(volumePath); ResourceMakeAvailable rma = new ResourceMakeAvailable(); ApiCallRcList answers = api.resourceMakeAvailableOnNode(rscName, localNodeName, rma); checkLinstorAnswersThrow(answers); + } catch (ApiException apiEx) { + s_logger.error(apiEx); + throw new CloudRuntimeException(apiEx.getBestMessage(), apiEx); + } + + try + { // allow 2 primaries for live migration, should be removed by disconnect on the other end ResourceDefinitionModify rdm = new ResourceDefinitionModify(); Properties props = new Properties(); props.put("DrbdOptions/Net/allow-two-primaries", "yes"); rdm.setOverrideProps(props); - answers = api.resourceDefinitionModify(rscName, rdm); + ApiCallRcList answers = api.resourceDefinitionModify(rscName, rdm); if (answers.hasError()) { s_logger.error("Unable to set 'allow-two-primaries' on " + rscName); - throw new CloudRuntimeException(answers.get(0).getMessage()); + // do not fail here as adding allow-two-primaries property is only a problem while live migrating } } catch (ApiException apiEx) { s_logger.error(apiEx); - throw new CloudRuntimeException(apiEx.getBestMessage(), apiEx); + // do not fail here as adding allow-two-primaries property is only a problem while live migrating } return true; } @@ -353,19 +361,21 @@ public class LinstorStorageAdaptor implements StorageAdaptor { ApiCallRcList answers = api.resourceDefinitionModify(rsc.get().getName(), rdm); if (answers.hasError()) { - s_logger.error("Failed to remove 'allow-two-primaries' on " + rsc.get().getName()); - throw new CloudRuntimeException(answers.get(0).getMessage()); + s_logger.error( + String.format("Failed to remove 'allow-two-primaries' on %s: %s", + rsc.get().getName(), LinstorUtil.getBestErrorMessage(answers))); + // do not fail here as removing allow-two-primaries property isn't fatal } return true; } s_logger.warn("Linstor: Couldn't find resource for this path: " + localPath); } catch (ApiException apiEx) { - s_logger.error(apiEx); - throw new CloudRuntimeException(apiEx.getBestMessage(), apiEx); + s_logger.error(apiEx.getBestMessage()); + // do not fail here as removing allow-two-primaries property isn't fatal } } - return false; + return true; } @Override diff --git a/plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java b/plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java index ddd15a5984a..cc85c9834eb 100644 --- a/plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java +++ b/plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java @@ -20,6 +20,8 @@ import com.linbit.linstor.api.ApiClient; import com.linbit.linstor.api.ApiException; import com.linbit.linstor.api.Configuration; import com.linbit.linstor.api.DevelopersApi; +import com.linbit.linstor.api.model.ApiCallRc; +import com.linbit.linstor.api.model.ApiCallRcList; import com.linbit.linstor.api.model.ProviderKind; import com.linbit.linstor.api.model.ResourceGroup; import com.linbit.linstor.api.model.StoragePool; @@ -47,6 +49,15 @@ public class LinstorUtil { return new DevelopersApi(client); } + public static String getBestErrorMessage(ApiCallRcList answers) { + return answers != null && !answers.isEmpty() ? + answers.stream() + .filter(ApiCallRc::isError) + .findFirst() + .map(ApiCallRc::getMessage) + .orElse((answers.get(0)).getMessage()) : null; + } + public static long getCapacityBytes(String linstorUrl, String rscGroupName) { DevelopersApi linstorApi = getLinstorAPI(linstorUrl); try { From 1d1b33214162eb42870f7d543e7a96f433c60721 Mon Sep 17 00:00:00 2001 From: slavkap <51903378+slavkap@users.noreply.github.com> Date: Thu, 8 Feb 2024 20:35:34 +0200 Subject: [PATCH 2/2] remove StorPool tags from detached volumes (#8377) * remove tags from detached volumes * Adress comments * address comments * Address comments --- .../StorPoolPrimaryDataStoreDriver.java | 16 +++- .../storage/datastore/util/StorPoolUtil.java | 9 ++- .../snapshot/StorPoolVMSnapshotStrategy.java | 2 +- .../plugins/storpool/TestTagsOnStorPool.py | 75 +++++++++++++++---- tools/marvin/marvin/lib/base.py | 6 +- 5 files changed, 86 insertions(+), 22 deletions(-) diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java index d42a2ba0a35..08a3252d869 100644 --- a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java +++ b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java @@ -190,6 +190,15 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver { @Override public void revokeAccess(DataObject data, Host host, DataStore dataStore) { + if (DataObjectType.VOLUME == data.getType()) { + final VolumeVO volume = volumeDao.findById(data.getId()); + if (volume.getInstanceId() == null) { + StorPoolUtil.spLog("Removing tags from detached volume=%s", volume.toString()); + SpConnectionDesc conn = StorPoolUtil.getSpConnection(dataStore.getUuid(), dataStore.getId(), storagePoolDetailsDao, primaryStoreDao); + StorPoolUtil.volumeRemoveTags(StorPoolStorageAdaptor.getVolumeNameFromPath(volume.getPath(), true), conn); + } + } + } private void updateStoragePool(final long poolId, final long deltaUsedBytes) { @@ -1038,7 +1047,7 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver { } if (vinfo.getMaxIops() != null) { - response = StorPoolUtil.volumeUpdateTags(volumeName, null, vinfo.getMaxIops(), conn, null); + response = StorPoolUtil.volumeUpdateIopsAndTags(volumeName, null, vinfo.getMaxIops(), conn, null); if (response.getError() != null) { StorPoolUtil.spLog("Volume was reverted successfully but max iops could not be set due to %s", response.getError().getDescr()); } @@ -1127,13 +1136,16 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver { @Override public void provideVmInfo(long vmId, long volumeId) { VolumeVO volume = volumeDao.findById(volumeId); + if (volume.getInstanceId() == null) { + return; + } StoragePoolVO poolVO = primaryStoreDao.findById(volume.getPoolId()); if (poolVO != null) { try { SpConnectionDesc conn = StorPoolUtil.getSpConnection(poolVO.getUuid(), poolVO.getId(), storagePoolDetailsDao, primaryStoreDao); String volName = StorPoolStorageAdaptor.getVolumeNameFromPath(volume.getPath(), true); VMInstanceVO userVM = vmInstanceDao.findById(vmId); - SpApiResponse resp = StorPoolUtil.volumeUpdateTags(volName, volume.getInstanceId() != null ? userVM.getUuid() : "", null, conn, getVcPolicyTag(vmId)); + SpApiResponse resp = StorPoolUtil.volumeUpdateIopsAndTags(volName, volume.getInstanceId() != null ? userVM.getUuid() : "", null, conn, getVcPolicyTag(vmId)); if (resp.getError() != null) { log.warn(String.format("Could not update VC policy tags of a volume with id [%s]", volume.getUuid())); } diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java index e176d67c12d..675dffbda5b 100644 --- a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java +++ b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java @@ -533,7 +533,14 @@ public class StorPoolUtil { return POST("MultiCluster/VolumeUpdate/" + name, json, conn); } - public static SpApiResponse volumeUpdateTags(final String name, final String uuid, Long iops, + public static SpApiResponse volumeRemoveTags(String name, SpConnectionDesc conn) { + Map json = new HashMap<>(); + Map tags = StorPoolHelper.addStorPoolTags(null, "", null, ""); + json.put("tags", tags); + return POST("MultiCluster/VolumeUpdate/" + name, json, conn); + } + + public static SpApiResponse volumeUpdateIopsAndTags(final String name, final String uuid, Long iops, SpConnectionDesc conn, String vcPolicy) { Map json = new HashMap<>(); Map tags = StorPoolHelper.addStorPoolTags(null, uuid, null, vcPolicy); diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolVMSnapshotStrategy.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolVMSnapshotStrategy.java index 1172600c342..489f64f9025 100644 --- a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolVMSnapshotStrategy.java +++ b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolVMSnapshotStrategy.java @@ -335,7 +335,7 @@ public class StorPoolVMSnapshotStrategy extends DefaultVMSnapshotStrategy { } VolumeInfo vinfo = volFactory.getVolume(volumeObjectTO.getId()); if (vinfo.getMaxIops() != null) { - resp = StorPoolUtil.volumeUpdateTags(volumeName, null, vinfo.getMaxIops(), conn, null); + resp = StorPoolUtil.volumeUpdateIopsAndTags(volumeName, null, vinfo.getMaxIops(), conn, null); if (resp.getError() != null) { StorPoolUtil.spLog("Volume was reverted successfully but max iops could not be set due to %s", diff --git a/test/integration/plugins/storpool/TestTagsOnStorPool.py b/test/integration/plugins/storpool/TestTagsOnStorPool.py index 6d13e2081d9..a66fb3570f4 100644 --- a/test/integration/plugins/storpool/TestTagsOnStorPool.py +++ b/test/integration/plugins/storpool/TestTagsOnStorPool.py @@ -218,6 +218,19 @@ class TestStoragePool(cloudstackTestCase): hypervisor=cls.hypervisor, rootdisksize=10 ) + cls.virtual_machine3 = VirtualMachine.create( + cls.apiclient, + {"name":"StorPool-%s" % uuid.uuid4() }, + zoneid=cls.zone.id, + templateid=template.id, + accountid=cls.account.name, + domainid=cls.account.domainid, + serviceofferingid=cls.service_offering.id, + hypervisor=cls.hypervisor, + diskofferingid=cls.disk_offerings.id, + size=2, + rootdisksize=10 + ) cls.template = template cls.random_data_0 = random_gen(size=100) cls.test_dir = "/tmp" @@ -270,7 +283,7 @@ class TestStoragePool(cloudstackTestCase): virtualmachineid = self.virtual_machine.id, listall=True ) - self.vc_policy_tags(volumes, vm_tags, vm) + self.vc_policy_tags(volumes, vm_tags, vm, True) @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true") @@ -310,7 +323,7 @@ class TestStoragePool(cloudstackTestCase): vm = list_virtual_machines(self.apiclient,id = self.virtual_machine.id, listall=True) vm_tags = vm[0].tags - self.vc_policy_tags(volumes, vm_tags, vm) + self.vc_policy_tags(volumes, vm_tags, vm, True) self.assertEqual(volume_attached.id, self.volume.id, "Is not the same volume ") @@ -442,7 +455,7 @@ class TestStoragePool(cloudstackTestCase): vm = list_virtual_machines(self.apiclient,id = self.virtual_machine.id, listall=True) vm_tags = vm[0].tags - self.vc_policy_tags(volumes, vm_tags, vm) + self.vc_policy_tags(volumes, vm_tags, vm, True) self.assertEqual( self.random_data_0, @@ -490,18 +503,17 @@ class TestStoragePool(cloudstackTestCase): def test_06_remove_vcpolicy_tag_when_disk_detached(self): """ Test remove vc-policy tag to disk detached from VM""" time.sleep(60) - volume_detached = self.virtual_machine.detach_volume( - self.apiclient, - self.volume_2 - ) vm = list_virtual_machines(self.apiclient,id = self.virtual_machine.id, listall=True) vm_tags = vm[0].tags volumes = list_volumes( self.apiclient, - virtualmachineid = self.virtual_machine.id, listall=True + id= self.volume_2.id, listall=True, ) - - self.vc_policy_tags( volumes, vm_tags, vm) + volume_detached = self.virtual_machine.detach_volume( + self.apiclient, + self.volume_2 + ) + self.vc_policy_tags( volumes, vm_tags, vm, False) @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true") def test_07_delete_vcpolicy_tag(self): @@ -538,7 +550,7 @@ class TestStoragePool(cloudstackTestCase): virtualmachineid = self.virtual_machine2.id, listall=True, type = "ROOT" ) - self.vc_policy_tags(volume, vm_tags, vm) + self.vc_policy_tags(volume, vm_tags, vm, True) snapshot = Snapshot.create( self.apiclient, @@ -560,11 +572,36 @@ class TestStoragePool(cloudstackTestCase): vm_tags = vm[0].tags vol = list_volumes(self.apiclient, id = snapshot.volumeid, listall=True) - self.vc_policy_tags(vol, vm_tags, vm) + self.vc_policy_tags(vol, vm_tags, vm, True) + @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true") + def test_09_remove_vm_tags_on_datadisks_attached_to_destroyed_vm(self): + tag = Tag.create( + self.apiclient, + resourceIds=self.virtual_machine3.id, + resourceType='UserVm', + tags={'vc-policy': 'testing_vc-policy'} + ) + vm = list_virtual_machines(self.apiclient,id = self.virtual_machine3.id, listall=True) + vm_tags = vm[0].tags + volumes = list_volumes( + self.apiclient, + virtualmachineid = self.virtual_machine3.id, listall=True + ) - def vc_policy_tags(self, volumes, vm_tags, vm): - flag = False + self.vc_policy_tags(volumes, vm_tags, vm, True) + + volumes = list_volumes( + self.apiclient, + virtualmachineid = self.virtual_machine3.id, listall=True, type="DATADISK" + ) + self.virtual_machine3.delete(self.apiclient, expunge=True) + + self.vc_policy_tags(volumes, vm_tags, vm, False) + + def vc_policy_tags(self, volumes, vm_tags, vm, should_tags_exists=None): + vcPolicyTag = False + cvmTag = False for v in volumes: name = v.path.split("/")[3] spvolume = self.spapi.volumeList(volumeName="~" + name) @@ -572,9 +609,15 @@ class TestStoragePool(cloudstackTestCase): for t in tags: for vm_tag in vm_tags: if t == vm_tag.key: - flag = True + vcPolicyTag = True self.assertEqual(tags[t], vm_tag.value, "Tags are not equal") if t == 'cvm': + cvmTag = True self.assertEqual(tags[t], vm[0].id, "CVM tag is not the same as vm UUID") #self.assertEqual(tag.tags., second, msg) - self.assertTrue(flag, "There aren't volumes with vm tags") + if should_tags_exists: + self.assertTrue(vcPolicyTag, "There aren't volumes with vm tags") + self.assertTrue(cvmTag, "There aren't volumes with vm tags") + else: + self.assertFalse(vcPolicyTag, "The tags should be removed") + self.assertFalse(cvmTag, "The tags should be removed") diff --git a/tools/marvin/marvin/lib/base.py b/tools/marvin/marvin/lib/base.py index f9b6d53626f..98923775fe6 100755 --- a/tools/marvin/marvin/lib/base.py +++ b/tools/marvin/marvin/lib/base.py @@ -527,7 +527,7 @@ class VirtualMachine: customcpuspeed=None, custommemory=None, rootdisksize=None, rootdiskcontroller=None, vpcid=None, macaddress=None, datadisktemplate_diskoffering_list={}, properties=None, nicnetworklist=None, bootmode=None, boottype=None, dynamicscalingenabled=None, - userdataid=None, userdatadetails=None, extraconfig=None): + userdataid=None, userdatadetails=None, extraconfig=None, size=None): """Create the instance""" cmd = deployVirtualMachine.deployVirtualMachineCmd() @@ -649,7 +649,9 @@ class VirtualMachine: if rootdiskcontroller: cmd.details[0]["rootDiskController"] = rootdiskcontroller - if "size" in services: + if size: + cmd.size = size + elif "size" in services: cmd.size = services["size"] if group: