abh1sar commented on code in PR #12898:
URL: https://github.com/apache/cloudstack/pull/12898#discussion_r3988082181
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -206,6 +246,9 @@ public Pair<Boolean, Backup> takeBackup(final
VirtualMachine vm, Boolean quiesce
command.setMountOptions(backupRepository.getMountOptions());
command.setQuiesce(quiesceVM);
+ // Pass optional backup enhancement settings from zone-scoped configs
+ applyBackupEnhancementDetails(command, vm.getDataCenterId());
Review 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.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -87,6 +91,75 @@ sanity_checks() {
log -ne "Environment Sanity Checks successfully passed"
}
+encrypt_backup() {
+ local backup_dir="$1"
+ if [[ -z "$ENCRYPT_PASSFILE" ]]; then
+ return
+ fi
+ if [[ ! -f "$ENCRYPT_PASSFILE" ]]; then
+ echo "Encryption passphrase file not found: $ENCRYPT_PASSFILE"
+ return 1
+ fi
+ log -ne "Encrypting backup files with LUKS"
+ # Preserve compression if it was requested upstream — otherwise the
+ # encrypt-step re-convert produces an uncompressed (but encrypted) qcow2,
+ # silently discarding the compression work done earlier.
+ 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 \
Review 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.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -254,14 +366,30 @@ backup_stopped_vm() {
volUuid="${disk##*/}"
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
Review 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?
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -254,14 +366,30 @@ backup_stopped_vm() {
volUuid="${disk##*/}"
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
Review 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.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -87,6 +91,75 @@ sanity_checks() {
log -ne "Environment Sanity Checks successfully passed"
}
+encrypt_backup() {
+ local backup_dir="$1"
+ if [[ -z "$ENCRYPT_PASSFILE" ]]; then
+ return
+ fi
+ if [[ ! -f "$ENCRYPT_PASSFILE" ]]; then
+ echo "Encryption passphrase file not found: $ENCRYPT_PASSFILE"
+ return 1
+ fi
+ log -ne "Encrypting backup files with LUKS"
+ # Preserve compression if it was requested upstream — otherwise the
+ # encrypt-step re-convert produces an uncompressed (but encrypted) qcow2,
+ # silently discarding the compression work done earlier.
+ 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 \
+ --object "secret,id=sec0,file=$ENCRYPT_PASSFILE" \
+ -o "encrypt.format=luks,encrypt.key-secret=sec0" \
+ "$img" "$tmp_img" >> "$logFile" 2>&1; then
+ mv "$tmp_img" "$img"
+ log -ne "Encrypted: $img"
+ else
+ echo "Encryption failed for $img"
+ rm -f "$tmp_img"
+ return 1
+ fi
+ done
+}
+
+verify_backup() {
+ local backup_dir="$1"
+ local failed=0
+ # If encryption was applied to this backup, qemu-img check has to open the
+ # qcow2 with the same LUKS secret — otherwise every verification call fails
+ # with a "Could not open" error and --verify is unusable on encrypted
+ # backups.
+ local check_secret=()
+ if [[ -n "$ENCRYPT_PASSFILE" && -f "$ENCRYPT_PASSFILE" ]]; then
+ check_secret=(--object "secret,id=sec0,file=$ENCRYPT_PASSFILE")
+ fi
+ for img in "$backup_dir"/*.qcow2; do
+ [[ -f "$img" ]] || continue
+ 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"
Review 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?
Separately, deleting the backup on a verification failure removes the
evidence needed to diagnose it. Leaving it in place and reporting the failure
would probably be more useful for an integrity check.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -176,6 +249,14 @@ backup_running_vm() {
exit 1
fi
+ # Throttle backup bandwidth if requested (MiB/s per disk)
+ 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"
Review Comment:
Two things here.
1. stderr is sent to `/dev/null` and 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?
2. I am not sure libvirt accepts `blockjob --bandwidth` against a push-mode
backup job. Backups are reported through `domjobinfo` rather than `blockjob`,
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.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -257,31 +266,66 @@ private String getBackupPath(String mountDirectory,
String backupPath, String ba
return bkpPath;
}
- private boolean checkBackupFileImage(String backupPath) {
- int exitValue =
Script.runSimpleBashScriptForExitValue(String.format("qemu-img check %s",
backupPath));
- return exitValue == 0;
+ private boolean checkBackupFileImage(String backupPath, File keyFile) {
+ if (!isEncryptedImage(backupPath)) {
+ int exitValue =
Script.runSimpleBashScriptForExitValue(String.format("qemu-img check %s",
backupPath));
+ return exitValue == 0;
+ }
+ List<String> cmd = new ArrayList<>(List.of("qemu-img", "check"));
+ cmd.addAll(encryptedSourceArgs(backupPath, keyFile));
+ return Script.executeCommandForExitValue(cmd.toArray(new String[0]))
== 0;
+ }
+
+ /**
+ * True when qemu reports the backup qcow2 as encrypted (LUKS, produced by
nasbackup.sh {@code -e}).
+ * Reading the header needs no secret, so this works before any passphrase
is involved.
+ */
+ private boolean isEncryptedImage(String backupPath) {
+ String info = Script.executeCommand("qemu-img", "info",
"--output=json", backupPath);
+ return info != null && info.replaceAll("\\s",
"").contains("\"encrypted\":true");
+ }
Review 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?
--
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]
