Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12898 +/- ##
============================================
+ Coverage 18.01% 18.04% +0.02%
- Complexity 16265 16305 +40
============================================
Files 5936 5937 +1
Lines 536395 536554 +159
Branches 65701 65722 +21
============================================
+ Hits 96658 96812 +154
+ Misses 428710 428706 -4
- Partials 11027 11036 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds optional, zone-scoped enhancements for KVM NAS backups (compression, LUKS encryption, bandwidth throttling, and post-backup integrity verification) by plumbing config from management server → TakeBackupCommand details → KVM agent wrapper → nasbackup.sh flags.
Changes:
- Add new CLI flags and implementation in
nasbackup.shfor compression (-c), encryption (-e), bandwidth throttling (-b), and verification (--verify). - Extend
TakeBackupCommandwith adetailsmap to carry optional settings to the agent. - Add zone-scoped NAS backup ConfigKeys and populate command details; update KVM wrapper to translate details into script args and manage a temporary passphrase file.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
scripts/vm/hypervisor/kvm/nasbackup.sh |
Implements compression/encryption/throttle/verify logic and argument parsing for NAS backup operations. |
core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java |
Adds a details map to carry optional backup feature settings from management to agent. |
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java |
Introduces zone-scoped ConfigKeys and passes enabled settings into TakeBackupCommand details. |
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java |
Builds dynamic nasbackup.sh command args from TakeBackupCommand details and writes an encryption passphrase temp file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- nasbackup.sh: Replace exit 1 with return 1 in encrypt_backup and verify_backup so callers can run cleanup before terminating - nasbackup.sh: Append (>>) instead of truncate (>) agent.log in qemu-img convert for stopped VM backups - nasbackup.sh: Add return 1 after cleanup on qemu-img convert failure to stop execution - nasbackup.sh: Callers of encrypt_backup/verify_backup now check return code and run cleanup on failure - LibvirtTakeBackupCommandWrapper: Fail with error when encryption is enabled but passphrase is missing instead of silently skipping - LibvirtTakeBackupCommandWrapper: Delete temp passphrase file in finally block, set 0600 permissions, use explicit UTF-8 charset - NASBackupProvider: Throw CloudRuntimeException when encryption is enabled but passphrase is null/empty - NASBackupProviderTest: Add tests for compression, bandwidth, integrity check, encryption+passphrase, and encryption-without- passphrase failure scenarios - TakeBackupCommand: Add @loglevel(Off) to details field to prevent passphrase leaking in debug logs - TakeBackupCommand: Normalize null to empty HashMap in setDetails
|
@blueorangutan package |
|
@sureshanaparti 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. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17323 |
|
@jmsperu can you check/fix the build failure. |
Address remaining Copilot review feedback on PR apache#12898: - Replace `2>&1 | tee -a` with `>> logFile 2>&1` in encrypt_backup, compress, and mount_operation to prevent tee from masking non-zero exit codes of qemu-img and mount commands - Add `return 1` after cleanup on virsh backup job failure to prevent continuing execution with broken state
|
@sureshanaparti Fixed. The test failures were caused by The fix also sets Also addressed in the previous commit: replaced Could you please retrigger the build? |
|
@jmsperu a [SL] Jenkins job has been kicked to build packages. It will be bundled with` SystemVM template(s). I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17346 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
scripts/vm/hypervisor/kvm/nasbackup.sh:321
mount_operationruns underset -e, butmountis executed as a standalone command and then$?is checked. Withset -e, a mount failure will exit the script immediately and bypass the custom error message/handling. Consider rewriting this as a conditionalif ! mount ...; then ...; fiso failures are handled deterministically while still logging stderr.
mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) >> "$logFile" 2>&1
if [ $? -eq 0 ]; then
log -ne "Successfully mounted ${NAS_TYPE} store"
else
echo "Failed to mount ${NAS_TYPE} store"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DaanHoogland
moved this from Backlog
to conflict/waiting for author
in CloudStack Testing
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
Synced with 4.22 again (a39a95d). One conflict, in Verified on JDK 17: |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19185 |
| local compress_flag="" | ||
| if [[ "$COMPRESS" == "true" ]]; then | ||
| compress_flag="-c" | ||
| fi | ||
| for img in "$backup_dir"/*.qcow2; do | ||
| [[ -f "$img" ]] || continue | ||
| local tmp_img="${img}.luks" | ||
| if qemu-img convert $compress_flag -O qcow2 \ |
There was a problem hiding this comment.
Blocking: qcow2 cannot compress and encrypt at the same time, so enabling both nas.backup.compression.enabled and nas.backup.encryption.enabled fails every backup and then deletes it.
Reproduced with qemu-img 9.2.3:
$ qemu-img convert -c -O qcow2 --object secret,id=sec0,file=pass.key \
-o encrypt.format=luks,encrypt.key-secret=sec0 plain.qcow2 out.qcow2
qemu-img: Compression and encryption not supported at the same time
With both settings on, encrypt_backup returns 1, the caller runs cleanup, and the whole backup directory is removed. This hits both the running-VM and stopped-VM paths.
The comment above compress_flag says it exists so compression is not silently discarded, but the result is a hard failure instead. Could you either reject the combination up front in applyBackupEnhancementDetails (clearest, the admin gets told why) or drop -c here and document that encryption wins? Either way it would be good to add a test for the two together, since neither the unit tests nor the test-plan checklist covers that combination today.
There was a problem hiding this comment.
You were right, and thanks for the reproduction. I got the same error with qemu-img 11.1.2, and the old script built the backup and then deleted it. I went with rejecting the combination:
- f82d33f:
applyBackupEnhancementDetailsfails the request before anything is created when both settings are on for the zone. The error names both settings and says no backup was taken, and both setting descriptions now say they cannot be combined. - 1f596ed:
nasbackup.shrefuses-ctogether with-ebefore it mounts anything (defence in depth for direct calls), andencrypt_backupno longer passes-c.
Tests: testTakeBackupRejectsCompressionWithEncryptionBeforeCreatingBackup (no persist, no agent call), plus a script run with -c -e against real qemu-img: it now exits 1 with the message and never mounts. Without the fix, the backup was created and then deleted.
| command.setQuiesce(quiesceVM); | ||
|
|
||
| // Pass optional backup enhancement settings from zone-scoped configs | ||
| applyBackupEnhancementDetails(command, vm.getDataCenterId()); |
There was a problem hiding this comment.
Blocking: createBackupObject() on line 242 has already persisted a BackupVO in BackingUp state by the time this runs, so when encryption is enabled without a passphrase the CloudRuntimeException thrown from applyBackupEnhancementDetails leaves an orphaned backup row stuck in BackingUp forever.
Every other failure path in takeBackup calls backupDao.remove(backupVO.getId()) before throwing. Since the validation needs nothing from the backup object, moving this call above line 242 fixes it cleanly.
There was a problem hiding this comment.
Right, thanks. Fixed in f82d33f: the command is built and applyBackupEnhancementDetails runs before createBackupObject, so a missing passphrase (or the compression + encryption combination) fails before any row exists. testTakeBackupEncryptionWithoutPassphraseThrows now verifies that backupDao.persist and the agent are never called. Mockito's strict stubs also flagged that the volume lookup no longer runs, which confirms the new order.
| fi | ||
| output="$dest/$name.$volUuid.qcow2" | ||
| if ! qemu-img convert -O qcow2 "$disk" "$output" > "$logFile" 2> >(cat >&2); then | ||
| if ! ionice -c 3 qemu-img convert $([[ "$COMPRESS" == "true" ]] && echo "-c") $([[ -n "$BANDWIDTH" ]] && echo "-r" "${BANDWIDTH}M") -O qcow2 "$disk" "$output" >> "$logFile" 2> >(cat >&2); then |
There was a problem hiding this comment.
Blocking: qemu-img convert -r is not available on the minimum QEMU this script supports. sanity_checks() only requires QEMU >= 4.2.0, but -r rate_limit was added much later (it is present in 9.2, absent in 4.2). On a 4.2/5.x host (RHEL 8, Ubuntu 20.04) enabling nas.backup.bandwidth.limit.mbps would make every stopped-VM backup fail with invalid option -- 'r'.
Could you check the exact QEMU version that introduced it and either probe for support with a graceful fallback, or raise the minimum version check when bandwidth limiting is requested?
There was a problem hiding this comment.
Good catch. -r was added in QEMU 5.2.0 (qemu commit 0c8c4895a6, "qemu-img: add support for rate limit in qemu-img convert"), so 4.2 and 5.0/5.1 reject it. In 1f596ed the script probes for it by running qemu-img convert -r on a throwaway 1 MiB image, rather than parsing --help, whose format has changed between releases. If the probe fails, the backup still runs at idle I/O priority but without -r, and the agent log gets a WARNING saying the limit could not be applied and that QEMU >= 5.2 is required. I chose not to fail the backup in that case, because a missing backup is worse than an unthrottled one.
I checked both paths with the PATH-shimmed harness against real qemu-img and a shim that rejects -r like old QEMU. Before the fix, the backup failed with invalid option -- 'r'. After it, the backup succeeds and the warning is logged. While doing this I also found that the old $(...) substitution passed -r 50M as a single word, because IFS=, is set in that function. The convert arguments are now built as arrays.
| fi | ||
| output="$dest/$name.$volUuid.qcow2" | ||
| if ! qemu-img convert -O qcow2 "$disk" "$output" > "$logFile" 2> >(cat >&2); then | ||
| if ! ionice -c 3 qemu-img convert $([[ "$COMPRESS" == "true" ]] && echo "-c") $([[ -n "$BANDWIDTH" ]] && echo "-r" "${BANDWIDTH}M") -O qcow2 "$disk" "$output" >> "$logFile" 2> >(cat >&2); then |
There was a problem hiding this comment.
ionice -c 3 is applied unconditionally here, so it also affects backups where none of the four new settings are enabled. That is a silent default behaviour change for existing users. Could it be gated on $BANDWIDTH being set?
Worth noting too that -c 3 (idle) only has an effect under CFQ/BFQ. With mq-deadline or none, the default for NVMe on current kernels, it is a no-op, so it may not buy much even when the feature is on.
There was a problem hiding this comment.
Agreed, that was an unintended default change. In 1f596ed ionice -c 3 is only added when a bandwidth limit is set, so backups without the new settings run the exact command they ran before (I checked the harness call log for both cases). You are also right that the idle class does nothing under mq-deadline or none. It is now only a best-effort extra next to -r, and on hosts without -r the warning says the backup ran "at idle I/O priority only" rather than claiming it was throttled.
| if [[ -n "$BANDWIDTH" ]]; then | ||
| for disk in $(virsh -c qemu:///system domblklist $VM --details 2>/dev/null | awk '/disk/{print$3}'); do | ||
| virsh -c qemu:///system blockjob $VM $disk --bandwidth "${BANDWIDTH}" 2>/dev/null || true | ||
| done | ||
| log -ne "Backup bandwidth limited to ${BANDWIDTH} MiB/s per disk for $VM" |
There was a problem hiding this comment.
Two things here.
-
stderr is sent to
/dev/nulland the failure is swallowed by|| true, but line 257 then logs that the limit was applied regardless. If the call fails, the admin sees a log line claiming throttling is active while nothing is throttled. Could you capture the exit status and log the actual outcome? -
I am not sure libvirt accepts
blockjob --bandwidthagainst a push-mode backup job. Backups are reported throughdomjobinforather thanblockjob, so the lookup may not resolve. Could you confirm on your target libvirt version that the bandwidth is actually applied, rather than the command erroring out silently?
Minor: awk '/disk/{print$3}' matches any line containing "disk", including a cdrom row whose source path happens to contain it. The existing code a few lines below uses awk '$2=="disk"', which would be more robust and consistent.
There was a problem hiding this comment.
-
Fixed in 1f596ed. Each
blockjob --bandwidthcall's exit status and stderr are captured. A disk that could not be throttled gets a WARNING with libvirt's error, and the summary line says how many disks were throttled and how many were not, so the log no longer claims a limit that is not in place. -
Fair question. From reading libvirt's qemu driver,
backup-beginregisters a block job on each disk, which is whatvirDomainBlockJobSetSpeedlooks up, so I expect it to resolve. I have not confirmed that on a real host with the current change yet. I will test it on a lab host and report back here. If it does not work there, I will drop the running-VM throttle and document the limit as stopped-VM only, rather than keep something that is a no-op.
Minor: done. It now uses awk '$2=="disk"{print $3}'. The harness showed the old pattern also hit a cdrom row whose ISO path contained "disk".
There was a problem hiding this comment.
did the lab run show the limit actually applies while the vm is running?
| local check_ok=0 | ||
| if [[ ${#check_secret[@]} -gt 0 ]]; then | ||
| qemu-img check "${check_secret[@]}" --image-opts \ | ||
| "driver=qcow2,file.filename=$img,encrypt.key-secret=sec0" \ | ||
| > /dev/null 2>&1 && check_ok=1 | ||
| else | ||
| qemu-img check "$img" > /dev/null 2>&1 && check_ok=1 | ||
| fi | ||
| if [[ $check_ok -eq 1 ]]; then | ||
| log -ne "Backup verification passed: $img" | ||
| else | ||
| echo "Backup verification failed for $img" | ||
| log -ne "Backup verification FAILED: $img" |
There was a problem hiding this comment.
Only exit code 0 is accepted, and a non-zero result makes the caller run cleanup, deleting the entire backup.
qemu-img check uses 2 for a corrupt image and 3 for leaked clusters. Leaks are benign, so as written a backup with leaked clusters is declared failed and destroyed. Could you confirm the exit code semantics on your side and treat 3 as a warning rather than a failure?
There was a problem hiding this comment.
Confirmed, you are right: 0 is clean, 2 is corruption, 3 is leaked clusters only, 1 means the check could not complete, and 63 means the format cannot be checked. In 1f596ed verify_backup accepts 0, accepts 3 with a log line ("passed with leaked clusters (wasted space only, data intact)"), and fails on everything else with the exit code in the message.
I tested this with real qemu-img by editing the refcount table of a backup image: one with a leaked cluster (qemu-img check exits 3) and one with a refcount that is too low (exits 2). Plain and LUKS-encrypted images were both covered. Before the fix, the leaked image was declared failed and deleted. Now it is kept with the log line, and the corrupt one is still rejected. d32f570 applies the same rule on the restore side, so a backup with leaked clusters can be restored.
| private boolean isEncryptedImage(String backupPath) { | ||
| String info = Script.executeCommand("qemu-img", "info", "--output=json", backupPath); | ||
| return info != null && info.replaceAll("\\s", "").contains("\"encrypted\":true"); | ||
| } |
There was a problem hiding this comment.
This fails open. If Script.executeCommand returns null (qemu-img missing, path unreadable, timeout) the method returns false, and replaceVolumeWithBackup then falls through to rsync, copying an encrypted qcow2 verbatim onto the volume. The restore reports success but the volume is unbootable.
Could you distinguish "not encrypted" from "could not determine" and fail loudly on the latter?
There was a problem hiding this comment.
Agreed, that was fail-open. Fixed in d32f570: isEncryptedImage now throws when qemu-img info returns nothing usable (missing binary, unreadable file, timeout), and the restore fails with "... not known whether it is encrypted; refusing to restore it" before any copy. The "not encrypted" result is only returned when qemu-img actually read the header and it has no "encrypted": true. The new test testRestoreFailsClosedWhenEncryptionStateCannotBeDetermined checks that neither rsync nor qemu-img convert runs in that case. Two existing tests had been passing only because their mocked qemu-img info returned null; they now return a real plain-qcow2 result.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
Synced with 4.22 after #13877 and #14006 landed (merge 2a9bbbb). Both sets of settings are kept and the enhancement flags now live in the new One behavioural point for review: compression and LUKS encryption rewrite each backup file with Local results (JDK 17): NASBackupProviderTest 28/28, Restore wrapper 22/22, Take wrapper 5/5. |
|
@abh1sar can you have another look? |
…he backup row exists, reject compression with encryption applyBackupEnhancementDetails now runs before createBackupObject, so encryption without a passphrase no longer leaves an orphaned BackupVO in BackingUp. qcow2 cannot compress and encrypt the same image, so enabling both settings for a zone fails the request up front with a message naming both settings, instead of building the backup and deleting it. The setting descriptions say so, and the passphrase description warns that restores use the current passphrase.
…ard, rate-limit probe, honest throttle logging, accept leaked clusters - refuse -c together with -e before mounting anything (qcow2 cannot do both) - qemu-img convert -r only exists since QEMU 5.2: probe it on a throwaway image and fall back to an unthrottled copy with a WARNING in the log - ionice -c 3 only when a bandwidth limit is set; build the convert arguments as arrays (the old command substitution passed '-r 50M' as one word under IFS=,) - running VMs: log per disk whether blockjob --bandwidth succeeded, and match disks with $2=="disk" so cdrom rows are skipped - --verify: qemu-img check exit 3 (leaked clusters) passes with a log line; 2 (corruption) and other codes still fail
…on state cannot be read isEncryptedImage() now throws when qemu-img info returns nothing usable, so a restore never falls back to rsync for a file that might be LUKS-encrypted. qemu-img check exit 3 (leaked clusters) is accepted on restore as well; 2 still reports the backup as corrupt. Tests cover both, and the existing tests now stub a real plain-qcow2 qemu-img info result instead of relying on a null.
|
@abh1sar thanks for the thorough review. All seven inline points are answered in their threads; the fixes are in f82d33f (management server), 1f596ed (nasbackup.sh) and d32f570 (restore wrapper). Local results: NASBackupProviderTest 29/29, LibvirtRestoreBackupCommandWrapperTest 24/24, LibvirtTakeBackupCommandWrapperTest 5/5, checkstyle clean. I also ran the script changes through a PATH-shimmed harness with real qemu-img, comparing old and new, for compression + encryption, On your question about rotating the passphrase: yes, today a restore only gets the zone's current |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
… integrity check Rebased onto 4.22 as a single commit (the branch history had several merges of 4.22, which a commit-by-commit rebase cannot replay cleanly). Resolved against "Nasbackup quiesce fixes (apache#14305)": TakeBackupCommand and NASBackupProvider keep both the enhancement details and the new quiesce timeout and cleanup alert; nasbackup.sh keeps both option sets and takes the upstream 'exit 1' after a failed backup job; the PR's enhancement-flag tests move to LibvirtTakeBackupCommandWrapperEnhancementFlagsTest next to the upstream LibvirtTakeBackupCommandWrapperTest. Squashed from: - address review (apache#12898): restore fails closed when the encryption state cannot be read - address review (apache#12898): nasbackup.sh compression/encryption guard, rate-limit probe, honest throttle logging, accept leaked clusters - address review (apache#12898): validate enhancement settings before the backup row exists, reject compression with encryption - Restore wrapper tests: count only qemu-img calls - address review (apache#12898): restore LUKS-encrypted backups - address review (apache#12898): reorder java.io imports alphabetically; drop trailing '(Copilot review)' comment artifacts - refactor(backup): extract NAS backup enhancement logic into dedicated methods + tests - refactor(backup): consolidate detail keys in TakeBackupCommand - Fix build: use reflection for protected ConfigKey.valueOf() in test - Fix NASBackupProviderTest failures for zone-scoped ConfigKey values - Fix tee masking exit codes and missing return in nasbackup.sh - Address Copilot review feedback on NAS backup PR apache#12898 - NAS backup enhancements: compression, encryption, bandwidth throttle, integrity check Claude-Session: https://claude.ai/code/session_01BhiE1Abhs315m3n3UPy6V7
d32f570 to
1d2eec1
Compare
|
Rebased on the current 4.22 (1d2eec1). The branch history had several merges of 4.22, so it is now one commit on top of 4.22; the message lists the commits it replaces. The only upstream change that conflicted was "Nasbackup quiesce fixes" (#14305):
core, NAS plugin and KVM plugin build with checkstyle; NASBackupProviderTest 31/31, Libvirt take/restore backup wrapper tests 32/32; bash -n clean. Ready for review. |
|
A note on the rebase: the branch had picked up several merges of 4.22, so I rebased it as a single commit on top of 4.22 (its message lists the 13 commits it replaces) rather than replaying each one. Commit hashes quoted in earlier review threads are therefore no longer on the branch. If reviewers would rather see the separate commits again, let me know and I will restore them; otherwise I will keep it as one commit, since the PR would be squash-merged anyway. |
@jmsperu one commit is fine. checked the rebased commit against abhisars seven points and they look fixed except the running vm throttle test on a real host |
Summary
Adds four optional, zone-scoped features to NAS backup operations on KVM, all disabled by default:
-c): Uses qcow2 internal compression (qemu-img convert -c) to reduce backup size-e): Encrypts backup files at rest using LUKS viaqemu-img convert --object secret-b): Limits backup I/O —virsh blockjob --bandwidthfor running VMs,qemu-img convert -r+ionicefor stopped VMs--verify): Runsqemu-img checkon each backup file after creationConfiguration Keys (Zone scope)
nas.backup.compression.enablednas.backup.encryption.enablednas.backup.encryption.passphrasenas.backup.bandwidth.limit.mbpsnas.backup.integrity.checkArchitecture
detailsmap onTakeBackupCommandnasbackup.shCLI flagsFiles Changed
scripts/vm/hypervisor/kvm/nasbackup.sh— new-c,-b,-e,--verifyflags withencrypt_backup()andverify_backup()functionscore/.../TakeBackupCommand.java— addeddetailsmap (HashMap) with getter/setter/addDetailplugins/backup/nas/.../NASBackupProvider.java— 5 new ConfigKeys, populate command details intakeBackup()plugins/hypervisors/kvm/.../LibvirtTakeBackupCommandWrapper.java— extract details, build dynamic CLI args, temp passphrase file lifecycleNotes
Test plan
nas.backup.compression.enabledat zone scope, take backup, verify qcow2 files are compressednas.backup.bandwidth.limit.mbps(e.g. 50), take backup of running VM, verifyvirsh blockjobbandwidth is appliednas.backup.bandwidth.limit.mbps, take backup of stopped VM, verifyqemu-img -rrate limit is appliednas.backup.encryption.enabledwith passphrase, take backup, verify files are LUKS encrypted (qemu-img infoshows encryption)nas.backup.integrity.check, take backup, verifyqemu-img checkruns and passes