diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index 46a2cf99e10..9b5672e228f 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -51,6 +51,7 @@ import javax.inject.Inject; 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; @@ -162,6 +163,7 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co if (VirtualMachine.State.Stopped.equals(vm.getState())) { List vmVolumes = volumeDao.findByInstance(vm.getId()); + vmVolumes.sort(Comparator.comparing(Volume::getDeviceId)); List volumePaths = getVolumePaths(vmVolumes); command.setVolumePaths(volumePaths); } @@ -212,7 +214,10 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co @Override public boolean restoreVMFromBackup(VirtualMachine vm, Backup backup) { List backedVolumes = backup.getBackedUpVolumes(); - List volumes = backedVolumes.stream().map(volume -> volumeDao.findByUuid(volume.getUuid())).collect(Collectors.toList()); + List 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); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java index 2810f98c935..8abc359250c 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java @@ -62,16 +62,25 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper(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 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper 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 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper 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 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper 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 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper(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) { diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 84e4bbf258f..440da7e2c72 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -2605,7 +2605,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic excludeLocalStorageIfNeeded(volumeToAttach); - checkForDevicesInCopies(vmId, vm); + checkForVMSnapshots(vmId, vm); + + checkForBackups(vm, true); checkRightsToAttach(caller, volumeToAttach, vm); @@ -2707,18 +2709,12 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic } } - private void checkForDevicesInCopies(Long vmId, UserVmVO vm) { + private void checkForVMSnapshots(Long vmId, UserVmVO vm) { // if target VM has associated VM snapshots List 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 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())); - } } /** @@ -2818,7 +2814,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic 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; } @@ -3038,7 +3034,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic 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) { diff --git a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java index 43fc705ad76..1e6ef1a7852 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -19,6 +19,7 @@ package org.apache.cloudstack.backup; 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; @@ -277,6 +278,7 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { public static String createVolumeInfoFromVolumes(List vmVolumes) { List 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())); } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 7dcf30c55e4..6c25e69876f 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -666,7 +666,6 @@ public class VolumeApiServiceImplTest { 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); @@ -1305,7 +1304,7 @@ public class VolumeApiServiceImplTest { 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()); } @@ -1316,7 +1315,7 @@ public class VolumeApiServiceImplTest { 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()); } @@ -1326,7 +1325,7 @@ public class VolumeApiServiceImplTest { public void validateIfVmHaveBackupsTestSuccessWhenVMDontHaveBackupOffering() { UserVmVO vm = Mockito.mock(UserVmVO.class); when(vm.getBackupOfferingId()).thenReturn(null); - volumeApiServiceImpl.validateIfVmHasBackups(vm, true); + volumeApiServiceImpl.checkForBackups(vm, true); } @Test