NAS backup: compression, encryption, bandwidth throttle, integrity check - #12898
NAS backup: compression, encryption, bandwidth throttle, integrity check#12898jmsperu wants to merge 13 commits into
Conversation
… integrity check Adds four optional features to NAS backup operations, configurable at zone scope via CloudStack global settings: - Compression (-c): qcow2 internal compression of backup files Config: nas.backup.compression.enabled (default: false) - LUKS Encryption (-e): encrypt backup files at rest using qemu-img Config: nas.backup.encryption.enabled (default: false) Config: nas.backup.encryption.passphrase (Secure category) - Bandwidth Throttle (-b): limit backup I/O bandwidth via virsh blockjob for running VMs or qemu-img -r for stopped VMs Config: nas.backup.bandwidth.limit.mbps (default: 0/unlimited) - Integrity Check (--verify): qemu-img check after backup creation Config: nas.backup.integrity.check (default: false) All features are disabled by default and fully backward compatible. Settings are read from zone-scoped ConfigKeys in NASBackupProvider, passed to the KVM agent via TakeBackupCommand details map, and translated to nasbackup.sh CLI flags in LibvirtTakeBackupCommandWrapper. Changes: - nasbackup.sh: add -c, -b, -e, --verify flags with encrypt_backup() and verify_backup() helper functions - TakeBackupCommand.java: add details map for passing config to agent - NASBackupProvider.java: add 5 ConfigKeys, populate command details - LibvirtTakeBackupCommandWrapper.java: extract details, build CLI args, handle passphrase temp file lifecycle Combines and supersedes PRs apache#12844, apache#12846, apache#12848, apache#12845
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12898 +/- ##
==========================================
Coverage 17.86% 17.86%
- Complexity 16037 16063 +26
==========================================
Files 5928 5929 +1
Lines 534479 534629 +150
Branches 65410 65428 +18
==========================================
+ Hits 95468 95537 +69
- Misses 428173 428244 +71
- Partials 10838 10848 +10
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
The test helper overrideConfigValue() was only setting _value on ConfigKey, but zone-scoped configs (valueIn(zoneId)) fall back to _defaultValue when s_depot is null in test context. Also set _defaultValue via ReflectionTestUtils to ensure valueIn() returns the expected test value. Fixes: 4 assertion failures (compression, bandwidth, encryption, integrity_check details all returned null) and 1 error (encryption without passphrase expected CloudRuntimeException but got NullPointerException from null config value).
|
@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 |
|
The failing |
The restore path ran plain 'qemu-img check' and rsync/convert, so a backup taken with nas.backup.encryption.enabled could not be verified or restored. - RestoreBackupCommand carries the zone's passphrase (@loglevel Off); the NAS provider sets it whenever one is configured so older encrypted backups stay restorable after encryption is switched off. - LibvirtRestoreBackupCommandWrapper probes 'qemu-img info' for an encrypted image and then checks/converts with '--object secret' + '--image-opts'. File based pools are decrypted during a qcow2 convert instead of being rsync'd (a copied LUKS volume would be unbootable); RBD/LINSTOR use the same secret on the raw convert. A clear error is returned when the backup is encrypted and no passphrase is configured. - NasBackupPassphraseFile is the shared 0600 temp key file helper for the take and restore wrappers. - Unit tests for the encrypted check/convert path, the missing-passphrase failure and the provider side. Signed-off-by: James Peru <jmsperu@gmail.com>
|
Pushed 4d919dc. The important one is Copilot's most recent point: encrypted backups could be taken but not restored, because the restore wrapper opened the qcow2 without the LUKS secret. That is now fixed end to end (details in-thread): the passphrase travels on RestoreBackupCommand, the wrapper detects an encrypted image and checks/converts with |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
…ments-combined Signed-off-by: James Peru <jmsperu@gmail.com> # Conflicts: # plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java # plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java
|
Rebased onto current 4.22 (merge Two files conflicted, both from the recent command-injection hardening on 4.22:
No behaviour from either side was dropped: the hardening applies to the plain path, the decrypt-on-restore applies to the encrypted path, and they do not overlap. Verified locally on JDK17:
Ready for |
|
@jmsperu a [SL] Jenkins job has been kicked to build packages. It will be bundled with |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19011 |
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. |
…ments-combined Signed-off-by: James Peru <jmsperu@gmail.com> # Conflicts: # plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java
|
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.
| 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.
| 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?
| 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.
| 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.
| 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?
| 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?
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