jmsperu commented on code in PR #12898:
URL: https://github.com/apache/cloudstack/pull/12898#discussion_r4196841801
##########
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:
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:
- f82d33f80b: `applyBackupEnhancementDetails` fails 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.
- 1f596ed58f: `nasbackup.sh` refuses `-c` together with `-e` before it
mounts anything (defence in depth for direct calls), and `encrypt_backup` no
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.
##########
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:
Right, thanks. Fixed in f82d33f80b: 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.
##########
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:
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 1f596ed58f 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.
##########
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:
Agreed, that was an unintended default change. In 1f596ed58f `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.
##########
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:
1. Fixed in 1f596ed58f. Each `blockjob --bandwidth` call'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.
2. Fair question. From reading libvirt's qemu driver, `backup-begin`
registers a block job on each disk, which is what `virDomainBlockJobSetSpeed`
looks 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".
##########
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:
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 1f596ed58f `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. d32f5707ab
applies the same rule on the restore side, so a backup with leaked clusters can
be restored.
--
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]
