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]
