Fix npe when migrating vm with volume (#4698)

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

Author: GutoVeronezi

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

@@ -217,8 +217,10 @@ protected void copyTemplateToTargetFilesystemStorageIfNeeded(VolumeInfo srcVolum
                 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()));
     }
 
     /**

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

@@ -134,6 +134,7 @@
 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);
@@ -1816,7 +1817,12 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
                     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);
@@ -1920,9 +1926,12 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
 
                 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);
         } finally {
             CopyCmdAnswer copyCmdAnswer = new CopyCmdAnswer(errMsg);
@@ -1935,6 +1944,16 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
         }
     }
 
+    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<VolumeInfo, DataStore> 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).
      */

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

@@ -55,6 +55,7 @@
 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 @@ private void configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(StoragePoolTy
         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)));
+    }
 }