Adding device ID to a StorPool volume (#10587)

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

@@ -222,7 +222,6 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver {
                 StorPoolUtil.volumeRemoveTags(StorPoolStorageAdaptor.getVolumeNameFromPath(volume.getPath(), true), conn);
             }
         }
-
     }
 
     private void updateStoragePool(final long poolId, final long deltaUsedBytes) {
@@ -327,19 +326,14 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver {
             Long vmId, SpConnectionDesc conn) {
         SpApiResponse resp = new SpApiResponse();
         Map<String, String> tags = StorPoolHelper.addStorPoolTags(name, getVMInstanceUUID(vmId), "volume", getVcPolicyTag(vmId), tier);
-        if (tier != null || template != null) {
+        if (vinfo.getDeviceId() != null) {
-            StorPoolUtil.spLog(
+            tags.put("disk", vinfo.getDeviceId().toString());
-                    "Creating volume [%s] with template [%s] or tier tags [%s] described in disk/service offerings details",
-                    vinfo.getUuid(), template, tier);
-            resp = StorPoolUtil.volumeCreate(size, null, template, tags, conn);
-        } else {
-            StorPoolUtil.spLog(
-                    "StorpoolPrimaryDataStoreDriver.createAsync volume: name=%s, uuid=%s, isAttached=%s vm=%s, payload=%s, template: %s",
-                    vinfo.getName(), vinfo.getUuid(), vinfo.isAttachedVM(), vinfo.getAttachedVmName(),
-                    vinfo.getpayload(), conn.getTemplateName());
-            resp = StorPoolUtil.volumeCreate(name, null, size, getVMInstanceUUID(vinfo.getInstanceId()), null,
-                    "volume", vinfo.getMaxIops(), conn);
         }
+        if (template == null) {
+            template = conn.getTemplateName();
+        }
+        StorPoolVolumeDef volume = new StorPoolVolumeDef(null, size, tags, null, vinfo.getMaxIops(), template, null, null, null);
+        resp = StorPoolUtil.volumeCreate(volume, conn);
         return resp;
     }
 
@@ -827,20 +821,24 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver {
                     if (tier == null) {
                         template = getTemplateFromOfferingDetail(vinfo.getDiskOfferingId());
                     }
-                }
-
-                if (tier != null || template != null) {
-                    Map<String, String> tags = StorPoolHelper.addStorPoolTags(name, getVMInstanceUUID(vmId), "volume", getVcPolicyTag(vmId), tier);
-
                     StorPoolUtil.spLog(
                             "Creating volume [%s] with template [%s] or tier tags [%s] described in disk/service offerings details",
                             vinfo.getUuid(), template, tier);
-                    resp = StorPoolUtil.volumeCreate(size, parentName, template, tags, conn);
-                } else {
-                    resp = StorPoolUtil.volumeCreate(name, parentName, size, getVMInstanceUUID(vmId),
-                            getVcPolicyTag(vmId), "volume", vinfo.getMaxIops(), conn);
                 }
 
+                Map<String, String> tags = StorPoolHelper.addStorPoolTags(name, getVMInstanceUUID(vmId), "volume", getVcPolicyTag(vmId), tier);
+
+                if (vinfo.getDeviceId() != null) {
+                    tags.put("disk", vinfo.getDeviceId().toString());
+                }
+
+                if (template == null) {
+                    template = conn.getTemplateName();
+                }
+
+                StorPoolVolumeDef volumeDef = new StorPoolVolumeDef(null, size, tags, parentName, null, template, null, null, null);
+                resp = StorPoolUtil.volumeCreate(volumeDef, conn);
+
                 if (resp.getError() == null) {
                     updateStoragePool(dstData.getDataStore().getId(), vinfo.getSize());
                     updateVolumePoolType(vinfo);
@@ -1309,7 +1307,13 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver {
                 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.volumeUpdateIopsAndTags(volName, volume.getInstanceId() != null ? userVM.getUuid() : "", null, conn, getVcPolicyTag(vmId));
+                Map<String, String> tags = StorPoolHelper.addStorPoolTags(null, userVM.getUuid(), null, getVcPolicyTag(vmId), null);
+                if (volume.getDeviceId() != null) {
+                    tags.put("disk", volume.getDeviceId().toString());
+                }
+                StorPoolVolumeDef spVolume = new StorPoolVolumeDef(volName, null, tags, null, null, null, null, null, null);
+
+                SpApiResponse resp = StorPoolUtil.volumeUpdate(spVolume, conn);
                 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

@@ -520,6 +520,10 @@ public class StorPoolUtil {
         return POST("MultiCluster/VolumeCreate", json, conn);
     }
 
+    public static SpApiResponse volumeCreate(StorPoolVolumeDef volume, SpConnectionDesc conn) {
+        return POST("MultiCluster/VolumeCreate", volume, conn);
+    }
+
     public static SpApiResponse volumeCreate(SpConnectionDesc conn) {
         Map<String, Object> json = new LinkedHashMap<>();
         json.put("name", "");
@@ -568,6 +572,7 @@ public class StorPoolUtil {
     public static SpApiResponse volumeRemoveTags(String name, SpConnectionDesc conn) {
         Map<String, Object> json = new HashMap<>();
         Map<String, String> tags = StorPoolHelper.addStorPoolTags(null, "", null, "", null);
+        tags.put("disk", "");
         json.put("tags", tags);
         return POST("MultiCluster/VolumeUpdate/" + name, json, conn);
     }

test/integration/plugins/storpool/TestTagsOnStorPool.py

@@ -283,7 +283,7 @@ class TestStoragePool(cloudstackTestCase):
             virtualmachineid = self.virtual_machine.id, listall=True
             )
 
-        self.vc_policy_tags(volumes, vm_tags, vm, True)
+        self.vc_policy_tags(volumes, vm_tags, vm, should_tags_exists=True)
 
 
     @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true")
@@ -323,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, True)
+        self.vc_policy_tags(volumes, vm_tags, vm, should_tags_exists=True)
 
 
         self.assertEqual(volume_attached.id, self.volume.id, "Is not the same volume ")
@@ -455,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, True)
+        self.vc_policy_tags(volumes, vm_tags, vm, should_tags_exists=True)
 
         self.assertEqual(
             self.random_data_0,
@@ -491,14 +491,12 @@ class TestStoragePool(cloudstackTestCase):
 
         list_snapshot_response = VmSnapshot.list(
             self.apiclient,
-            #vmid=self.virtual_machine.id,
             virtualmachineid=self.virtual_machine.id,
             listall=False)
         self.debug('list_snapshot_response -------------------- %s' % list_snapshot_response)
 
         self.assertIsNone(list_snapshot_response, "snapshot is already deleted")
 
-
     @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true")
     def test_06_remove_vcpolicy_tag_when_disk_detached(self):
         """ Test remove vc-policy tag to disk detached from VM"""
@@ -513,7 +511,7 @@ class TestStoragePool(cloudstackTestCase):
                 self.apiclient,
                 self.volume_2
                 )
-        self.vc_policy_tags( volumes, vm_tags, vm, False)
+        self.vc_policy_tags( volumes, vm_tags, vm, should_tags_exists=False)
 
     @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true")
     def test_07_delete_vcpolicy_tag(self):
@@ -550,7 +548,7 @@ class TestStoragePool(cloudstackTestCase):
             virtualmachineid = self.virtual_machine2.id, listall=True,
             type = "ROOT"
             )
-        self.vc_policy_tags(volume, vm_tags, vm, True)
+        self.vc_policy_tags(volume, vm_tags, vm, should_tags_exists=True)
 
         snapshot = Snapshot.create(
             self.apiclient,
@@ -571,8 +569,8 @@ class TestStoragePool(cloudstackTestCase):
         vm = list_virtual_machines(self.apiclient,id = self.virtual_machine2.id)
         vm_tags = vm[0].tags
 
-        vol = list_volumes(self.apiclient, id = snapshot.volumeid, listall=True)
+        vol = list_volumes(self.apiclient, id=snapshot.volumeid, listall=True)
-        self.vc_policy_tags(vol, vm_tags, vm, True)
+        self.vc_policy_tags(vol, vm_tags, vm, should_tags_exists=True)
 
     @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true")
     def test_09_remove_vm_tags_on_datadisks_attached_to_destroyed_vm(self):
@@ -586,38 +584,82 @@ class TestStoragePool(cloudstackTestCase):
         vm_tags = vm[0].tags
         volumes = list_volumes(
             self.apiclient,
-            virtualmachineid = self.virtual_machine3.id, listall=True
+            virtualmachineid=self.virtual_machine3.id, listall=True
             )
 
-        self.vc_policy_tags(volumes, vm_tags, vm, True)
+        self.vc_policy_tags(volumes, vm_tags, vm, should_tags_exists=True)
 
         volumes = list_volumes(
             self.apiclient,
-            virtualmachineid = self.virtual_machine3.id, listall=True, type="DATADISK"
+            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)
+        self.vc_policy_tags(volumes, vm_tags, vm, should_tags_exists=False)
 
-    def vc_policy_tags(self, volumes, vm_tags, vm, should_tags_exists=None):
+    @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true")
-        vcPolicyTag = False
+    def test_10_check_tags_on_deployed_vm_with_data_disk(self):
-        cvmTag = False
+        """
+            Check disk and cvm tags are set on all volumes when VM is deployed with additional DATA disk
+            Detach the DATA disk
+        """
+        vm = VirtualMachine.create(
+            self.apiclient,
+            {"name":"StorPool-%s" % uuid.uuid4() },
+            zoneid=self.zone.id,
+            templateid=self.template.id,
+            accountid=self.account.name,
+            domainid=self.account.domainid,
+            serviceofferingid=self.service_offering.id,
+            hypervisor=self.hypervisor,
+            diskofferingid=self.disk_offerings.id,
+            size=2,
+            rootdisksize=10
+        )
+        volumes = list_volumes(
+            self.apiclient,
+            virtualmachineid=vm.id, listall=True
+            )
+        vm1 = list_virtual_machines(self.apiclient,id=vm.id, listall=True)
+        vm_tags = vm1[0].tags
+        self.vc_policy_tags(volumes, vm_tags, vm1, False, True)
+        vm.stop(self.apiclient, forced=True)
+        volumes = list_volumes(
+            self.apiclient,
+            virtualmachineid=vm.id, listall=True, type="DATADISK"
+            )
+
+        self.debug("detaching volume %s" % volumes)
+        VirtualMachine.detach_volume(vm, self.apiclient, volumes[0])
+        self.vc_policy_tags(volumes, vm_tags, vm1, False, False)
+
+    def vc_policy_tags(self, volumes, vm_tags, vm, tag_check=True, should_tags_exists=None,):
+        vc_policy_tag = False
+        cvm_tag = False
+        disk_id_tag = False
         for v in volumes:
             name = v.path.split("/")[3]
-            spvolume = self.spapi.volumeList(volumeName="~" + name)
+            volume = self.spapi.volumeList(volumeName="~" + name)
-            tags = spvolume[0].tags
+            tags = volume[0].tags
+            self.debug("Tags %s" % tags)
             for t in tags:
                 for vm_tag in vm_tags:
                     if t == vm_tag.key:
-                        vcPolicyTag = True
+                        vc_policy_tag = True
                         self.assertEqual(tags[t], vm_tag.value, "Tags are not equal")
-                    if t == 'cvm':
+                if t == 'cvm':
-                        cvmTag = True
+                    cvm_tag = True
-                        self.assertEqual(tags[t], vm[0].id, "CVM tag is not the same as vm UUID")
+                    self.assertEqual(tags[t], vm[0].id, "CVM tag is not the same as vm UUID")
-            #self.assertEqual(tag.tags., second, msg)
+                if t == 'disk':
+                    disk_id_tag = True
+                    self.assertEqual(tags[t], str(v.deviceid), "Disk tag is not equal to the device ID")
         if should_tags_exists:
-            self.assertTrue(vcPolicyTag, "There aren't volumes with vm tags")
+            if tag_check:
-            self.assertTrue(cvmTag, "There aren't volumes with vm tags")
+                self.assertTrue(vc_policy_tag, "There aren't volumes with vc policy tags")
+            self.assertTrue(cvm_tag, "There aren't volumes with vm UUID tags")
+            self.assertTrue(disk_id_tag, "There aren't volumes with vm disk tag")
         else:
-            self.assertFalse(vcPolicyTag, "The tags should be removed")
+            if tag_check:
-            self.assertFalse(cvmTag, "The tags should be removed")
+                self.assertFalse(vc_policy_tag, "The vc policy tag should be removed")
+            self.assertFalse(cvm_tag, "The cvm tag should be removed")
+            self.assertFalse(disk_id_tag, "The disk tag should be removed")