From 5dc982d8bab4ed16533dd56b39e98dadc79eeafe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20Beims=20Br=C3=A4scher?= Date: Wed, 7 Aug 2019 07:11:30 -0300 Subject: [PATCH] KVM local migration issue #3521 (#3533) Fix regression bug that affects KVM local storage migration. Some of the desired execution flows for KVM local storage migration had been altered to allow only managed storage to execute. Fixed allowing managed and non managed storages to execute. Fixes #3521 --- .../com/cloud/agent/api/MigrateCommand.java | 9 +++ .../StorageSystemDataMotionStrategy.java | 40 ++++++++++--- .../StorageSystemDataMotionStrategyTest.java | 60 +++++++++++++++++++ .../kvm/resource/MigrateKVMAsync.java | 12 ++-- .../wrapper/LibvirtMigrateCommandWrapper.java | 4 +- 5 files changed, 109 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/com/cloud/agent/api/MigrateCommand.java b/core/src/main/java/com/cloud/agent/api/MigrateCommand.java index 93324d20730..c7aeb69cff0 100644 --- a/core/src/main/java/com/cloud/agent/api/MigrateCommand.java +++ b/core/src/main/java/com/cloud/agent/api/MigrateCommand.java @@ -32,6 +32,7 @@ public class MigrateCommand extends Command { private String destIp; private Map migrateStorage; private boolean migrateStorageManaged; + private boolean migrateNonSharedInc; private boolean autoConvergence; private String hostGuid; private boolean isWindows; @@ -75,6 +76,14 @@ public class MigrateCommand extends Command { this.migrateStorageManaged = migrateStorageManaged; } + public boolean isMigrateNonSharedInc() { + return migrateNonSharedInc; + } + + public void setMigrateNonSharedInc(boolean migrateNonSharedInc) { + this.migrateNonSharedInc = migrateNonSharedInc; + } + public void setAutoConvergence(boolean autoConvergence) { this.autoConvergence = autoConvergence; } 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 45cd2954f15..4d3ec184ac1 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 @@ -1828,16 +1828,19 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { String destPath = generateDestPath(destHost, destStoragePool, destVolumeInfo); MigrateCommand.MigrateDiskInfo migrateDiskInfo; - if (managedStorageDestination) { - migrateDiskInfo = configureMigrateDiskInfo(srcVolumeInfo, destPath); - migrateDiskInfo.setSourceDiskOnStorageFileSystem(isStoragePoolTypeOfFile(sourceStoragePool)); - migrateDiskInfoList.add(migrateDiskInfo); - } else { + + boolean isNonManagedNfsToNfs = sourceStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem + && destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && !managedStorageDestination; + if (isNonManagedNfsToNfs) { migrateDiskInfo = new MigrateCommand.MigrateDiskInfo(srcVolumeInfo.getPath(), MigrateCommand.MigrateDiskInfo.DiskType.FILE, MigrateCommand.MigrateDiskInfo.DriverType.QCOW2, MigrateCommand.MigrateDiskInfo.Source.FILE, connectHostToVolume(destHost, destVolumeInfo.getPoolId(), volumeIdentifier)); + } else { + migrateDiskInfo = configureMigrateDiskInfo(srcVolumeInfo, destPath); + migrateDiskInfo.setSourceDiskOnStorageFileSystem(isStoragePoolTypeOfFile(sourceStoragePool)); + migrateDiskInfoList.add(migrateDiskInfo); } migrateStorage.put(srcVolumeInfo.getPath(), migrateDiskInfo); @@ -1864,11 +1867,14 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { VMInstanceVO vm = _vmDao.findById(vmTO.getId()); boolean isWindows = _guestOsCategoryDao.findById(_guestOsDao.findById(vm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows"); + boolean migrateNonSharedInc = isSourceAndDestinationPoolTypeOfNfs(volumeDataStoreMap); + MigrateCommand migrateCommand = new MigrateCommand(vmTO.getName(), destHost.getPrivateIpAddress(), isWindows, vmTO, true); migrateCommand.setWait(StorageManager.KvmStorageOnlineMigrationWait.value()); migrateCommand.setMigrateStorage(migrateStorage); migrateCommand.setMigrateDiskInfoList(migrateDiskInfoList); migrateCommand.setMigrateStorageManaged(managedStorageDestination); + migrateCommand.setMigrateNonSharedInc(migrateNonSharedInc); boolean kvmAutoConvergence = StorageManager.KvmAutoConvergence.value(); migrateCommand.setAutoConvergence(kvmAutoConvergence); @@ -1905,6 +1911,23 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { } } + /** + * Returns true if at least one of the entries on the map 'volumeDataStoreMap' has both source and destination storage pools of Network Filesystem (NFS). + */ + protected boolean isSourceAndDestinationPoolTypeOfNfs(Map volumeDataStoreMap) { + for (Map.Entry entry : volumeDataStoreMap.entrySet()) { + VolumeInfo srcVolumeInfo = entry.getKey(); + DataStore destDataStore = entry.getValue(); + + StoragePoolVO destStoragePool = _storagePoolDao.findById(destDataStore.getId()); + StoragePoolVO sourceStoragePool = _storagePoolDao.findById(srcVolumeInfo.getPoolId()); + if (sourceStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem) { + return true; + } + } + return false; + } + /** * Returns true. This method was implemented considering the classes that extend this {@link StorageSystemDataMotionStrategy} and cannot migrate volumes from certain types of source storage pools and/or to a different kind of destiny storage pool. */ @@ -2157,7 +2180,7 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { } } - /* + /** * At a high level: The source storage cannot be managed and * the destination storages can be all managed or all not managed, not mixed. */ @@ -2191,9 +2214,8 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { throw new CloudRuntimeException("Destination storage pools must be either all managed or all not managed"); } - if (!destStoragePoolVO.isManaged()) { - if (destStoragePoolVO.getPoolType() == StoragePoolType.NetworkFilesystem && - destStoragePoolVO.getScope() != ScopeType.CLUSTER) { + if (!destStoragePoolVO.isManaged() && destStoragePoolVO.getPoolType() == StoragePoolType.NetworkFilesystem) { + if (destStoragePoolVO.getScope() != ScopeType.CLUSTER) { throw new CloudRuntimeException("KVM live storage migrations currently support cluster-wide " + "not managed NFS destination storage"); } 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 1b383d91574..288243c85ba 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 @@ -205,4 +205,64 @@ public class StorageSystemDataMotionStrategyTest { } } + @Test + public void isSourceAndDestinationPoolTypeOfNfsTestNfsNfs() { + configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem, true); + } + + @Test + public void isSourceAndDestinationPoolTypeOfNfsTestNfsAny() { + StoragePoolType[] storagePoolTypeArray = StoragePoolType.values(); + for (int i = 0; i < storagePoolTypeArray.length - 1; i++) { + if (storagePoolTypeArray[i] != StoragePoolType.NetworkFilesystem) { + configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(StoragePoolType.NetworkFilesystem, storagePoolTypeArray[i], false); + } + } + } + + @Test + public void isSourceAndDestinationPoolTypeOfNfsTestAnyNfs() { + StoragePoolType[] storagePoolTypeArray = StoragePoolType.values(); + for (int i = 0; i < storagePoolTypeArray.length - 1; i++) { + if (storagePoolTypeArray[i] != StoragePoolType.NetworkFilesystem) { + configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(storagePoolTypeArray[i], StoragePoolType.NetworkFilesystem, false); + } + } + } + + @Test + public void isSourceAndDestinationPoolTypeOfNfsTestAnyAny() { + StoragePoolType[] storagePoolTypeArray = StoragePoolType.values(); + for (int i = 0; i < storagePoolTypeArray.length - 1; i++) { + for (int j = 0; j < storagePoolTypeArray.length - 1; j++) { + if (storagePoolTypeArray[i] != StoragePoolType.NetworkFilesystem || storagePoolTypeArray[j] != StoragePoolType.NetworkFilesystem) { + configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(storagePoolTypeArray[i], storagePoolTypeArray[j], false); + } + } + } + } + + private void configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(StoragePoolType destStoragePoolType, StoragePoolType sourceStoragePoolType, boolean expected) { + VolumeInfo srcVolumeInfo = Mockito.mock(VolumeObject.class); + Mockito.when(srcVolumeInfo.getId()).thenReturn(0l); + + DataStore destDataStore = Mockito.mock(PrimaryDataStoreImpl.class); + Mockito.when(destDataStore.getId()).thenReturn(1l); + + StoragePoolVO destStoragePool = Mockito.mock(StoragePoolVO.class); + Mockito.when(destStoragePool.getPoolType()).thenReturn(destStoragePoolType); + + StoragePoolVO sourceStoragePool = Mockito.mock(StoragePoolVO.class); + Mockito.when(sourceStoragePool.getPoolType()).thenReturn(sourceStoragePoolType); + + Map volumeDataStoreMap = new HashMap<>(); + volumeDataStoreMap.put(srcVolumeInfo, destDataStore); + + Mockito.doReturn(sourceStoragePool).when(primaryDataStoreDao).findById(0l); + Mockito.doReturn(destStoragePool).when(primaryDataStoreDao).findById(1l); + + boolean result = strategy.isSourceAndDestinationPoolTypeOfNfs(volumeDataStoreMap); + Assert.assertEquals(expected, result); + } + } \ No newline at end of file diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java index c3e3e6edf41..c3917314250 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java @@ -34,7 +34,7 @@ public class MigrateKVMAsync implements Callable { private String vmName = ""; private String destIp = ""; private boolean migrateStorage; - private boolean migrateStorageManaged; + private boolean migrateNonSharedInc; private boolean autoConvergence; // Libvirt Migrate Flags reference: @@ -87,14 +87,14 @@ public class MigrateKVMAsync implements Callable { private static final int LIBVIRT_VERSION_SUPPORTS_AUTO_CONVERGE = 1002003; public MigrateKVMAsync(final LibvirtComputingResource libvirtComputingResource, final Domain dm, final Connect dconn, final String dxml, - final boolean migrateStorage, final boolean migrateStorageManaged, final boolean autoConvergence, final String vmName, final String destIp) { + final boolean migrateStorage, final boolean migrateNonSharedInc, final boolean autoConvergence, final String vmName, final String destIp) { this.libvirtComputingResource = libvirtComputingResource; this.dm = dm; this.dconn = dconn; this.dxml = dxml; this.migrateStorage = migrateStorage; - this.migrateStorageManaged = migrateStorageManaged; + this.migrateNonSharedInc = migrateNonSharedInc; this.autoConvergence = autoConvergence; this.vmName = vmName; this.destIp = destIp; @@ -109,11 +109,11 @@ public class MigrateKVMAsync implements Callable { } if (migrateStorage) { - if (migrateStorageManaged) { - flags |= VIR_MIGRATE_NON_SHARED_DISK; - } else { + if (migrateNonSharedInc) { flags |= VIR_MIGRATE_PERSIST_DEST; flags |= VIR_MIGRATE_NON_SHARED_INC; + } else { + flags |= VIR_MIGRATE_NON_SHARED_DISK; } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java index e6d366e3b04..f0eb287c5ea 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java @@ -164,8 +164,10 @@ public final class LibvirtMigrateCommandWrapper extends CommandWrapper worker = new MigrateKVMAsync(libvirtComputingResource, dm, dconn, xmlDesc, - migrateStorage, migrateStorageManaged, + migrateStorage, migrateNonSharedInc, command.isAutoConvergence(), vmName, command.getDestinationIp()); final Future migrateThread = executor.submit(worker); executor.shutdown();