backup(nas): move backup-mode policy + stdout markers from script to wrapper

jmsperu · jmsperu · commit 73c4206c211d · 2026-06-14T00:40:47.000+03:00
Address abh1sar review on PR #13074 (nasbackup.sh:155, :193, :358; LibvirtTake BackupCommandWrapper.java:124). The script was carrying caller-side policy: arg validation, fallback decisions, and stdout markers that the wrapper had to parse out before the size-parsing logic could run. Move that policy into Java and use dedicated exit codes for the signals the wrapper needs. Script (scripts/vm/hypervisor/kvm/nasbackup.sh): * Drop the per-mode required-args checks (the wrapper now pre-validates). * Replace the INCREMENTAL_FALLBACK stdout marker with exit code 21 (EXIT_INCREMENTAL_UNSUPPORTED): emitted when the running-VM path can't re-register the parent checkpoint, and when the stopped-VM path was asked for incremental. The wrapper retries the script as a full backup and sets incrementalFallback on the BackupAnswer. * Replace the BITMAP_CREATED stdout marker with exit code 22 (EXIT_BITMAP_NOT_SEEDED), emitted only by the stopped-VM path when qemu-img bitmap --add failed for every source disk. Backup file is valid but no usable bitmap exists on the host; wrapper records bitmapCreated=null so NASBackupProvider clears active_checkpoint_id and the next backup starts a fresh full chain. Running-VM success path no longer needs a marker — libvirt's backup-begin atomically creates the checkpoint. LibvirtTakeBackupCommandWrapper.java: * Pre-validate incremental args (mode-vs-bitmapNew/Parent/parentPaths) before invoking the script. Returns a failed BackupAnswer on missing args, keeping the script agnostic to caller policy. * Extract runBackupScript() so the same code can fire the retry-as-full after EXIT_INCREMENTAL_UNSUPPORTED without duplicating arg assembly. * On EXIT_INCREMENTAL_UNSUPPORTED + requestedMode==incremental, re-invoke with mode=full and only --bitmap-new (drop --bitmap-parent/--parent-paths); set incrementalFallback=true on the eventual answer. * On EXIT_BITMAP_NOT_SEEDED, treat as success but set bitmapCreated=null. * Drop the stdout-marker stripping loop (markers no longer emitted), and the separate BITMAP_CREATED parsing — bitmapCreated mirrors command.getBitmapNew() unless the not-seeded exit code says otherwise. NASBackupProvider.java: * Refresh the two comment blocks that referenced the old BITMAP_CREATED= stdout signal to describe the new exit-code path. No behaviour change in this file.
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
@@ -406,12 +406,12 @@ private List<String> composeParentBackupPaths(Backup parent, long vmId) {
      * other providers can implement their own chain semantics without schema changes.
      */
     private void persistChainMetadata(Backup backup, ChainDecision decision, String bitmapFromAgent) {
-        // Only persist nas.bitmap_name when the agent confirmed it via BITMAP_CREATED=. If we
-        // fall back to decision.bitmapNew when the agent didn't emit BITMAP_CREATED= (e.g.,
-        // stopped-VM path where the qemu-img pre-seed failed, or running-VM path where libvirt
-        // backup-begin succeeded but the bitmap line wasn't surfaced for any reason), we'd
-        // anchor the next incremental on a bitmap that doesn't exist on the host. Better to
-        // leave it empty so the next backup sees no checkpoint and starts a fresh full chain.
+        // Only persist nas.bitmap_name when the agent confirmed the bitmap exists on the host.
+        // The agent wrapper sets bitmapFromAgent=null when nasbackup.sh exits
+        // EXIT_BITMAP_NOT_SEEDED (=22) — currently only the stopped-VM path where qemu-img
+        // bitmap --add failed on every source disk. Anchoring the next incremental on a
+        // bitmap that doesn't exist would force a non-recoverable failure, so we leave the
+        // detail empty and let the next backup start a fresh full chain.
         if (bitmapFromAgent != null && !bitmapFromAgent.isEmpty()) {
             backupDetailsDao.persist(new BackupDetailVO(backup.getId(), NASBackupChainKeys.BITMAP_NAME, bitmapFromAgent, true));
         }
@@ -562,10 +562,11 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
                 persistChainMetadata(backupVO, effective, answer.getBitmapCreated());
                 // Pin the VM's active_checkpoint_id to whichever bitmap the agent actually
                 // created. This is the only valid parent for the next incremental (see
-                // decideChain). For the stopped-VM offline path BITMAP_CREATED is null —
-                // no bitmap exists on the host, so we also clear any stale detail from a
-                // prior online backup. Either way, after this step the detail accurately
-                // reflects what's on the running QEMU (or absence thereof).
+                // decideChain). When the agent wrapper sets bitmapCreated=null (script exited
+                // EXIT_BITMAP_NOT_SEEDED — stopped-VM path where qemu-img bitmap --add failed),
+                // no bitmap exists on the host, so we also clear any stale detail from a prior
+                // online backup. Either way, after this step the detail accurately reflects
+                // what's on the running QEMU (or absence thereof).
                 String confirmedBitmap = answer.getBitmapCreated();
                 if (confirmedBitmap != null) {
                     upsertVmActiveCheckpoint(vm.getId(), confirmedBitmap);
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java
@@ -42,6 +42,14 @@
 @ResourceWrapper(handles = TakeBackupCommand.class)
 public class LibvirtTakeBackupCommandWrapper extends CommandWrapper<TakeBackupCommand, Answer, LibvirtComputingResource> {
     private static final Integer EXIT_CLEANUP_FAILED = 20;
+    private static final Integer EXIT_INCREMENTAL_UNSUPPORTED = 21;
+    // Backup file is valid, but the host has no bitmap to anchor the next incremental on.
+    // Used by the stopped-VM path when qemu-img bitmap --add failed for every source disk.
+    private static final Integer EXIT_BITMAP_NOT_SEEDED = 22;
+
+    private static final String MODE_FULL = "full";
+    private static final String MODE_INCREMENTAL = "incremental";
+
     @Override
     public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvirtComputingResource) {
         final String vmName = command.getVmName();
@@ -54,6 +62,13 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
         KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr();
         int timeout = command.getWait() > 0 ? command.getWait() * 1000 : libvirtComputingResource.getCmdsTimeout();
 
+        // Pre-validate incremental args here rather than relying on the script to error out.
+        // Keeps the script agnostic to caller policy (it just does what it's told).
+        String validationError = validateBackupArgs(command);
+        if (validationError != null) {
+            return new BackupAnswer(command, false, validationError);
+        }
+
         List<String> diskPaths = new ArrayList<>();
         if (Objects.nonNull(volumePaths)) {
             for (int idx = 0; idx < volumePaths.size(); idx++) {
@@ -69,6 +84,59 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
             }
         }
 
+        final String requestedMode = command.getMode();
+        Pair<Integer, String> result = runBackupScript(libvirtComputingResource, command, vmName, backupRepoType, backupRepoAddress,
+                mountOptions, backupPath, diskPaths, requestedMode,
+                command.getBitmapNew(), command.getBitmapParent(), command.getParentPaths(), timeout);
+
+        boolean incrementalFallback = false;
+        if (result.first() == EXIT_INCREMENTAL_UNSUPPORTED && MODE_INCREMENTAL.equals(requestedMode)) {
+            // Script told us the incremental can't proceed (parent checkpoint can't be
+            // re-registered, or VM is stopped). Re-invoke as full with the same --bitmap-new
+            // so the chain restarts cleanly. Drop --bitmap-parent + --parent-paths since
+            // they no longer apply.
+            logger.info("nasbackup.sh signalled incremental unsupported for VM " + vmName + " — retrying as full");
+            result = runBackupScript(libvirtComputingResource, command, vmName, backupRepoType, backupRepoAddress,
+                    mountOptions, backupPath, diskPaths, MODE_FULL,
+                    command.getBitmapNew(), null, null, timeout);
+            incrementalFallback = true;
+        }
+
+        boolean bitmapSeeded = true;
+        if (result.first() == EXIT_BITMAP_NOT_SEEDED) {
+            // Backup file is valid; the host just has no bitmap. Treat as success but
+            // mark bitmapCreated=null so the orchestrator clears active_checkpoint_id.
+            bitmapSeeded = false;
+        } else if (result.first() != 0) {
+            logger.debug("Failed to take VM backup: " + result.second());
+            BackupAnswer answer = new BackupAnswer(command, false, result.second().trim());
+            if (result.first() == EXIT_CLEANUP_FAILED) {
+                logger.debug("Backup cleanup failed");
+                answer.setNeedsCleanup(true);
+            }
+            return answer;
+        }
+
+        String stdout = result.second().trim();
+        long backupSize = parseBackupSize(stdout, diskPaths);
+
+        BackupAnswer answer = new BackupAnswer(command, true, stdout);
+        answer.setSize(backupSize);
+        // bitmapCreated mirrors what we asked the script to create — except when the
+        // script exited EXIT_BITMAP_NOT_SEEDED, in which case the host has no bitmap
+        // and the orchestrator must clear active_checkpoint_id.
+        answer.setBitmapCreated(bitmapSeeded ? command.getBitmapNew() : null);
+        answer.setIncrementalFallback(incrementalFallback);
+        return answer;
+    }
+
+    /**
+     * Run nasbackup.sh once with the given args. Returns the exit code + captured stdout.
+     */
+    private Pair<Integer, String> runBackupScript(LibvirtComputingResource libvirtComputingResource,
+            TakeBackupCommand command, String vmName, String backupRepoType, String backupRepoAddress,
+            String mountOptions, String backupPath, List<String> diskPaths, String mode,
+            String bitmapNew, String bitmapParent, List<String> parentPaths, int timeout) {
         List<String> argv = new ArrayList<>(Arrays.asList(
                 libvirtComputingResource.getNasBackupPath(),
                 "-o", "backup",
@@ -80,83 +148,75 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
                 "-q", command.getQuiesce() != null && command.getQuiesce() ? "true" : "false",
                 "-d", diskPaths.isEmpty() ? "" : String.join(",", diskPaths)
         ));
-        // Incremental NAS backup args (only added when the orchestrator asked for full/inc mode).
-        if (command.getMode() != null && !command.getMode().isEmpty()) {
+        if (mode != null && !mode.isEmpty()) {
             argv.add("-M");
-            argv.add(command.getMode());
+            argv.add(mode);
         }
-        if (command.getBitmapNew() != null && !command.getBitmapNew().isEmpty()) {
+        if (bitmapNew != null && !bitmapNew.isEmpty()) {
             argv.add("--bitmap-new");
-            argv.add(command.getBitmapNew());
+            argv.add(bitmapNew);
         }
-        if (command.getBitmapParent() != null && !command.getBitmapParent().isEmpty()) {
+        if (bitmapParent != null && !bitmapParent.isEmpty()) {
             argv.add("--bitmap-parent");
-            argv.add(command.getBitmapParent());
+            argv.add(bitmapParent);
         }
-        if (command.getParentPaths() != null && !command.getParentPaths().isEmpty()) {
+        if (parentPaths != null && !parentPaths.isEmpty()) {
             argv.add("--parent-paths");
-            argv.add(String.join(",", command.getParentPaths()));
+            argv.add(String.join(",", parentPaths));
         }
 
         List<String[]> commands = new ArrayList<>();
         commands.add(argv.toArray(new String[0]));
+        return Script.executePipedCommands(commands, timeout);
+    }
 
-        Pair<Integer, String> result = Script.executePipedCommands(commands, timeout);
-
-        if (result.first() != 0) {
-            logger.debug("Failed to take VM backup: " + result.second());
-            BackupAnswer answer = new BackupAnswer(command, false, result.second().trim());
-            if (result.first() == EXIT_CLEANUP_FAILED) {
-                logger.debug("Backup cleanup failed");
-                answer.setNeedsCleanup(true);
-            }
-            return answer;
+    /**
+     * Return a human-readable validation error string, or {@code null} if the command's
+     * incremental-backup args are internally consistent.
+     */
+    private String validateBackupArgs(TakeBackupCommand command) {
+        String mode = command.getMode();
+        if (mode == null || mode.isEmpty()) {
+            return null; // legacy full-only — no extra args expected
         }
-
-        // Strip out our incremental marker lines before parsing size, so the legacy
-        // numeric-suffix parser keeps working.
-        String stdout = result.second().trim();
-        String bitmapCreated = null;
-        boolean incrementalFallback = false;
-        StringBuilder filtered = new StringBuilder();
-        for (String line : stdout.split("\n")) {
-            String trimmed = line.trim();
-            if (trimmed.startsWith("BITMAP_CREATED=")) {
-                // The marker only confirms the bitmap was actually created on the disks
-                // (it isn't, e.g., for stopped-VM RBD/LINSTOR sources). The name itself is
-                // the value we already passed via --bitmap-new, so use that rather than
-                // re-parsing the echoed value.
-                bitmapCreated = command.getBitmapNew();
-                continue;
+        if (MODE_INCREMENTAL.equals(mode)) {
+            if (command.getBitmapNew() == null || command.getBitmapNew().isEmpty()) {
+                return "incremental mode requires bitmapNew";
             }
-            if (trimmed.startsWith("INCREMENTAL_FALLBACK=")) {
-                incrementalFallback = true;
-                continue;
+            if (command.getBitmapParent() == null || command.getBitmapParent().isEmpty()) {
+                return "incremental mode requires bitmapParent";
             }
-            if (filtered.length() > 0) {
-                filtered.append("\n");
+            if (command.getParentPaths() == null || command.getParentPaths().isEmpty()) {
+                return "incremental mode requires parentPaths";
             }
-            filtered.append(line);
+            return null;
         }
-        String numericOutput = filtered.toString().trim();
+        if (MODE_FULL.equals(mode)) {
+            if (command.getBitmapNew() == null || command.getBitmapNew().isEmpty()) {
+                return "full mode requires bitmapNew (the bitmap to create for the next incremental)";
+            }
+            return null;
+        }
+        return "Unknown backup mode: " + mode;
+    }
 
+    /**
+     * Sum the per-disk size lines emitted by nasbackup.sh. Single-volume mode emits one
+     * line containing just the byte count; multi-volume mode emits one line per disk
+     * whose first whitespace-separated token is the byte count.
+     */
+    private long parseBackupSize(String stdout, List<String> diskPaths) {
         long backupSize = 0L;
         if (CollectionUtils.isNullOrEmpty(diskPaths)) {
-            List<String> outputLines = Arrays.asList(numericOutput.split("\n"));
+            List<String> outputLines = Arrays.asList(stdout.split("\n"));
             if (!outputLines.isEmpty()) {
                 backupSize = Long.parseLong(outputLines.get(outputLines.size() - 1).trim());
             }
         } else {
-            String[] outputLines = numericOutput.split("\n");
-            for(String line : outputLines) {
+            for (String line : stdout.split("\n")) {
                 backupSize = backupSize + Long.parseLong(line.split(" ")[0].trim());
             }
         }
-
-        BackupAnswer answer = new BackupAnswer(command, true, stdout);
-        answer.setSize(backupSize);
-        answer.setBitmapCreated(bitmapCreated);
-        answer.setIncrementalFallback(incrementalFallback);
-        return answer;
+        return backupSize;
     }
 }
diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh
@@ -49,6 +49,11 @@ logFile="/var/log/cloudstack/agent/agent.log"
 
 EXIT_CLEANUP_FAILED=20
 EXIT_INCREMENTAL_UNSUPPORTED=21
+# Stopped-VM backup succeeded but the qemu-img bitmap pre-seed failed on every disk
+# (e.g. RBD/LINSTOR sources, or the bitmap --add operation errored). The backup file
+# is valid; the wrapper records bitmapCreated=null so the orchestrator clears the
+# VM's active_checkpoint_id and the next backup starts a fresh full chain.
+EXIT_BITMAP_NOT_SEEDED=22
 
 log() {
   [[ "$verb" -eq 1 ]] && builtin echo "$@"
@@ -128,23 +133,12 @@ backup_running_vm() {
 
   # Determine effective mode for this run.
   # Legacy callers (no -M argument) get the original full-only behavior with no checkpoint.
+  # The Java wrapper (LibvirtTakeBackupCommandWrapper) pre-validates required args before
+  # invoking the script; the case below is a defensive fallback for direct invocations.
   local effective_mode="${MODE:-legacy-full}"
   local make_checkpoint=0
   case "$effective_mode" in
-    incremental)
-      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
-      make_checkpoint=1
-      ;;
-    full)
-      if [[ -z "$BITMAP_NEW" ]]; then
-        echo "full mode requires --bitmap-new (the bitmap to create for the next incremental)"
-        cleanup
-        exit 1
-      fi
+    incremental|full)
       make_checkpoint=1
       ;;
     legacy-full)
@@ -174,10 +168,12 @@ backup_running_vm() {
         : # parent checkpoint re-registered; the incremental can proceed against it
       else
         # No saved checkpoint XML (e.g. a backup taken before this fix) or redefine failed.
-        # Fall back to a full so the chain restarts cleanly instead of failing the backup.
-        echo "INCREMENTAL_FALLBACK=full (parent checkpoint $BITMAP_PARENT could not be re-registered)"
-        effective_mode="full"
-        BITMAP_PARENT=""
+        # Signal the Java wrapper to retry as a full backup so the chain restarts cleanly
+        # instead of failing the backup. The wrapper is responsible for the retry and for
+        # recording incrementalFallback=true on the resulting BackupAnswer.
+        log -e "incremental: parent checkpoint $BITMAP_PARENT could not be re-registered — exiting $EXIT_INCREMENTAL_UNSUPPORTED for caller-driven fallback"
+        cleanup
+        exit $EXIT_INCREMENTAL_UNSUPPORTED
       fi
     fi
   fi
@@ -338,9 +334,9 @@ backup_running_vm() {
     # Persist the FULL checkpoint XML next to this backup so a later incremental can
     # re-register this checkpoint with --redefine after the VM restarts (which wipes
     # libvirt's checkpoint registry but leaves the bitmap on the qcow2).
+    # The Java wrapper records bitmapCreated from its own --bitmap-new arg on success,
+    # so no stdout signal is needed here.
     virsh -c qemu:///system checkpoint-dumpxml "$VM" "$BITMAP_NEW" > "$dest/$BITMAP_NEW.checkpoint.xml" 2>/dev/null || true
-    # Echo the bitmap name on its own line so the Java caller can capture it for backup_details.
-    echo "BITMAP_CREATED=$BITMAP_NEW"
   fi
 
   umount $mount_point
@@ -349,13 +345,11 @@ backup_running_vm() {
 
 backup_stopped_vm() {
   # Stopped VMs cannot use libvirt's backup-begin (no QEMU process). Take a full
-  # backup via qemu-img convert. If the caller asked for incremental, fall back
-  # to full and signal the fallback so the orchestrator can record it as a full
-  # in the chain.
+  # backup via qemu-img convert. If the caller asked for incremental, signal the
+  # Java wrapper to retry as full and record the fallback on the BackupAnswer.
   if [[ "$MODE" == "incremental" ]]; then
-    # Emit on stdout so Script.executePipedCommands in LibvirtTakeBackupCommandWrapper
-    # can parse it and record the backup as FULL.
-    echo "INCREMENTAL_FALLBACK=full (VM stopped — incremental requires running VM)"
+    log -e "incremental: VM stopped — exiting $EXIT_INCREMENTAL_UNSUPPORTED for caller-driven fallback to full"
+    exit $EXIT_INCREMENTAL_UNSUPPORTED
   fi
 
   mount_operation
@@ -397,16 +391,17 @@ backup_stopped_vm() {
   done
   sync
 
-  # Surface the bitmap name we created so the orchestrator can persist it as
-  # the VM's active_checkpoint_id. Empty when sources weren't file-backed or
-  # qemu-img bitmap failed — orchestrator handles either case.
-  # Stdout (not stderr) so Script.executePipedCommands in the Java wrapper
-  # can parse it — matches the backup_running_vm path.
-  if [[ "$bitmap_seeded" == "1" ]]; then
-    echo "BITMAP_CREATED=$BITMAP_NEW"
-  fi
-
   ls -l --numeric-uid-gid $dest | awk '{print $5}'
+
+  # Signal to the Java wrapper whether a usable bitmap exists on the host. When the
+  # caller asked for BITMAP_NEW but we couldn't pre-seed it (RBD/LINSTOR sources, or
+  # qemu-img bitmap --add failed on every disk), exit $EXIT_BITMAP_NOT_SEEDED so the
+  # wrapper records bitmapCreated=null. The orchestrator (NASBackupProvider) then
+  # clears the VM's active_checkpoint_id and the next backup starts a fresh chain.
+  if [[ -n "$BITMAP_NEW" && "$bitmap_seeded" != "1" ]]; then
+    log -e "stopped-VM backup: bitmap pre-seed skipped or failed for all disks — exiting $EXIT_BITMAP_NOT_SEEDED so wrapper clears active_checkpoint_id"
+    exit $EXIT_BITMAP_NOT_SEEDED
+  fi
 }
 
 delete_backup() {
