Merge remote-tracking branch 'origin/4.19'

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

Author: rohityadavcloud

plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java

@@ -276,27 +276,35 @@ public boolean connectPhysicalDisk(String volumePath, KVMStoragePool pool, Map<S
         }
 
         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()) {
                 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) {
             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;
     }
@@ -358,21 +366,22 @@ public boolean disconnectPhysicalDiskByPath(String localPath)
                     ResourceDefinitionModify rdm = new ResourceDefinitionModify();
                     rdm.deleteProps(Collections.singletonList("DrbdOptions/Net/allow-two-primaries"));
                     ApiCallRcList answers = api.resourceDefinitionModify(rsc.get().getName(), rdm);
-                    if (answers.hasError())
-                    {
-                        logger.error("Failed to remove 'allow-two-primaries' on " + rsc.get().getName());
-                        throw new CloudRuntimeException(answers.get(0).getMessage());
+                    if (answers.hasError()) {
+                        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;
                 }
                 logger.warn("Linstor: Couldn't find resource for this path: " + localPath);
             } catch (ApiException apiEx) {
-                logger.error(apiEx);
-                throw new CloudRuntimeException(apiEx.getBestMessage(), apiEx);
+                logger.error(apiEx.getBestMessage());
+                // do not fail here as removing allow-two-primaries property isn't fatal
             }
         }
-        return false;
+        return true;
     }
 
     @Override

plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java

@@ -191,6 +191,15 @@ public boolean grantAccess(DataObject data, Host host, DataStore dataStore) {
 
     @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) {
@@ -1039,7 +1048,7 @@ public void revertSnapshot(final SnapshotInfo snapshot, final SnapshotInfo snaps
         }
 
         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());
             }
@@ -1128,13 +1137,16 @@ public boolean isVmInfoNeeded() {
     @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) {
                     logger.warn(String.format("Could not update VC policy tags of a volume with id [%s]", volume.getUuid()));
                 }

plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java

@@ -534,7 +534,14 @@ public static SpApiResponse volumeUpdate(final String name, final Long newSize,
         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<String, Object> json = new HashMap<>();
+        Map<String, String> 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<String, Object> json = new HashMap<>();
         Map<String, String> tags = StorPoolHelper.addStorPoolTags(null, uuid, null, vcPolicy);

plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolVMSnapshotStrategy.java

@@ -333,7 +333,7 @@ public boolean revertVMSnapshot(VMSnapshot vmSnapshot) {
                 }
                 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",

test/integration/plugins/storpool/TestTagsOnStorPool.py

@@ -218,6 +218,19 @@ def setUpCloudStack(cls):
             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 @@ def test_01_set_vcpolicy_tag_to_vm_with_attached_disks(self):
             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 @@ def test_03_create_vm_snapshot_vc_policy_tag(self):
         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 @@ def test_04_revert_vm_snapshots_vc_policy_tag(self):
         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 @@ def test_05_delete_vm_snapshots(self):
     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 @@ def test_08_vcpolicy_tag_to_reverted_disk(self):
             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,21 +572,52 @@ def test_08_vcpolicy_tag_to_reverted_disk(self):
         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
+            )
 
+        self.vc_policy_tags(volumes, vm_tags, vm, True)
 
-    def vc_policy_tags(self, volumes, vm_tags, vm):
-        flag = False
+        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)
             tags = spvolume[0].tags
             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")

tools/marvin/marvin/lib/base.py

@@ -527,7 +527,7 @@ def create(cls, apiclient, services, templateid=None, accountid=None,
                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 @@ def create(cls, apiclient, services, templateid=None, accountid=None,
         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: