-
Notifications
You must be signed in to change notification settings - Fork 1.3k
NAS backup: compression, encryption, bandwidth throttle, integrity check #12898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.22
Are you sure you want to change the base?
Changes from 1 commit
2c40fdd
05076db
4884980
3d65f34
c11e0b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,9 @@ | |||||
| import com.cloud.agent.api.LogLevel; | ||||||
| import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; | ||||||
|
|
||||||
| import java.util.HashMap; | ||||||
| import java.util.List; | ||||||
| import java.util.Map; | ||||||
|
|
||||||
| public class TakeBackupCommand extends Command { | ||||||
| private String vmName; | ||||||
|
|
@@ -35,6 +37,7 @@ public class TakeBackupCommand extends Command { | |||||
| private Boolean quiesce; | ||||||
| @LogLevel(LogLevel.Log4jLevel.Off) | ||||||
| private String mountOptions; | ||||||
| private Map<String, String> details = new HashMap<>(); | ||||||
|
|
||||||
| public TakeBackupCommand(String vmName, String backupPath) { | ||||||
| super(); | ||||||
|
|
@@ -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; | ||||||
|
||||||
| this.details = details; | |
| this.details = details != null ? details : new HashMap<>(); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -84,6 +84,46 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co | |||||||||||||||||||||||||||
| true, | ||||||||||||||||||||||||||||
| BackupFrameworkEnabled.key()); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ConfigKey<Boolean> NASBackupCompressionEnabled = new ConfigKey<>("Advanced", Boolean.class, | ||||||||||||||||||||||||||||
| "nas.backup.compression.enabled", | ||||||||||||||||||||||||||||
| "false", | ||||||||||||||||||||||||||||
| "Enable qcow2 compression for NAS backup files.", | ||||||||||||||||||||||||||||
| true, | ||||||||||||||||||||||||||||
| ConfigKey.Scope.Zone, | ||||||||||||||||||||||||||||
| BackupFrameworkEnabled.key()); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ConfigKey<Boolean> NASBackupEncryptionEnabled = new ConfigKey<>("Advanced", Boolean.class, | ||||||||||||||||||||||||||||
| "nas.backup.encryption.enabled", | ||||||||||||||||||||||||||||
| "false", | ||||||||||||||||||||||||||||
| "Enable LUKS encryption for NAS backup files.", | ||||||||||||||||||||||||||||
| true, | ||||||||||||||||||||||||||||
| ConfigKey.Scope.Zone, | ||||||||||||||||||||||||||||
| BackupFrameworkEnabled.key()); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ConfigKey<String> NASBackupEncryptionPassphrase = new ConfigKey<>("Secure", String.class, | ||||||||||||||||||||||||||||
| "nas.backup.encryption.passphrase", | ||||||||||||||||||||||||||||
| "", | ||||||||||||||||||||||||||||
| "Passphrase for LUKS encryption of NAS backup files. Required when encryption is enabled.", | ||||||||||||||||||||||||||||
| true, | ||||||||||||||||||||||||||||
| ConfigKey.Scope.Zone, | ||||||||||||||||||||||||||||
| BackupFrameworkEnabled.key()); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||||
| ConfigKey.Scope.Zone, | ||||||||||||||||||||||||||||
| BackupFrameworkEnabled.key()); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ConfigKey<Boolean> NASBackupIntegrityCheckEnabled = new ConfigKey<>("Advanced", Boolean.class, | ||||||||||||||||||||||||||||
| "nas.backup.integrity.check", | ||||||||||||||||||||||||||||
| "false", | ||||||||||||||||||||||||||||
| "Run qemu-img check on backup files after creation to verify integrity.", | ||||||||||||||||||||||||||||
| true, | ||||||||||||||||||||||||||||
| ConfigKey.Scope.Zone, | ||||||||||||||||||||||||||||
| BackupFrameworkEnabled.key()); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Inject | ||||||||||||||||||||||||||||
| private BackupDao backupDao; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -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); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| command.addDetail("encryption", "true"); | |
| String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId); | |
| if (passphrase != null && !passphrase.isEmpty()) { | |
| command.addDetail("encryption_passphrase", passphrase); | |
| } | |
| 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); |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,9 +34,13 @@ | |||||||||||
| import org.apache.cloudstack.backup.TakeBackupCommand; | ||||||||||||
| import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; | ||||||||||||
|
|
||||||||||||
| import java.io.File; | ||||||||||||
| import java.io.FileWriter; | ||||||||||||
| import java.io.IOException; | ||||||||||||
| import java.util.ArrayList; | ||||||||||||
| import java.util.Arrays; | ||||||||||||
| import java.util.List; | ||||||||||||
| import java.util.Map; | ||||||||||||
| import java.util.Objects; | ||||||||||||
|
|
||||||||||||
| @ResourceWrapper(handles = TakeBackupCommand.class) | ||||||||||||
|
|
@@ -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"); | ||||||||||||
|
Comment on lines
+92
to
+100
|
||||||||||||
| 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()); | ||||||||||||
| } | ||||||||||||
|
||||||||||||
| } | |
| } | |
| } 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"); |
Outdated
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,10 @@ MOUNT_OPTS="" | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BACKUP_DIR="" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DISK_PATHS="" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| QUIESCE="" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| COMPRESS="" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BANDWIDTH="" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ENCRYPT_PASSFILE="" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VERIFY="" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logFile="/var/log/cloudstack/agent/agent.log" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| EXIT_CLEANUP_FAILED=20 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+99
to
+102
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+126
to
+131
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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" | |
| 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 |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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 | |
| 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 |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| cleanup | |
| cleanup | |
| return 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detailsmay carry sensitive values (e.g., an encryption passphrase). CloudStack’s Gson logging usesLoggingExclusionStrategywith@LogLevelto exclude fields, so leaving this unannotated can leak secrets in debug logs. Annotatedetailswith@LogLevel(Off)(or avoid putting secrets in this map).