Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaanHoogland merged 2 commits into from
Jul 23, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26ed64f
Fix restore when datadisk is older than the root disk.
abh1sar Jul 22, 2025
d013c2f
address review comments
abh1sar Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,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;
Expand Down Expand Up @@ -162,6 +163,7 @@ public boolean 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);
}
Expand Down Expand Up @@ -212,7 +214,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);
Expand Down
56 changes: 28 additions & 28 deletions ...in/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,21 @@ 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) {
logger.error("Failed to restore backup for VM: " + vmName, e);
return new BackupAnswer(command, false, e.getMessage());
Comment thread
DaanHoogland marked this conversation as resolved.
Outdated
}

return new BackupAnswer(command, true, newVolumeId);
Expand All @@ -86,10 +91,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 {
Expand All @@ -108,10 +111,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 {
Expand All @@ -126,15 +127,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())) {
Comment thread
sureshanaparti marked this conversation as resolved.
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);
Expand Down Expand Up @@ -194,8 +193,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) {
Expand Down
16 changes: 6 additions & 10 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2605,7 +2605,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);

Expand Down Expand Up @@ -2707,18 +2709,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()));
}
}

/**
Expand Down Expand Up @@ -2818,7 +2814,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;
}
Expand Down Expand Up @@ -3038,7 +3034,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) {
Expand Down
4 changes: 3 additions & 1 deletion server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -277,7 +278,8 @@ public boolean deleteBackupOffering(final Long offeringId) {

public static String createVolumeInfoFromVolumes(List<VolumeVO> vmVolumes) {
Comment thread
DaanHoogland marked this conversation as resolved.
List<Backup.VolumeInfo> list = new ArrayList<>();
for (VolumeVO vol : vmVolumes) {
vmVolumes.sort(Comparator.comparing(Volume::getDeviceId));
for (Volume vol : vmVolumes) {
list.add(new Backup.VolumeInfo(vol.getUuid(), vol.getPath(), vol.getVolumeType(), vol.getSize()));
}
return new Gson().toJson(list.toArray(), Backup.VolumeInfo[].class);
Expand Down
7 changes: 3 additions & 4 deletions server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,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);
Expand Down Expand Up @@ -1305,7 +1304,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());
}
Expand All @@ -1316,7 +1315,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());
}
Expand All @@ -1326,7 +1325,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
Expand Down