abh1sar commented on code in PR #14194:
URL: https://github.com/apache/cloudstack/pull/14194#discussion_r4250058877


##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -134,6 +137,217 @@ get_linstor_uuid_from_device() {
   return 1
 }
 
+# ---------------------------------------------------------------------------
+# Content-based backup path (libvirt pull mode).
+#
+# Used for raw block-device disks such as LINSTOR/DRBD. Those cannot carry QEMU
+# persistent dirty bitmaps -- persistence is a qcow2-only feature -- so libvirt
+# checkpoints, and with them the push-mode incremental path, are unavailable.
+#
+# Instead libvirt is asked for a pull-mode backup: it starts an NBD server 
exposing a
+# point-in-time view of each disk, held consistent by copy-before-write into 
local
+# fleecing scratch images. The backup is then derived from that export:
+#
+#   content-full         qemu-img convert -> sparse standalone qcow2. Zero 
detection
+#                        happens while streaming, so the fully allocated 
push-mode
+#                        output and its separate re-convert pass are both 
avoided.
+#   content-incremental  a qcow2 overlay backed by the NBD export, then a SAFE 
rebase
+#                        onto the parent backup. That merges exactly the 
clusters which
+#                        differ between export and parent into the overlay, 
yielding a
+#                        delta qcow2 backed by the parent -- the same chain 
shape the
+#                        push-mode incremental produces, so restore/delete are 
unchanged.
+#
+# Unlike a storage-snapshot based diff there is no trailing DRBD metadata to 
clip: the
+# DRBD device, and therefore the NBD export, is already net-sized.
+#
+# Assumes mount_operation() already ran, so $dest and $mount_point are set.
+# ---------------------------------------------------------------------------
+backup_running_vm_pull() {
+  local effective_mode="$1"
+
+  # Scratch area for the NBD socket and libvirt's fleecing images. Never on 
the NAS:
+  # copy-before-write runs inline with guest writes, so it wants fast local 
storage with
+  # room for the write churn during the backup. 711 lets the qemu process 
traverse in to
+  # the files libvirt creates as root.
+  local scratch_dir
+  scratch_dir=$(mktemp -d "$SCRATCH_PARENT/csbackup-scratch.XXXXXX") || {
+    echo "Failed to create scratch directory under $SCRATCH_PARENT"; cleanup; 
exit 1;
+  }
+  chmod 711 "$scratch_dir"
+  local nbd_sock="$scratch_dir/nbd.sock"
+  local backupxml="$scratch_dir/backup.xml"
+
+  # The kernel caps UNIX socket paths at 108 bytes. A long scratch dir would 
otherwise
+  # fail deep inside qemu-img with an opaque error, so reject it up front.
+  if [[ ${#nbd_sock} -ge 108 ]]; then
+    echo "Pull-mode NBD socket path is too long (${#nbd_sock} bytes, limit 
108): $nbd_sock"
+    echo "Set a shorter scratch dir via -S (agent.properties 
nas.backup.pull.scratch.dir)"
+    rm -rf "$scratch_dir"
+    cleanup
+    exit 1
+  fi
+
+  # Snapshot the disk list once so the backup XML and the read-back loop stay 
in sync.
+  local -a disks
+  mapfile -t disks < <(
+    virsh -c qemu:///system domblklist "$VM" --details 2>/dev/null | awk 
'$2=="disk"{print $3, $4}'
+  )
+  if [[ ${#disks[@]} -eq 0 ]]; then
+    echo "No disks found for vm $VM"
+    rm -rf "$scratch_dir"
+    cleanup
+    exit 1
+  fi
+
+  # Resolve each disk's parent backup up front. PARENT_PATHS is comma 
separated, one entry
+  # per volume in the same order as the disk list. Anything missing degrades 
this run to a
+  # full backup rather than failing it, mirroring the push-mode fallback.
+  local -a parent_paths_arr=()
+  if [[ "$effective_mode" == "content-incremental" ]]; then
+    if [[ -z "$PARENT_PATHS" ]]; then
+      log -e "content-incremental: no parent paths supplied -- falling back to 
full"
+      echo "INCREMENTAL_FALLBACK=true"
+      effective_mode="content-full"
+    else
+      IFS=',' read -ra parent_paths_arr <<< "$PARENT_PATHS"
+      if [[ ${#parent_paths_arr[@]} -lt ${#disks[@]} ]]; then
+        log -e "content-incremental: parent path list shorter than disk list 
-- falling back to full"
+        echo "INCREMENTAL_FALLBACK=true"
+        effective_mode="content-full"
+      else
+        local pp
+        for pp in "${parent_paths_arr[@]}"; do
+          if [[ ! -f "$mount_point/$pp" ]]; then
+            log -e "content-incremental: parent $mount_point/$pp missing on 
NAS -- falling back to full"
+            echo "INCREMENTAL_FALLBACK=true"
+            effective_mode="content-full"
+            break
+          fi
+        done
+      fi
+    fi
+  fi
+
+  # Pull-mode backup XML: one NBD export and one fleecing scratch image per 
disk.
+  {
+    echo "<domainbackup mode='pull'>"
+    echo "<server transport='unix' socket='$nbd_sock'/>"
+    echo "<disks>"
+    local entry
+    for entry in "${disks[@]}"; do
+      echo "<disk name='${entry%% *}' backup='yes' type='file' 
backupmode='full'><scratch file='$scratch_dir/${entry%% *}.scratch'/></disk>"
+    done
+    echo "</disks></domainbackup>"
+  } > "$backupxml"
+
+  local thaw=0
+  if [[ ${QUIESCE} == "true" ]]; then
+    if virsh -c qemu:///system qemu-agent-command "$VM" 
'{"execute":"guest-fsfreeze-freeze"}' > /dev/null 2>/dev/null; then
+      thaw=1
+    fi
+  fi
+
+  # backup-begin establishes the point-in-time and starts the NBD server.
+  local backup_begin=0
+  local backup_out
+  if backup_out=$(virsh -c qemu:///system backup-begin --domain "$VM" 
--backupxml "$backupxml" 2>&1); then
+    backup_begin=1
+  fi
+

Review Comment:
   The backup job will be left forever running if there is an interrupt or 
error before calling domjobabort.
   Consider adding a trap and cleanup.



##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -255,8 +264,30 @@ static ChainDecision incremental(String bitmapNew, String 
bitmapParent, List<Str
                     parentPaths, chainId, chainPosition);
         }
 
+        /**
+         * Full backup on the content-based (pull mode) path: no bitmap, no 
checkpoint, but it
+         * does anchor a chain so later content-incrementals can hang off it.
+         */
+        static ChainDecision contentFull() {
+            return new ChainDecision(NASBackupChainKeys.TYPE_CONTENT_FULL, 
null, null, null,

Review Comment:
   Instead of overloading the `mode` for each ChainDecision, I suggest adding a 
new `method` field to it.
   The `method` can be qcow2 / content / none / rbd_diff(ceph - can be added 
later).
   The mode will still say full and incremental for each method. 
   
    #14386 also adds rbd-full and rbd-incremental. I think a new method will be 
a better choice for both PRs.
   
   cc @weizhouapache 
   



-- 
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]

Reply via email to