jmsperu commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3535033020
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -172,9 +297,50 @@ backup_running_vm() {
sleep 5
done
- # Use qemu-img convert to sparsify linstor backups which get bloated due to
virsh backup-begin.
+ # Sparsify behavior:
+ # - For LINSTOR backups (existing): qemu-img convert sparsifies the bloated
output.
+ # - 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
Review Comment:
Done in 560c6d3. Removed the Linstor branch from the incremental path; kept
`get_linstor_uuid_from_path` in the full-backup paths, where Linstor is
supported.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -191,18 +357,69 @@ backup_running_vm() {
virsh -c qemu:///system domblklist "$VM" --details 2>/dev/null | awk
'$2=="disk"{print $3, $4}'
)
- rm -f $dest/backup.xml
+ rm -f $dest/backup.xml $dest/checkpoint.xml
sync
+ # Free the parent bitmap now that the incremental has been written and
rebased. The parent's
+ # delta is fully captured in this backup and BITMAP_NEW already tracks
changes going forward, so
+ # the parent bitmap is dead weight — left in place it accumulates metadata
and IO cost over a
+ # long chain. Remove it directly per-disk with block-dirty-bitmap-remove (a
clean free) rather
+ # than checkpoint-delete, which would MERGE the parent's dirty bits into
BITMAP_NEW and make the
+ # next incremental needlessly re-copy already-backed-up regions.
Best-effort: a failure here does
+ # not fail the backup (the data is already safe) — the bitmap is simply
reclaimed on a later run.
Review Comment:
Done in 560c6d3, compacted.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -113,20 +122,128 @@ backup_running_vm() {
mount_operation
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit
1; }
+ # 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|full)
+ make_checkpoint=1
+ ;;
+ legacy-full)
+ make_checkpoint=0
+ ;;
+ *)
+ echo "Unknown mode: $effective_mode"
+ cleanup
+ exit 1
+ ;;
+ esac
+
+ # When incremental, make sure the parent checkpoint is registered with
libvirt. CloudStack
+ # rebuilds the domain XML on every VM start, which wipes libvirt's in-memory
checkpoint
+ # registry, while the dirty bitmap persists on the qcow2 (QEMU re-loads it
on start). A
+ # fresh checkpoint-create cannot be used then — QEMU reports "Bitmap already
exists" — so the
+ # parent must be re-registered with --redefine. libvirt only needs the
checkpoint name and a
+ # creationTime for a redefine (the value need not be accurate — checkpoints
are ephemeral),
+ # so we synthesize a minimal XML on the fly instead of persisting the full
checkpoint dump.
+ #
+ # First verify the parent bitmap actually exists on the running qcow2 — it
can be absent after
+ # a migration even though the orchestrator's active_checkpoint says it
should be there. If it
+ # is gone, fall back to a full backup rather than letting backup-begin fail
below.
Review Comment:
Done in 560c6d3, compacted.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -113,20 +122,128 @@ backup_running_vm() {
mount_operation
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit
1; }
+ # 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|full)
+ make_checkpoint=1
+ ;;
+ legacy-full)
+ make_checkpoint=0
+ ;;
+ *)
+ echo "Unknown mode: $effective_mode"
+ cleanup
+ exit 1
+ ;;
+ esac
+
+ # When incremental, make sure the parent checkpoint is registered with
libvirt. CloudStack
+ # rebuilds the domain XML on every VM start, which wipes libvirt's in-memory
checkpoint
+ # registry, while the dirty bitmap persists on the qcow2 (QEMU re-loads it
on start). A
+ # fresh checkpoint-create cannot be used then — QEMU reports "Bitmap already
exists" — so the
+ # parent must be re-registered with --redefine. libvirt only needs the
checkpoint name and a
+ # creationTime for a redefine (the value need not be accurate — checkpoints
are ephemeral),
+ # so we synthesize a minimal XML on the fly instead of persisting the full
checkpoint dump.
+ #
+ # First verify the parent bitmap actually exists on the running qcow2 — it
can be absent after
+ # a migration even though the orchestrator's active_checkpoint says it
should be there. If it
+ # is gone, fall back to a full backup rather than letting backup-begin fail
below.
+ if [[ "$effective_mode" == "incremental" ]]; then
+ # The parent bitmap must be present on EVERY disk's qcow2, not just one of
them. A volume
+ # snapshot restore (or a partial migration) can wipe the bitmap on some
disks while leaving
+ # it on others; a plain "is the name anywhere in query-block" check passes
in that case and
+ # backup-begin then fails on the disk that is missing the bitmap. Require
the bitmap on all
+ # disks: compare the disk count to the number of disks reporting the
bitmap (tests 17/19).
+ disk_count=$(virsh -c qemu:///system domblklist "$VM" --details
2>/dev/null | awk '$2=="disk"{c++} END{print c+0}')
+ # Count DISKS that actually carry the parent bitmap, not raw name
occurrences. query-block
+ # lists each disk's bitmap under more than one node, so "grep -o name | wc
-l" double-counts:
+ # with two disks where only one has the bitmap it returns 2, is misread as
present-on-all, and
+ # the incremental then fails on the disk missing it (test 19). Parse
per-device exactly as
+ # LibvirtStartBackupCommandWrapper.getVmDiskPathHasFromCheckpointMap()
does (one count per
+ # inserted.file whose dirty-bitmaps contains the parent). The trailing "||
echo 0" also keeps a
+ # no-match from aborting the script under "set -eo pipefail" before the
fallback below runs
+ # (a snapshot restore wipes the bitmap on all disks, so nothing matches —
tests 17/18).
Review Comment:
Done in 560c6d3. Compacted to the essential logic and dropped the
inter-commit / test-number details.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -191,18 +357,69 @@ backup_running_vm() {
virsh -c qemu:///system domblklist "$VM" --details 2>/dev/null | awk
'$2=="disk"{print $3, $4}'
)
- rm -f $dest/backup.xml
+ rm -f $dest/backup.xml $dest/checkpoint.xml
sync
+ # Free the parent bitmap now that the incremental has been written and
rebased. The parent's
+ # delta is fully captured in this backup and BITMAP_NEW already tracks
changes going forward, so
+ # the parent bitmap is dead weight — left in place it accumulates metadata
and IO cost over a
+ # long chain. Remove it directly per-disk with block-dirty-bitmap-remove (a
clean free) rather
+ # than checkpoint-delete, which would MERGE the parent's dirty bits into
BITMAP_NEW and make the
+ # next incremental needlessly re-copy already-backed-up regions.
Best-effort: a failure here does
+ # not fail the backup (the data is already safe) — the bitmap is simply
reclaimed on a later run.
+ if [[ "$effective_mode" == "incremental" && -n "$BITMAP_PARENT" ]]; then
+ removed_from=0
+ parent_disk_count=0
+ while read -r node; do
+ [[ -z "$node" ]] && continue
+ parent_disk_count=$((parent_disk_count + 1))
+ if virsh -c qemu:///system qemu-monitor-command "$VM" \
+
"{\"execute\":\"block-dirty-bitmap-remove\",\"arguments\":{\"node\":\"$node\",\"name\":\"$BITMAP_PARENT\"}}"
\
+ > /dev/null 2>>"$logFile"; then
+ removed_from=$((removed_from + 1))
+ else
+ log -e "cleanup: failed to remove parent bitmap $BITMAP_PARENT on node
$node (non-fatal)"
+ fi
+ done < <(
+ virsh -c qemu:///system qemu-monitor-command "$VM"
'{"execute":"query-block"}' 2>/dev/null | python3 -c '
+import sys, json
+target = sys.argv[1]
+try:
+ data = json.load(sys.stdin)
+except Exception:
+ sys.exit(0)
+seen = set()
+for dev in data.get("return", []) or []:
+ inserted = dev.get("inserted") or {}
+ node = inserted.get("node-name")
+ if not node or node in seen:
+ continue
+ if any((b or {}).get("name") == target for b in
(inserted.get("dirty-bitmaps") or [])):
+ seen.add(node)
+ print(node)
+' "$BITMAP_PARENT" 2>/dev/null || true
+ )
+ if [[ "$parent_disk_count" -gt 0 && "$removed_from" -eq
"$parent_disk_count" ]]; then
+ # Signal the wrapper (mirrors the INCREMENTAL_FALLBACK marker
convention) so the management
+ # server can record that the parent bitmap was reclaimed. Printed before
the size line below.
+ echo "PARENT_BITMAP_DELETED=true"
+ log "cleanup: removed parent bitmap $BITMAP_PARENT from $removed_from
disk(s)"
+ fi
+ fi
Review Comment:
Done in 560c6d3. Removed the `PARENT_BITMAP_DELETED` marker end to end
(script echo + counters, the wrapper constant/parse/set/strip, the
`BackupAnswer` field and accessors, and the NAS consumer). The parent-bitmap
removal is now plain best-effort, with the else block logging any per-node
failure. The consumer only wrote a debug log and drove no orchestrator state
(the chain is pinned to `bitmapCreated`), so nothing else changes.
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -1701,6 +1705,14 @@ private boolean deleteCheckedBackup(Boolean forced,
BackupProvider backupProvide
reservationDao, resourceLimitMgr)) {
boolean result = backupProvider.deleteBackup(backup, forced);
if (result) {
+ // Chain-aware providers (e.g. NAS) physically remove several
backups per call
+ // (leaf + swept delete-pending ancestors) and decrement
resource count/usage and
+ // remove each DB row themselves, exactly once per removed
backup. Decrementing or
+ // removing again here would double-handle and destroy
delete-pending tombstones,
+ // so defer entirely to the provider for those.
+ if (backupProvider.handlesChainDeleteResourceAccounting()) {
Review Comment:
Done in 560c6d3. The chain-delete-accounting branch now calls
`checkAndGenerateUsageForLastBackupDeletedAfterOfferingRemove(vm, backup)`
before returning true, so the usage event still fires when a chain-aware
provider deletes the last backup.
--
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]
