Copilot commented on code in PR #12898:
URL: https://github.com/apache/cloudstack/pull/12898#discussion_r3008588977
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -205,6 +245,26 @@ 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
+ Long zoneId = vm.getDataCenterId();
+ if (Boolean.TRUE.equals(NASBackupCompressionEnabled.valueIn(zoneId))) {
+ command.addDetail("compression", "true");
+ }
+ if (Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) {
+ command.addDetail("encryption", "true");
+ String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId);
+ if (passphrase != null && !passphrase.isEmpty()) {
+ command.addDetail("encryption_passphrase", passphrase);
+ }
+ }
+ Integer bandwidthLimit = NASBackupBandwidthLimitMbps.valueIn(zoneId);
+ if (bandwidthLimit != null && bandwidthLimit > 0) {
+ command.addDetail("bandwidth_limit",
String.valueOf(bandwidthLimit));
+ }
+ if
(Boolean.TRUE.equals(NASBackupIntegrityCheckEnabled.valueIn(zoneId))) {
+ command.addDetail("integrity_check", "true");
+ }
Review Comment:
The new zone-scoped feature flags are translated into `TakeBackupCommand`
details here, but the existing `NASBackupProviderTest.takeBackupSuccessfully`
doesn’t assert the details map contents. Add/extend unit tests to verify the
correct details are added for each config (compression, bandwidth limit,
integrity check, and encryption+passphrase; and that encryption without
passphrase fails).
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -174,14 +256,23 @@ 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
echo "qemu-img convert failed for $disk $output"
cleanup
Review Comment:
On `qemu-img convert` failure, this calls `cleanup` but then continues
execution (no `exit`/`return`). If cleanup succeeds, the function proceeds to
later steps with an unmounted/removed `dest`, which can cause confusing
follow-on failures and potentially report incorrect results. Exit the script
(or `return 1`) after `cleanup` here.
```suggestion
cleanup
return 1
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##########
@@ -68,21 +72,59 @@ 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");
+ if (passphrase != null && !passphrase.isEmpty()) {
+ try {
+ passphraseFile = File.createTempFile("cs-backup-enc-",
".key");
+ passphraseFile.deleteOnExit();
+ try (FileWriter fw = new FileWriter(passphraseFile)) {
+ fw.write(passphrase);
+ }
+ cmdArgs.add("-e");
cmdArgs.add(passphraseFile.getAbsolutePath());
+ } catch (IOException e) {
+ logger.error("Failed to create encryption passphrase
file", e);
+ return new BackupAnswer(command, false, "Failed to
create encryption passphrase file: " + e.getMessage());
+ }
Review Comment:
The temp passphrase file can be left behind if an exception is thrown after
`createTempFile` succeeds (e.g., `FileWriter` fails) because the catch returns
without deleting it. Also consider setting strict permissions (0600) and using
an explicit charset (UTF-8) when writing the passphrase.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -174,14 +256,23 @@ 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:
This redirects stdout to `> "$logFile"`, which truncates
`/var/log/cloudstack/agent/agent.log` each time a stopped-VM disk conversion
runs (and for every disk). Use append (`>>`) or pipe through `tee -a` to avoid
destroying the agent log.
```suggestion
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
```
##########
core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java:
##########
@@ -106,6 +109,18 @@ public void setQuiesce(Boolean quiesce) {
this.quiesce = quiesce;
}
+ public Map<String, String> getDetails() {
+ return details;
+ }
+
+ public void setDetails(Map<String, String> details) {
+ this.details = details;
Review Comment:
`setDetails` assigns the map directly and can set it to null; later
`addDetail` will NPE. Normalize null to an empty map inside `setDetails` (or
remove the setter / keep `details` final) to make the command robust.
```suggestion
this.details = details != null ? details : new HashMap<>();
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##########
@@ -68,21 +72,59 @@ 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");
+ if (passphrase != null && !passphrase.isEmpty()) {
+ try {
+ passphraseFile = File.createTempFile("cs-backup-enc-",
".key");
+ passphraseFile.deleteOnExit();
+ try (FileWriter fw = new FileWriter(passphraseFile)) {
+ fw.write(passphrase);
+ }
+ cmdArgs.add("-e");
cmdArgs.add(passphraseFile.getAbsolutePath());
+ } catch (IOException e) {
+ logger.error("Failed to create encryption passphrase
file", e);
+ return new BackupAnswer(command, false, "Failed to
create encryption passphrase file: " + e.getMessage());
+ }
Review Comment:
If `details` indicates encryption is enabled but the passphrase is
missing/empty, the wrapper silently skips adding `-e` and the backup proceeds
unencrypted. This should fail the command (or at least log and return an error)
to avoid reporting a successful encrypted backup when encryption was requested.
```suggestion
}
} else {
logger.error("Encryption requested for backup but no
encryption passphrase was provided");
return new BackupAnswer(command, false, "Encryption
requested but encryption_passphrase is missing or empty");
```
##########
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"
+ exit 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" 2>&1 | tee -a "$logFile"; then
+ mv "$tmp_img" "$img"
+ log -ne "Encrypted: $img"
+ else
+ echo "Encryption failed for $img"
+ rm -f "$tmp_img"
+ exit 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"
+ fi
+ done
+ if [[ $failed -ne 0 ]]; then
+ echo "One or more backup files failed verification"
+ exit 1
+ fi
Review Comment:
`verify_backup` exits directly on failure, which skips `cleanup()`/unmount
in the calling backup paths and can leave the NAS store mounted and temp
directories behind. Return failure to the caller and perform cleanup/unmount
before exiting.
##########
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"
+ exit 1
+ fi
Review Comment:
`encrypt_backup` calls `exit 1` on missing/invalid passphrase file, which
bypasses `cleanup()`/unmount logic and can leave the NAS mount + temp dir
behind. Prefer returning a non-zero status and letting callers invoke
`cleanup()` (or add a trap-based cleanup) so failures don’t leak
mounts/directories.
##########
core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java:
##########
@@ -35,6 +37,7 @@ public class TakeBackupCommand extends Command {
private Boolean quiesce;
@LogLevel(LogLevel.Log4jLevel.Off)
private String mountOptions;
Review Comment:
`details` may carry sensitive values (e.g., an encryption passphrase).
CloudStack’s Gson logging uses `LoggingExclusionStrategy` with `@LogLevel` to
exclude fields, so leaving this unannotated can leak secrets in debug logs.
Annotate `details` with `@LogLevel(Off)` (or avoid putting secrets in this map).
```suggestion
private String mountOptions;
@LogLevel(LogLevel.Log4jLevel.Off)
```
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -205,6 +245,26 @@ 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
+ Long zoneId = vm.getDataCenterId();
+ if (Boolean.TRUE.equals(NASBackupCompressionEnabled.valueIn(zoneId))) {
+ command.addDetail("compression", "true");
+ }
+ if (Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) {
+ command.addDetail("encryption", "true");
+ String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId);
+ if (passphrase != null && !passphrase.isEmpty()) {
+ command.addDetail("encryption_passphrase", passphrase);
+ }
Review Comment:
When encryption is enabled at zone scope, the code sets the `encryption`
detail even if the passphrase is missing/empty, which will currently result in
an unencrypted backup on the agent side. Since the passphrase is required, fail
fast here with a clear error instead of proceeding (e.g., throw a
CloudRuntimeException / mark backup failed).
```suggestion
String passphrase =
NASBackupEncryptionPassphrase.valueIn(zoneId);
if (passphrase == null || passphrase.trim().isEmpty()) {
throw new CloudRuntimeException(String.format(
"NAS backup encryption is enabled for zone %d but no
encryption passphrase is configured",
zoneId));
}
command.addDetail("encryption", "true");
command.addDetail("encryption_passphrase", passphrase);
```
--
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]
