refactor(backup): per-volume parent paths for incremental NAS backup

Address abh1sar's review at NASBackupProvider.java:340 — backup files on the
NAS are named after each volume's own UUID (root.<uuid>.qcow2 and
datadisk.<uuid>.qcow2), so a single parentPath shared across every disk in
the loop would have rebased every data disk's new qcow2 onto the *root*
parent file. Mirroring how RestoreBackupCommand passes a list of per-volume
backup files, TakeBackupCommand now carries a parentPaths list (one per VM
volume in deviceId order) and the script rebases each disk against the
matching entry.

Wire changes:
* TakeBackupCommand: replace parentPath:String with parentPaths:List<String>
* LibvirtTakeBackupCommandWrapper: pass --parent-paths <csv> (was --parent-path)
* nasbackup.sh: parse --parent-paths into an array, index by disk position
  in the rebase loop, fail loudly if the list is shorter than DISK_PATHS
* NASBackupProvider: new composeParentBackupPaths(parent, vmId) reads the
  parent's backedUpVolumes list (uuid order = deviceId order at parent time)
  and emits the correctly-named file per position. Returns null when the
  current VM's volume count differs from the parent — decideChain then
  falls back to a fresh full rather than risk a misaligned rebase.
* ChainDecision: parentPath:String -> parentPaths:List<String>

All 14 unit tests still pass. The bash script's --parent-path arg is
renamed to --parent-paths (no orchestrator outside this PR ever populated
the old single-path arg, so no compatibility shim needed).

Author: jmsperu

core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java

@@ -40,7 +40,13 @@ public class TakeBackupCommand extends Command {
     private String mode;          // "full" or "incremental"; null => legacy behaviour (script default)
     private String bitmapNew;     // Checkpoint/bitmap name to create with this backup (timestamp-based)
     private String bitmapParent;  // Incremental: parent bitmap to read changes since
-    private String parentPath;    // Incremental: parent backup file path on the mounted NAS (for qemu-img rebase)
+
+    // Per-volume parent backup file paths (one per VM volume, ordered by deviceId — same
+    // order as volumePaths). The script rebases each new qcow2 onto the matching parent.
+    // Addresses abh1sar review at NASBackupProvider.java:340 — backup file UUIDs differ
+    // across volumes, so a single parentPath would have rebased every data disk onto the
+    // root file. New callers MUST populate parentPaths.
+    private List<String> parentPaths;
 
     public TakeBackupCommand(String vmName, String backupPath) {
         super();
@@ -136,12 +142,12 @@ public void setBitmapParent(String bitmapParent) {
         this.bitmapParent = bitmapParent;
     }
 
-    public String getParentPath() {
-        return parentPath;
+    public List<String> getParentPaths() {
+        return parentPaths;
     }
 
-    public void setParentPath(String parentPath) {
-        this.parentPath = parentPath;
+    public void setParentPaths(List<String> parentPaths) {
+        this.parentPaths = parentPaths;
     }
 
     @Override

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

@@ -204,19 +204,23 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
      * parent bitmap and parent file path.
      */
     static final class ChainDecision {
-        final String mode;          // "full" or "incremental"
+        final String mode;            // "full" or "incremental"
         final String bitmapNew;
-        final String bitmapParent;  // null for full
-        final String parentPath;    // null for full
-        final String chainId;       // chain identifier this backup belongs to
-        final int chainPosition;    // 0 for full, N for the Nth incremental in the chain
-
-        private ChainDecision(String mode, String bitmapNew, String bitmapParent, String parentPath,
+        final String bitmapParent;    // null for full
+        // Per-volume parent backup file paths, one per current VM volume in deviceId order.
+        // null/empty for full. Replaces the single parentPath field — each volume needs its
+        // own parent file because backup files are named after each volume's own UUID
+        // (root.<uuid>.qcow2 / datadisk.<uuid>.qcow2), abh1sar review at line 340.
+        final List<String> parentPaths;
+        final String chainId;         // chain identifier this backup belongs to
+        final int chainPosition;      // 0 for full, N for the Nth incremental in the chain
+
+        private ChainDecision(String mode, String bitmapNew, String bitmapParent, List<String> parentPaths,
                               String chainId, int chainPosition) {
             this.mode = mode;
             this.bitmapNew = bitmapNew;
             this.bitmapParent = bitmapParent;
-            this.parentPath = parentPath;
+            this.parentPaths = parentPaths;
             this.chainId = chainId;
             this.chainPosition = chainPosition;
         }
@@ -226,10 +230,10 @@ static ChainDecision fullStart(String bitmapName) {
                     UUID.randomUUID().toString(), 0);
         }
 
-        static ChainDecision incremental(String bitmapNew, String bitmapParent, String parentPath,
+        static ChainDecision incremental(String bitmapNew, String bitmapParent, List<String> parentPaths,
                                          String chainId, int chainPosition) {
             return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL, bitmapNew, bitmapParent,
-                    parentPath, chainId, chainPosition);
+                    parentPaths, chainId, chainPosition);
         }
 
         boolean isIncremental() {
@@ -305,11 +309,18 @@ protected ChainDecision decideChain(VirtualMachine vm) {
             return ChainDecision.fullStart(newBitmap);
         }
 
-        // The script needs the parent backup's on-NAS file path so it can rebase the new
-        // qcow2 onto it. The path is stored relative to the NAS mount point — the script
-        // resolves it inside its mount session.
-        String parentPath = composeParentBackupPath(parent);
-        return ChainDecision.incremental(newBitmap, activeCheckpoint, parentPath,
+        // The script needs the parent backup's on-NAS file path PER VOLUME so it can rebase
+        // each new qcow2 onto the matching parent. The paths are stored relative to the NAS
+        // mount root — the script resolves them inside its mount session. When alignment
+        // fails (volume count changed, etc.) compose returns null and we fall back to full
+        // so we don't risk corrupting the chain.
+        List<String> parentPaths = composeParentBackupPaths(parent, vm.getId());
+        if (parentPaths == null) {
+            LOG.debug("VM {} parent backup {} volume layout no longer matches current VM — forcing full",
+                    vm.getInstanceName(), parent.getUuid());
+            return ChainDecision.fullStart(newBitmap);
+        }
+        return ChainDecision.incremental(newBitmap, activeCheckpoint, parentPaths,
                 parentChainId, parentChainPosition + 1);
     }
 
@@ -353,15 +364,51 @@ private String readDetail(Backup backup, String key) {
     }
 
     /**
-     * Compose the on-NAS path of a parent backup's root-disk qcow2. Relative to the NAS mount,
-     * matches the layout written by {@code nasbackup.sh} ({@code <backupPath>/root.<volUuid>.qcow2}).
+     * Compose the on-NAS path of EVERY parent backup file (one per VM volume) in the same
+     * order the script will iterate the current VM's disks (deviceId asc). Relative to the
+     * NAS mount, matches the layout written by {@code nasbackup.sh}:
+     *   first  disk -> {@code <backupPath>/root.<volUuid>.qcow2}
+     *   others      -> {@code <backupPath>/datadisk.<volUuid>.qcow2}
+     *
+     * Returns {@code null} if the parent's stored volume list can't be aligned with the
+     * current VM's volumes (count mismatch / different volume identities). In that case the
+     * caller should force a fresh full so we don't accidentally rebase a data disk onto the
+     * root parent — exactly the bug abh1sar flagged at line 340.
      */
-    private String composeParentBackupPath(Backup parent) {
-        // backupPath is stored as externalId by createBackupObject — e.g. "i-2-1234-VM/2026.04.27.13.45.00".
-        // Volume UUID for the root volume is what the script keys backup files on.
-        VolumeVO rootVolume = volumeDao.getInstanceRootVolume(parent.getVmId());
-        String volUuid = rootVolume == null ? "root" : rootVolume.getUuid();
-        return parent.getExternalId() + "/root." + volUuid + ".qcow2";
+    private List<String> composeParentBackupPaths(Backup parent, long vmId) {
+        // backupPath is stored as externalId by createBackupObject — e.g.
+        // "i-2-1234-VM/2026.04.27.13.45.00".
+        String dir = parent.getExternalId();
+        if (dir == null || dir.isEmpty()) {
+            return null;
+        }
+
+        // Read the parent's backed-up volume list (uuid order = deviceId order at the time
+        // the parent was taken). The script names files as root.<uuid>.qcow2 for the first
+        // entry and datadisk.<uuid>.qcow2 for subsequent entries — match that here.
+        List<Backup.VolumeInfo> parentVols = parent.getBackedUpVolumes();
+        if (parentVols == null || parentVols.isEmpty()) {
+            return null;
+        }
+
+        // Sanity: the current VM must have the same number of volumes. If it doesn't (volume
+        // added or removed since the parent), positional alignment is unsafe — fall back to
+        // full at the caller.
+        List<VolumeVO> currentVols = volumeDao.findByInstance(vmId);
+        if (currentVols == null || currentVols.size() != parentVols.size()) {
+            return null;
+        }
+
+        List<String> paths = new ArrayList<>(parentVols.size());
+        for (int i = 0; i < parentVols.size(); i++) {
+            String volUuid = parentVols.get(i).getUuid();
+            if (volUuid == null || volUuid.isEmpty()) {
+                return null;
+            }
+            String prefix = (i == 0) ? "root" : "datadisk";
+            paths.add(dir + "/" + prefix + "." + volUuid + ".qcow2");
+        }
+        return paths;
     }
 
     /**
@@ -479,7 +526,7 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
         command.setMode(decision.mode);
         command.setBitmapNew(decision.bitmapNew);
         command.setBitmapParent(decision.bitmapParent);
-        command.setParentPath(decision.parentPath);
+        command.setParentPaths(decision.parentPaths);
 
         if (VirtualMachine.State.Stopped.equals(vm.getState())) {
             List<VolumeVO> vmVolumes = volumeDao.findByInstance(vm.getId());

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

@@ -93,9 +93,9 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
             argv.add("--bitmap-parent");
             argv.add(command.getBitmapParent());
         }
-        if (command.getParentPath() != null && !command.getParentPath().isEmpty()) {
-            argv.add("--parent-path");
-            argv.add(command.getParentPath());
+        if (command.getParentPaths() != null && !command.getParentPaths().isEmpty()) {
+            argv.add("--parent-paths");
+            argv.add(String.join(",", command.getParentPaths()));
         }
 
         List<String[]> commands = new ArrayList<>();

scripts/vm/hypervisor/kvm/nasbackup.sh

@@ -37,7 +37,11 @@ QUIESCE=""
 MODE=""               # "full" or "incremental"; empty => legacy full-only behavior (no checkpoint created)
 BITMAP_NEW=""         # Bitmap/checkpoint name to create with this backup (e.g. "backup-1711586400")
 BITMAP_PARENT=""      # For incremental: parent bitmap name to read changes since
-PARENT_PATH=""        # For incremental: parent backup file path (used as backing for qemu-img rebase)
+PARENT_PATHS=""       # For incremental: comma-separated list of parent backup file paths,
+                      # one per VM volume in the same order as DISK_PATHS. Each new qcow2
+                      # is rebased onto its corresponding parent file. Required because
+                      # data-disk backup files don't share the root volume's UUID — abh1sar
+                      # review at NASBackupProvider.java:340.
 # Rebase operation parameters (used only with -o rebase, for chain repair on delete-middle)
 REBASE_TARGET=""      # The qcow2 file to repoint at a new backing (mount-relative path)
 REBASE_NEW_BACKING="" # The new backing parent file (mount-relative path)
@@ -128,8 +132,8 @@ backup_running_vm() {
   local make_checkpoint=0
   case "$effective_mode" in
     incremental)
-      if [[ -z "$BITMAP_PARENT" || -z "$BITMAP_NEW" || -z "$PARENT_PATH" ]]; then
-        echo "incremental mode requires --bitmap-parent, --bitmap-new, and --parent-path"
+      if [[ -z "$BITMAP_PARENT" || -z "$BITMAP_NEW" || -z "$PARENT_PATHS" ]]; then
+        echo "incremental mode requires --bitmap-parent, --bitmap-new, and --parent-paths"
         cleanup
         exit 1
       fi
@@ -278,17 +282,29 @@ XML
   # - For INCREMENTAL: rebase the resulting thin qcow2 onto its parent so the chain is self-describing
   #   (so a future restore can flatten without external chain metadata).
   name="root"
+  # PARENT_PATHS arrives as a comma-separated list, one entry per VM volume in the same
+  # order as DISK_PATHS. Split into a bash array so we can index by disk position.
+  local -a parent_paths_arr=()
+  if [[ "$effective_mode" == "incremental" && -n "$PARENT_PATHS" ]]; then
+    IFS=',' read -ra parent_paths_arr <<< "$PARENT_PATHS"
+  fi
+  local disk_idx=0
   while read -r disk fullpath; do
     if [[ "$effective_mode" == "incremental" ]]; then
       volUuid="${fullpath##*/}"
       if [[ "$fullpath" == /dev/drbd/by-res/* ]]; then
         volUuid=$(get_linstor_uuid_from_path "$fullpath")
       fi
-      # PARENT_PATH from the orchestrator is the parent backup's path relative to the
-      # NAS mount root (e.g. "i-2-X/2026.04.27.12.00.00/root.UUID.qcow2"). Convert it to
-      # a path relative to THIS new qcow2's directory so the backing reference resolves
-      # correctly the next time the NAS is mounted (mount points are ephemeral).
-      local parent_abs="$mount_point/$PARENT_PATH"
+      # Pick this disk's specific parent file. Each volume's backup is named after its
+      # own UUID so a single PARENT_PATH would rebase data disks onto the root parent —
+      # exactly the bug abh1sar called out at NASBackupProvider.java:340.
+      if [[ $disk_idx -ge ${#parent_paths_arr[@]} ]]; then
+        echo "PARENT_PATHS list shorter than DISK_PATHS — missing parent for disk index $disk_idx"
+        cleanup
+        exit 1
+      fi
+      local this_parent_rel="${parent_paths_arr[$disk_idx]}"
+      local parent_abs="$mount_point/$this_parent_rel"
       if [[ ! -f "$parent_abs" ]]; then
         echo "Parent backup file does not exist on NAS: $parent_abs"
         cleanup
@@ -302,6 +318,7 @@ XML
         exit 1
       fi
       name="datadisk"
+      disk_idx=$((disk_idx + 1))
       continue
     fi
     if [[ "$fullpath" != /dev/drbd/by-res/* ]]; then
@@ -556,8 +573,8 @@ while [[ $# -gt 0 ]]; do
       shift
       shift
       ;;
-    --parent-path)
-      PARENT_PATH="$2"
+    --parent-paths)
+      PARENT_PATHS="$2"
       shift
       shift
       ;;