Add SharedMountPoint to KVMs supported storage pool types (#4780)

* Add SharedMountPoint to KVMs supported storage pool types

* Fix live migration to iSCSI and improve logs

Co-authored-by: Daniel Augusto Veronezi Salvador <daniel@scclouds.com.br>

engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java

@@ -88,7 +88,8 @@ public class KvmNonManagedStorageDataMotionStrategy extends StorageSystemDataMot
 
                 for (VolumeInfo volumeInfo : volumeInfoSet) {
                     StoragePoolVO storagePoolVO = _storagePoolDao.findById(volumeInfo.getPoolId());
-                    if (storagePoolVO.getPoolType() != StoragePoolType.Filesystem && storagePoolVO.getPoolType() != StoragePoolType.NetworkFilesystem) {
+
+                    if (!supportStoragePoolType(storagePoolVO.getPoolType())) {
                         return StrategyPriority.CANT_HANDLE;
                     }
                 }
@@ -187,7 +188,7 @@ public class KvmNonManagedStorageDataMotionStrategy extends StorageSystemDataMot
      */
     @Override
     protected boolean shouldMigrateVolume(StoragePoolVO sourceStoragePool, Host destHost, StoragePoolVO destStoragePool) {
-        return sourceStoragePool.getPoolType() == StoragePoolType.Filesystem || sourceStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem;
+        return supportStoragePoolType(sourceStoragePool.getPoolType());
     }
 
     /**
@@ -201,7 +202,7 @@ public class KvmNonManagedStorageDataMotionStrategy extends StorageSystemDataMot
         }
 
         VMTemplateStoragePoolVO sourceVolumeTemplateStoragePoolVO = vmTemplatePoolDao.findByPoolTemplate(destStoragePool.getId(), srcVolumeInfo.getTemplateId(), null);
-        if (sourceVolumeTemplateStoragePoolVO == null && destStoragePool.getPoolType() == StoragePoolType.Filesystem) {
+        if (sourceVolumeTemplateStoragePoolVO == null && (isStoragePoolTypeInList(destStoragePool.getPoolType(), StoragePoolType.Filesystem, StoragePoolType.SharedMountPoint))) {
             DataStore sourceTemplateDataStore = dataStoreManagerImpl.getRandomImageStore(srcVolumeInfo.getDataCenterId());
             if (sourceTemplateDataStore != null) {
                 TemplateInfo sourceTemplateInfo = templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(), sourceTemplateDataStore);
@@ -270,4 +271,8 @@ public class KvmNonManagedStorageDataMotionStrategy extends StorageSystemDataMot
             LOGGER.error(generateFailToCopyTemplateMessage(sourceTemplate, destDataStore) + failureDetails);
         }
     }
+
+    protected Boolean supportStoragePoolType(StoragePoolType storagePoolType) {
+        return super.supportStoragePoolType(storagePoolType, StoragePoolType.Filesystem);
+    }
 }

engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java

@@ -134,7 +134,10 @@ import com.cloud.vm.VirtualMachine;
 import com.cloud.vm.VirtualMachineManager;
 import com.cloud.vm.dao.VMInstanceDao;
 import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.HashSet;
 import java.util.stream.Collectors;
+import org.apache.commons.collections.CollectionUtils;
 
 public class StorageSystemDataMotionStrategy implements DataMotionStrategy {
     private static final Logger LOGGER = Logger.getLogger(StorageSystemDataMotionStrategy.class);
@@ -1861,9 +1864,8 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy {
 
                 MigrateCommand.MigrateDiskInfo migrateDiskInfo;
 
-                boolean isNonManagedNfsToNfs = sourceStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem
+                boolean isNonManagedNfsToNfsOrSharedMountPointToNfs = supportStoragePoolType(sourceStoragePool.getPoolType()) && destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && !managedStorageDestination;
-                        && destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && !managedStorageDestination;
+                if (isNonManagedNfsToNfsOrSharedMountPointToNfs) {
-                if (isNonManagedNfsToNfs) {
                     migrateDiskInfo = new MigrateCommand.MigrateDiskInfo(srcVolumeInfo.getPath(),
                             MigrateCommand.MigrateDiskInfo.DiskType.FILE,
                             MigrateCommand.MigrateDiskInfo.DriverType.QCOW2,
@@ -2897,4 +2899,27 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy {
 
         return copyCmdAnswer;
     }
+
+    protected Boolean supportStoragePoolType(StoragePoolType storagePoolTypeToValidate, StoragePoolType... extraAcceptedValues) {
+        List<StoragePoolType> values = new ArrayList<>();
+
+        values.add(StoragePoolType.NetworkFilesystem);
+        values.add(StoragePoolType.SharedMountPoint);
+
+        if (extraAcceptedValues != null) {
+            CollectionUtils.addAll(values, extraAcceptedValues);
+        }
+
+        return isStoragePoolTypeInList(storagePoolTypeToValidate, values.toArray(new StoragePoolType[values.size()]));
+    }
+
+    protected Boolean isStoragePoolTypeInList(StoragePoolType storagePoolTypeToValidate, StoragePoolType... acceptedValues){
+        Set<StoragePoolType> supportedTypes = new HashSet<>();
+
+        if (acceptedValues != null) {
+            supportedTypes.addAll(Arrays.asList(acceptedValues));
+        }
+
+        return supportedTypes.contains(storagePoolTypeToValidate);
+    };
 }

engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageSystemDataMotionTest.java

@@ -75,6 +75,10 @@ import com.cloud.storage.dao.DiskOfferingDao;
 import com.cloud.storage.dao.VMTemplatePoolDao;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.vm.VirtualMachineManager;
+import java.util.HashSet;
+import java.util.Set;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 @RunWith(MockitoJUnitRunner.class)
 public class KvmNonManagedStorageSystemDataMotionTest {
@@ -159,13 +163,23 @@ public class KvmNonManagedStorageSystemDataMotionTest {
         Assert.assertEquals(expectedStrategyPriority, strategyPriority);
     }
 
+    public Boolean supportStoragePoolType(StoragePoolType storagePoolType) {
+        Set<StoragePoolType> supportedTypes = new HashSet<>();
+        supportedTypes.add(StoragePoolType.Filesystem);
+        supportedTypes.add(StoragePoolType.NetworkFilesystem);
+        supportedTypes.add(StoragePoolType.SharedMountPoint);
+
+        return supportedTypes.contains(storagePoolType);
+    }
+
     @Test
     public void internalCanHandleTestNonManaged() {
         StoragePoolType[] storagePoolTypeArray = StoragePoolType.values();
         for (int i = 0; i < storagePoolTypeArray.length; i++) {
             Map<VolumeInfo, DataStore> volumeMap = configureTestInternalCanHandle(false, storagePoolTypeArray[i]);
             StrategyPriority strategyPriority = kvmNonManagedStorageDataMotionStrategy.internalCanHandle(volumeMap, new HostVO("sourceHostUuid"), new HostVO("destHostUuid"));
-            if (storagePoolTypeArray[i] == StoragePoolType.Filesystem || storagePoolTypeArray[i] == StoragePoolType.NetworkFilesystem) {
+
+            if (supportStoragePoolType(storagePoolTypeArray[i])) {
                 Assert.assertEquals(StrategyPriority.HYPERVISOR, strategyPriority);
             } else {
                 Assert.assertEquals(StrategyPriority.CANT_HANDLE, strategyPriority);
@@ -243,7 +257,7 @@ public class KvmNonManagedStorageSystemDataMotionTest {
         for (int i = 0; i < storagePoolTypes.length; i++) {
             Mockito.doReturn(storagePoolTypes[i]).when(sourceStoragePool).getPoolType();
             boolean result = kvmNonManagedStorageDataMotionStrategy.shouldMigrateVolume(sourceStoragePool, destHost, destStoragePool);
-            if (storagePoolTypes[i] == StoragePoolType.Filesystem || storagePoolTypes[i] == StoragePoolType.NetworkFilesystem) {
+            if (supportStoragePoolType(storagePoolTypes[i])) {
                 Assert.assertTrue(result);
             } else {
                 Assert.assertFalse(result);
@@ -472,4 +486,22 @@ public class KvmNonManagedStorageSystemDataMotionTest {
         lenient().when(pool2.isManaged()).thenReturn(false);
         kvmNonManagedStorageDataMotionStrategy.verifyLiveMigrationForKVM(migrationMap, host2);
     }
+
+    @Test
+    public void validateSupportStoragePoolType() {
+        Set<StoragePoolType> supportedTypes = new HashSet<>();
+        supportedTypes.add(StoragePoolType.Filesystem);
+        supportedTypes.add(StoragePoolType.NetworkFilesystem);
+        supportedTypes.add(StoragePoolType.SharedMountPoint);
+
+        for (StoragePoolType poolType : StoragePoolType.values()) {
+            boolean isSupported = kvmNonManagedStorageDataMotionStrategy.supportStoragePoolType(poolType);
+            if (supportedTypes.contains(poolType)) {
+                assertTrue(isSupported);
+            } else {
+                assertFalse(isSupported);
+            }
+        }
+    }
+
 }

engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java

@@ -19,6 +19,7 @@
 package org.apache.cloudstack.storage.motion;
 
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertFalse;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
@@ -56,6 +57,8 @@ import com.cloud.storage.Storage.StoragePoolType;
 import com.cloud.storage.Volume;
 import com.cloud.storage.VolumeVO;
 import java.util.AbstractMap;
+import java.util.HashSet;
+import java.util.Set;
 
 @RunWith(MockitoJUnitRunner.class)
 public class StorageSystemDataMotionStrategyTest {
@@ -288,4 +291,58 @@ public class StorageSystemDataMotionStrategyTest {
 
         Assert.assertEquals(String.format("{volume: \"%s\", from: \"%s\", to:\"%s\"}", volume, from, to), strategy.formatEntryOfVolumesAndStoragesAsJsonToDisplayOnLog(new AbstractMap.SimpleEntry<>(volumeInfo, dataStore)));
     }
+
+    @Test
+    public void validateSupportStoragePoolTypeDefaultValues() {
+        Set<StoragePoolType> supportedTypes = new HashSet<>();
+        supportedTypes.add(StoragePoolType.NetworkFilesystem);
+        supportedTypes.add(StoragePoolType.SharedMountPoint);
+
+        for (StoragePoolType poolType : StoragePoolType.values()) {
+            boolean isSupported = strategy.supportStoragePoolType(poolType);
+            if (supportedTypes.contains(poolType)) {
+                assertTrue(isSupported);
+            } else {
+                assertFalse(isSupported);
+            }
+        }
+    }
+
+    @Test
+    public void validateSupportStoragePoolTypeExtraValues() {
+        Set<StoragePoolType> supportedTypes = new HashSet<>();
+        supportedTypes.add(StoragePoolType.NetworkFilesystem);
+        supportedTypes.add(StoragePoolType.SharedMountPoint);
+        supportedTypes.add(StoragePoolType.Iscsi);
+        supportedTypes.add(StoragePoolType.CLVM);
+
+        for (StoragePoolType poolType : StoragePoolType.values()) {
+            boolean isSupported = strategy.supportStoragePoolType(poolType, StoragePoolType.Iscsi, StoragePoolType.CLVM);
+            if (supportedTypes.contains(poolType)) {
+                assertTrue(isSupported);
+            } else {
+                assertFalse(isSupported);
+            }
+        }
+    }
+
+    @Test
+    public void validateIsStoragePoolTypeInListReturnsTrue() {
+        StoragePoolType[] listTypes = new StoragePoolType[3];
+        listTypes[0] = StoragePoolType.LVM;
+        listTypes[1] = StoragePoolType.NetworkFilesystem;
+        listTypes[2] = StoragePoolType.SharedMountPoint;
+
+        assertTrue(strategy.isStoragePoolTypeInList(StoragePoolType.SharedMountPoint, listTypes));
+    }
+
+    @Test
+    public void validateIsStoragePoolTypeInListReturnsFalse() {
+        StoragePoolType[] listTypes = new StoragePoolType[3];
+        listTypes[0] = StoragePoolType.LVM;
+        listTypes[1] = StoragePoolType.NetworkFilesystem;
+        listTypes[2] = StoragePoolType.RBD;
+
+        assertFalse(strategy.isStoragePoolTypeInList(StoragePoolType.SharedMountPoint, listTypes));
+    }
 }

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java

@@ -64,6 +64,10 @@ import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.utils.script.Script;
 
 import static com.cloud.utils.NumbersUtil.toHumanReadableSize;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 public class LibvirtStorageAdaptor implements StorageAdaptor {
     private static final Logger s_logger = Logger.getLogger(LibvirtStorageAdaptor.class);
@@ -80,6 +84,9 @@ public class LibvirtStorageAdaptor implements StorageAdaptor {
     private int rbdFeatures = RBD_FEATURE_LAYERING + RBD_FEATURE_EXCLUSIVE_LOCK + RBD_FEATURE_OBJECT_MAP + RBD_FEATURE_FAST_DIFF + RBD_FEATURE_DEEP_FLATTEN;
     private int rbdOrder = 0; /* Order 0 means 4MB blocks (the default) */
 
+    private static final Set<StoragePoolType> poolTypesThatEnableCreateDiskFromTemplateBacking = new HashSet<>(Arrays.asList(StoragePoolType.NetworkFilesystem,
+      StoragePoolType.Filesystem));
+
     public LibvirtStorageAdaptor(StorageLayer storage) {
         _storageLayer = storage;
         _manageSnapshotPath = Script.findScript("scripts/storage/qcow2/", "managesnapshot.sh");
@@ -98,28 +105,36 @@ public class LibvirtStorageAdaptor implements StorageAdaptor {
     @Override
     public KVMPhysicalDisk createDiskFromTemplateBacking(KVMPhysicalDisk template, String name, PhysicalDiskFormat format, long size,
                                                          KVMStoragePool destPool, int timeout) {
-        s_logger.info("Creating volume " + name + " with template backing " + template.getName() + " in pool " + destPool.getUuid() +
+        String volumeDesc = String.format("volume [%s], with template backing [%s], in pool [%s] (%s), with size [%s]", name, template.getName(), destPool.getUuid(),
-                " (" + destPool.getType().toString() + ") with size " + size);
+          destPool.getType(), size);
 
-        KVMPhysicalDisk disk = null;
+        if (!poolTypesThatEnableCreateDiskFromTemplateBacking.contains(destPool.getType())) {
-        String destPath = destPool.getLocalPath().endsWith("/") ?
+            s_logger.info(String.format("Skipping creation of %s due to pool type is none of the following types %s.", volumeDesc, poolTypesThatEnableCreateDiskFromTemplateBacking.stream()
-                destPool.getLocalPath() + name :
+              .map(type -> type.toString()).collect(Collectors.joining(", "))));
-                destPool.getLocalPath() + "/" + name;
 
-        if (destPool.getType() == StoragePoolType.NetworkFilesystem) {
+            return null;
-            try {
-                if (format == PhysicalDiskFormat.QCOW2) {
-                    QemuImg qemu = new QemuImg(timeout);
-                    QemuImgFile destFile = new QemuImgFile(destPath, format);
-                    destFile.setSize(size);
-                    QemuImgFile backingFile = new QemuImgFile(template.getPath(), template.getFormat());
-                    qemu.create(destFile, backingFile);
-                }
-            } catch (QemuImgException e) {
-                s_logger.error("Failed to create " + destPath + " due to a failed executing of qemu-img: " + e.getMessage());
-            }
         }
-        return disk;
+
+        if (format != PhysicalDiskFormat.QCOW2) {
+            s_logger.info(String.format("Skipping creation of %s due to format [%s] is not [%s].", volumeDesc, format, PhysicalDiskFormat.QCOW2));
+            return null;
+        }
+
+        s_logger.info(String.format("Creating %s.", volumeDesc));
+
+        String destPoolLocalPath = destPool.getLocalPath();
+        String destPath = String.format("%s%s%s", destPoolLocalPath, destPoolLocalPath.endsWith("/") ? "" : "/", name);
+
+        try {
+            QemuImgFile destFile = new QemuImgFile(destPath, format);
+            destFile.setSize(size);
+            QemuImgFile backingFile = new QemuImgFile(template.getPath(), template.getFormat());
+            new QemuImg(timeout).create(destFile, backingFile);
+        } catch (QemuImgException e) {
+            s_logger.error(String.format("Failed to create %s in [%s] due to [%s].", volumeDesc, destPath, e.getMessage()), e);
+        }
+
+        return null;
     }
 
     /**