From 65a48dcb74e53ea977ea133b46f4acc10378ab34 Mon Sep 17 00:00:00 2001 From: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com> Date: Mon, 16 Aug 2021 12:32:19 -0300 Subject: [PATCH] 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 --- ...vmNonManagedStorageDataMotionStrategy.java | 11 +++- .../StorageSystemDataMotionStrategy.java | 31 +++++++++- ...NonManagedStorageSystemDataMotionTest.java | 36 +++++++++++- .../StorageSystemDataMotionStrategyTest.java | 57 +++++++++++++++++++ .../kvm/storage/LibvirtStorageAdaptor.java | 53 ++++++++++------- 5 files changed, 161 insertions(+), 27 deletions(-) diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java index a61b44be072..bc951e669f3 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java +++ b/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); + } } diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java index 460c3370744..3c68793e348 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java +++ b/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 - && destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && !managedStorageDestination; - if (isNonManagedNfsToNfs) { + boolean isNonManagedNfsToNfsOrSharedMountPointToNfs = supportStoragePoolType(sourceStoragePool.getPoolType()) && destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && !managedStorageDestination; + if (isNonManagedNfsToNfsOrSharedMountPointToNfs) { 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 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 supportedTypes = new HashSet<>(); + + if (acceptedValues != null) { + supportedTypes.addAll(Arrays.asList(acceptedValues)); + } + + return supportedTypes.contains(storagePoolTypeToValidate); + }; } diff --git a/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageSystemDataMotionTest.java b/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageSystemDataMotionTest.java index 609742b79fd..601f6bbd104 100644 --- a/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageSystemDataMotionTest.java +++ b/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 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 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 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); + } + } + } + } diff --git a/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java b/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java index dcb7a44394e..3793a795077 100644 --- a/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java +++ b/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 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 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)); + } } \ No newline at end of file diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java index 3532996c87f..85fa36771a7 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java +++ b/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 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() + - " (" + destPool.getType().toString() + ") with size " + size); + String volumeDesc = String.format("volume [%s], with template backing [%s], in pool [%s] (%s), with size [%s]", name, template.getName(), destPool.getUuid(), + destPool.getType(), size); - KVMPhysicalDisk disk = null; - String destPath = destPool.getLocalPath().endsWith("/") ? - destPool.getLocalPath() + name : - destPool.getLocalPath() + "/" + name; + if (!poolTypesThatEnableCreateDiskFromTemplateBacking.contains(destPool.getType())) { + s_logger.info(String.format("Skipping creation of %s due to pool type is none of the following types %s.", volumeDesc, poolTypesThatEnableCreateDiskFromTemplateBacking.stream() + .map(type -> type.toString()).collect(Collectors.joining(", ")))); - if (destPool.getType() == StoragePoolType.NetworkFilesystem) { - 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 null; } - 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; } /**