Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4918150885 @jmsperu Can you please review https://github.com/apache/cloudstack/pull/13571? -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
weizhouapache merged PR #13074: URL: https://github.com/apache/cloudstack/pull/13074 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
harikrishna-patnala commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4914605033 great work @jmsperu and thank you @abh1sar -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
weizhouapache commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4914586848 great, thanks @abh1sar @jmsperu for your work ! merging -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
weizhouapache commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4913250627 @abh1sar thanks a lot for the review and testing. are all your comments addressed ? -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4912835287 [SF] Trillian test result (tid-16502) Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8 Total time taken: 59288 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13074-t16502-kvm-ol8.zip Smoke tests completed. 150 look OK, 3 have errors, 0 did not run Only failed and skipped tests results shown below: Test | Result | Time (s) | Test File --- | --- | --- | --- ContextSuite context=TestNASBackupAndRecovery>:setup | `Error` | 0.00 | test_backup_recovery_nas.py test_05_list_volumes_isrecursive | `Failure` | 0.06 | test_list_volumes.py test_07_list_volumes_listall | `Failure` | 0.09 | test_list_volumes.py test_01_vpn_usage | `Error` | 1.54 | test_usage.py -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4905060933 @abh1sar a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4905048528 @blueorangutan test -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4904682440 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18488 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4904198679 @abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4904191193 @blueorangutan package -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
github-actions[bot] commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4904129309 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4902088399
Merged latest `main` and resolved the conflict in `BackupManagerImpl.java`.
It was two complementary status criteria on the same search builder: kept both,
my `statusNeq` (excludes the `Hidden` chain tombstones) and the `backupStatus`
EQ filter from #13254. Both apply downstream (`setParameters("statusNeq",
Hidden)` and `setParametersIfNotNull("backupStatus", ...)`). Pushed as 4707ccd.
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
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
+incr
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3533661058
##
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:
```suggestion
```
Linstor doesn't support incremental backup. this can be removed.
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3528951522
##
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:
Comment too verbose
##
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 "|
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3528879718
##
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:
Don't need the PARENT_BITMAP_DELETED marker. Let's keep the bitmap delete
best effort to simplify the code. The else block should anyway catch and log
any errors.
We don't need to read the bitmap again.
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3528844959
##
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:
call checkAndGenerateUsageForLastBackupDeletedAfterOfferingRemove before
returning true
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4892685468 @jmsperu Need to resolve the conflict. This is from PR https://github.com/apache/cloudstack/pull/13254 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4878871879 +1 for **Option 1** from me too. Let this land in main first, then I will open a clean cherry-pick PR against `4.22` for 4.22.2. Rebasing this onto 4.22 and forward-merging (Option 2) is higher effort, more conflict-prone across the diverged branches, and would block the 4.23 merge on 4.22 landing first. The backport should be a small, self-contained cherry-pick once the diff is final. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4878871669 Thanks @abh1sar — both addressed in 4552c441c3. **1. Storage guard in `decideChain`:** added `allVolumesOnCheckpointCapableStorage()` — if any of the VM's volumes sits on storage that cannot carry a per-disk checkpoint (Ceph-RBD, Linstor), the backup falls back to legacy-full, so those storages are never regressed by an incremental attempt. Covered by 3 new unit tests. **2. Parent-bitmap cleanup:** implemented, with one deliberate deviation from the sketch I want to flag. Rather than `checkpoint-delete` in the wrapper, the cleanup runs in `nasbackup.sh` after a successful incremental and frees the parent **per-disk via `block-dirty-bitmap-remove`**. Reason: `checkpoint-delete` on a parent that has a child makes libvirt **merge** the parent's dirty bits into the new bitmap, which would make the *next* incremental re-copy already-backed-up regions. `block-dirty-bitmap-remove` is a clean free with no merge. It is gated exactly as you suggested (`!incrementalFallback && bitmapParent != null`) and is best-effort (a failure logs a warning and never fails the backup, since the data is already written). The reclaim is surfaced to the orchestrator via a `PARENT_BITMAP_DELETED` marker -> `BackupAnswer.parentBitmapDeleted` -> provider log, mirroring the existing `INCREMENTAL_FALLBACK` pattern, so it is auditable. Validated on a live host (libvirt/QEMU 10.0.0): after the incremental the parent bitmap is gone and only the new one remains, the **next incremental still succeeds** (chain intact), and incrementals stay small — confirming the free-not-merge behavior. `NASBackupProviderTest` (18 tests) is green. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3521100737
##
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##
@@ -69,8 +86,63 @@ public Answer execute(TakeBackupCommand command,
LibvirtComputingResource libvir
}
}
-List commands = new ArrayList<>();
-commands.add(new String[]{
+Pair result =
runBackupScript(libvirtComputingResource, command, vmName, backupRepoType,
backupRepoAddress,
+mountOptions, backupPath, diskPaths, command.getMode(),
+command.getBitmapNew(), command.getBitmapParent(),
command.getParentPaths(), timeout);
+
+if (result.first() != 0) {
+logger.debug("Failed to take VM backup: " + result.second());
+BackupAnswer answer = new BackupAnswer(command, false,
result.second().trim());
+if (EXIT_CLEANUP_FAILED.equals(result.first())) {
+logger.debug("Backup cleanup failed");
+answer.setNeedsCleanup(true);
+}
+return answer;
+}
+
+// The script self-heals to a full backup when an incremental can't
proceed (e.g. the
+// parent checkpoint can't be re-registered) and signals it with
INCREMENTAL_FALLBACK
+// on stdout. Detect it, then strip the marker line before parsing the
backup size.
+String rawStdout = result.second();
+boolean incrementalFallback =
rawStdout.contains(INCREMENTAL_FALLBACK_MARKER);
+String stdout = stripMarkerLines(rawStdout).trim();
+long backupSize = parseBackupSize(stdout, diskPaths);
+
Review Comment:
We shouldn't store the old bitmap data forever. With time it will build up
into metadata storage cost and IO cost.
We should delete the parent_bitmap from the disks if the incremental backup
was successful.
This can be done by deleting the checkpoint.
```
if (!incremenalFallback && command.getBitmapParent() != null) {
Script.runSimpleBashScript(String.format(CHECKPOINT_DELETE_COMMAND,
vmName, checkpointName));
}
```
Can you implement and test?
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4878102126 ## Test Status - 3 July Added tests for Shared Mount Point and Local Storage Remaining - 0 Failing - 0 Passing - 40 | # | Section | TestCase | Expected | Result | |-|-|---||---| | 1 | No Harm test (config disabled) | Create backup | No change in behaviour | Pass | | 2 | | Delete backups | No change in behaviour | Pass | | 3 | | Restore from backup | No change in behaviour | Pass | | 4 | | Restore and attach volume from backup | No change in behaviour | Pass | | 5 | | Create new instance from Backup | No change in behaviour | Pass | | 6 | | Assign/Remove backup offering | No change in behaviour | Pass | | 7 | | Backup VM with multiple data disks | No change in behaviour — all disks backed up, no extra metadata | Pass | | | | | | | | 8 | Create Backups | Create incremental backups | Verify incremental backups are taken with the correct db entries and bitmap in the qcow2 file. Verify that the backing file is set to the parent | Pass | | 9 | | Incremental backups after stop-start VM | Verify that incremental backup was taken after stopping and starting the VM | Pass | | 10 | | Test backup chain is terminated after N bac
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3521041772
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -1701,6 +1701,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()) {
+return true;
Review Comment:
https://github.com/apache/cloudstack/commit/ae2a6b2afeed2db8ca0cfc0b12ea34f0edd22ec3
doesnt fix this issue
```suggestion
checkAndGenerateUsageForLastBackupDeletedAfterOfferingRemove(vm, backup);
return true;
```
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4878051799 @jmsperu Please fix the conflicts also -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3521026199
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -168,6 +205,290 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+/**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+static final class ChainDecision {
+final String mode;// "full" or "incremental"
+final String bitmapNew;
+final String bitmapParent;// null for full
+// Per-volume parent backup file paths, one per current VM volume in
deviceId order.
+// null/empty for full. Each volume needs its own parent file because
backup files
+// are named after each volume's own UUID (root..qcow2 /
datadisk..qcow2).
+final List 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 parentPaths,
+ String chainId, int chainPosition) {
+this.mode = mode;
+this.bitmapNew = bitmapNew;
+this.bitmapParent = bitmapParent;
+this.parentPaths = parentPaths;
+this.chainId = chainId;
+this.chainPosition = chainPosition;
+}
+
+static ChainDecision fullStart(String bitmapName) {
+return new ChainDecision(NASBackupChainKeys.TYPE_FULL, bitmapName,
null, null,
+UUID.randomUUID().toString(), 0);
+}
+
+/**
+ * Decision used when the incremental feature is disabled: a plain
full backup that
+ * creates no bitmap and carries no chain identity, so nothing
chain/checkpoint-related
+ * is sent to the agent or persisted. Keeps the feature-off path
byte-for-byte legacy.
+ */
+static ChainDecision legacyFull() {
+return new ChainDecision(NASBackupChainKeys.TYPE_LEGACY_FULL,
null, null, null, null, 0);
+}
+
+static ChainDecision incremental(String bitmapNew, String
bitmapParent, List parentPaths,
+ String chainId, int chainPosition) {
+return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL,
bitmapNew, bitmapParent,
+parentPaths, chainId, chainPosition);
+}
+
+boolean isIncremental() {
+return NASBackupChainKeys.TYPE_INCREMENTAL.equals(mode);
+}
+
+boolean isLegacyFull() {
+return NASBackupChainKeys.TYPE_LEGACY_FULL.equals(mode);
+}
+}
+
+/**
+ * Decides whether the next backup for {@code vm} should be a fresh full
or an incremental
+ * appended to the existing chain. Stopped VMs are always full (libvirt
{@code backup-begin}
+ * requires a running QEMU process). The {@code nas.backup.full.every}
ConfigKey controls
+ * how many backups (full + incrementals) form one chain before a new full
is forced.
+ *
+ * The decision is anchored on the VM's {@code
nas.active_checkpoint_id} detail, which
+ * records the bitmap that currently exists on the running QEMU. After a
restore that
+ * detail is cleared, so the next backup is automatically full — even
though there may be
+ * a more recent "last backup taken" row in the database. The decision
deliberately avoids
+ * relying on "last backup taken", because that row is misleading after a
restore.
+ */
+protected ChainDecision decideChain(VirtualMachine vm) {
+// Master switch — when the operator disables incrementals at the zone
level the backup
+// behaves exactly like the pre-incremental full-only path: no bitmap
is generated and no
+// chain/checkpoint metadata is created, sent to the agent, or
persisted (legacy-full).
+Boolean incrementalEnabled =
NASBackupIncrementalEnabled.valueIn(vm.getDataCenterId());
+if (incrementalEnabled == null || !incrementalEnabled) {
+return ChainDecision.legacyFull();
+}
+
Review Comment:
Can we add the condition that if any of the volumes in the VM is not one of
NFS/SharedMountPoint/LocalStorage then return legacyFull?
```
i.e, !(ScopeType.HOST.equals(storagePool.getScope()) ||
Storage.StoragePoolType.SharedMountPoint.equals(storagePool.getPoolType() ||
Storage.StoragePoolType.NetworkFilesystem.equals(storagePool.getPoolType())
```
I think this is
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
github-actions[bot] commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4877522202 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4876962015 > @jmsperu firstly, thanks for all the work. I see the current PR is targeted for main branch and seems to be in a good state to get this in 4.23 release. On the other hand I'm also thinking if we can somehow make this into current LTS release of 4.22 (the next one 4.22.2) that will help many other users on the current LTS release. > > I'm thinking of 2 options, > > 1. Let this PR merge in main branch and once it is done we can plan for a backporting of the same changes to 4.22 branch targeting 4.22.2 release. > 2. Rebase this PR itself to 4.22 branch and then forward merge to main once it is merged into 4.22 branch. Not sure about the efforts for this option. > > Let me know your thoughts too. > > cc @weizhouapache @sureshanaparti @abh1sar @harikrishna-patnala I personally would prefer 1. 2 would be higher effort and might delay merging of this PR. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
harikrishna-patnala commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4862453791 @jmsperu firstly, thanks for all the work. I see the current PR is targeted for main branch and seems to be in a good state to get this in 4.23 release. On the other hand I'm also thinking if we can somehow make this into current LTS release of 4.22 (the next one 4.22.2) that will help many other users on the current LTS release. I'm thinking of 2 options, 1. Let this PR merge in main branch and once it is done we can plan for a backporting of the same changes to 4.22 branch targeting 4.22.2 release. 2. Rebase this PR itself to 4.22 branch and then forward merge to main once it is merged into 4.22 branch. Not sure about the efforts for this option. Let me know your thoughts too. cc @weizhouapache @sureshanaparti @abh1sar -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4862099927 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18435 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4861867911 @abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4861864796 @blueorangutan package -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3508213029
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -113,20 +122,104 @@ 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}')
+bitmap_count=$(virsh -c qemu:///system qemu-monitor-command "$VM"
'{"execute":"query-block"}' 2>/dev/null | grep -o "\"$BITMAP_PARENT\"" | wc -l
| tr -d ' ')
Review Comment:
Good catch, fixed in ae2a6b2. The probe no longer aborts on a no-match: I
replaced the `grep | wc` count with a per-device parse that returns 0 when
nothing matches and ends in `|| echo 0`, so a snapshot restore that clears the
bitmap on all disks now falls through to the full-backup fallback instead of
exiting under `set -eo pipefail`. Verified tests 17 and 18.
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -113,20 +122,104 @@ 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
+
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3507991179
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -1701,6 +1701,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()) {
+return true;
Review Comment:
@jmsperu This needs to be addressed.
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3507984969
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -113,20 +122,104 @@ 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}')
+bitmap_count=$(virsh -c qemu:///system qemu-monitor-command "$VM"
'{"execute":"query-block"}' 2>/dev/null | grep -o "\"$BITMAP_PARENT\"" | wc -l
| tr -d ' ')
Review Comment:
can you reconsider using
LibvirtStartBackupCommandWrapper.getVmDiskPathHasFromCheckpointMap() for this
purpose?
otherwise you can modify the script accordingly
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3507980232
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -113,20 +122,104 @@ 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}')
+bitmap_count=$(virsh -c qemu:///system qemu-monitor-command "$VM"
'{"execute":"query-block"}' 2>/dev/null | grep -o "\"$BITMAP_PARENT\"" | wc -l
| tr -d ' ')
Review Comment:
For the case where we have two volumes and one of the volume is missing
bitmap, bitmap_count is returned as 2 because the query-block command outputs
the bitmap twice for the same volume.
So it tries to take incremental backup and fails
```
"iops_wr": 0,
"ro": false,
"node-name": "libvirt-1-format",
"backing_file_depth": 0,
"drv": "qcow2",
"iops": 0,
"bps_wr": 0,
"write_threshold": 0,
"dirty-bitmaps": [
{
"name": "backup-1782927110",
"recording": true,
"persistent": true,
"busy": false,
"granularity": 65536,
"count": 0
},
{
"name": "backup-1782927118",
"recording": true,
"persistent": true,
"busy": false,
"granularity": 65536,
"count": 0
}
],
"encrypted": false,
"bps": 0,
"bps_rd": 0,
"cache": {
"no-flush": false,
"direct": true,
"writeback": true
},
"file":
"/mnt/c6880607-9fcc-395b-912a-7e7d1ba1b77a/140b1d43-6318-4dc1-b0e5-fcd675dfc2d2"
```
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3507774229
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -113,20 +122,104 @@ 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}')
+bitmap_count=$(virsh -c qemu:///system qemu-monitor-command "$VM"
'{"execute":"query-block"}' 2>/dev/null | grep -o "\"$BITMAP_PARENT\"" | wc -l
| tr -d ' ')
Review Comment:
```suggestion
bitmap_count=$(virsh -c qemu:///system qemu-monitor-command "$VM"
'{"execute":"query-block"}' 2>/dev/null | grep -o "\"$BITMAP_PARENT\"" | wc -l
| tr -d ' ') || true
```
the bitmap-presence check breaks under set -e.
When the parent bitmap is not present on any disk (volume snapshot restore),
grep -o matches nothing and exits 1. With pipefail, that makes the whole
pipeline — and thus this assignment — exit 1, and set -e kills the script right
here, before the fallback-to-full check on line 164 ever runs. And backup fails
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4858264191 ## Test Status - 1 July Remaining - 0 Failing - 1 (18 - Review comments posted for the fix) Passing - 37 | # | Section | TestCase | Expected | Result | |-|-|---||---| | 1 | No Harm test (config disabled) | Create backup | No change in behaviour | Pass | | 2 | | Delete backups | No change in behaviour | Pass | | 3 | | Restore from backup | No change in behaviour | Pass | | 4 | | Restore and attach volume from backup | No change in behaviour | Pass | | 5 | | Create new instance from Backup | No change in behaviour | Pass | | 6 | | Assign/Remove backup offering | No change in behaviour | Pass | | 7 | | Backup VM with multiple data disks | No change in behaviour — all disks backed up, no extra metadata | Pass | | | | | | | | 8 | Create Backups | Create incremental backups | Verify incremental backups are taken with the correct db entries and bitmap in the qcow2 file. Verify that the backing file is set to the parent | Pass | | 9 | | Incremental backups after stop-start VM | Verify that incremental backup was taken after stopping and starting the VM | Pass | | 10 | | Test backup chain is terminated after N backups
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4846358756 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18420 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4845927488 @abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4845909660 @blueorangutan package -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4833885143 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18409 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3492256410
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -239,9 +568,31 @@ public Pair takeBackup(final
VirtualMachine vm, Boolean quiesce
backupVO.setDate(new Date());
backupVO.setSize(answer.getSize());
backupVO.setStatus(Backup.Status.BackedUp);
+// If the agent fell back to full (stopped VM mid-incremental
cycle), record this
+// backup as a full and start a new chain.
+ChainDecision effective = decision;
+if (answer.getIncrementalFallback()) {
+effective = ChainDecision.fullStart(decision.bitmapNew);
+backupVO.setType("FULL");
Review Comment:
Bug: backupVO.setType("FULL") is silently dropped by the UpdateBuilder in
the incremental fallback path.
CloudStack's GenericDaoBase.update() uses a CGLib proxy (UpdateBuilder) to
track which fields changed. It
derives the tracked field name from the setter name — setType → "type".
But the actual Java field in
BackupVO is named backupType, so _allAttributes.get("type") returns null
and the change is never added to
the UPDATE SQL.
This only manifests in the incremental-fallback branch
(answer.getIncrementalFallback() == true) because
that is the only place where setType is called on an enhanced entity
returned from backupDao.persist.
Everywhere else, setType is called on a plain new BackupVO() before
persist, which goes through a raw
INSERT that reads field values directly — bypassing UpdateBuilder
entirely.
Fix: rename BackupVO.backupType → BackupVO.type so the field name matches
what UpdateBuilder resolves from
the setter.
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-486409 @DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
DaanHoogland commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4833324531 @blueorangutan package -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4827330491 @abh1sar pushed `7c2574b2` addressing the 5 failing cases from your 24-Jun run: - **13** (full→incr after volume migration): `composeParentBackupPaths` now names the parent file from the volume **path** (`Backup.VolumeInfo.getPath()`) instead of the uuid — they diverge after a volume migration. - **17 / 19** (bitmap missing on all/one disk): the incremental pre-flight now requires the parent bitmap on **every** disk (was "present on any disk"), so a volume snapshot restore or partial migration self-heals to a full backup. - **35** (delete last backup): `deleteBackupFileAndRow` clears the VM's `active_checkpoint_id` when the deleted backup owns that bitmap, so the next backup starts a fresh full chain. - **37** (delete non-leaf still shows in list view): replaced the `nas.delete_pending` detail with **`Backup.Status.Hidden`** as you suggested — tombstones are filtered from `listBackups`, and Restore/RestoreAndAttach/CreateInstance/Delete are refused for free since they already require `BackedUp`. Chain GC is unaffected because `listByVmId` is status-agnostic, so the sweep still sees Hidden ancestors. Dropped the now-unused `DELETE_PENDING` key. Re your **No-Harm when the feature is disabled** note (and the `sanity_checks` / `rebase_backup` comments) — those are already handled by the earlier commits: the legacy-full path persists no chain/checkpoint metadata and doesn't touch `active_checkpoint_id` via the `isLegacyFull()` guard. `NASBackupProviderTest` passes 15/15 (updated for the Hidden tombstone). Could you re-run the suite when you have a slot? 🙏 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4789527735 ## Test Status - 24 June Remaining - 8 Failing - 5 (Review comments posted for fixes) Passing - 25 | Status| Count | Test case numbers | |---|---|| | Pass | 25| 1–12, 14–16, 18, 20–22, 31–34, 36, 38 | | Fail | 5 | 13, 17, 19, 35, 37 | | Remaining | 8 | 23–30 | | # | Section | TestCase | Expected | Result | |-|-|---||---| | 1 | No Harm test (config disabled) | Create backup | No change in behaviour | Pass | | 2 | | Delete backups | No change in behaviour | Pass | | 3 | | Restore from backup | No change in behaviour | Pass | | 4 | | Restore and attach volume from backup | No change in behaviour | Pass | | 5 | | Create new instance from Backup | No change in behaviour | Pass | | 6 | | Assign/Remove backup offering | No change in behaviour | Pass | | 7 | | Backup VM with multiple data disks | No change in behaviour — all disks backed up, no extra metadata | Pass | | | | | | | | 8 | Create Backups | Create incremental backups | Verify incremental backups are taken with the correct db entries and bitmap in the qcow2 file. Verify that the backing file is set to the parent | Pass | | 9 | | Incremental backups after stop-start
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3467102369
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -113,20 +122,97 @@ 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
+if ! virsh -c qemu:///system qemu-monitor-command "$VM"
'{"execute":"query-block"}' 2>/dev/null | grep -q "\"$BITMAP_PARENT\""; then
+ log -e "incremental: parent bitmap $BITMAP_PARENT not present on the
qcow2 — falling back to full"
+ echo "INCREMENTAL_FALLBACK=true"
+ effective_mode="full"
+fi
+ fi
Review Comment:
This doesn't work if only one of the disks is missing the bitmap. This
checks if bitmap is present on any of the disks. We have to check all disks.
This check and decision can be moved to Java.
`LibvirtStartBackupCommandWrapper.getVmDiskPathHasFromCheckpointMap()`
already does this. It can be moved to a common file like
LibvirtComputingResource and both LibvirtStartBackupCommandWrapper and
LibvirtTakeBackupCommandWrapper using it.
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -495,9 +863,42 @@ public boolean deleteBackup(Backup backup, boolean forced)
{
throw new CloudRuntimeException(String.format("Unable to find a
running KVM host in zone %d to delete backup %s", backup.getZoneId(),
backup.getUuid()));
}
-DeleteBackupCommand command = new
DeleteBackupCommand(backup.getExternalId(), backupRepository.getType(),
-backupRepository.getAddress(),
backupRepository.getMountOptions());
+// Backups outside any tracked chain (legacy or non-incremental
providers) are
+// deleted straight away — no children semantics apply.
+if (readDetail(backup, NASBackupChainKeys.CHAIN_ID) == null) {
+return deleteBackupFileAndRow(backup, backupRepository, host);
+}
+
+// Snapshot-style cascade: defer the on-NAS rm + DB row while there
are live children,
+// mark this backup as delete-pending, and let the leaf's deletion
sweep it up later.
+// See DefaultSnapshotStrategy#deleteSnapshotChain for the same
pattern on incremental
+// snapshots. forced=true means the caller wants the entire subtree
gone right now.
+if (forced) {
+return cascadeDeleteSubtree(backup, backupRepository, host);
+}
+
Review Comment:
Or better yet, can we set it to the parent backup's checkpoint_id so that
the next backup is again incremental instead of full. Up to you, if it will
complicate things, we can document and defer it to later improvements
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -168,6 +205,287 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+/**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+static final
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3466287925
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -168,6 +200,305 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+/**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+static final class ChainDecision {
+final String mode;// "full" or "incremental"
+final String bitmapNew;
+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..qcow2 / datadisk..qcow2), abh1sar review at line
340.
+final List 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 parentPaths,
+ String chainId, int chainPosition) {
+this.mode = mode;
+this.bitmapNew = bitmapNew;
+this.bitmapParent = bitmapParent;
+this.parentPaths = parentPaths;
+this.chainId = chainId;
+this.chainPosition = chainPosition;
+}
+
+static ChainDecision fullStart(String bitmapName) {
+return new ChainDecision(NASBackupChainKeys.TYPE_FULL, bitmapName,
null, null,
+UUID.randomUUID().toString(), 0);
+}
+
+static ChainDecision incremental(String bitmapNew, String
bitmapParent, List parentPaths,
+ String chainId, int chainPosition) {
+return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL,
bitmapNew, bitmapParent,
+parentPaths, chainId, chainPosition);
+}
+
+boolean isIncremental() {
+return NASBackupChainKeys.TYPE_INCREMENTAL.equals(mode);
+}
+}
+
+/**
+ * Decides whether the next backup for {@code vm} should be a fresh full
or an incremental
+ * appended to the existing chain. Stopped VMs are always full (libvirt
{@code backup-begin}
+ * requires a running QEMU process). The {@code nas.backup.full.every}
ConfigKey controls
+ * how many backups (full + incrementals) form one chain before a new full
is forced.
+ *
+ * The decision is anchored on the VM's {@code
nas.active_checkpoint_id} detail, which
+ * records the bitmap that currently exists on the running QEMU. After a
restore that
+ * detail is cleared, so the next backup is automatically full — even
though there may be
+ * a more recent "last backup taken" row in the database. This matches the
prescription in
+ * the PR review (avoid relying on "last backup" because that breaks after
restore).
+ */
+protected ChainDecision decideChain(VirtualMachine vm) {
+final String newBitmap = "backup-" + System.currentTimeMillis() /
1000L;
+
+// Master switch — when the operator disables incrementals at the zone
level every
+// backup is taken as a fresh full. Existing chains stay restorable
because each
+// backup's metadata is kept independently; restoring an incremental
still walks its
+// own chain (the per-backup chain_id / parent_backup_id details
persist regardless
+// of the live config). The next backup with this flag back on starts
a new chain.
+Boolean incrementalEnabled =
NASBackupIncrementalEnabled.valueIn(vm.getDataCenterId());
+if (incrementalEnabled == null || !incrementalEnabled) {
+return ChainDecision.fullStart(newBitmap);
+}
+
+// Stopped VMs cannot do incrementals — script will also fall back,
but we make the
+// decision here so we register the right type up-front.
+if (VirtualMachine.State.Stopped.equals(vm.getState())) {
+return ChainDecision.fullStart(newBitmap);
+}
+
+Integer fullEvery = NASBackupFullEvery.valueIn(vm.getDataCenterId());
+if (fullEvery == null || fullEvery <= 1) {
+// Disabled or every-backup-is-full mode.
+return ChainDecision.fullStart(newBitmap);
+}
+
+// 1. If the VM has no active_checkpoint_id, there is no bitmap on the
host to use as
+//a parent. This is the case
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3457076881
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -495,9 +863,42 @@ public boolean deleteBackup(Backup backup, boolean forced)
{
throw new CloudRuntimeException(String.format("Unable to find a
running KVM host in zone %d to delete backup %s", backup.getZoneId(),
backup.getUuid()));
}
-DeleteBackupCommand command = new
DeleteBackupCommand(backup.getExternalId(), backupRepository.getType(),
-backupRepository.getAddress(),
backupRepository.getMountOptions());
+// Backups outside any tracked chain (legacy or non-incremental
providers) are
+// deleted straight away — no children semantics apply.
+if (readDetail(backup, NASBackupChainKeys.CHAIN_ID) == null) {
+return deleteBackupFileAndRow(backup, backupRepository, host);
+}
+
+// Snapshot-style cascade: defer the on-NAS rm + DB row while there
are live children,
+// mark this backup as delete-pending, and let the leaf's deletion
sweep it up later.
+// See DefaultSnapshotStrategy#deleteSnapshotChain for the same
pattern on incremental
+// snapshots. forced=true means the caller wants the entire subtree
gone right now.
+if (forced) {
+return cascadeDeleteSubtree(backup, backupRepository, host);
+}
+
Review Comment:
set vm's active checkpoint_id to null if the backup corresponding to the
active_checkpoint_id is deleted.
##
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##
@@ -1701,6 +1701,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()) {
+return true;
Review Comment:
call `checkAndGenerateUsageForLastBackupDeletedAfterOfferingRemove` before
returning true
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -506,13 +907,215 @@ public boolean deleteBackup(Backup backup, boolean
forced) {
} catch (OperationTimedoutException e) {
throw new CloudRuntimeException("Operation to delete backup timed
out, please try again");
}
+if (answer == null || !answer.getResult()) {
+logger.warn("Failed to delete backup file for {} ({}); leaving DB
row intact",
+backup.getUuid(), backup.getExternalId());
+return false;
+}
+backupDao.remove(backup.getId());
+// Exactly-once resource accounting: decrement at the single point a
backup row + file are
+// physically removed. This runs for the leaf and for every swept
delete-pending ancestor,
+// so a chain delete decrements once per actually-removed backup. The
manager skips its own
+// accounting for this provider (see
handlesChainDeleteResourceAccounting()).
+long size = backup.getSize() != null ? backup.getSize() : 0L;
+resourceLimitMgr.decrementResourceCount(backup.getAccountId(),
Resource.ResourceType.backup);
+resourceLimitMgr.decrementResourceCount(backup.getAccountId(),
Resource.ResourceType.backup_storage, size);
+return true;
+}
-if (answer != null && answer.getResult()) {
-return backupDao.remove(backup.getId());
+/**
+ * Mark {@code backup} as delete-pending in {@code backup_details}.
Idempotent.
+ */
+private void markDeletePending(Backup backup) {
+BackupDetailVO existing = backupDetailsDao.findDetail(backup.getId(),
NASBackupChainKeys.DELETE_PENDING);
Review Comment:
Sorry, about going back on this but can we use a new `Backup.Status.Hidden`
state similar to `Snapshot.State.Hidden` instead of the `DELETE_PENDING` detail?
We'll also need a change in BackupManagerImpl.listBackups to not return
backups that are `Hidden`
Also, no other operations such as
Restore/RestoreandAttachVolume/CreateInstanceFromBackup/Delete should be
allowed on a `Hidden` 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
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4768863886 Implemented **Option A** in `a51f335`: - `BackupProvider.handlesChainDeleteResourceAccounting()` (default `false`). - `NASBackupProvider` overrides it `true` and decrements `backup` + `backup_storage` at the single physical-removal choke-point (`deleteBackupFileAndRow`) — which runs for the leaf and every swept delete-pending ancestor, so accounting is exactly-once per actually-removed backup. A tombstoned (`delete-pending`) backup is not decremented until it is swept. - `deleteCheckedBackup` skips its own decrement + `backupDao.remove` for such providers (keeps the `CheckedReservation` limit guard). Unit tests (`NASBackupProviderTest`): the leaf+sweep path decrements both removed backups; the live-children (tombstone) path decrements none. This also fixes the two concrete bugs noted above — the manager no longer destroys the `delete-pending` tombstone, and swept ancestors are no longer leaked. Happy to switch to option (b) (have `deleteBackup` return the removed set and decrement in the manager) if you'd prefer that split. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3452405878
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -168,6 +200,305 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+/**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+static final class ChainDecision {
+final String mode;// "full" or "incremental"
+final String bitmapNew;
+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..qcow2 / datadisk..qcow2), abh1sar review at line
340.
+final List 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 parentPaths,
+ String chainId, int chainPosition) {
+this.mode = mode;
+this.bitmapNew = bitmapNew;
+this.bitmapParent = bitmapParent;
+this.parentPaths = parentPaths;
+this.chainId = chainId;
+this.chainPosition = chainPosition;
+}
+
+static ChainDecision fullStart(String bitmapName) {
+return new ChainDecision(NASBackupChainKeys.TYPE_FULL, bitmapName,
null, null,
+UUID.randomUUID().toString(), 0);
+}
+
+static ChainDecision incremental(String bitmapNew, String
bitmapParent, List parentPaths,
+ String chainId, int chainPosition) {
+return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL,
bitmapNew, bitmapParent,
+parentPaths, chainId, chainPosition);
+}
+
+boolean isIncremental() {
+return NASBackupChainKeys.TYPE_INCREMENTAL.equals(mode);
+}
+}
+
+/**
+ * Decides whether the next backup for {@code vm} should be a fresh full
or an incremental
+ * appended to the existing chain. Stopped VMs are always full (libvirt
{@code backup-begin}
+ * requires a running QEMU process). The {@code nas.backup.full.every}
ConfigKey controls
+ * how many backups (full + incrementals) form one chain before a new full
is forced.
+ *
+ * The decision is anchored on the VM's {@code
nas.active_checkpoint_id} detail, which
+ * records the bitmap that currently exists on the running QEMU. After a
restore that
+ * detail is cleared, so the next backup is automatically full — even
though there may be
+ * a more recent "last backup taken" row in the database. This matches the
prescription in
+ * the PR review (avoid relying on "last backup" because that breaks after
restore).
+ */
+protected ChainDecision decideChain(VirtualMachine vm) {
+final String newBitmap = "backup-" + System.currentTimeMillis() /
1000L;
+
+// Master switch — when the operator disables incrementals at the zone
level every
+// backup is taken as a fresh full. Existing chains stay restorable
because each
+// backup's metadata is kept independently; restoring an incremental
still walks its
+// own chain (the per-backup chain_id / parent_backup_id details
persist regardless
+// of the live config). The next backup with this flag back on starts
a new chain.
+Boolean incrementalEnabled =
NASBackupIncrementalEnabled.valueIn(vm.getDataCenterId());
+if (incrementalEnabled == null || !incrementalEnabled) {
+return ChainDecision.fullStart(newBitmap);
+}
+
+// Stopped VMs cannot do incrementals — script will also fall back,
but we make the
+// decision here so we register the right type up-front.
+if (VirtualMachine.State.Stopped.equals(vm.getState())) {
+return ChainDecision.fullStart(newBitmap);
+}
+
+Integer fullEvery = NASBackupFullEvery.valueIn(vm.getDataCenterId());
+if (fullEvery == null || fullEvery <= 1) {
+// Disabled or every-backup-is-full mode.
+return ChainDecision.fullStart(newBitmap);
+}
+
+// 1. If the VM has no active_checkpoint_id, there is no bitmap on the
host to use as
+//a parent. This is the case
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4768508884 @abh1sar — on the delete-logic / resource-accounting point: I dug into how `deleteCheckedBackup` interacts with the NAS chain delete and confirmed two concrete problems: 1. **Tombstone case (live children):** the NAS provider keeps the row as `delete-pending` for chain tracking, but `deleteCheckedBackup` then calls `backupDao.remove(target)` — which destroys the tombstone the later sweep relies on. 2. **Leaf + sweep case:** the provider physically removes the leaf row **plus the swept delete-pending ancestors** (N rows), but `deleteCheckedBackup` re-removes the leaf (already gone → `false` → spurious failure) and decrements `backup` count / `backup_storage` usage for **only one** backup — leaking the other N−1. Comparing with `SnapshotManagerImpl.deleteSnapshot`: the strategy owns storage + chain/state, and the manager decrements based on the **post-delete state** of the entity rather than blindly removing the row. **Proposed rule (rigid, exactly-once):** a backup's `backup` count + `backup_storage` usage is decremented **once, at the single point its row+file are physically removed** — i.e. inside `deleteBackupFileAndRow`, which runs for the leaf *and* every swept ancestor. `deleteCheckedBackup` keeps only the `CheckedReservation` limit guard and stops calling `decrementResourceCount`/`backupDao.remove` for NAS chain backups; a tombstone (`delete-pending`) is **not** decremented until the sweep finally removes it. One open question before I implement — the manager currently decrements for **all** providers (Veeam/Networker too). To keep their accounting intact while the NAS provider owns chain accounting, do you prefer: (a) a `BackupProvider` capability flag (e.g. `handlesChainDeleteAccounting()`, default `false`; NAS returns `true`) so the manager skips its own decrement/remove only for such providers, or (b) `deleteBackup` returning the set/count of backups actually removed, so the manager decrements per-removed? I'm leaning (a) as the smaller, more rigid change — happy to implement whichever you prefer. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3450216741
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -324,6 +553,36 @@ while [[ $# -gt 0 ]]; do
shift
shift
;;
+-M|--mode)
+ MODE="$2"
+ shift
+ shift
+ ;;
+--bitmap-new)
+ BITMAP_NEW="$2"
+ shift
+ shift
+ ;;
+--bitmap-parent)
+ BITMAP_PARENT="$2"
+ shift
+ shift
+ ;;
+--parent-paths)
+ PARENT_PATHS="$2"
+ shift
+ shift
+ ;;
+--rebase-target)
+ REBASE_TARGET="$2"
+ shift
+ shift
+ ;;
+--rebase-new-backing)
+ REBASE_NEW_BACKING="$2"
+ shift
+ shift
+ ;;
Review Comment:
remove these as well
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -335,8 +594,13 @@ while [[ $# -gt 0 ]]; do
esac
done
-# Perform Initial sanity checks
-sanity_checks
+# QEMU >= 4.2 and libvirt >= 7.2 are only required for backup-begin
(incremental
+# checkpoints and per-bitmap exports). Legacy full-only backups, plus delete /
+# stats / rebase operations, run on older versions just fine. Gate the version
+# check to the paths that actually need it to preserve backward compatibility.
+if [ "$OP" = "backup" ] && [ -n "$MODE" ]; then
+ sanity_checks
+fi
Review Comment:
This doesn't look right.
`sanity_checks` was being called unconditionally before. It has nothing to
do with incremental backups.
This change should be removed.
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -168,6 +200,276 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+/**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+static final class ChainDecision {
+final String mode;// "full" or "incremental"
+final String bitmapNew;
+final String bitmapParent;// null for full
+// Per-volume parent backup file paths, one per current VM volume in
deviceId order.
+// null/empty for full. Each volume needs its own parent file because
backup files
+// are named after each volume's own UUID (root..qcow2 /
datadisk..qcow2).
+final List 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 parentPaths,
+ String chainId, int chainPosition) {
+this.mode = mode;
+this.bitmapNew = bitmapNew;
+this.bitmapParent = bitmapParent;
+this.parentPaths = parentPaths;
+this.chainId = chainId;
+this.chainPosition = chainPosition;
+}
+
+static ChainDecision fullStart(String bitmapName) {
+return new ChainDecision(NASBackupChainKeys.TYPE_FULL, bitmapName,
null, null,
+UUID.randomUUID().toString(), 0);
+}
+
+static ChainDecision incremental(String bitmapNew, String
bitmapParent, List parentPaths,
+ String chainId, int chainPosition) {
+return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL,
bitmapNew, bitmapParent,
+parentPaths, chainId, chainPosition);
+}
+
+boolean isIncremental() {
+return NASBackupChainKeys.TYPE_INCREMENTAL.equals(mode);
+}
+}
+
+/**
+ * Decides whether the next backup for {@code vm} should be a fresh full
or an incremental
+ * appended to the existing chain. Stopped VMs are always full (libvirt
{@code backup-begin}
+ * requires a running QEMU process). The {@code nas.backup.full.every}
ConfigKey controls
+ * how many backups (full + incrementals) form one chain before a new full
is forced.
+ *
+ * The decision is anchored on the VM's {@code
nas.active_checkpoint_id} detail, which
+ * records the bitmap that currently exists on the running QEMU. After a
restore that
+ * detail is cleared, so the next backup is automatically full — even
though there may be
+ * a more recent "last backup taken" row in the database. The decision
deliberately avoids
+ * relying on "last backup taken", because that row is misleading after a
restore.
+ */
+protected ChainDecision decideChain(VirtualMachine vm) {
+final String newBitmap = "backup-" + System.currentTimeMillis() /
1000L;
+
+// Master switch — when t
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4762431277 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18327 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
DaanHoogland commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4762257768 @blueorangutan package -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4762259696 @DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4756939467 @harikrishna-patnala Done — fixed in `f574628e` (dropped the unnecessary `PARENT_BACKUP_ID` stub in the sweep-pending test). The `build`/unit-test jobs are green now. The only remaining red is the smoke batch (`test_list_accounts`, `test_list_disk_offerings`, `test_list_domains`…), which is unrelated to the NAS backup changes and looks like a flaky/infra run — could a committer kick a re-run when convenient? Thanks! -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
harikrishna-patnala commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4718041139 @jmsperu can you please fix the failing test ``` Error: Errors: Error:NASBackupProviderTest.unnecessary Mockito stubbings » UnnecessaryStubbing ``` -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4700926199 [SF] Trillian test result (tid-16304) Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8 Total time taken: 53051 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13074-t16304-kvm-ol8.zip Smoke tests completed. 142 look OK, 9 have errors, 0 did not run Only failed and skipped tests results shown below: Test | Result | Time (s) | Test File --- | --- | --- | --- test_DeployVmAntiAffinityGroup_in_project | `Error` | 63.89 | test_affinity_groups_projects.py test_DeployVmAntiAffinityGroup | `Error` | 7.90 | test_affinity_groups.py ContextSuite context=TestNASBackupAndRecovery>:setup | `Error` | 0.00 | test_backup_recovery_nas.py test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 29.11 | test_kubernetes_clusters.py test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.11 | test_kubernetes_clusters.py test_12_test_deploy_cluster_different_offerings_per_node_type | `Failure` | 77.67 | test_kubernetes_clusters.py test_05_list_volumes_isrecursive | `Failure` | 0.05 | test_list_volumes.py test_07_list_volumes_listall | `Failure` | 0.04 | test_list_volumes.py test_01_non_strict_host_anti_affinity | `Failure` | 78.27 | test_nonstrict_affinity_group.py test_02_non_strict_host_affinity | `Error` | 28.54 | test_nonstrict_affinity_group.py test_01_vpn_usage | `Error` | 1.13 | test_usage.py ContextSuite context=TestMigrateVMStrictTags>:setup | `Error` | 0.00 | test_vm_strict_host_tags.py test_hostha_enable_ha_when_host_in_maintenance | `Error` | 302.30 | test_hostha_kvm.py -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4699876791 @abh1sar — I picked up the 2026-06-13 review batch in one author for style consistency with the earlier rounds. Two commits pushed: **1. `096bef1292` — `backup(nas):` collapse N+1 chain queries when sweeping delete-pending ancestors** Addresses your comment on `NASBackupProvider.java:995`. Added `getChainOrderedLeafToRoot(member)` which materialises the chain via a single `listByVmId` call ordered leaf-first by `CHAIN_POSITION`. `deleteLeafBackupAndSweepPendingAncestors` now snapshots that chain **before** the leaf delete (so the in-memory list stays resolvable after the row is gone), then iterates ancestors from the snapshot. `cascadeDeleteSubtree` is now a plain leaf-first walk — NAS backups are a linear chain so no tree traversal is needed. `findChainParent` is kept (still the right primitive for single parent-row lookups) with a Javadoc note recommending the new method when looping. **2. `73c4206c21` — `backup(nas):` move backup-mode policy + stdout markers from script to wrapper** Addresses your comments on `nasbackup.sh:155`, `nasbackup.sh:193`, `nasbackup.sh:358`, and `LibvirtTakeBackupCommandWrapper.java:124`. The script was carrying caller-side policy (arg validation, fallback decisions) and emitting stdout markers the wrapper had to parse around. Both have moved into Java; the script now uses dedicated exit codes for the signals the wrapper actually needs: - `EXIT_INCREMENTAL_UNSUPPORTED=21` replaces the `INCREMENTAL_FALLBACK=` stdout marker. Emitted when (a) the running-VM path can't re-register the parent checkpoint, or (b) the stopped-VM path was asked for incremental. **Java owns the retry policy** — wrapper sees the exit code and re-invokes the script with `--mode=full` + the same `--bitmap-new`, then sets `incrementalFallback=true` on the answer. - `EXIT_BITMAP_NOT_SEEDED=22` replaces the `BITMAP_CREATED=` stdout marker. Emitted only by the stopped-VM path when `qemu-img bitmap --add` failed on every source disk. Backup file is valid; wrapper records `bitmapCreated=null` so `NASBackupProvider` clears `active_checkpoint_id` and the next backup starts a fresh chain. The running-VM success path no longer needs a marker — `backup-begin` is atomic. - `validateBackupArgs(command)` in the wrapper pre-validates the mode + bitmap args before invoking the script. The script's per-mode required-args block is gone; the agnostic case statement remains as defensive cover for direct invocations. - `runBackupScript()` extracted so the EXIT_INCREMENTAL_UNSUPPORTED retry doesn't duplicate the argv-assembly logic. - Wrapper's stdout-marker stripping loop is removed, and the BITMAP_CREATED re-parse is gone (mirrors `command.getBitmapNew()` directly, gated on the not-seeded exit code). `NASBackupProvider.java` only changes are two comment refreshes describing the new exit-code path instead of the old marker. Note on the parent-checkpoint redefine itself: I kept the actual `virsh checkpoint-create --redefine` call in the script because it already has the NAS mounted at that point (the parent's `.checkpoint.xml` lives next to the parent backup file, mount-relative). Moving the redefine into Java would mean duplicating the mount logic in the wrapper, which felt worse than the current shape. What did move out is the **decision** that comes after the redefine fails — which is what carried the stdout-marker complexity. Happy to revisit if you'd rather see the redefine itself in Java too. Tested: chain N+1 fix is straightforward refactor against the existing unit tests; for the script + exit codes I'd appreciate a fresh `@blueorangutan test` run since I don't have access to a libvirt-10/qemu-8.2 host on my side. Thanks for the patience on this batch. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4698872356 @abh1sar a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4698867367 @blueorangutan test -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4698862077 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18250 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3408100947
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -506,13 +874,170 @@ public boolean deleteBackup(Backup backup, boolean
forced) {
} catch (OperationTimedoutException e) {
throw new CloudRuntimeException("Operation to delete backup timed
out, please try again");
}
+if (answer == null || !answer.getResult()) {
+logger.warn("Failed to delete backup file for {} ({}); leaving DB
row intact",
+backup.getUuid(), backup.getExternalId());
+return false;
+}
+backupDao.remove(backup.getId());
+return true;
+}
-if (answer != null && answer.getResult()) {
-return backupDao.remove(backup.getId());
+/**
+ * Mark {@code backup} as delete-pending in {@code backup_details}.
Idempotent.
+ */
+private void markDeletePending(Backup backup) {
+BackupDetailVO existing = backupDetailsDao.findDetail(backup.getId(),
NASBackupChainKeys.DELETE_PENDING);
+if (existing == null) {
+backupDetailsDao.persist(new BackupDetailVO(backup.getId(),
+NASBackupChainKeys.DELETE_PENDING, "true", true));
}
+}
-logger.debug("There was an error removing the backup with id {}",
backup.getId());
-return false;
+/**
+ * @return true if this backup carries the delete-pending tombstone.
+ */
+private boolean isDeletePending(Backup backup) {
+BackupDetailVO d = backupDetailsDao.findDetail(backup.getId(),
NASBackupChainKeys.DELETE_PENDING);
+return d != null && "true".equalsIgnoreCase(d.getValue());
+}
+
+/**
+ * Return the live (not delete-pending, not Removed) children of {@code
parent} within the
+ * same chain. Equivalent to "incrementals whose parent_backup_id points
at parent".
+ */
+private List findLiveChildren(Backup parent) {
+String parentUuid = parent.getUuid();
+String chainId = readDetail(parent, NASBackupChainKeys.CHAIN_ID);
+if (parentUuid == null || chainId == null) {
+return Collections.emptyList();
+}
+List children = new ArrayList<>();
+for (Backup b : backupDao.listByVmId(null, parent.getVmId())) {
+if (b.getId() == parent.getId()) {
+continue;
+}
+if (!chainId.equals(readDetail(b, NASBackupChainKeys.CHAIN_ID))) {
+continue;
+}
+if (!parentUuid.equals(readDetail(b,
NASBackupChainKeys.PARENT_BACKUP_ID))) {
+continue;
+}
+if (isDeletePending(b)) {
+// Tombstoned children don't keep us alive — they're already
on the way out.
+continue;
+}
+children.add(b);
+}
+return children;
+}
+
+/**
+ * Look up this backup's immediate parent in the chain (by {@code
PARENT_BACKUP_ID}).
+ * Returns {@code null} if this is the full (no parent) or the parent row
is gone.
+ */
+private Backup findChainParent(Backup backup) {
+String parentUuid = readDetail(backup,
NASBackupChainKeys.PARENT_BACKUP_ID);
+if (parentUuid == null || parentUuid.isEmpty()) {
+return null;
+}
+for (Backup b : backupDao.listByVmId(null, backup.getVmId())) {
+if (parentUuid.equals(b.getUuid())) {
+return b;
+}
+}
+return null;
+}
+
+/**
+ * Physically delete the leaf {@code backup}, then walk up the chain while
each ancestor
+ * is in delete-pending state. Mirrors the snapshot subsystem pattern:
once a leaf is
+ * gone, garbage-collect any tombstoned parents.
+ *
+ * Caller must guarantee {@code backup} is a leaf (no live children).
Each tombstoned
+ * ancestor is by definition childless once its sole child is deleted
here, so no extra
+ * live-children check is needed inside the loop.
+ */
+private boolean deleteLeafBackupAndSweepPendingAncestors(Backup backup,
BackupRepository repo, Host host) {
+// Record the parent BEFORE the delete — deleteBackupFileAndRow
removes the backup row,
+// after which findChainParent can't resolve PARENT_BACKUP_ID.
+Backup parent = findChainParent(backup);
+if (!deleteBackupFileAndRow(backup, repo, host)) {
+return false;
+}
+while (parent != null && isDeletePending(parent)) {
+Backup nextParent = findChainParent(parent);
+if (!deleteBackupFileAndRow(parent, repo, host)) {
+// Stop the sweep; the rest of the tombstoned chain will be
collected on a
+// future delete that re-runs the sweep.
+return true;
+}
+
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4698748453 @abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4698747486 @blueorangutan package -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3408051562
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -113,20 +126,104 @@ 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.
+ 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
+ make_checkpoint=1
+ ;;
+legacy-full)
+ make_checkpoint=0
+ ;;
+*)
+ echo "Unknown mode: $effective_mode"
+ cleanup
Review Comment:
@jmsperu this as well?
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3408049656
##
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##
@@ -94,21 +113,46 @@ public Answer execute(TakeBackupCommand command,
LibvirtComputingResource libvir
return answer;
}
+// 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=")) {
Review Comment:
@jmsperu please address this.
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -191,24 +337,39 @@ 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
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
du -sb $dest | cut -f1
+ if [[ -n "$BITMAP_NEW" ]]; then
+# 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
rmdir $mount_point
}
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.
+ 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)"
Review Comment:
@jmsperu this as well. thanks
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4698717726 > Re-register the parent with **`checkpoint-create --redefine`** using the **full** `checkpoint-dumpxml` output (a minimal/synthesized XML is rejected by libvirt's checkpoint RNG schema). So: persist `.checkpoint.xml` next to each backup on the NAS, and on recreate `--redefine` from the parent backup's saved XML @jmsperu In my testing, I found that a full checkpoint xml was not required. Just the `checkpoint name` and the `created` tag is enough for redefine. We don't have to store `created` as it doesn't have to be accurate. Checkpoints are ephemeral anyway. Can you please check https://github.com/shapeblue/cloudstack/blob/integration-veeam-kvm/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapper.java#L127? This way we don't have to persist the full checkpoint xml. > On the larger "move the checks into Java" suggestion: I started it, but testing showed the recreate needs checkpoint-dumpxml + --redefine against NAS-side XML, which is cohesive in the script — I've kept it there for now and can revisit the Java move as a follow-up. With the above change the Virsh checkpoint redefine logic can be moved to Java. If checkpoint redefine fails, the Java code can fallback to full and call nasbackup.sh without the -M flag. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4684182269 Added a regression test for this — `test_incremental_after_vm_restart` (commit `3ed8a30a`) — and here's how the fix was validated. **Manually, on a real libvirt 10.0.0 / qemu 8.2 host** — reproduced the failure and confirmed the fix using `virsh backup-begin` with QMP `qemu-io` to dirty real blocks (no CloudStack needed, isolated transient domain): ``` full backup-begin -> dirty blocks (qemu-io) -> incremental (small, dirty-only) -> checkpoint-delete --metadata # simulates the VM-restart registry wipe -> checkpoint-create --redefine# the fix: re-register the parent ✅ -> incremental against the re-registered parent ✅ ``` Before the fix, that recreate failed with `Bitmap already exists`, and the `qemu-img bitmap --remove` fallback hit `Failed to get "write" lock` on the running disk. **Automated** — the new test encodes the same flow in the smoke suite (`required_hardware=true`, runs in 4.23 CI): ``` FULL + marker1 -> stop/start the VM (wipes libvirt's checkpoint registry) -> INCREMENTAL + marker2 -> restore the tip -> assert both markers restore AND the post-restart backup is INCREMENTAL (not a FULL) ``` It preserves `nas.backup.full.every` in `finally` so it doesn't leak config on shared environments. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4684101637 Pushed updates addressing the review, plus a correctness fix found while testing the incremental path on a real **libvirt 10.0.0 / qemu 8.2** host. ### Commits 1. **`3456ba7d`** — review cleanups: removed the unused `RebaseBackupCommand` + its wrapper, and stripped GitHub-username / review-thread references from code comments (kept the technical explanation in each). 2. **`ba74778d`** — `LibvirtTakeBackupCommandWrapper` now uses the `--bitmap-new` value it already passes instead of re-parsing the `BITMAP_CREATED=` line. The marker stays only as a *success* signal (a stopped-VM on RBD/LINSTOR genuinely doesn't create a bitmap). 3. **`8adde4da`** — **bug fix: parent-checkpoint recreation for an incremental taken after a VM restart.** ### The bug (caught in testing) An incremental taken after the VM has been (re)started since the last backup **failed**. CloudStack rebuilds the domain XML on every start, wiping libvirt's checkpoint registry, while the dirty bitmap persists on the qcow2 (QEMU reloads it). The old code rebuilt a *minimal* checkpoint XML and did a **fresh** `checkpoint-create`, which QEMU rejects: ``` error: internal error: unable to execute QEMU command 'transaction': Bitmap already exists: ``` and the `qemu-img bitmap --remove` fallback can't run on a live VM: ``` qemu-img: Could not open '...qcow2': Failed to get "write" lock ``` ### The fix Re-register the parent with **`checkpoint-create --redefine`** using the **full** `checkpoint-dumpxml` output (a minimal/synthesized XML is rejected by libvirt's checkpoint RNG schema). So: persist `.checkpoint.xml` next to each backup on the NAS, and on recreate `--redefine` from the parent backup's saved XML; if it's missing (a pre-fix backup) or redefine fails, fall back to a full so the chain restarts cleanly. Verified end-to-end on libvirt 10.0.0: `full → dirty → incremental`, then `restart → redefine → incremental`, all succeed. ### On the remaining threads - **stderr (INCREMENTAL_FALLBACK / BITMAP_CREATED)** — already addressed; those markers are echoed to **stdout**. - **`BITMAP_RECREATED`, `findLiveChildren(parent).isEmpty()`, unused `NASBackupChainKeys` strings** — already removed in earlier pushes (the two strings flagged are now the in-use `ChainDecision.mode` sentinels `TYPE_FULL`/`TYPE_INCREMENTAL`). - **`INCREMENTAL_FALLBACK`** — keeping it: the running-vs-stopped decision is made at the last moment in the script (the `virsh list` immediately before the backup), and the marker is how the stopped path tells the orchestrator to record a FULL. Pulling it earlier into Java would *widen* the stop-mid-flight race. - **`getVolumePoolsAndPaths(parentVols)`** — that block computes parent *file paths* from `Backup.VolumeInfo` (historical backup metadata); the method takes live `VolumeVO`, so it's a different input/purpose, not a direct swap. On the larger "move the checks into Java" suggestion: I started it, but testing showed the recreate needs `checkpoint-dumpxml` + `--redefine` against NAS-side XML, which is cohesive in the script — I've kept it there for now and can revisit the Java move as a follow-up. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4667570937 > Hi @jmsperu, gentle nudge about the pending review comments. Thanks. Sorry about pushing you, but we are on the clock for the 4.23 release. This is a much anticipated feature and your efforts are very much appreciated. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4667532430 Hi @jmsperu, gentle nudge about the pending review comments. Thanks. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4621388328
@abh1sar — expanded Test Plan below. Results will follow as a separate
comment once I've run the manual scenarios end-to-end against the same Trillian
baseline (tid-16197).
## Test plan
### Environment
- Branch `feature/nas-backup-incremental` against `main` (4.23-SNAPSHOT)
- KVM on OL8 (Trillian `ol8 mgmt + kvm-ol8` profile)
- File-based primary storage (qcow2 on NFS); NAS repo on a separate NFS share
- libvirt 9.x + qemu 7.x+ (dirty bitmaps + `backup-begin --checkpointxml`)
### Automated coverage
| Layer | Suite | Cases |
|---|---|---|
| Unit | `NASBackupProviderTest` | 15 total, 5 new: chain decision under
master switch / no-active-checkpoint, restore-clears-checkpoint,
delete-with-live-child marks pending-delete, leaf-delete sweeps up pending
parent |
| Unit | `LibvirtRestoreBackupCommandWrapperTest` | stubs added for
incremental restore path |
| Smoke (Trillian) | `test/integration/smoke/test_backup_recovery_nas.py` |
5 new cases, all `required_hardware="true"` |
Smoke scenarios
| Case | What it asserts |
|---|---|
| `test_incremental_chain_cadence` | With `nas.backup.full.every=3` and 5
backups, observed type sequence is
`['FULL','INCREMENTAL','INCREMENTAL','FULL','INCREMENTAL']` |
| `test_restore_from_incremental` | Marker files written between each backup
are all present after restoring from the tail INC |
| `test_delete_middle_incremental_repairs_chain` | After deleting a middle
INC, child's `parent_id` is repointed to the surviving ancestor, backing file
is rebased, downstream restore still correct |
| `test_refuse_delete_full_with_children` | Deleting a FULL that has
descendants → `CloudRuntimeException`; `forced=true` cascades |
| `test_stopped_vm_falls_back_to_full` | Stopped VM → next backup is FULL,
no checkpoint XML in agent command |
### Manual scenarios (outside smoke scope)
| # | Scenario | Method | Expected |
|---|---|---|---|
| A | Long-run cadence stability | `full.every=10`; take 25 backups across 5
days | FULLs at positions 1, 11, 21; INCs at all others; no chain drift |
| B | 4.22 agent ↔ 4.23 mgmt | Run a 4.22 agent against the 4.23 mgmt; take
backup of a VM on that host | FULL succeeds; no new flags emitted in agent
command; `backup_details` carries no chain keys |
| C | Master-switch flip mid-chain | After a chain has formed, set
`nas.backup.incremental.enabled=false` zone-scoped | Next backup is FULL
regardless of cadence; new chain anchored |
| D | Bitmap recreation after VM stop/start | Take FULL+INC, stop and start
the VM, take next INC | Agent recreates checkpoint via `virsh
checkpoint-create`; INC succeeds; restore from this INC is correct |
| E | Relative-path rebase survives mount churn | Unmount/remount the NAS at
a different mount point between backup and restore | Relative backing paths
keep the chain valid |
| F | `nasbackup.sh` legacy invocation | Invoke without `-M` / `--bitmap-*`
| Behaves byte-for-byte as 4.22; no checkpoint side-effects |
### Backwards-compat checks
- `TakeBackupCommand` new fields default null → 4.22 agents ignore them
(covered by Scenario B).
- Pre-PR backups with no `chain_id` in `backup_details` are treated as
standalone FULLs; cascade-delete short-circuits without touching them.
- `RebaseBackupCommand` is only sent when chain metadata is present, so a
downgraded agent never receives it.
### Results
Test results from running this plan will be posted as a follow-up comment
after execution.
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3324551775
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -113,20 +126,104 @@ 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.
+ 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
+ make_checkpoint=1
+ ;;
+legacy-full)
+ make_checkpoint=0
+ ;;
+*)
+ echo "Unknown mode: $effective_mode"
+ cleanup
Review Comment:
Can we put this whole logic in the Java code?
Do the required checks there and have the simple logic here that if
BITMAP_NEW is passed, it means we have to create the new bitmap on the disks.
The motivation is to make the shell code as simple as possible and let the
Java code do the heavy lifting.
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4575332359 > Hi @jmsperu The Test Plan needs to be expanded for such a complicated PR. Please update the Test Plan and also update the testing results when done. Hi @jmsperu please take care of this also -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3324134863
##
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##
@@ -94,21 +113,46 @@ public Answer execute(TakeBackupCommand command,
LibvirtComputingResource libvir
return answer;
}
+// 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=")) {
Review Comment:
The wrapper is passing the new bitmap to nasbackup.sh.
So why read it again from the scipt. Can't we just used the passed value?
##
core/src/main/java/org/apache/cloudstack/backup/RebaseBackupCommand.java:
##
@@ -0,0 +1,73 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.backup;
+
+import com.cloud.agent.api.Command;
+import com.cloud.agent.api.LogLevel;
+
+/**
+ * Tells the KVM agent to rebase a NAS backup qcow2 onto a new backing parent.
Used by the
+ * NAS backup provider during chain repair when a middle incremental is being
deleted: the
+ * immediate child must absorb the soon-to-be-deleted parent's blocks and then
re-link to
+ * the grandparent. Both target and new-backing paths are NAS-mount-relative.
+ */
+public class RebaseBackupCommand extends Command {
Review Comment:
I think you mentioned this in a previous comment also. Can you please remove
theRebaseBackupCommand and its wrapper as it is not being used now.
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupChainKeys.java:
##
@@ -0,0 +1,66 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.backup;
+
+/**
+ * Keys used by the NAS backup provider when storing incremental-chain metadata
+ * in the existing {@code backup_details} key/value table. Stored here (not on
+ * the {@code backups} table) so other providers do not need a schema change to
+ * support their own incremental implementations.
+ */
+public final class NASBackupChainKeys {
+
+/** UUID of the parent backup (full or previous incremental). Empty for
full backups. */
+public static final String PARENT_BACKUP_ID = "nas.parent_backup_id";
+
+/** QEMU dirty-bitmap name created by this backup, used as the {@code
} reference for the next one. */
+public static final String BITMAP_NAME = "nas.bitmap_name";
+
+/** Identifier shared by every backup in the same chain (the full anchors
a chain; its incrementals inherit the id). */
+public static final String CHAIN_ID = "nas.chain_id";
+
+/** Position within the chain: 0 for the full, 1 for the first
incremental, and so on. */
+public static final String CHAIN_POSITION = "nas.chain_position";
+
+/**
+ * In-memory chain-mode sentinels used by {@code ChainDecision.mode}. The
persisted
+ * full-vs-incremental backup type lives on the {@code backup.type} column
(set in
+ * {@code takeBackup}) — single source of truth. Not duplicated into
backup_details.
+
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4560782785 [SF] Trillian test result (tid-16197) Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8 Total time taken: 53874 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13074-t16197-kvm-ol8.zip Smoke tests completed. 148 look OK, 3 have errors, 0 did not run Only failed and skipped tests results shown below: Test | Result | Time (s) | Test File --- | --- | --- | --- ContextSuite context=TestNASBackupAndRecovery>:setup | `Error` | 0.00 | test_backup_recovery_nas.py test_05_list_volumes_isrecursive | `Failure` | 0.03 | test_list_volumes.py test_07_list_volumes_listall | `Failure` | 0.04 | test_list_volumes.py test_01_vpn_usage | `Error` | 1.09 | test_usage.py -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4556978538 Thanks @abh1sar — all 9 review points addressed in commit f2210f4859: | # | Comment | Resolution | |---|---|---| | 1 | Strip GitHub-username refs from comments | done in `ChainDecision`, `composeParentBackupPaths` | | 2 | `backup.type` is the single source of truth | dropped `NASBackupChainKeys.TYPE` writes to `backup_details`; removed the `TYPE` constant; backupVO.type set by `takeBackup` is authoritative | | 3 | Just check the latest backup's bitmap | replaced `findLatestBackedUpBackupWithBitmap` (sort+scan) with `findLatestBackedUpBackup` (one max-by-date); `decideChain` now compares its bitmap inline and forces full on mismatch | | 4 | Reset active checkpoint on offering removal | `removeVMFromBackupOffering()` calls `clearVmActiveCheckpoint(vm.getId())` | | 5 | Drop manual volume-identity check in `composeParentBackupPaths` | done — count match is the only remaining safety check, since attach/detach is blocked while assigned and re-assignment clears the checkpoint per #4 | | 6 | Drop `BITMAP_RECREATED` everywhere | removed from `NASBackupProvider`, `BackupAnswer`, `LibvirtTakeBackupCommandWrapper`, `nasbackup.sh`, `NASBackupChainKeys` | | 7a | `deleteBackupAndSweepPendingAncestors` — drop always-true `findLiveChildren(parent).isEmpty()` check | done | | 7b | Rename to `deleteLeafBackupAndSweepPendingAncestors` | done | | 7c | Call `findChainParent` BEFORE delete | done (row is gone after `deleteBackupFileAndRow`) | | 8 | Drop BFS in `collectSubtree` | replaced with `findChainTail(root)` — highest `CHAIN_POSITION` for same `CHAIN_ID` — then walk leaf → root via `PARENT_BACKUP_ID`. `collectSubtree` removed. | Build is green and `NASBackupProviderTest` all 14 tests pass locally. On the `getVolumePoolsAndPaths` suggestion: that helper builds live primary-storage paths (`List` in, storage-pool-relative paths out), whereas `composeParentBackupPaths` builds NAS-relative backup-file paths from `List` (UUID-keyed `root..qcow2` / `datadisk..qcow2` per the script's naming). Different shape — but the spirit (simpler path composition, drop the manual sanity checks) is in. Happy to align further if you'd prefer. Diff is 84 insertions / 133 deletions — net leaner. Standing by for the agent-side review you mentioned next. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3310539625
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -168,6 +200,305 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+/**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+static final class ChainDecision {
+final String mode;// "full" or "incremental"
+final String bitmapNew;
+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..qcow2 / datadisk..qcow2), abh1sar review at line
340.
Review Comment:
Please remove any code comments that mention GitHub usernames or review
comments.
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -168,6 +200,305 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+/**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+static final class ChainDecision {
+final String mode;// "full" or "incremental"
+final String bitmapNew;
+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..qcow2 / datadisk..qcow2), abh1sar review at line
340.
+final List 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 parentPaths,
+ String chainId, int chainPosition) {
+this.mode = mode;
+this.bitmapNew = bitmapNew;
+this.bitmapParent = bitmapParent;
+this.parentPaths = parentPaths;
+this.chainId = chainId;
+this.chainPosition = chainPosition;
+}
+
+static ChainDecision fullStart(String bitmapName) {
+return new ChainDecision(NASBackupChainKeys.TYPE_FULL, bitmapName,
null, null,
+UUID.randomUUID().toString(), 0);
+}
+
+static ChainDecision incremental(String bitmapNew, String
bitmapParent, List parentPaths,
+ String chainId, int chainPosition) {
+return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL,
bitmapNew, bitmapParent,
+parentPaths, chainId, chainPosition);
+}
+
+boolean isIncremental() {
+return NASBackupChainKeys.TYPE_INCREMENTAL.equals(mode);
+}
+}
+
+/**
+ * Decides whether the next backup for {@code vm} should be a fresh full
or an incremental
+ * appended to the existing chain. Stopped VMs are always full (libvirt
{@code backup-begin}
+ * requires a running QEMU process). The {@code nas.backup.full.every}
ConfigKey controls
+ * how many backups (full + incrementals) form one chain before a new full
is forced.
+ *
+ * The decision is anchored on the VM's {@code
nas.active_checkpoint_id} detail, which
+ * records the bitmap that currently exists on the running QEMU. After a
restore that
+ * detail is cleared, so the next backup is automatically full — even
though there may be
+ * a more recent "last backup taken" row in the database. This matches the
prescription in
+ * the PR review (avoid relying on "last backup" because that breaks after
restore).
+ */
+protected ChainDecision decideChain(VirtualMachine vm) {
+final String newBitmap = "backup-" + System.currentTimeMillis() /
1000L;
+
+// Master switch — when the operator disables incrementals at the zone
level every
+// backup is taken as a fresh full. Existing chains s
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4554418829 @DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
DaanHoogland commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4554408659 @blueorangutan test -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4552885222 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18053 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4552483876 @jmsperu a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4552470165 On the failed `SL-JID 18046` packaging (✖ el8/el9/debian/suse15) — the diff between that and the green `SL-JID 18035` is purely Java + bash + Python (touches only `core/`, `plugins/backup/nas/`, `plugins/hypervisors/kvm/`, `scripts/vm/hypervisor/kvm/nasbackup.sh`, and the smoke test). **No packaging files** (`packaging/`, `debian/`, `.spec`, `pom.xml`) were modified. GitHub Actions `build` is green across all matrix entries, `nasbackup.sh` passes `bash -n`, and the script isn't explicitly enumerated in any RPM/deb spec. That points to a Jenkins-side transient (build agent / mirror / disk) rather than a code regression. @DaanHoogland or anyone with rights — could we get a re-run of `@blueorangutan package` when convenient? Happy to also bisect locally if it fails again with the same signature. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4550413921 Hi @jmsperu The Test Plan needs to be expanded for such a complicated PR. Please update the Test Plan and also update the testing results when done. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4547761358 Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18046 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4547495069 @DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
DaanHoogland commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4547478847 @blueorangutan package -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4547147787 Thanks for the thorough review @copilot-pull-request-reviewer (and @DaanHoogland for the nudge 🙏). Working through the comments — here's what's in the latest pushes (`3e2e144fc6` and `4f3375a918`): **Agent ↔ management server signalling** (the most impactful — these would silently break the feature): - `INCREMENTAL_FALLBACK=` and `BITMAP_CREATED=` (both the stopped-VM and the warn paths) now emit on stdout, matching what `Script.executePipedCommands` actually captures in `LibvirtTakeBackupCommandWrapper`. The original stderr emission meant management never saw these markers. - Fixed the redirection order in `virsh backup-begin` and `qemu-agent-command` calls — `2>&1 > /dev/null` was leaking stderr and swallowing the agent's thaw response. Now `> /dev/null 2>&1` for the discard-everything cases, and the thaw response is captured properly. **Per-volume parent paths + identity check** (mostly already done in 5be1910ff0, plus the alignment fix): - `composeParentBackupPaths` now sorts current VM volumes by `deviceId` before positional comparison, and verifies the UUID at each position matches the parent's recorded volume UUID. If a disk was detached and a different one attached in its place, the chain can't be safely continued — returns `null` so the caller forces a full instead of silently rebasing onto the wrong parent file. **Defaults & docs**: - `nas.backup.incremental.enabled` now defaults to `false`. Existing zones keep legacy full-only behavior on upgrade; opt in per-zone when ready to use chains. Description in the ConfigKey updated to be explicit about this. - `NASBackupChainKeys.TYPE` Javadoc now documents the lowercase-vs-uppercase distinction from `Backup.Status` instead of implying they match. - `nasbackup.sh` usage text now shows `--parent-paths` (plural, comma-separated) to match the implemented flag. - `sanity_checks` (QEMU >= 4.2 / libvirt >= 7.2) is now gated to `OP=backup && -n MODE` so legacy full-only callers and the delete / stats / rebase ops still work on older host versions. **Stopped-VM bitmap persistence**: - `persistChainMetadata` only stores `nas.bitmap_name` when the agent confirms it via `BITMAP_CREATED=`. Previously fell back to the requested bitmap, which could anchor the next incremental on a bitmap that doesn't exist if pre-seeding failed. **Tests**: - The incremental tests now capture the original zone-scoped `nas.backup.full.every` via `Configurations.list` in each test method and restore that exact value in `finally`, instead of hard-resetting to `10` (which leaked into shared environments). **Already addressed in earlier commits, noting for completeness**: per-volume parent paths refactor (5be1910ff0), chain repair per disk (b7b74c4f88), stopped-VM bitmap pre-seed (0bdcdb1484), incremental.enabled master switch (691931de23), RFC moved out of repo (9764025358). Will keep an eye on CI. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
Copilot commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3304723037
##
test/integration/smoke/test_backup_recovery_nas.py:
##
@@ -265,3 +265,222 @@ def
test_vm_backup_create_vm_from_backup_in_another_zone(self):
self.assertEqual(backup_repository.crosszoneinstancecreation, True,
"Cross-Zone Instance Creation could not be enabled on the backup repository")
self.vm_backup_create_vm_from_backup_int(template.id, [network.id])
+
+# --
+# Incremental backup tests (RFC #12899 / PR #13074)
+# --
+# These tests exercise the incremental NAS backup chain semantics:
+# full -> incN cadence, restore-from-incremental, delete-middle chain
+# repair, refuse-delete-full-with-children, and stopped-VM fallback.
+#
+# All tests set nas.backup.full.every to a small value (3) so a chain
+# forms quickly without needing many backup iterations. They restore
+# the original value at teardown.
+
+def _set_full_every(self, value):
+Configurations.update(self.apiclient, name='nas.backup.full.every',
+ value=str(value), zoneid=self.zone.id)
+
+def _backup_type(self, backup):
+# Backup objects expose `type`; for chained backups it's
"INCREMENTAL", else "FULL".
+return getattr(backup, 'type', 'FULL') or 'FULL'
+
+@attr(tags=["advanced", "backup"], required_hardware="true")
+def test_incremental_chain_cadence(self):
+"""
+With nas.backup.full.every=3, the sequence of backups should be
+FULL, INCREMENTAL, INCREMENTAL, FULL, INCREMENTAL, ...
+"""
+self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+self._set_full_every(3)
+try:
+ssh_client_vm = self.vm.get_ssh_client(reconnect=True)
+ssh_client_vm.execute("touch /root/incremental_marker_1.txt")
+
+created = []
+for i in range(5):
+Backup.create(self.apiclient, self.vm.id, "inc_chain_%d" % i)
+# write a small change so each incremental has something to
capture
+ssh_client_vm.execute("dd if=/dev/urandom of=/root/delta_%d
bs=64k count=4 2>/dev/null" % i)
+time.sleep(2)
+created = Backup.list(self.apiclient, self.vm.id)
+
+self.assertEqual(len(created), 5, "Expected 5 backups after 5
Backup.create calls")
+# Sort oldest-first by date
+created.sort(key=lambda b: b.created)
+
+expected = ['FULL', 'INCREMENTAL', 'INCREMENTAL', 'FULL',
'INCREMENTAL']
+actual = [self._backup_type(b).upper() for b in created]
+self.assertEqual(actual, expected,
+"With nas.backup.full.every=3, chain pattern should be %s but
was %s" % (expected, actual))
+
+# Cleanup all backups (newest first to satisfy chain rules without
forced=true)
+for b in reversed(created):
+Backup.delete(self.apiclient, b.id)
+finally:
+self._set_full_every(10)
+self.backup_offering.removeOffering(self.apiclient, self.vm.id)
Review Comment:
These incremental tests claim they “restore the original value at teardown”,
but the finally block hard-codes nas.backup.full.every back to 10. If the
environment’s original setting differs, this will leak config changes across
tests. Capture the original value (e.g., via Configurations.list/show) and
restore that instead of using a constant.
--
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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
Copilot commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3304722954
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -191,24 +337,37 @@ 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
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
du -sb $dest | cut -f1
+ if [[ -n "$BITMAP_NEW" ]]; then
+# 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
rmdir $mount_point
}
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.
+ if [[ "$MODE" == "incremental" ]]; then
+echo "INCREMENTAL_FALLBACK=full (VM stopped — incremental requires running
VM)" >&2
+ fi
Review Comment:
INCREMENTAL_FALLBACK is echoed to stderr, but the KVM agent wrapper
captures/parses only stdout from Script.executePipedCommands (it reads the last
process' stdout only). As a result, incrementalFallback will never be detected
by LibvirtTakeBackupCommandWrapper. Emit this marker on stdout (or change the
wrapper to also consume stderr) so management can reliably record the backup as
FULL when fallback happens.
##
scripts/vm/hypervisor/kvm/nasbackup.sh:
##
@@ -278,6 +505,13 @@ cleanup() {
function usage {
echo ""
echo "Usage: $0 -o -v|--vm -t -s
-m -p -d
-q|--quiesce "
+ echo " [-M|--mode ] [--bitmap-new ]
[--bitmap-parent ] [--parent-path ]"
+ echo ""
+ echo "Incremental backup options (running VMs only; requires QEMU >= 4.2 and
libvirt >= 7.2):"
+ echo " -M|--mode full Take a full backup AND create a checkpoint
(--bitmap-new required) for future incrementals."
+ echo " -M|--mode incremental Take an incremental backup since
--bitmap-parent and create new checkpoint --bitmap-new."
+ echo " Requires --bitmap-parent, --bitmap-new, and
--parent-path (parent backup file for rebase)."
Review Comment:
The usage/help text advertises `--parent-path`, but the implemented option
is `--parent-paths` (plural, comma-separated list). This makes `-h/--help`
misleading and can lead to incorrect invocations. Update the help strings to
match the actual flag name and semantics.
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -85,6 +88,27 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
true,
BackupFrameworkEnabled.key());
+ConfigKey NASBackupFullEvery = new ConfigKey<>("Advanced",
Integer.class,
+"nas.backup.full.every",
+"10",
+"Take a full NAS backup every Nth backup; remaining backups in
between are incremental. " +
+"Counts backups, not days, so it works for hourly, daily,
and ad-hoc schedules. " +
+"Set to 1 to disable incrementals (every backup is full).",
+true,
+ConfigKey.Scope.Zone,
+BackupFrameworkEnabled.key());
+
+ConfigKey NASBackupIncrementalEnabled = new
ConfigKey<>("Advanced", Boolean.class,
+"nas.backup.incremental.enabled",
+"true",
Review Comment:
`nas.backup.incremental.enabled` is introduced with default value "true".
The PR description/backwards-compatibility section describes incremental as
opt-in and legacy behavior as full-only unless enabled; defaulting to true
changes behavior for existing zones immediately after upgrade. Consider
defaulting this to "false" (and/or gating incrementals on an explicit zone
setting).
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupChainKeys.java:
##
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the Li
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4542667330 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18035 -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
blueorangutan commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4542012046 @DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
DaanHoogland commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4542005813 @blueorangutan package -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
DaanHoogland commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4541875899 @jmsperu , will you answer the rest of co-pilot’s remarks? -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4522732600 Thanks @harikrishna-patnala @abh1sar — pushed 4 commits addressing the feedback: - `9f4d61f31c` — drop unused `host.getId()` stub in `deleteWithLiveChildMarksDeletePendingAndPreservesFile` (fixes the UnnecessaryStubbingException Harikrishna flagged) - `691931de23` — add `nas.backup.incremental.enabled` ConfigKey (abh1sar review thread @ line 90) - `0bdcdb1484` — pre-seed bitmap on stopped-VM backup via `qemu-img bitmap --add` so the next backup can be incremental (abh1sar review thread @ nasbackup.sh:513) - `5be1910ff0` — per-volume parent paths: `TakeBackupCommand.parentPath` -> `parentPaths: List`, script rebases each disk onto its own parent file, falls back to fresh full when current/parent volume counts don't match (abh1sar review thread @ line 340) All 14 unit tests pass locally. Could one of you trigger `@blueorangutan test` once GHA goes green? Happy to address any further review comments. -- 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]
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
jmsperu commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3291231916
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -85,6 +86,16 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
true,
BackupFrameworkEnabled.key());
Review Comment:
Done in `691931de23` — added `nas.backup.incremental.enabled` ConfigKey
(zone-scoped, dynamic, default true). `decideChain()` checks the master switch
first so flipping it off forces every backup to be a fresh full while leaving
existing chains restorable; flipping it back on starts a new chain on the next
backup. Added `decideChainReturnsFullWhenIncrementalDisabled` unit test
covering the disabled path.
##
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##
@@ -168,6 +182,168 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+/**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+static final class ChainDecision {
+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,
+ String chainId, int chainPosition) {
+this.mode = mode;
+this.bitmapNew = bitmapNew;
+this.bitmapParent = bitmapParent;
+this.parentPath = parentPath;
+this.chainId = chainId;
+this.chainPosition = chainPosition;
+}
+
+static ChainDecision fullStart(String bitmapName) {
+return new ChainDecision(NASBackupChainKeys.TYPE_FULL, bitmapName,
null, null,
+UUID.randomUUID().toString(), 0);
+}
+
+static ChainDecision incremental(String bitmapNew, String
bitmapParent, String parentPath,
+ String chainId, int chainPosition) {
+return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL,
bitmapNew, bitmapParent,
+parentPath, chainId, chainPosition);
+}
+
+boolean isIncremental() {
+return NASBackupChainKeys.TYPE_INCREMENTAL.equals(mode);
+}
+}
+
+/**
+ * Decides whether the next backup for {@code vm} should be a fresh full
or an incremental
+ * appended to the existing chain. Stopped VMs are always full (libvirt
{@code backup-begin}
+ * requires a running QEMU process). The {@code nas.backup.full.every}
ConfigKey controls
+ * how many backups (full + incrementals) form one chain before a new full
is forced.
+ */
+protected ChainDecision decideChain(VirtualMachine vm) {
+final String newBitmap = "backup-" + System.currentTimeMillis() /
1000L;
+
+// Stopped VMs cannot do incrementals — script will also fall back,
but we make the
+// decision here so we register the right type up-front.
+if (VirtualMachine.State.Stopped.equals(vm.getState())) {
+return ChainDecision.fullStart(newBitmap);
+}
+
+Integer fullEvery = NASBackupFullEvery.valueIn(vm.getDataCenterId());
+if (fullEvery == null || fullEvery <= 1) {
+// Disabled or every-backup-is-full mode.
+return ChainDecision.fullStart(newBitmap);
+}
+
+// Walk this VM's backups newest→oldest, find the most recent BackedUp
backup that has a
+// bitmap stored. If we don't find one, this is the first backup in a
chain — start full.
+List history = backupDao.listByVmId(vm.getDataCenterId(),
vm.getId());
+if (history == null || history.isEmpty()) {
+return ChainDecision.fullStart(newBitmap);
+}
+history.sort(Comparator.comparing(Backup::getDate).reversed());
+
+Backup parent = null;
+String parentBitmap = null;
+String parentChainId = null;
+int parentChainPosition = -1;
+for (Backup b : history) {
+if (!Backup.Status.BackedUp.equals(b.getStatus())) {
+continue;
+}
+String bm = readDetail(b, NASBackupChainKeys.BITMAP_NAME);
+if (bm == null) {
+continue;
+}
+parent = b;
+
Re: [PR] feat(backup): incremental NAS backup support for KVM [cloudstack]
abh1sar commented on code in PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3265143177 ## scripts/vm/hypervisor/kvm/nasbackup.sh: ## @@ -324,6 +504,36 @@ while [[ $# -gt 0 ]]; do shift shift ;; +-M|--mode) + MODE="$2" + shift + shift + ;; +--bitmap-new) + BITMAP_NEW="$2" Review Comment: @jmsperu Are you working on this one too? -- 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]
