Skip to content

Commit 1d2eec1

Browse files
committed
NAS backup enhancements: compression, encryption, bandwidth throttle, integrity check
Rebased onto 4.22 as a single commit (the branch history had several merges of 4.22, which a commit-by-commit rebase cannot replay cleanly). Resolved against "Nasbackup quiesce fixes (#14305)": TakeBackupCommand and NASBackupProvider keep both the enhancement details and the new quiesce timeout and cleanup alert; nasbackup.sh keeps both option sets and takes the upstream 'exit 1' after a failed backup job; the PR's enhancement-flag tests move to LibvirtTakeBackupCommandWrapperEnhancementFlagsTest next to the upstream LibvirtTakeBackupCommandWrapperTest. Squashed from: - address review (#12898): restore fails closed when the encryption state cannot be read - address review (#12898): nasbackup.sh compression/encryption guard, rate-limit probe, honest throttle logging, accept leaked clusters - address review (#12898): validate enhancement settings before the backup row exists, reject compression with encryption - Restore wrapper tests: count only qemu-img calls - address review (#12898): restore LUKS-encrypted backups - address review (#12898): reorder java.io imports alphabetically; drop trailing '(Copilot review)' comment artifacts - refactor(backup): extract NAS backup enhancement logic into dedicated methods + tests - refactor(backup): consolidate detail keys in TakeBackupCommand - Fix build: use reflection for protected ConfigKey.valueOf() in test - Fix NASBackupProviderTest failures for zone-scoped ConfigKey values - Fix tee masking exit codes and missing return in nasbackup.sh - Address Copilot review feedback on NAS backup PR #12898 - NAS backup enhancements: compression, encryption, bandwidth throttle, integrity check Claude-Session: https://claude.ai/code/session_01BhiE1Abhs315m3n3UPy6V7
1 parent dfff32e commit 1d2eec1

10 files changed

Lines changed: 1170 additions & 36 deletions

File tree

‎core/src/main/java/org/apache/cloudstack/backup/RestoreBackupCommand.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,9 @@ public void setVmState(VirtualMachine.State vmState) {
143143

144144
@LogLevel(LogLevel.Log4jLevel.Off)
145145
private String mountOptions;
146+
/** LUKS passphrase for backups taken with nas.backup.encryption.enabled; null for plain backups. Never logged. */
147+
@LogLevel(LogLevel.Log4jLevel.Off)
148+
private String encryptionPassphrase;
146149
@Override
147150

148151
public boolean executeInSequence() {
@@ -153,6 +156,14 @@ public List<String> getBackupVolumesUUIDs() {
153156
return backupVolumesUUIDs;
154157
}
155158

159+
public String getEncryptionPassphrase() {
160+
return encryptionPassphrase;
161+
}
162+
163+
public void setEncryptionPassphrase(String encryptionPassphrase) {
164+
this.encryptionPassphrase = encryptionPassphrase;
165+
}
166+
156167
public void setBackupVolumesUUIDs(List<String> backupVolumesUUIDs) {
157168
this.backupVolumesUUIDs = backupVolumesUUIDs;
158169
}

‎core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,20 @@
2323
import com.cloud.agent.api.LogLevel;
2424
import org.apache.cloudstack.storage.to.PrimaryDataStoreTO;
2525

26+
import java.util.HashMap;
2627
import java.util.List;
28+
import java.util.Map;
2729

2830
public class TakeBackupCommand extends Command {
31+
// Detail-map keys shared between the management server (NASBackupProvider) and
32+
// the KVM agent wrapper. Defining them once here avoids drift between producer
33+
// and consumer when a key is renamed.
34+
public static final String DETAIL_COMPRESSION = "compression";
35+
public static final String DETAIL_ENCRYPTION = "encryption";
36+
public static final String DETAIL_ENCRYPTION_PASSPHRASE = "encryption_passphrase";
37+
public static final String DETAIL_BANDWIDTH_LIMIT = "bandwidth_limit";
38+
public static final String DETAIL_INTEGRITY_CHECK = "integrity_check";
39+
2940
private String vmName;
3041
private String backupPath;
3142
private String backupRepoType;
@@ -36,6 +47,8 @@ public class TakeBackupCommand extends Command {
3647
private Integer quiesceTimeout;
3748
@LogLevel(LogLevel.Log4jLevel.Off)
3849
private String mountOptions;
50+
@LogLevel(LogLevel.Log4jLevel.Off)
51+
private Map<String, String> details = new HashMap<>();
3952

4053
// Incremental backup fields (NAS provider; null/empty for legacy full-only callers).
4154
private String mode; // "full" or "incremental"; null => legacy behaviour (script default)
@@ -118,6 +131,18 @@ public void setQuiesce(Boolean quiesce) {
118131
this.quiesce = quiesce;
119132
}
120133

134+
public Map<String, String> getDetails() {
135+
return details;
136+
}
137+
138+
public void setDetails(Map<String, String> details) {
139+
this.details = details != null ? details : new HashMap<>();
140+
}
141+
142+
public void addDetail(String key, String value) {
143+
this.details.put(key, value);
144+
}
145+
121146
public Integer getQuiesceTimeout() {
122147
return quiesceTimeout;
123148
}

‎plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java‎

Lines changed: 110 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,15 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co
9595
true,
9696
BackupFrameworkEnabled.key());
9797

98+
ConfigKey<Boolean> NASBackupCompressionEnabled = new ConfigKey<>("Advanced", Boolean.class,
99+
"nas.backup.compression.enabled",
100+
"false",
101+
"Enable qcow2 compression for NAS backup files. Cannot be combined with nas.backup.encryption.enabled "
102+
+ "(qcow2 cannot compress and encrypt the same image); backups fail with a clear error if both are on.",
103+
true,
104+
ConfigKey.Scope.Zone,
105+
BackupFrameworkEnabled.key());
106+
98107
ConfigKey<Boolean> NASBackupIncrementalEnabled = new ConfigKey<>("Advanced", Boolean.class,
99108
"nas.backup.incremental.enabled",
100109
"false",
@@ -108,6 +117,39 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co
108117
ConfigKey.Scope.Zone,
109118
BackupFrameworkEnabled.key());
110119

120+
ConfigKey<Boolean> NASBackupEncryptionEnabled = new ConfigKey<>("Advanced", Boolean.class,
121+
"nas.backup.encryption.enabled",
122+
"false",
123+
"Enable LUKS encryption for NAS backup files. Cannot be combined with nas.backup.compression.enabled.",
124+
true,
125+
ConfigKey.Scope.Zone,
126+
BackupFrameworkEnabled.key());
127+
128+
ConfigKey<String> NASBackupEncryptionPassphrase = new ConfigKey<>("Secure", String.class,
129+
"nas.backup.encryption.passphrase",
130+
"",
131+
"Passphrase for LUKS encryption of NAS backup files. Required when encryption is enabled. Restores use the "
132+
+ "current value, so changing it makes backups taken with the old passphrase unrestorable until it is set back.",
133+
true,
134+
ConfigKey.Scope.Zone,
135+
BackupFrameworkEnabled.key());
136+
137+
ConfigKey<Integer> NASBackupBandwidthLimitMbps = new ConfigKey<>("Advanced", Integer.class,
138+
"nas.backup.bandwidth.limit.mbps",
139+
"0",
140+
"Bandwidth limit in MiB/s for backup operations (0 = unlimited).",
141+
true,
142+
ConfigKey.Scope.Zone,
143+
BackupFrameworkEnabled.key());
144+
145+
ConfigKey<Boolean> NASBackupIntegrityCheckEnabled = new ConfigKey<>("Advanced", Boolean.class,
146+
"nas.backup.integrity.check",
147+
"false",
148+
"Run qemu-img check on backup files after creation to verify integrity.",
149+
true,
150+
ConfigKey.Scope.Zone,
151+
BackupFrameworkEnabled.key());
152+
111153
ConfigKey<Integer> NASBackupFullEvery = new ConfigKey<>("Advanced", Integer.class,
112154
"nas.backup.full.every",
113155
"10",
@@ -298,6 +340,16 @@ protected ChainDecision decideChain(VirtualMachine vm) {
298340
return ChainDecision.legacyFull();
299341
}
300342

343+
// Compression and LUKS encryption rewrite each backup file with qemu-img convert. On an
344+
// incremental file that either flattens it through its backing chain (a full-size copy that
345+
// defeats the chain) or leaves an encrypted child whose parent needs its own secret to open.
346+
// Until both features are chain-aware, a zone with either one enabled takes full backups.
347+
Long zoneId = vm.getDataCenterId();
348+
if (Boolean.TRUE.equals(NASBackupCompressionEnabled.valueIn(zoneId))
349+
|| Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) {
350+
return ChainDecision.legacyFull();
351+
}
352+
301353
// Incremental backups rely on QEMU dirty bitmaps / libvirt checkpoints, which only exist
302354
// on file-based qcow2 storage. Storage such as Ceph-RBD and Linstor cannot carry per-disk
303355
// checkpoints, so a VM with any volume on such a pool must stay on the full-only (legacy)
@@ -585,8 +637,11 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
585637
// (libvirt backup-begin requires a running QEMU process).
586638
ChainDecision decision = decideChain(vm);
587639

588-
BackupVO backupVO = createBackupObject(vm, backupPath, decision.isIncremental() ? "INCREMENTAL" : "FULL");
589640
TakeBackupCommand command = new TakeBackupCommand(vm.getInstanceName(), backupPath);
641+
// Validate and apply the zone's enhancement settings before any backup row exists, so a
642+
// misconfiguration (encryption without a passphrase, compression together with encryption)
643+
// fails the request without leaving a backup stuck in BackingUp.
644+
applyBackupEnhancementDetails(command, vm.getDataCenterId());
590645
command.setBackupRepoType(backupRepository.getType());
591646
command.setBackupRepoAddress(backupRepository.getAddress());
592647
command.setMountOptions(backupRepository.getMountOptions());
@@ -605,6 +660,7 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
605660
command.setVolumePaths(volumePoolsAndPaths.second());
606661
}
607662

663+
BackupVO backupVO = createBackupObject(vm, backupPath, decision.isIncremental() ? "INCREMENTAL" : "FULL");
608664
BackupAnswer answer;
609665
try {
610666
answer = (BackupAnswer) agentManager.send(host.getId(), command);
@@ -669,6 +725,39 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
669725
}
670726
}
671727

728+
/**
729+
* Translates the zone-scoped backup-enhancement settings (compression, encryption,
730+
* bandwidth limit, integrity check) into details on the {@link TakeBackupCommand}.
731+
* Fails fast, before any backup row is created, if encryption is enabled without a configured
732+
* passphrase or together with compression (qcow2 cannot compress and encrypt the same image).
733+
*/
734+
protected void applyBackupEnhancementDetails(TakeBackupCommand command, Long zoneId) {
735+
if (Boolean.TRUE.equals(NASBackupCompressionEnabled.valueIn(zoneId))
736+
&& Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) {
737+
throw new CloudRuntimeException(String.format("%s and %s are both enabled for zone %d, but qcow2 cannot compress and "
738+
+ "encrypt the same image. Disable one of them for this zone; no backup was taken.",
739+
NASBackupCompressionEnabled.key(), NASBackupEncryptionEnabled.key(), zoneId));
740+
}
741+
if (Boolean.TRUE.equals(NASBackupCompressionEnabled.valueIn(zoneId))) {
742+
command.addDetail(TakeBackupCommand.DETAIL_COMPRESSION, "true");
743+
}
744+
if (Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) {
745+
String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId);
746+
if (passphrase == null || passphrase.isEmpty()) {
747+
throw new CloudRuntimeException("NAS backup encryption is enabled but no passphrase is configured (nas.backup.encryption.passphrase)");
748+
}
749+
command.addDetail(TakeBackupCommand.DETAIL_ENCRYPTION, "true");
750+
command.addDetail(TakeBackupCommand.DETAIL_ENCRYPTION_PASSPHRASE, passphrase);
751+
}
752+
Integer bandwidthLimit = NASBackupBandwidthLimitMbps.valueIn(zoneId);
753+
if (bandwidthLimit != null && bandwidthLimit > 0) {
754+
command.addDetail(TakeBackupCommand.DETAIL_BANDWIDTH_LIMIT, String.valueOf(bandwidthLimit));
755+
}
756+
if (Boolean.TRUE.equals(NASBackupIntegrityCheckEnabled.valueIn(zoneId))) {
757+
command.addDetail(TakeBackupCommand.DETAIL_INTEGRITY_CHECK, "true");
758+
}
759+
}
760+
672761
/**
673762
* Alerts when nasbackup.sh could not clean up after a failed backup. The script's output
674763
* names the backup repository mount point it left on the host.
@@ -706,6 +795,19 @@ private BackupVO createBackupObject(VirtualMachine vm, String backupPath, String
706795
return backupDao.persist(backup);
707796
}
708797

798+
/**
799+
* Restore-side counterpart of {@link #applyBackupEnhancementDetails}: hands the zone's LUKS passphrase
800+
* to the host so encrypted backup files can be checked and converted back. It is sent whenever a
801+
* passphrase is configured, not only while encryption is switched on, so backups taken before
802+
* encryption was disabled stay restorable; the host ignores it for plain backups.
803+
*/
804+
protected void applyRestoreEncryptionDetails(RestoreBackupCommand command, Long zoneId) {
805+
String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId);
806+
if (passphrase != null && !passphrase.isEmpty()) {
807+
command.setEncryptionPassphrase(passphrase);
808+
}
809+
}
810+
709811
@Override
710812
public Pair<Boolean, String> restoreBackupToVM(VirtualMachine vm, Backup backup, String hostIp, String dataStoreUuid) {
711813
return restoreVMBackup(vm, backup);
@@ -745,6 +847,7 @@ private Pair<Boolean, String> restoreVMBackup(VirtualMachine vm, Backup backup)
745847
restoreCommand.setVmExists(vm.getRemoved() == null);
746848
restoreCommand.setVmState(vm.getState());
747849
restoreCommand.setMountTimeout(NASBackupRestoreMountTimeout.value());
850+
applyRestoreEncryptionDetails(restoreCommand, vm.getDataCenterId());
748851

749852
BackupAnswer answer;
750853
try {
@@ -870,6 +973,7 @@ public Pair<Boolean, String> restoreBackedUpVolume(Backup backup, Backup.VolumeI
870973
restoreCommand.setVmState(vmNameAndState.second());
871974
restoreCommand.setMountTimeout(NASBackupRestoreMountTimeout.value());
872975
restoreCommand.setBackupFiles(Collections.singletonList(matchingVolume.getPath()));
976+
applyRestoreEncryptionDetails(restoreCommand, backup.getZoneId());
873977

874978
BackupAnswer answer;
875979
try {
@@ -1281,6 +1385,11 @@ public Boolean crossZoneInstanceCreationEnabled(BackupOffering backupOffering) {
12811385
public ConfigKey<?>[] getConfigKeys() {
12821386
return new ConfigKey[]{
12831387
NASBackupRestoreMountTimeout,
1388+
NASBackupCompressionEnabled,
1389+
NASBackupEncryptionEnabled,
1390+
NASBackupEncryptionPassphrase,
1391+
NASBackupBandwidthLimitMbps,
1392+
NASBackupIntegrityCheckEnabled,
12841393
NASBackupFullEvery,
12851394
NASBackupIncrementalEnabled,
12861395
NASBackupQuiesceAgentTimeout

0 commit comments

Comments
 (0)