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]
