Copilot commented on code in PR #12898:
URL: https://github.com/apache/cloudstack/pull/12898#discussion_r3036803297
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##########
@@ -68,20 +78,67 @@ public Answer execute(TakeBackupCommand command,
LibvirtComputingResource libvir
}
}
+ List<String> cmdArgs = new ArrayList<>();
+ cmdArgs.add(libvirtComputingResource.getNasBackupPath());
+ cmdArgs.add("-o"); cmdArgs.add("backup");
+ cmdArgs.add("-v"); cmdArgs.add(vmName);
+ cmdArgs.add("-t"); cmdArgs.add(backupRepoType);
+ cmdArgs.add("-s"); cmdArgs.add(backupRepoAddress);
+ cmdArgs.add("-m"); cmdArgs.add(Objects.nonNull(mountOptions) ?
mountOptions : "");
+ cmdArgs.add("-p"); cmdArgs.add(backupPath);
+ cmdArgs.add("-q"); cmdArgs.add(command.getQuiesce() != null &&
command.getQuiesce() ? "true" : "false");
+ cmdArgs.add("-d"); cmdArgs.add(diskPaths.isEmpty() ? "" :
String.join(",", diskPaths));
+
+ // Append optional enhancement flags from management server config
+ File passphraseFile = null;
+ Map<String, String> details = command.getDetails();
+ if (details != null) {
+ if ("true".equals(details.get("compression"))) {
+ cmdArgs.add("-c");
+ }
+ if ("true".equals(details.get("encryption"))) {
+ String passphrase = details.get("encryption_passphrase");
Review Comment:
The detail keys (e.g., "compression", "encryption_passphrase",
"bandwidth_limit") are hard-coded string literals shared across management
server, agent wrapper, and the shell script. This makes the feature wiring
fragile to typos and hard to refactor. Consider defining these keys as
constants in a shared place (e.g., `TakeBackupCommand`) and reusing them in
both NASBackupProvider and the KVM wrapper.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -87,6 +91,52 @@ 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"
+ for img in "$backup_dir"/*.qcow2; do
+ [[ -f "$img" ]] || continue
+ local tmp_img="${img}.luks"
+ if qemu-img convert -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
+ for img in "$backup_dir"/*.qcow2; do
+ [[ -f "$img" ]] || continue
+ if ! qemu-img check "$img" > /dev/null 2>&1; then
+ echo "Backup verification failed for $img"
+ log -ne "Backup verification FAILED: $img"
+ failed=1
+ else
+ log -ne "Backup verification passed: $img"
Review Comment:
`verify_backup` runs `qemu-img check` without providing the LUKS secret. If
encryption is enabled, `qemu-img check` will fail to open the encrypted qcow2
files, so `--verify` will consistently fail for encrypted backups. Consider
passing the same `--object secret,...` + encryption opts to `qemu-img check`,
or skipping verification when encryption is enabled unless a passphrase file is
available.
```suggestion
if [[ -n "$ENCRYPT_PASSFILE" ]]; then
if [[ ! -f "$ENCRYPT_PASSFILE" ]]; then
echo "Encryption passphrase file not found: $ENCRYPT_PASSFILE"
log -ne "Backup verification FAILED: missing encryption passphrase
file for $img"
failed=1
continue
fi
if ! qemu-img check \
--object "secret,id=sec0,file=$ENCRYPT_PASSFILE" \
--image-opts
"driver=qcow2,file.driver=file,file.filename=$img,encrypt.key-secret=sec0" \
> /dev/null 2>&1; then
echo "Backup verification failed for $img"
log -ne "Backup verification FAILED: $img"
failed=1
else
log -ne "Backup verification passed: $img"
fi
else
if ! qemu-img check "$img" > /dev/null 2>&1; then
echo "Backup verification failed for $img"
log -ne "Backup verification FAILED: $img"
failed=1
else
log -ne "Backup verification passed: $img"
fi
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -87,6 +91,52 @@ 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"
+ for img in "$backup_dir"/*.qcow2; do
+ [[ -f "$img" ]] || continue
+ local tmp_img="${img}.luks"
+ if qemu-img convert -O qcow2 \
+ --object "secret,id=sec0,file=$ENCRYPT_PASSFILE" \
+ -o "encrypt.format=luks,encrypt.key-secret=sec0" \
+ "$img" "$tmp_img" >> "$logFile" 2>&1; then
Review Comment:
When both compression and encryption are enabled, the encryption step
re-converts the image without `-c`, which likely produces an uncompressed (but
encrypted) qcow2 and discards the earlier compression work. If the intent is to
support combined features, include compression flags during the encryption
conversion (or fold encryption into the initial convert path).
```suggestion
local qemu_img_args=(
convert
-O qcow2
--object "secret,id=sec0,file=$ENCRYPT_PASSFILE"
-o "encrypt.format=luks,encrypt.key-secret=sec0"
)
if [[ -n "$COMPRESS" ]]; then
qemu_img_args+=(-c)
fi
qemu_img_args+=("$img" "$tmp_img")
if qemu-img "${qemu_img_args[@]}" >> "$logFile" 2>&1; then
```
--
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]
