Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@
import com.cloud.agent.api.LogLevel;
import com.cloud.vm.VirtualMachine;

import java.util.List;
import java.util.Map;

public class RestoreBackupCommand extends Command {
private String vmName;
private String backupPath;
private String backupRepoType;
private String backupRepoAddress;
private List<String> volumePaths;
private Map<String, String> volumePathsAndUuids;
private String diskType;
Comment on lines 31 to 34
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the agent command payload from volumePaths (List) to volumePathsAndUuids (Map) breaks compatibility with older KVM agents during upgrades (they will ignore the new field and restore will have no volumes to process). If mixed-version operation is expected, consider keeping/populating the old field as a fallback or introducing a new Command class to version the contract.

Copilot uses AI. Check for mistakes.
Comment on lines 31 to 34
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name volumePathsAndUuids is misleading: the restore flow uses this as current volume path -> backed-up volume path (for filename lookup), not UUIDs. This ambiguity is part of the agent-command API surface and can easily cause future bugs. Consider renaming or documenting key/value meaning explicitly.

Copilot uses AI. Check for mistakes.
private Boolean vmExists;
private String restoreVolumeUUID;
Expand Down Expand Up @@ -72,12 +72,12 @@ public void setBackupRepoAddress(String backupRepoAddress) {
this.backupRepoAddress = backupRepoAddress;
}

public List<String> getVolumePaths() {
return volumePaths;
public Map<String, String> getVolumePathsAndUuids() {
return volumePathsAndUuids;
}

public void setVolumePaths(List<String> volumePaths) {
this.volumePaths = volumePaths;
public void setVolumePathsAndUuids(Map<String, String> volumePathsAndUuids) {
this.volumePathsAndUuids = volumePathsAndUuids;
}

public Boolean isVmExists() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@
import com.cloud.agent.api.Command;
import com.cloud.agent.api.LogLevel;

import java.util.List;
import java.util.Map;

public class TakeBackupCommand extends Command {
private String vmName;
private String backupPath;
private String backupRepoType;
private String backupRepoAddress;
private List<String> volumePaths;
private Map<String, String> volumePathsAndUuids;
@LogLevel(LogLevel.Log4jLevel.Off)
Comment on lines 31 to 33
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the agent command payload from volumePaths (List) to volumePathsAndUuids (Map) is a wire-compatibility break for mixed-version management server/agent deployments: older agents will ignore the new field and see no disk paths for stopped-VM backups. If rolling upgrades are supported, keep the old field for backward compatibility (populate both, and have the agent wrapper fall back), or introduce a new Command class/versioned field.

Copilot uses AI. Check for mistakes.
Comment on lines 31 to 33
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name volumePathsAndUuids is misleading: current usage passes a mapping of current volume path -> backed-up volume path (used for backup filename lookup), not UUIDs. This makes the command contract easy to misuse. Consider renaming to reflect semantics or add Javadoc clarifying what the map key/value represent.

Copilot uses AI. Check for mistakes.
private String mountOptions;

Expand Down Expand Up @@ -79,12 +79,12 @@ public void setMountOptions(String mountOptions) {
this.mountOptions = mountOptions;
}

public List<String> getVolumePaths() {
return volumePaths;
public Map<String, String> getVolumePathsAndUuids() {
return volumePathsAndUuids;
}

public void setVolumePaths(List<String> volumePaths) {
this.volumePaths = volumePaths;
public void setVolumePathsAndUuids(Map<String, String> volumePathsAndUuids) {
this.volumePathsAndUuids = volumePathsAndUuids;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ 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);
Map<String, String> volumePaths = getVolumePaths(vmVolumes, Collections.emptyList());
command.setVolumePathsAndUuids(volumePaths);
}

BackupAnswer answer = null;
Expand Down Expand Up @@ -229,7 +229,7 @@ public boolean restoreVMFromBackup(VirtualMachine vm, Backup backup) {
restoreCommand.setBackupRepoAddress(backupRepository.getAddress());
restoreCommand.setMountOptions(backupRepository.getMountOptions());
restoreCommand.setVmName(vm.getName());
restoreCommand.setVolumePaths(getVolumePaths(volumes));
restoreCommand.setVolumePathsAndUuids(getVolumePaths(volumes, backedVolumes));
restoreCommand.setVmExists(vm.getRemoved() == null);
restoreCommand.setVmState(vm.getState());

Expand All @@ -244,8 +244,8 @@ public boolean restoreVMFromBackup(VirtualMachine vm, Backup backup) {
return answer.getResult();
}

private List<String> getVolumePaths(List<VolumeVO> volumes) {
List<String> volumePaths = new ArrayList<>();
private Map<String, String> getVolumePaths(List<VolumeVO> volumes, List<Backup.VolumeInfo> backedVolumes) {
Map<String, String> volumePaths = new HashMap<>();
for (VolumeVO volume : volumes) {
StoragePoolVO storagePool = primaryDataStoreDao.findById(volume.getPoolId());
Comment on lines +247 to 250
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getVolumePaths builds a HashMap, but both backup (nasbackup.sh) and restore logic rely on the first iterated disk being the ROOT volume (deviceId 0). HashMap iteration order is undefined and can reorder volumes, causing root/datadisk filenames to be generated/looked up incorrectly. Use an insertion-ordered structure (e.g., LinkedHashMap) and ensure volumes are inserted in deviceId order (root first).

Copilot uses AI. Check for mistakes.
if (Objects.isNull(storagePool)) {
Expand All @@ -259,8 +259,27 @@ private List<String> getVolumePaths(List<VolumeVO> volumes) {
} else {
volumePathPrefix = String.format("/mnt/%s", storagePool.getUuid());
}
volumePaths.add(String.format("%s/%s", volumePathPrefix, volume.getPath()));
// Build current volume path (destination for restore)
String currentVolumePath = String.format("%s/%s", volumePathPrefix, volume.getPath());

// Find backed volume path (used for backup filename lookup)
String backedVolumePath = volume.getPath();
boolean hasBackedVolumes = backedVolumes != null && !backedVolumes.isEmpty();
if (hasBackedVolumes) {
Optional<Backup.VolumeInfo> opt = backedVolumes.stream()
.filter(bv -> bv.getUuid().equals(volume.getUuid())).findFirst();
if (opt.isPresent()) {
Backup.VolumeInfo backedVolume = opt.get();
if (backedVolume.getPath() != null && !backedVolume.getPath().isEmpty()) {
// Use the backed volume path (path at time of backup) for filename lookup
backedVolumePath = backedVolume.getPath();
}
}
}

volumePaths.put(currentVolumePath, backedVolumePath);
}

return volumePaths;
}

Expand Down Expand Up @@ -299,7 +318,7 @@ public Pair<Boolean, String> restoreBackedUpVolume(Backup backup, String volumeU
restoreCommand.setBackupRepoType(backupRepository.getType());
restoreCommand.setBackupRepoAddress(backupRepository.getAddress());
restoreCommand.setVmName(vmNameAndState.first());
restoreCommand.setVolumePaths(Collections.singletonList(String.format("%s/%s", dataStore.getLocalPath(), volumeUUID)));
restoreCommand.setVolumePathsAndUuids(Collections.singletonMap(String.format("%s/%s", dataStore.getLocalPath(), volumeUUID), volumeUUID));
restoreCommand.setDiskType(volume.getVolumeType().name().toLowerCase(Locale.ROOT));
restoreCommand.setMountOptions(backupRepository.getMountOptions());
restoreCommand.setVmExists(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;

@ResourceWrapper(handles = RestoreBackupCommand.class)
Expand All @@ -58,21 +58,22 @@ public Answer execute(RestoreBackupCommand command, LibvirtComputingResource ser
String mountOptions = command.getMountOptions();
Boolean vmExists = command.isVmExists();
String diskType = command.getDiskType();
List<String> volumePaths = command.getVolumePaths();
Map<String, String> volumePathsAndUuids = command.getVolumePathsAndUuids();
String restoreVolumeUuid = command.getRestoreVolumeUUID();

String newVolumeId = null;
try {
if (Objects.isNull(vmExists)) {
String volumePath = volumePaths.get(0);
Map.Entry<String, String> firstEntry = volumePathsAndUuids.entrySet().iterator().next();
String volumePath = firstEntry.getKey();
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);
restoreVolumesOfExistingVM(volumePathsAndUuids, backupPath, backupRepoType, backupRepoAddress, mountOptions);
} else {
restoreVolumesOfDestroyedVMs(volumePaths, vmName, backupPath, backupRepoType, backupRepoAddress, mountOptions);
restoreVolumesOfDestroyedVMs(volumePathsAndUuids, vmName, backupPath, backupRepoType, backupRepoAddress, mountOptions);
}
} catch (CloudRuntimeException e) {
String errorMessage = "Failed to restore backup for VM: " + vmName + ".";
Expand All @@ -86,16 +87,17 @@ public Answer execute(RestoreBackupCommand command, LibvirtComputingResource ser
return new BackupAnswer(command, true, newVolumeId);
}

private void restoreVolumesOfExistingVM(List<String> volumePaths, String backupPath,
private void restoreVolumesOfExistingVM(Map<String,String> volumePaths, String backupPath,
String backupRepoType, String backupRepoAddress, String mountOptions) {
String diskType = "root";
String mountDirectory = mountBackupDirectory(backupRepoAddress, backupRepoType, mountOptions);
try {
for (int idx = 0; idx < volumePaths.size(); idx++) {
String volumePath = volumePaths.get(idx);
Pair<String, String> bkpPathAndVolUuid = getBackupPath(mountDirectory, volumePath, backupPath, diskType, null);
for (Map.Entry<String, String> entry : volumePaths.entrySet()) {
String currentVolumePath = entry.getKey();
String backedVolumePath = entry.getValue();
Pair<String, String> bkpPathAndVolUuid = getBackupPath(mountDirectory, backedVolumePath, backupPath, diskType, null);
diskType = "datadisk";
if (!replaceVolumeWithBackup(volumePath, bkpPathAndVolUuid.first())) {
if (!replaceVolumeWithBackup(currentVolumePath, bkpPathAndVolUuid.first())) {
Comment on lines +90 to +100
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restoreVolumesOfExistingVM derives diskType as root for the first iterated entry and datadisk for the rest. With a generic Map, entry iteration order is not guaranteed, so this can look for root.<uuid>.qcow2 using a datadisk UUID (or vice versa) and fail restore. Pass an ordered collection (or explicit per-volume diskType/deviceId) so ROOT is handled deterministically.

Copilot uses AI. Check for mistakes.
throw new CloudRuntimeException(String.format("Unable to restore backup for volume [%s].", bkpPathAndVolUuid.second()));
}
}
Expand All @@ -106,16 +108,17 @@ private void restoreVolumesOfExistingVM(List<String> volumePaths, String backupP

}

private void restoreVolumesOfDestroyedVMs(List<String> volumePaths, String vmName, String backupPath,
private void restoreVolumesOfDestroyedVMs(Map<String, String> volumePaths, String vmName, String backupPath,
String backupRepoType, String backupRepoAddress, String mountOptions) {
String mountDirectory = mountBackupDirectory(backupRepoAddress, backupRepoType, mountOptions);
String diskType = "root";
try {
for (int i = 0; i < volumePaths.size(); i++) {
String volumePath = volumePaths.get(i);
Pair<String, String> bkpPathAndVolUuid = getBackupPath(mountDirectory, volumePath, backupPath, diskType, null);
for (Map.Entry<String, String> entry : volumePaths.entrySet()) {
String currentVolumePath = entry.getKey();
String backedVolumePath = entry.getValue();
Pair<String, String> bkpPathAndVolUuid = getBackupPath(mountDirectory, backedVolumePath, backupPath, diskType, null);
diskType = "datadisk";
if (!replaceVolumeWithBackup(volumePath, bkpPathAndVolUuid.first())) {
if (!replaceVolumeWithBackup(currentVolumePath, bkpPathAndVolUuid.first())) {
throw new CloudRuntimeException(String.format("Unable to restore backup for volume [%s].", bkpPathAndVolUuid.second()));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package com.cloud.hypervisor.kvm.resource.wrapper;

import com.amazonaws.util.CollectionUtils;
import com.cloud.agent.api.Answer;
import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
import com.cloud.resource.CommandWrapper;
Expand All @@ -32,6 +31,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Objects;

@ResourceWrapper(handles = TakeBackupCommand.class)
Expand All @@ -43,7 +43,7 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
final String backupRepoType = command.getBackupRepoType();
final String backupRepoAddress = command.getBackupRepoAddress();
final String mountOptions = command.getMountOptions();
final List<String> diskPaths = command.getVolumePaths();
final Map<String, String> diskPathsAndUuids = command.getVolumePathsAndUuids();

List<String[]> commands = new ArrayList<>();
commands.add(new String[]{
Expand All @@ -54,7 +54,7 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
"-s", backupRepoAddress,
"-m", Objects.nonNull(mountOptions) ? mountOptions : "",
"-p", backupPath,
"-d", (Objects.nonNull(diskPaths) && !diskPaths.isEmpty()) ? String.join(",", diskPaths) : ""
"-d", (Objects.nonNull(diskPathsAndUuids) && !diskPathsAndUuids.isEmpty()) ? String.join(",", diskPathsAndUuids.keySet()) : ""
});

Pair<Integer, String> result = Script.executePipedCommands(commands, libvirtComputingResource.getCmdsTimeout());
Expand All @@ -65,7 +65,7 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
}

long backupSize = 0L;
if (CollectionUtils.isNullOrEmpty(diskPaths)) {
if (diskPathsAndUuids == null || diskPathsAndUuids.isEmpty()) {
List<String> outputLines = Arrays.asList(result.second().trim().split("\n"));
if (!outputLines.isEmpty()) {
backupSize = Long.parseLong(outputLines.get(outputLines.size() - 1).trim());
Expand Down
Loading