Fix restore from NAS backup when datadisk is older than the root disk. (apache#11258)

Author: abh1sar

plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java

@@ -58,6 +58,7 @@
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.Date;
 import java.util.List;
 import java.util.Locale;
@@ -169,6 +170,7 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm) {
 
         if (VirtualMachine.State.Stopped.equals(vm.getState())) {
             List<VolumeVO> vmVolumes = volumeDao.findByInstance(vm.getId());
+            vmVolumes.sort(Comparator.comparing(Volume::getDeviceId));
             List<String> volumePaths = getVolumePaths(vmVolumes);
             command.setVolumePaths(volumePaths);
         }
@@ -223,7 +225,10 @@ private BackupVO createBackupObject(VirtualMachine vm, String backupPath) {
     @Override
     public boolean restoreVMFromBackup(VirtualMachine vm, Backup backup) {
         List<Backup.VolumeInfo> backedVolumes = backup.getBackedUpVolumes();
-        List<VolumeVO> volumes = backedVolumes.stream().map(volume -> volumeDao.findByUuid(volume.getUuid())).collect(Collectors.toList());
+        List<VolumeVO> volumes = backedVolumes.stream()
+                .map(volume -> volumeDao.findByUuid(volume.getUuid()))
+                .sorted((v1, v2) -> Long.compare(v1.getDeviceId(), v2.getDeviceId()))
+                .collect(Collectors.toList());
 
         LOG.debug("Restoring vm {} from backup {} on the NAS Backup Provider", vm, backup);
         BackupRepository backupRepository = getBackupRepository(vm, backup);

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java

@@ -62,16 +62,25 @@ public Answer execute(RestoreBackupCommand command, LibvirtComputingResource ser
         String restoreVolumeUuid = command.getRestoreVolumeUUID();
 
         String newVolumeId = null;
-        if (Objects.isNull(vmExists)) {
-            String volumePath = volumePaths.get(0);
-            int lastIndex = volumePath.lastIndexOf("/");
-            newVolumeId = volumePath.substring(lastIndex + 1);
-            restoreVolume(backupPath, backupRepoType, backupRepoAddress, volumePath, diskType, restoreVolumeUuid,
-                    new Pair<>(vmName, command.getVmState()), mountOptions);
-        } else if (Boolean.TRUE.equals(vmExists)) {
-            restoreVolumesOfExistingVM(volumePaths, backupPath, backupRepoType, backupRepoAddress, mountOptions);
-        } else {
-            restoreVolumesOfDestroyedVMs(volumePaths, vmName, backupPath, backupRepoType, backupRepoAddress, mountOptions);
+        try {
+            if (Objects.isNull(vmExists)) {
+                String volumePath = volumePaths.get(0);
+                int lastIndex = volumePath.lastIndexOf("/");
+                newVolumeId = volumePath.substring(lastIndex + 1);
+                restoreVolume(backupPath, backupRepoType, backupRepoAddress, volumePath, diskType, restoreVolumeUuid,
+                        new Pair<>(vmName, command.getVmState()), mountOptions);
+            } else if (Boolean.TRUE.equals(vmExists)) {
+                restoreVolumesOfExistingVM(volumePaths, backupPath, backupRepoType, backupRepoAddress, mountOptions);
+            } else {
+                restoreVolumesOfDestroyedVMs(volumePaths, vmName, backupPath, backupRepoType, backupRepoAddress, mountOptions);
+            }
+        } catch (CloudRuntimeException e) {
+            String errorMessage = "Failed to restore backup for VM: " + vmName + ".";
+            if (e.getMessage() != null && !e.getMessage().isEmpty()) {
+                errorMessage += " Details: " + e.getMessage();
+            }
+            logger.error(errorMessage);
+            return new BackupAnswer(command, false, errorMessage);
         }
 
         return new BackupAnswer(command, true, newVolumeId);
@@ -86,10 +95,8 @@ private void restoreVolumesOfExistingVM(List<String> volumePaths, String backupP
                 String volumePath = volumePaths.get(idx);
                 Pair<String, String> bkpPathAndVolUuid = getBackupPath(mountDirectory, volumePath, backupPath, diskType, null);
                 diskType = "datadisk";
-                try {
-                    replaceVolumeWithBackup(volumePath, bkpPathAndVolUuid.first());
-                } catch (IOException e) {
-                    throw new CloudRuntimeException(String.format("Unable to revert backup for volume [%s] due to [%s].", bkpPathAndVolUuid.second(), e.getMessage()), e);
+                if (!replaceVolumeWithBackup(volumePath, bkpPathAndVolUuid.first())) {
+                    throw new CloudRuntimeException(String.format("Unable to restore backup for volume [%s].", bkpPathAndVolUuid.second()));
                 }
             }
         } finally {
@@ -108,10 +115,8 @@ private void restoreVolumesOfDestroyedVMs(List<String> volumePaths, String vmNam
                 String volumePath = volumePaths.get(i);
                 Pair<String, String> bkpPathAndVolUuid = getBackupPath(mountDirectory, volumePath, backupPath, diskType, null);
                 diskType = "datadisk";
-                try {
-                    replaceVolumeWithBackup(volumePath, bkpPathAndVolUuid.first());
-                } catch (IOException e) {
-                    throw new CloudRuntimeException(String.format("Unable to revert backup for volume [%s] due to [%s].", bkpPathAndVolUuid.second(), e.getMessage()), e);
+                if (!replaceVolumeWithBackup(volumePath, bkpPathAndVolUuid.first())) {
+                    throw new CloudRuntimeException(String.format("Unable to restore backup for volume [%s].", bkpPathAndVolUuid.second()));
                 }
             }
         } finally {
@@ -126,15 +131,13 @@ private void restoreVolume(String backupPath, String backupRepoType, String back
         Pair<String, String> bkpPathAndVolUuid;
         try {
             bkpPathAndVolUuid = getBackupPath(mountDirectory, volumePath, backupPath, diskType, volumeUUID);
-            try {
-                replaceVolumeWithBackup(volumePath, bkpPathAndVolUuid.first());
-                if (VirtualMachine.State.Running.equals(vmNameAndState.second())) {
-                    if (!attachVolumeToVm(vmNameAndState.first(), volumePath)) {
-                        throw new CloudRuntimeException(String.format("Failed to attach volume to VM: %s", vmNameAndState.first()));
-                    }
+            if (!replaceVolumeWithBackup(volumePath, bkpPathAndVolUuid.first())) {
+                throw new CloudRuntimeException(String.format("Unable to restore backup for volume [%s].", bkpPathAndVolUuid.second()));
+            }
+            if (VirtualMachine.State.Running.equals(vmNameAndState.second())) {
+                if (!attachVolumeToVm(vmNameAndState.first(), volumePath)) {
+                    throw new CloudRuntimeException(String.format("Failed to attach volume to VM: %s", vmNameAndState.first()));
                 }
-            } catch (IOException e) {
-                throw new CloudRuntimeException(String.format("Unable to revert backup for volume [%s] due to [%s].", bkpPathAndVolUuid.second(), e.getMessage()), e);
             }
         } catch (Exception e) {
             throw new CloudRuntimeException("Failed to restore volume", e);
@@ -194,8 +197,9 @@ private Pair<String, String> getBackupPath(String mountDirectory, String volumeP
         return new Pair<>(bkpPath, volUuid);
     }
 
-    private void replaceVolumeWithBackup(String volumePath, String backupPath) throws IOException {
-        Script.runSimpleBashScript(String.format(RSYNC_COMMAND, backupPath, volumePath));
+    private boolean replaceVolumeWithBackup(String volumePath, String backupPath) {
+        int exitValue = Script.runSimpleBashScriptForExitValue(String.format(RSYNC_COMMAND, backupPath, volumePath));
+        return exitValue == 0;
     }
 
     private boolean attachVolumeToVm(String vmName, String volumePath) {

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

@@ -2782,7 +2782,9 @@ public Volume attachVolumeToVM(Long vmId, Long volumeId, Long deviceId, Boolean
 
         excludeLocalStorageIfNeeded(volumeToAttach);
 
-        checkForDevicesInCopies(vmId, vm);
+        checkForVMSnapshots(vmId, vm);
+
+        checkForBackups(vm, true);
 
         checkRightsToAttach(caller, volumeToAttach, vm);
 
@@ -2881,18 +2883,12 @@ private void checkRightsToAttach(Account caller, VolumeInfo volumeToAttach, User
         }
     }
 
-    private void checkForDevicesInCopies(Long vmId, UserVmVO vm) {
+    private void checkForVMSnapshots(Long vmId, UserVmVO vm) {
         // if target VM has associated VM snapshots
         List<VMSnapshotVO> vmSnapshots = _vmSnapshotDao.findByVm(vmId);
         if (vmSnapshots.size() > 0) {
             throw new InvalidParameterValueException(String.format("Unable to attach volume to VM %s/%s, please specify a VM that does not have VM snapshots", vm.getName(), vm.getUuid()));
         }
-
-        // if target VM has backups
-        List<Backup> backups = backupDao.listByVmId(vm.getDataCenterId(), vm.getId());
-        if (vm.getBackupOfferingId() != null && !backups.isEmpty()) {
-            throw new InvalidParameterValueException(String.format("Unable to attach volume to VM %s/%s, please specify a VM that does not have any backups", vm.getName(), vm.getUuid()));
-        }
     }
 
     /**
@@ -2992,7 +2988,7 @@ private void checkDeviceId(Long deviceId, VolumeInfo volumeToAttach, UserVmVO vm
         return volumeToAttach;
     }
 
-    protected void validateIfVmHasBackups(UserVmVO vm, boolean attach) {
+    protected void checkForBackups(UserVmVO vm, boolean attach) {
         if ((vm.getBackupOfferingId() == null || CollectionUtils.isEmpty(vm.getBackupVolumeList())) || BooleanUtils.isTrue(BackupManager.BackupEnableAttachDetachVolumes.value())) {
             return;
         }
@@ -3231,7 +3227,7 @@ public Volume detachVolumeFromVM(DetachVolumeCmd cmmd) {
             throw new InvalidParameterValueException("Unable to detach volume, please specify a VM that does not have VM snapshots");
         }
 
-        validateIfVmHasBackups(vm, false);
+        checkForBackups(vm, false);
 
         AsyncJobExecutionContext asyncExecutionContext = AsyncJobExecutionContext.getCurrentExecutionContext();
         if (asyncExecutionContext != null) {

server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java

@@ -19,6 +19,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.List;
@@ -293,6 +294,7 @@ public boolean deleteBackupOffering(final Long offeringId) {
 
     public static String createVolumeInfoFromVolumes(List<VolumeVO> vmVolumes) {
         List<Backup.VolumeInfo> list = new ArrayList<>();
+        vmVolumes.sort(Comparator.comparing(VolumeVO::getDeviceId));
         for (VolumeVO vol : vmVolumes) {
             list.add(new Backup.VolumeInfo(vol.getUuid(), vol.getPath(), vol.getVolumeType(), vol.getSize()));
         }

server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java

@@ -672,7 +672,6 @@ public void testResourceLimitCheckForUploadedVolume() throws NoSuchFieldExceptio
         when(vm.getState()).thenReturn(State.Running);
         when(vm.getDataCenterId()).thenReturn(34L);
         when(vm.getBackupOfferingId()).thenReturn(null);
-        when(backupDaoMock.listByVmId(anyLong(), anyLong())).thenReturn(Collections.emptyList());
         when(volumeDaoMock.findByInstanceAndType(anyLong(), any(Volume.Type.class))).thenReturn(new ArrayList<>(10));
         when(volumeDataFactoryMock.getVolume(9L)).thenReturn(volumeToAttach);
         when(volumeToAttach.getState()).thenReturn(Volume.State.Uploaded);
@@ -1311,7 +1310,7 @@ public void validateIfVmHaveBackupsTestExceptionWhenTryToDetachVolumeFromVMWhich
         try {
             UserVmVO vm = Mockito.mock(UserVmVO.class);
             when(vm.getBackupOfferingId()).thenReturn(1l);
-            volumeApiServiceImpl.validateIfVmHasBackups(vm, false);
+            volumeApiServiceImpl.checkForBackups(vm, false);
         } catch (Exception e) {
             Assert.assertEquals("Unable to detach volume, cannot detach volume from a VM that has backups. First remove the VM from the backup offering or set the global configuration 'backup.enable.attach.detach.of.volumes' to true.", e.getMessage());
         }
@@ -1322,7 +1321,7 @@ public void validateIfVmHaveBackupsTestExceptionWhenTryToAttachVolumeFromVMWhich
         try {
             UserVmVO vm = Mockito.mock(UserVmVO.class);
             when(vm.getBackupOfferingId()).thenReturn(1l);
-            volumeApiServiceImpl.validateIfVmHasBackups(vm, true);
+            volumeApiServiceImpl.checkForBackups(vm, true);
         } catch (Exception e) {
             Assert.assertEquals("Unable to attach volume, please specify a VM that does not have any backups or set the global configuration 'backup.enable.attach.detach.of.volumes' to true.", e.getMessage());
         }
@@ -1332,7 +1331,7 @@ public void validateIfVmHaveBackupsTestExceptionWhenTryToAttachVolumeFromVMWhich
     public void validateIfVmHaveBackupsTestSuccessWhenVMDontHaveBackupOffering() {
         UserVmVO vm = Mockito.mock(UserVmVO.class);
         when(vm.getBackupOfferingId()).thenReturn(null);
-        volumeApiServiceImpl.validateIfVmHasBackups(vm, true);
+        volumeApiServiceImpl.checkForBackups(vm, true);
     }
 
     @Test