NAS backup: compression, encryption, bandwidth throttle, integrity check - #12898
NAS backup: compression, encryption, bandwidth throttle, integrity check#12898jmsperu wants to merge 12 commits into
Conversation
Sorry, something went wrong.
… 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 61.45251% with 69 lines in your changes missing coverage. Please review. @@ Coverage Diff @@
## 4.22 #12898 +/- ##
==========================================
Coverage 17.79% 17.79%
- Complexity 15995 16019 +24
==========================================
Files 5928 5929 +1
Lines 534306 534456 +150
Branches 65383 65401 +18
==========================================
+ Hits 95069 95124 +55
- Misses 428467 428560 +93
- Partials 10770 10772 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
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.sh for compression (-c), encryption (-e), bandwidth throttling (-b), and verification (--verify).
- Extend TakeBackupCommand with a details map 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.
Sorry, something went wrong.
- 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
|
@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. |
Sorry, something went wrong.
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17323 |
Sorry, something went wrong.
|
@jmsperu can you check/fix the build failure. |
Sorry, something went wrong.
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 overrideConfigValue() in NASBackupProviderTest only setting _value on ConfigKey, but the zone-scoped configs (valueIn(zoneId)) fall back to _defaultValue when s_depot is null in the test context. All 5 config values (compression, bandwidth, encryption, encryption passphrase, integrity check) were returning null instead of the test values. The fix also sets _defaultValue via ReflectionTestUtils so valueIn() correctly resolves test values. Also addressed in the previous commit: replaced 2>&1 | tee -a with >> logFile 2>&1 in nasbackup.sh to prevent tee from masking non-zero exit codes, and added return 1 after cleanup on virsh backup job failure. Could you please retrigger the build? @blueorangutan package |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17346 |
Sorry, something went wrong.
|
@DaanHoogland Thanks for the review — addressed both points in 0effda26e4: Method extraction — pulled the inline enhancement logic into dedicated methods:
(The shell side was already factored — encrypt_backup() / verify_backup() are standalone functions in nasbackup.sh.) Testing — added LibvirtTakeBackupCommandWrapperTest (5 cases: compression/bandwidth/integrity flag mapping, bandwidth=0 skip, encryption writes the 0600 passphrase file + -e flag, and fail-fast when encryption is enabled without a passphrase). The existing NASBackupProviderTest (14 cases, covering the details map) stays green — confirming the refactor is behavior-preserving. |
Sorry, something went wrong.
|
@DaanHoogland 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. |
Sorry, something went wrong.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18329 |
Sorry, something went wrong.
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
Sorry, something went wrong.
|
[SF] Trillian test result (tid-16388)
|
Sorry, something went wrong.
…; drop trailing '(Copilot review)' comment artifacts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)scripts/vm/hypervisor/kvm/nasbackup.sh:461
- The -b/--bandwidth flag is accepted without validating that it is a positive integer. If a non-numeric value (or 0/negative) is passed, the later virsh/qemu-img calls will fail in confusing ways (and could treat the value as additional CLI args). Validate the argument at parse time and reject invalid values with a clear error.
-b|--bandwidth)
BANDWIDTH="$2"
shift
shift
;;
scripts/vm/hypervisor/kvm/nasbackup.sh:387
- On mount failure, the script now only prints a generic "Failed to mount" message and redirects the actual mount error output solely into the agent log. This makes failures hard to diagnose from the management server side (stdout/stderr). Capture the mount output and include it in the error message while still appending it to the log.
if mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) >> "$logFile" 2>&1; then
log -ne "Successfully mounted ${NAS_TYPE} store"
else
echo "Failed to mount ${NAS_TYPE} store"
exit 1
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:112
- The temporary passphrase file deletion is currently best-effort but silent (passphraseFile.delete() return value is ignored). If deletion fails, the key file may remain on disk without any indication. Prefer Files.deleteIfExists(...) and log failures so operators can detect and remediate leftover secret files.
// Clean up passphrase file after backup completes (best-effort).
if (passphraseFile != null && passphraseFile.exists()) {
passphraseFile.delete();
}
scripts/vm/hypervisor/kvm/nasbackup.sh:233
- The bandwidth-throttling virsh calls use unquoted $VM and $disk, which can break if either contains unexpected characters/whitespace. Quote these arguments to avoid word-splitting/globbing and keep behavior consistent with the other virsh invocations in this function.
This issue also appears in the following locations of the same file:
- line 383
- line 457
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
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:116
- The config key name "nas.backup.bandwidth.limit.mbps" suggests megabits/sec (Mbps), but the description and the script usage treat the value as MiB/sec. This mismatch is likely to confuse operators and lead to misconfiguration. Consider renaming the key (or changing the documented units and implementing conversion) to make the units unambiguous.
ConfigKey<Integer> NASBackupBandwidthLimitMbps = new ConfigKey<>("Advanced", Integer.class,
"nas.backup.bandwidth.limit.mbps",
"0",
"Bandwidth limit in MiB/s for backup operations (0 = unlimited).",
true,
Sorry, something went wrong.
|
The failing build shard here (test_lb_secondary_ip, test_list_nics, test_list_pod, …) is unrelated to this backup change — those are load-balancer/networking smoke tests, and the other 24 shards pass. Looks like a flaky/infra failure. Could a committer kick off a re-run of that shard? Thanks. |
Sorry, something went wrong.
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 --object secret + --image-opts, file-based pools are decrypted during a qcow2 convert instead of being rsync'd, and a missing passphrase gives an explicit error. The older Copilot threads were already addressed in the branch and I've replied on each. CI on the previous SHA was fully green and Trillian passed on 22 Jun; would appreciate approve-and-run on the new SHA and a review whenever someone has time. |
Sorry, something went wrong.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Sorry, something went wrong.
…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 7f2c57ab1a) — this is mergeable again. 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 @blueorangutan package / test whenever a committer can approve-and-run. |
Sorry, something went wrong.
|
@jmsperu a [SL] Jenkins job has been kicked to build packages. It will be bundled with /test` whenever a committer can approve-and-run. SystemVM template(s). I'll keep you posted as I make progress. |
Sorry, something went wrong.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19011 |
Sorry, something went wrong.
Summary
Adds four optional, zone-scoped features to NAS backup operations on KVM, all disabled by default:
Configuration Keys (Zone scope)
Architecture
Files Changed
Notes
Test plan