Skip to content

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.

Sign up for GitHub

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

Open
jmsperu wants to merge 1 commit into
base: 4.22
Choose a base branch
from
+245 −14
Open

NAS backup: compression, encryption, bandwidth throttle, integrity check #12898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -35,6 +37,7 @@ public class TakeBackupCommand extends Command {
private Boolean quiesce;
@LogLevel(LogLevel.Log4jLevel.Off)
private String mountOptions;
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
private String mountOptions;
private String mountOptions;
@LogLevel(LogLevel.Log4jLevel.Off)

Copilot uses AI. Check for mistakes.
private Map<String, String> details = new HashMap<>();

public TakeBackupCommand(String vmName, String backupPath) {
super();
Expand Down Expand Up @@ -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;
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
this.details = details;
this.details = details != null ? details : new HashMap<>();

Copilot uses AI. Check for mistakes.
}

public void addDetail(String key, String value) {
this.details.put(key, value);
}

@Override
public boolean executeInSequence() {
return true;
Expand Down
67 changes: 66 additions & 1 deletion plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
Comment on lines +254 to +258
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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 uses AI. Check for mistakes.
}
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");
}
Comment on lines +248 to +266
Copy link

Copilot AI Mar 30, 2026

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).

Copilot uses AI. Check for mistakes.

if (VirtualMachine.State.Stopped.equals(vm.getState())) {
List<VolumeVO> vmVolumes = volumeDao.findByInstance(vm.getId());
vmVolumes.sort(Comparator.comparing(Volume::getDeviceId));
Expand Down Expand Up @@ -594,7 +654,12 @@ public Boolean crossZoneInstanceCreationEnabled(BackupOffering backupOffering) {
@Override
public ConfigKey<?>[] getConfigKeys() {
return new ConfigKey[]{
NASBackupRestoreMountTimeout
NASBackupRestoreMountTimeout,
NASBackupCompressionEnabled,
NASBackupEncryptionEnabled,
NASBackupEncryptionPassphrase,
NASBackupBandwidthLimitMbps,
NASBackupIntegrityCheckEnabled
};
}

Expand Down
64 changes: 53 additions & 11 deletions .../main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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());
}
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
}
}
} 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");

Copilot uses AI. Check for mistakes.
Comment on lines +97 to +106
Copy link

Copilot AI Mar 30, 2026

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.

Copilot uses AI. Check for mistakes.
}
}
String bwLimit = details.get("bandwidth_limit");
if (bwLimit != null && !"0".equals(bwLimit)) {
cmdArgs.add("-b"); cmdArgs.add(bwLimit);
}
if ("true".equals(details.get("integrity_check"))) {
cmdArgs.add("--verify");
}
}

List<String[]> commands = new ArrayList<>();
commands.add(new String[]{
libvirtComputingResource.getNasBackupPath(),
"-o", "backup",
"-v", vmName,
"-t", backupRepoType,
"-s", backupRepoAddress,
"-m", Objects.nonNull(mountOptions) ? mountOptions : "",
"-p", backupPath,
"-q", command.getQuiesce() != null && command.getQuiesce() ? "true" : "false",
"-d", diskPaths.isEmpty() ? "" : String.join(",", diskPaths)
});
commands.add(cmdArgs.toArray(new String[0]));

Pair<Integer, String> result = Script.executePipedCommands(commands, libvirtComputingResource.getCmdsTimeout());

// Clean up passphrase file after backup completes
if (passphraseFile != null && passphraseFile.exists()) {
passphraseFile.delete();
}

if (result.first() != 0) {
logger.debug("Failed to take VM backup: " + result.second());
BackupAnswer answer = new BackupAnswer(command, false, result.second().trim());
Expand Down
113 changes: 111 additions & 2 deletions scripts/vm/hypervisor/kvm/nasbackup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
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
Comment on lines +134 to +137
Copy link

Copilot AI Mar 30, 2026

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.

Copilot uses AI. Check for mistakes.
}

### Operation methods ###

backup_running_vm() {
Expand Down Expand Up @@ -128,6 +178,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"
fi

# Backup domain information
virsh -c qemu:///system dumpxml $VM > $dest/domain-config.xml 2>/dev/null
virsh -c qemu:///system dominfo $VM > $dest/dominfo.xml 2>/dev/null
Expand All @@ -147,8 +205,32 @@ backup_running_vm() {
done

rm -f $dest/backup.xml

# Compress backup files if requested
if [[ "$COMPRESS" == "true" ]]; then
log -ne "Compressing backup files for $VM"
for img in "$dest"/*.qcow2; do
[[ -f "$img" ]] || continue
local tmp_img="${img}.tmp"
if qemu-img convert -c -O qcow2 "$img" "$tmp_img" 2>&1 | tee -a "$logFile"; then
mv "$tmp_img" "$img"
else
log -ne "Warning: compression failed for $img, keeping uncompressed"
rm -f "$tmp_img"
fi
done
fi

# Encrypt backup files if requested
encrypt_backup "$dest"

sync

# Verify backup integrity if requested
if [[ "$VERIFY" == "true" ]]; then
verify_backup "$dest"
fi

# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
du -sb $dest | cut -f1
Expand All @@ -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
Copy link

Copilot AI Mar 30, 2026

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.

Suggested change
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 uses AI. Check for mistakes.
echo "qemu-img convert failed for $disk $output"
cleanup
Copy link

Copilot AI Mar 30, 2026

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.

Suggested change
cleanup
cleanup
return 1

Copilot uses AI. Check for mistakes.
fi
name="datadisk"
done

# Encrypt backup files if requested
encrypt_backup "$dest"

sync

# Verify backup integrity if requested
if [[ "$VERIFY" == "true" ]]; then
verify_backup "$dest"
fi

ls -l --numeric-uid-gid $dest | awk '{print $5}'
}

Expand Down Expand Up @@ -233,7 +324,7 @@ cleanup() {

function usage {
echo ""
echo "Usage: $0 -o <operation> -v|--vm <domain name> -t <storage type> -s <storage address> -m <mount options> -p <backup path> -d <disks path> -q|--quiesce <true|false>"
echo "Usage: $0 -o <operation> -v|--vm <domain name> -t <storage type> -s <storage address> -m <mount options> -p <backup path> -d <disks path> -q|--quiesce <true|false> [-c] [-b <MiB/s>] [-e <passphrase file>] [--verify]"
echo ""
exit 1
}
Expand Down Expand Up @@ -280,6 +371,24 @@ while [[ $# -gt 0 ]]; do
shift
shift
;;
-c|--compress)
COMPRESS="true"
shift
;;
-b|--bandwidth)
BANDWIDTH="$2"
shift
shift
;;
-e|--encrypt)
ENCRYPT_PASSFILE="$2"
shift
shift
;;
--verify)
VERIFY="true"
shift
;;
-h|--help)
usage
shift
Expand Down
Loading