Copilot commented on code in PR #12549:
URL: https://github.com/apache/cloudstack/pull/12549#discussion_r2781449366
##########
core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java:
##########
@@ -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)
Review Comment:
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.
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -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());
Review Comment:
`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).
##########
core/src/main/java/org/apache/cloudstack/backup/RestoreBackupCommand.java:
##########
@@ -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;
Review Comment:
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.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##########
@@ -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()) :
""
});
Review Comment:
The `-d` argument order controls whether nasbackup.sh names the first disk
as `root.<uuid>.qcow2` and the rest as `datadisk.<uuid>.qcow2`.
`String.join(",", diskPathsAndUuids.keySet())` inherits whatever iteration
order the `Map` has, which can be non-deterministic and lead to misnamed
backups. Ensure the disk paths are provided in a deterministic deviceId order
(root first), e.g., by using an ordered collection or explicitly ordering
before joining.
##########
core/src/main/java/org/apache/cloudstack/backup/RestoreBackupCommand.java:
##########
@@ -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;
Review Comment:
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.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -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())) {
Review Comment:
`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.
##########
core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java:
##########
@@ -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)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
