From cb91a769d337cf56eda8115907f8d1419a799130 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20Beims=20Br=C3=A4scher?= Date: Tue, 6 Apr 2021 03:24:29 -0300 Subject: [PATCH] Fix npe when migrating vm with volume (#4698) (#4775) Cherry-pick commit 59fba4916b and fix conflict. Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com> --- ...vmNonManagedStorageDataMotionStrategy.java | 2 ++ .../StorageSystemDataMotionStrategy.java | 26 ++++++++++++++++--- .../StorageSystemDataMotionStrategyTest.java | 22 ++++++++++++++++ 3 files changed, 46 insertions(+), 4 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 e6b5c85b924..c00abb6b696 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 @@ -212,8 +212,10 @@ public class KvmNonManagedStorageDataMotionStrategy extends StorageSystemDataMot if (copyCommandAnswer != null && copyCommandAnswer.getResult()) { updateTemplateReferenceIfSuccessfulCopy(srcVolumeInfo, srcStoragePool, destTemplateInfo, destDataStore); } + return; } } + LOGGER.debug(String.format("Skipping 'copy template to target filesystem storage before migration' due to the template [%s] already exist on the storage pool [%s].", srcVolumeInfo.getTemplateId(), destStoragePool.getId())); } /** 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 4d3ec184ac1..70fe8e9b6fd 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,6 +134,7 @@ 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.stream.Collectors; public class StorageSystemDataMotionStrategy implements DataMotionStrategy { private static final Logger LOGGER = Logger.getLogger(StorageSystemDataMotionStrategy.class); @@ -1790,7 +1791,12 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { continue; } - copyTemplateToTargetFilesystemStorageIfNeeded(srcVolumeInfo, sourceStoragePool, destDataStore, destStoragePool, destHost); + if (srcVolumeInfo.getTemplateId() != null) { + LOGGER.debug(String.format("Copying template [%s] of volume [%s] from source storage pool [%s] to target storage pool [%s].", srcVolumeInfo.getTemplateId(), srcVolumeInfo.getId(), sourceStoragePool.getId(), destStoragePool.getId())); + copyTemplateToTargetFilesystemStorageIfNeeded(srcVolumeInfo, sourceStoragePool, destDataStore, destStoragePool, destHost); + } else { + LOGGER.debug(String.format("Skipping copy template from source storage pool [%s] to target storage pool [%s] before migration due to volume [%s] does not have a template.", sourceStoragePool.getId(), destStoragePool.getId(), srcVolumeInfo.getId())); + } VolumeVO destVolume = duplicateVolumeOnAnotherStorage(srcVolume, destStoragePool); VolumeInfo destVolumeInfo = _volumeDataFactory.getVolume(destVolume.getId(), destDataStore); @@ -1894,9 +1900,11 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { throw new CloudRuntimeException(errMsg); } - } - catch (Exception ex) { - errMsg = "Copy operation failed in 'StorageSystemDataMotionStrategy.copyAsync': " + ex.getMessage(); + } catch (AgentUnavailableException | OperationTimedoutException | CloudRuntimeException ex) { + String volumesAndStorages = volumeDataStoreMap.entrySet().stream().map(entry -> formatEntryOfVolumesAndStoragesAsJsonToDisplayOnLog(entry)).collect(Collectors.joining(",")); + + errMsg = String.format("Copy volume(s) to storage(s) [%s] and VM to host [%s] failed in StorageSystemDataMotionStrategy.copyAsync. Error message: [%s].", volumesAndStorages, formatMigrationElementsAsJsonToDisplayOnLog("vm", vmTO.getId(), srcHost.getId(), destHost.getId()), ex.getMessage()); + LOGGER.error(errMsg, ex); throw new CloudRuntimeException(errMsg); } @@ -1911,6 +1919,16 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { } } + protected String formatMigrationElementsAsJsonToDisplayOnLog(String objectName, Object object, Object from, Object to){ + return String.format("{%s: \"%s\", from: \"%s\", to:\"%s\"}", objectName, object, from, to); + } + + protected String formatEntryOfVolumesAndStoragesAsJsonToDisplayOnLog(Map.Entry entry ){ + VolumeInfo srcVolumeInfo = entry.getKey(); + DataStore destDataStore = entry.getValue(); + return formatMigrationElementsAsJsonToDisplayOnLog("volume", srcVolumeInfo.getId(), srcVolumeInfo.getPoolId(), destDataStore.getId()); + } + /** * Returns true if at least one of the entries on the map 'volumeDataStoreMap' has both source and destination storage pools of Network Filesystem (NFS). */ 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 4e62c9477c0..dcb7a44394e 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 @@ -55,6 +55,7 @@ import com.cloud.storage.Storage; import com.cloud.storage.Storage.StoragePoolType; import com.cloud.storage.Volume; import com.cloud.storage.VolumeVO; +import java.util.AbstractMap; @RunWith(MockitoJUnitRunner.class) public class StorageSystemDataMotionStrategyTest { @@ -266,4 +267,25 @@ public class StorageSystemDataMotionStrategyTest { Assert.assertEquals(expected, result); } + @Test + public void formatMigrationElementsAsJsonToDisplayOnLogValidateFormat(){ + String objectName = "test"; + Long object = 1L, from = 2L, to = 3L; + + Assert.assertEquals(String.format("{%s: \"%s\", from: \"%s\", to:\"%s\"}", objectName, object, from, to), strategy.formatMigrationElementsAsJsonToDisplayOnLog(objectName, + object, from, to)); + } + + @Test + public void formatEntryOfVolumesAndStoragesAsJsonToDisplayOnLogValidateFormat(){ + Long volume = 1L, from = 2L, to = 3L; + VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class); + DataStore dataStore = Mockito.mock(DataStore.class); + + Mockito.when(volumeInfo.getId()).thenReturn(volume); + Mockito.when(volumeInfo.getPoolId()).thenReturn(from); + Mockito.when(dataStore.getId()).thenReturn(to); + + Assert.assertEquals(String.format("{volume: \"%s\", from: \"%s\", to:\"%s\"}", volume, from, to), strategy.formatEntryOfVolumesAndStoragesAsJsonToDisplayOnLog(new AbstractMap.SimpleEntry<>(volumeInfo, dataStore))); + } } \ No newline at end of file