From 2c40fddcd21b41d978ca862ee180b05212fa7230 Mon Sep 17 00:00:00 2001 From: jmsperu Date: Fri, 27 Mar 2026 01:13:48 +0300 Subject: [PATCH 1/6] NAS backup enhancements: compression, encryption, bandwidth throttle, integrity check Adds four optional features to NAS backup operations, configurable at zone scope via CloudStack global settings: - Compression (-c): qcow2 internal compression of backup files Config: nas.backup.compression.enabled (default: false) - LUKS Encryption (-e): encrypt backup files at rest using qemu-img Config: nas.backup.encryption.enabled (default: false) Config: nas.backup.encryption.passphrase (Secure category) - Bandwidth Throttle (-b): limit backup I/O bandwidth via virsh blockjob for running VMs or qemu-img -r for stopped VMs Config: nas.backup.bandwidth.limit.mbps (default: 0/unlimited) - Integrity Check (--verify): qemu-img check after backup creation Config: nas.backup.integrity.check (default: false) All features are disabled by default and fully backward compatible. Settings are read from zone-scoped ConfigKeys in NASBackupProvider, passed to the KVM agent via TakeBackupCommand details map, and translated to nasbackup.sh CLI flags in LibvirtTakeBackupCommandWrapper. Changes: - nasbackup.sh: add -c, -b, -e, --verify flags with encrypt_backup() and verify_backup() helper functions - TakeBackupCommand.java: add details map for passing config to agent - NASBackupProvider.java: add 5 ConfigKeys, populate command details - LibvirtTakeBackupCommandWrapper.java: extract details, build CLI args, handle passphrase temp file lifecycle Combines and supersedes PRs #12844, #12846, #12848, #12845 --- .../cloudstack/backup/TakeBackupCommand.java | 15 +++ .../cloudstack/backup/NASBackupProvider.java | 67 ++++++++++- .../LibvirtTakeBackupCommandWrapper.java | 64 ++++++++-- scripts/vm/hypervisor/kvm/nasbackup.sh | 113 +++++++++++++++++- 4 files changed, 245 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java index 5402b6b24760..ee97a6c63c67 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java @@ -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 details = new HashMap<>(); public TakeBackupCommand(String vmName, String backupPath) { super(); @@ -106,6 +109,18 @@ public void setQuiesce(Boolean quiesce) { this.quiesce = quiesce; } + public Map getDetails() { + return details; + } + + public void setDetails(Map details) { + this.details = details; + } + + public void addDetail(String key, String value) { + this.details.put(key, value); + } + @Override public boolean executeInSequence() { return true; diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index f2ea8ac71c91..ae436225857e 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -84,6 +84,46 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co true, BackupFrameworkEnabled.key()); + ConfigKey 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 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 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 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 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 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"); + } + if (VirtualMachine.State.Stopped.equals(vm.getState())) { List vmVolumes = volumeDao.findByInstance(vm.getId()); vmVolumes.sort(Comparator.comparing(Volume::getDeviceId)); @@ -594,7 +654,12 @@ public Boolean crossZoneInstanceCreationEnabled(BackupOffering backupOffering) { @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[]{ - NASBackupRestoreMountTimeout + NASBackupRestoreMountTimeout, + NASBackupCompressionEnabled, + NASBackupEncryptionEnabled, + NASBackupEncryptionPassphrase, + NASBackupBandwidthLimitMbps, + NASBackupIntegrityCheckEnabled }; } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java index 11fa605908a6..7ff5ff7a1843 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java @@ -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 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 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()); + } + } + } + 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 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 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()); diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index e298006f7a8c..aa54a2ad7cff 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -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 + 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 +} + ### Operation methods ### backup_running_vm() { @@ -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 @@ -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 @@ -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 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}' } @@ -233,7 +324,7 @@ cleanup() { function usage { echo "" - echo "Usage: $0 -o -v|--vm -t -s -m -p -d -q|--quiesce " + echo "Usage: $0 -o -v|--vm -t -s -m -p -d -q|--quiesce [-c] [-b ] [-e ] [--verify]" echo "" exit 1 } @@ -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 From 05076dbb6809a6d0ce48b4faef9296a45cd13cc3 Mon Sep 17 00:00:00 2001 From: jmsperu Date: Mon, 30 Mar 2026 18:06:08 +0300 Subject: [PATCH 2/6] Address Copilot review feedback on NAS backup PR #12898 - nasbackup.sh: Replace exit 1 with return 1 in encrypt_backup and verify_backup so callers can run cleanup before terminating - nasbackup.sh: Append (>>) instead of truncate (>) agent.log in qemu-img convert for stopped VM backups - nasbackup.sh: Add return 1 after cleanup on qemu-img convert failure to stop execution - nasbackup.sh: Callers of encrypt_backup/verify_backup now check return code and run cleanup on failure - LibvirtTakeBackupCommandWrapper: Fail with error when encryption is enabled but passphrase is missing instead of silently skipping - LibvirtTakeBackupCommandWrapper: Delete temp passphrase file in finally block, set 0600 permissions, use explicit UTF-8 charset - NASBackupProvider: Throw CloudRuntimeException when encryption is enabled but passphrase is null/empty - NASBackupProviderTest: Add tests for compression, bandwidth, integrity check, encryption+passphrase, and encryption-without- passphrase failure scenarios - TakeBackupCommand: Add @LogLevel(Off) to details field to prevent passphrase leaking in debug logs - TakeBackupCommand: Normalize null to empty HashMap in setDetails --- .../cloudstack/backup/TakeBackupCommand.java | 3 +- .../cloudstack/backup/NASBackupProvider.java | 7 +- .../backup/NASBackupProviderTest.java | 204 ++++++++++++++++++ .../LibvirtTakeBackupCommandWrapper.java | 49 +++-- scripts/vm/hypervisor/kvm/nasbackup.sh | 29 ++- 5 files changed, 263 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java index ee97a6c63c67..ce0de62149a9 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java @@ -37,6 +37,7 @@ public class TakeBackupCommand extends Command { private Boolean quiesce; @LogLevel(LogLevel.Log4jLevel.Off) private String mountOptions; + @LogLevel(LogLevel.Log4jLevel.Off) private Map details = new HashMap<>(); public TakeBackupCommand(String vmName, String backupPath) { @@ -114,7 +115,7 @@ public Map getDetails() { } public void setDetails(Map details) { - this.details = details; + this.details = details != null ? details : new HashMap<>(); } public void addDetail(String key, String value) { diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index ae436225857e..2e1d7f77a9ed 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -251,11 +251,12 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce 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); + if (passphrase == null || passphrase.isEmpty()) { + throw new CloudRuntimeException("NAS backup encryption is enabled but no passphrase is configured (nas.backup.encryption.passphrase)"); } + command.addDetail("encryption", "true"); + command.addDetail("encryption_passphrase", passphrase); } Integer bandwidthLimit = NASBackupBandwidthLimitMbps.valueIn(zoneId); if (bandwidthLimit != null && bandwidthLimit > 0) { diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java index 7540cabbbf52..48a4b5e8562c 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java @@ -19,14 +19,18 @@ import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.mock; +import java.lang.reflect.Field; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; +import org.apache.cloudstack.framework.config.ConfigKey; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; @@ -47,6 +51,7 @@ import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.VolumeDao; import com.cloud.utils.Pair; +import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.dao.VMInstanceDao; @@ -349,4 +354,203 @@ public void testGetVMHypervisorHostFallbackToZoneWideKVMHost() { Mockito.verify(hostDao).findHypervisorHostInCluster(clusterId); Mockito.verify(resourceManager).findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM, zoneId); } + + private void overrideConfigValue(final ConfigKey configKey, final Object value) { + try { + Field f = ConfigKey.class.getDeclaredField("_value"); + f.setAccessible(true); + f.set(configKey, value); + } catch (IllegalAccessException | NoSuchFieldException e) { + Assert.fail(e.getMessage()); + } + } + + private VMInstanceVO setupVmForTakeBackup(Long vmId, Long hostId, Long backupOfferingId, + Long accountId, Long domainId, Long zoneId) { + VMInstanceVO vm = mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(vmId); + Mockito.when(vm.getHostId()).thenReturn(hostId); + Mockito.when(vm.getInstanceName()).thenReturn("test-vm"); + Mockito.when(vm.getBackupOfferingId()).thenReturn(backupOfferingId); + Mockito.when(vm.getAccountId()).thenReturn(accountId); + Mockito.when(vm.getDomainId()).thenReturn(domainId); + Mockito.when(vm.getDataCenterId()).thenReturn(zoneId); + Mockito.when(vm.getState()).thenReturn(VMInstanceVO.State.Running); + return vm; + } + + private void setupHostAndRepo(Long hostId, Long backupOfferingId) { + BackupRepository backupRepository = mock(BackupRepository.class); + Mockito.when(backupRepository.getType()).thenReturn("nfs"); + Mockito.when(backupRepository.getAddress()).thenReturn("address"); + Mockito.when(backupRepository.getMountOptions()).thenReturn("sync"); + Mockito.when(backupRepositoryDao.findByBackupOfferingId(backupOfferingId)).thenReturn(backupRepository); + + HostVO host = mock(HostVO.class); + Mockito.when(host.getId()).thenReturn(hostId); + Mockito.when(host.getStatus()).thenReturn(Status.Up); + Mockito.when(host.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.KVM); + Mockito.when(hostDao.findById(hostId)).thenReturn(host); + } + + @Test + public void testTakeBackupDetailsCompressionEnabled() throws AgentUnavailableException, OperationTimedoutException { + Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L; + Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L; + + VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId); + setupHostAndRepo(hostId, backupOfferingId); + + VolumeVO volume = mock(VolumeVO.class); + Mockito.when(volume.getState()).thenReturn(Volume.State.Ready); + Mockito.when(volume.getSize()).thenReturn(100L); + Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume)); + + overrideConfigValue(nasBackupProvider.NASBackupCompressionEnabled, "true"); + + BackupAnswer answer = mock(BackupAnswer.class); + Mockito.when(answer.getResult()).thenReturn(true); + Mockito.when(answer.getSize()).thenReturn(100L); + + ArgumentCaptor cmdCaptor = ArgumentCaptor.forClass(TakeBackupCommand.class); + Mockito.when(agentManager.send(anyLong(), cmdCaptor.capture())).thenReturn(answer); + Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0)); + Mockito.when(backupDao.update(Mockito.anyLong(), Mockito.any(BackupVO.class))).thenReturn(true); + + nasBackupProvider.takeBackup(vm, false); + + TakeBackupCommand capturedCmd = cmdCaptor.getValue(); + Map details = capturedCmd.getDetails(); + Assert.assertEquals("true", details.get("compression")); + + // Reset config + overrideConfigValue(nasBackupProvider.NASBackupCompressionEnabled, "false"); + } + + @Test + public void testTakeBackupDetailsBandwidthLimit() throws AgentUnavailableException, OperationTimedoutException { + Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L; + Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L; + + VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId); + setupHostAndRepo(hostId, backupOfferingId); + + VolumeVO volume = mock(VolumeVO.class); + Mockito.when(volume.getState()).thenReturn(Volume.State.Ready); + Mockito.when(volume.getSize()).thenReturn(100L); + Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume)); + + overrideConfigValue(nasBackupProvider.NASBackupBandwidthLimitMbps, "50"); + + BackupAnswer answer = mock(BackupAnswer.class); + Mockito.when(answer.getResult()).thenReturn(true); + Mockito.when(answer.getSize()).thenReturn(100L); + + ArgumentCaptor cmdCaptor = ArgumentCaptor.forClass(TakeBackupCommand.class); + Mockito.when(agentManager.send(anyLong(), cmdCaptor.capture())).thenReturn(answer); + Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0)); + Mockito.when(backupDao.update(Mockito.anyLong(), Mockito.any(BackupVO.class))).thenReturn(true); + + nasBackupProvider.takeBackup(vm, false); + + TakeBackupCommand capturedCmd = cmdCaptor.getValue(); + Map details = capturedCmd.getDetails(); + Assert.assertEquals("50", details.get("bandwidth_limit")); + + overrideConfigValue(nasBackupProvider.NASBackupBandwidthLimitMbps, "0"); + } + + @Test + public void testTakeBackupDetailsIntegrityCheck() throws AgentUnavailableException, OperationTimedoutException { + Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L; + Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L; + + VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId); + setupHostAndRepo(hostId, backupOfferingId); + + VolumeVO volume = mock(VolumeVO.class); + Mockito.when(volume.getState()).thenReturn(Volume.State.Ready); + Mockito.when(volume.getSize()).thenReturn(100L); + Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume)); + + overrideConfigValue(nasBackupProvider.NASBackupIntegrityCheckEnabled, "true"); + + BackupAnswer answer = mock(BackupAnswer.class); + Mockito.when(answer.getResult()).thenReturn(true); + Mockito.when(answer.getSize()).thenReturn(100L); + + ArgumentCaptor cmdCaptor = ArgumentCaptor.forClass(TakeBackupCommand.class); + Mockito.when(agentManager.send(anyLong(), cmdCaptor.capture())).thenReturn(answer); + Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0)); + Mockito.when(backupDao.update(Mockito.anyLong(), Mockito.any(BackupVO.class))).thenReturn(true); + + nasBackupProvider.takeBackup(vm, false); + + TakeBackupCommand capturedCmd = cmdCaptor.getValue(); + Map details = capturedCmd.getDetails(); + Assert.assertEquals("true", details.get("integrity_check")); + + overrideConfigValue(nasBackupProvider.NASBackupIntegrityCheckEnabled, "false"); + } + + @Test + public void testTakeBackupDetailsEncryptionWithPassphrase() throws AgentUnavailableException, OperationTimedoutException { + Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L; + Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L; + + VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId); + setupHostAndRepo(hostId, backupOfferingId); + + VolumeVO volume = mock(VolumeVO.class); + Mockito.when(volume.getState()).thenReturn(Volume.State.Ready); + Mockito.when(volume.getSize()).thenReturn(100L); + Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume)); + + overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "true"); + overrideConfigValue(nasBackupProvider.NASBackupEncryptionPassphrase, "my-secret-passphrase"); + + BackupAnswer answer = mock(BackupAnswer.class); + Mockito.when(answer.getResult()).thenReturn(true); + Mockito.when(answer.getSize()).thenReturn(100L); + + ArgumentCaptor cmdCaptor = ArgumentCaptor.forClass(TakeBackupCommand.class); + Mockito.when(agentManager.send(anyLong(), cmdCaptor.capture())).thenReturn(answer); + Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0)); + Mockito.when(backupDao.update(Mockito.anyLong(), Mockito.any(BackupVO.class))).thenReturn(true); + + nasBackupProvider.takeBackup(vm, false); + + TakeBackupCommand capturedCmd = cmdCaptor.getValue(); + Map details = capturedCmd.getDetails(); + Assert.assertEquals("true", details.get("encryption")); + Assert.assertEquals("my-secret-passphrase", details.get("encryption_passphrase")); + + overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "false"); + overrideConfigValue(nasBackupProvider.NASBackupEncryptionPassphrase, ""); + } + + @Test(expected = CloudRuntimeException.class) + public void testTakeBackupEncryptionWithoutPassphraseThrows() throws AgentUnavailableException, OperationTimedoutException { + Long vmId = 1L; Long hostId = 2L; Long backupOfferingId = 3L; + Long accountId = 4L; Long domainId = 5L; Long zoneId = 6L; + + VMInstanceVO vm = setupVmForTakeBackup(vmId, hostId, backupOfferingId, accountId, domainId, zoneId); + setupHostAndRepo(hostId, backupOfferingId); + + VolumeVO volume = mock(VolumeVO.class); + Mockito.when(volume.getState()).thenReturn(Volume.State.Ready); + Mockito.when(volume.getSize()).thenReturn(100L); + Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume)); + + overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "true"); + overrideConfigValue(nasBackupProvider.NASBackupEncryptionPassphrase, ""); + + Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation -> invocation.getArgument(0)); + + try { + nasBackupProvider.takeBackup(vm, false); + } finally { + overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "false"); + } + } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java index 7ff5ff7a1843..c3f94dcee1a7 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java @@ -35,10 +35,16 @@ import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; import java.io.File; -import java.io.FileWriter; import java.io.IOException; +import java.io.OutputStreamWriter; +import java.io.FileOutputStream; +import java.io.Writer; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.attribute.PosixFilePermission; import java.util.ArrayList; import java.util.Arrays; +import java.util.EnumSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -92,18 +98,24 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir } 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()); + if (passphrase == null || passphrase.isEmpty()) { + return new BackupAnswer(command, false, "Encryption is enabled but no passphrase was provided"); + } + try { + passphraseFile = File.createTempFile("cs-backup-enc-", ".key"); + passphraseFile.deleteOnExit(); + Files.setPosixFilePermissions(passphraseFile.toPath(), + EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)); + try (Writer fw = new OutputStreamWriter(new FileOutputStream(passphraseFile), StandardCharsets.UTF_8)) { + fw.write(passphrase); + } + cmdArgs.add("-e"); cmdArgs.add(passphraseFile.getAbsolutePath()); + } catch (IOException e) { + logger.error("Failed to create encryption passphrase file", e); + if (passphraseFile != null && passphraseFile.exists()) { + passphraseFile.delete(); } + return new BackupAnswer(command, false, "Failed to create encryption passphrase file: " + e.getMessage()); } } String bwLimit = details.get("bandwidth_limit"); @@ -118,11 +130,14 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir List commands = new ArrayList<>(); commands.add(cmdArgs.toArray(new String[0])); - Pair result = Script.executePipedCommands(commands, libvirtComputingResource.getCmdsTimeout()); - - // Clean up passphrase file after backup completes - if (passphraseFile != null && passphraseFile.exists()) { - passphraseFile.delete(); + Pair result; + try { + result = Script.executePipedCommands(commands, libvirtComputingResource.getCmdsTimeout()); + } finally { + // Clean up passphrase file after backup completes + if (passphraseFile != null && passphraseFile.exists()) { + passphraseFile.delete(); + } } if (result.first() != 0) { diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index aa54a2ad7cff..77c8374f1aef 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -98,7 +98,7 @@ encrypt_backup() { fi if [[ ! -f "$ENCRYPT_PASSFILE" ]]; then echo "Encryption passphrase file not found: $ENCRYPT_PASSFILE" - exit 1 + return 1 fi log -ne "Encrypting backup files with LUKS" for img in "$backup_dir"/*.qcow2; do @@ -113,7 +113,7 @@ encrypt_backup() { else echo "Encryption failed for $img" rm -f "$tmp_img" - exit 1 + return 1 fi done } @@ -133,7 +133,7 @@ verify_backup() { done if [[ $failed -ne 0 ]]; then echo "One or more backup files failed verification" - exit 1 + return 1 fi } @@ -222,13 +222,19 @@ backup_running_vm() { fi # Encrypt backup files if requested - encrypt_backup "$dest" + if ! encrypt_backup "$dest"; then + cleanup + return 1 + fi sync # Verify backup integrity if requested if [[ "$VERIFY" == "true" ]]; then - verify_backup "$dest" + if ! verify_backup "$dest"; then + cleanup + return 1 + fi fi # Print statistics @@ -256,21 +262,28 @@ backup_stopped_vm() { volUuid="${disk##*/}" fi output="$dest/$name.$volUuid.qcow2" - 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 echo "qemu-img convert failed for $disk $output" cleanup + return 1 fi name="datadisk" done # Encrypt backup files if requested - encrypt_backup "$dest" + if ! encrypt_backup "$dest"; then + cleanup + return 1 + fi sync # Verify backup integrity if requested if [[ "$VERIFY" == "true" ]]; then - verify_backup "$dest" + if ! verify_backup "$dest"; then + cleanup + return 1 + fi fi ls -l --numeric-uid-gid $dest | awk '{print $5}' From 488498057ad93f567eab193d392ab034991e15e9 Mon Sep 17 00:00:00 2001 From: jmsperu Date: Thu, 2 Apr 2026 00:45:32 +0300 Subject: [PATCH 3/6] Fix tee masking exit codes and missing return in nasbackup.sh Address remaining Copilot review feedback on PR #12898: - Replace `2>&1 | tee -a` with `>> logFile 2>&1` in encrypt_backup, compress, and mount_operation to prevent tee from masking non-zero exit codes of qemu-img and mount commands - Add `return 1` after cleanup on virsh backup job failure to prevent continuing execution with broken state --- scripts/vm/hypervisor/kvm/nasbackup.sh | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index 77c8374f1aef..9e4b4d30e430 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -107,7 +107,7 @@ encrypt_backup() { 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 + "$img" "$tmp_img" >> "$logFile" 2>&1; then mv "$tmp_img" "$img" log -ne "Encrypted: $img" else @@ -199,7 +199,8 @@ backup_running_vm() { break ;; Failed) echo "Virsh backup job failed" - cleanup ;; + cleanup + return 1 ;; esac sleep 5 done @@ -212,7 +213,7 @@ backup_running_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 + if qemu-img convert -c -O qcow2 "$img" "$tmp_img" >> "$logFile" 2>&1; then mv "$tmp_img" "$img" else log -ne "Warning: compression failed for $img, keeping uncompressed" @@ -313,7 +314,7 @@ mount_operation() { if [ ${NAS_TYPE} == "cifs" ]; then MOUNT_OPTS="${MOUNT_OPTS},nobrl" fi - mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) 2>&1 | tee -a "$logFile" + mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) >> "$logFile" 2>&1 if [ $? -eq 0 ]; then log -ne "Successfully mounted ${NAS_TYPE} store" else From 3d65f34286c5ba2b37660daddb2bd38fbf0ef8cf Mon Sep 17 00:00:00 2001 From: jmsperu Date: Thu, 2 Apr 2026 18:37:12 +0300 Subject: [PATCH 4/6] Fix NASBackupProviderTest failures for zone-scoped ConfigKey values The test helper overrideConfigValue() was only setting _value on ConfigKey, but zone-scoped configs (valueIn(zoneId)) fall back to _defaultValue when s_depot is null in test context. Also set _defaultValue via ReflectionTestUtils to ensure valueIn() returns the expected test value. Fixes: 4 assertion failures (compression, bandwidth, encryption, integrity_check details all returned null) and 1 error (encryption without passphrase expected CloudRuntimeException but got NullPointerException from null config value). --- .../org/apache/cloudstack/backup/NASBackupProviderTest.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java index 48a4b5e8562c..1aaca70a5de9 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java @@ -357,9 +357,13 @@ public void testGetVMHypervisorHostFallbackToZoneWideKVMHost() { private void overrideConfigValue(final ConfigKey configKey, final Object value) { try { + // Set _value for value() calls Field f = ConfigKey.class.getDeclaredField("_value"); f.setAccessible(true); - f.set(configKey, value); + f.set(configKey, value != null ? configKey.valueOf((String) value) : null); + + // Also set _defaultValue via Spring's ReflectionTestUtils (handles final fields) + ReflectionTestUtils.setField(configKey, "_defaultValue", String.valueOf(value)); } catch (IllegalAccessException | NoSuchFieldException e) { Assert.fail(e.getMessage()); } From c11e0b86defae2fb48bcf38126848b18785145f3 Mon Sep 17 00:00:00 2001 From: jmsperu Date: Wed, 8 Apr 2026 19:59:55 +0300 Subject: [PATCH 5/6] Fix build: use reflection for protected ConfigKey.valueOf() in test - NASBackupProviderTest.overrideConfigValue() was calling configKey.valueOf() directly, but valueOf() has protected access. Use Method.setAccessible(true) to invoke it via reflection instead. - nasbackup.sh: rewrite mount check as `if mount ...; then` to prevent set -e from bypassing custom error handling. --- .../apache/cloudstack/backup/NASBackupProviderTest.java | 9 +++++++-- scripts/vm/hypervisor/kvm/nasbackup.sh | 3 +-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java index 1aaca70a5de9..61c294048e0c 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java @@ -357,14 +357,19 @@ public void testGetVMHypervisorHostFallbackToZoneWideKVMHost() { private void overrideConfigValue(final ConfigKey configKey, final Object value) { try { + // Use reflection to invoke protected valueOf() + java.lang.reflect.Method valueOfMethod = ConfigKey.class.getDeclaredMethod("valueOf", String.class); + valueOfMethod.setAccessible(true); + Object typedValue = value != null ? valueOfMethod.invoke(configKey, String.valueOf(value)) : null; + // Set _value for value() calls Field f = ConfigKey.class.getDeclaredField("_value"); f.setAccessible(true); - f.set(configKey, value != null ? configKey.valueOf((String) value) : null); + f.set(configKey, typedValue); // Also set _defaultValue via Spring's ReflectionTestUtils (handles final fields) ReflectionTestUtils.setField(configKey, "_defaultValue", String.valueOf(value)); - } catch (IllegalAccessException | NoSuchFieldException e) { + } catch (Exception e) { Assert.fail(e.getMessage()); } } diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index 9e4b4d30e430..e24571fea1e8 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -314,8 +314,7 @@ mount_operation() { if [ ${NAS_TYPE} == "cifs" ]; then MOUNT_OPTS="${MOUNT_OPTS},nobrl" fi - mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) >> "$logFile" 2>&1 - if [ $? -eq 0 ]; then + if mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) >> "$logFile" 2>&1; then log -ne "Successfully mounted ${NAS_TYPE} store" else echo "Failed to mount ${NAS_TYPE} store" From 2150959f232a505fd9e1b9b215d1f10a0d435e2f Mon Sep 17 00:00:00 2001 From: jmsperu Date: Sat, 23 May 2026 01:20:39 +0300 Subject: [PATCH 6/6] refactor(backup): consolidate detail keys in TakeBackupCommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the final outstanding Copilot review point on #12898 (shared constants). The detail-map keys exchanged between NASBackupProvider, the KVM agent wrapper, and the nasbackup.sh interface were defined twice — once as private string literals in the provider and once as static finals in the wrapper — which makes a typo or rename silently break the wire protocol. Moved the constants to TakeBackupCommand so they live on the command class itself, and updated both the provider's addDetail() sites and the wrapper's details.get() sites to use them. Test file updated to assert via the same constants for consistency. No behaviour change. All 14 NAS tests and 611 KVM tests pass. --- .../cloudstack/backup/TakeBackupCommand.java | 9 +++++++++ .../cloudstack/backup/NASBackupProvider.java | 10 +++++----- .../backup/NASBackupProviderTest.java | 10 +++++----- .../LibvirtTakeBackupCommandWrapper.java | 18 +++++------------- 4 files changed, 24 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java index ce0de62149a9..1d531c455a82 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java @@ -28,6 +28,15 @@ import java.util.Map; public class TakeBackupCommand extends Command { + // Detail-map keys shared between the management server (NASBackupProvider) and + // the KVM agent wrapper. Defining them once here avoids drift between producer + // and consumer when a key is renamed. + public static final String DETAIL_COMPRESSION = "compression"; + public static final String DETAIL_ENCRYPTION = "encryption"; + public static final String DETAIL_ENCRYPTION_PASSPHRASE = "encryption_passphrase"; + public static final String DETAIL_BANDWIDTH_LIMIT = "bandwidth_limit"; + public static final String DETAIL_INTEGRITY_CHECK = "integrity_check"; + private String vmName; private String backupPath; private String backupRepoType; diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index 49397229ea28..4520caa7a314 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -249,22 +249,22 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce // Pass optional backup enhancement settings from zone-scoped configs Long zoneId = vm.getDataCenterId(); if (Boolean.TRUE.equals(NASBackupCompressionEnabled.valueIn(zoneId))) { - command.addDetail("compression", "true"); + command.addDetail(TakeBackupCommand.DETAIL_COMPRESSION, "true"); } if (Boolean.TRUE.equals(NASBackupEncryptionEnabled.valueIn(zoneId))) { String passphrase = NASBackupEncryptionPassphrase.valueIn(zoneId); if (passphrase == null || passphrase.isEmpty()) { throw new CloudRuntimeException("NAS backup encryption is enabled but no passphrase is configured (nas.backup.encryption.passphrase)"); } - command.addDetail("encryption", "true"); - command.addDetail("encryption_passphrase", passphrase); + command.addDetail(TakeBackupCommand.DETAIL_ENCRYPTION, "true"); + command.addDetail(TakeBackupCommand.DETAIL_ENCRYPTION_PASSPHRASE, passphrase); } Integer bandwidthLimit = NASBackupBandwidthLimitMbps.valueIn(zoneId); if (bandwidthLimit != null && bandwidthLimit > 0) { - command.addDetail("bandwidth_limit", String.valueOf(bandwidthLimit)); + command.addDetail(TakeBackupCommand.DETAIL_BANDWIDTH_LIMIT, String.valueOf(bandwidthLimit)); } if (Boolean.TRUE.equals(NASBackupIntegrityCheckEnabled.valueIn(zoneId))) { - command.addDetail("integrity_check", "true"); + command.addDetail(TakeBackupCommand.DETAIL_INTEGRITY_CHECK, "true"); } if (VirtualMachine.State.Stopped.equals(vm.getState())) { diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java index 61c294048e0c..e331af0ebe74 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java @@ -430,7 +430,7 @@ public void testTakeBackupDetailsCompressionEnabled() throws AgentUnavailableExc TakeBackupCommand capturedCmd = cmdCaptor.getValue(); Map details = capturedCmd.getDetails(); - Assert.assertEquals("true", details.get("compression")); + Assert.assertEquals("true", details.get(TakeBackupCommand.DETAIL_COMPRESSION)); // Reset config overrideConfigValue(nasBackupProvider.NASBackupCompressionEnabled, "false"); @@ -464,7 +464,7 @@ public void testTakeBackupDetailsBandwidthLimit() throws AgentUnavailableExcepti TakeBackupCommand capturedCmd = cmdCaptor.getValue(); Map details = capturedCmd.getDetails(); - Assert.assertEquals("50", details.get("bandwidth_limit")); + Assert.assertEquals("50", details.get(TakeBackupCommand.DETAIL_BANDWIDTH_LIMIT)); overrideConfigValue(nasBackupProvider.NASBackupBandwidthLimitMbps, "0"); } @@ -497,7 +497,7 @@ public void testTakeBackupDetailsIntegrityCheck() throws AgentUnavailableExcepti TakeBackupCommand capturedCmd = cmdCaptor.getValue(); Map details = capturedCmd.getDetails(); - Assert.assertEquals("true", details.get("integrity_check")); + Assert.assertEquals("true", details.get(TakeBackupCommand.DETAIL_INTEGRITY_CHECK)); overrideConfigValue(nasBackupProvider.NASBackupIntegrityCheckEnabled, "false"); } @@ -531,8 +531,8 @@ public void testTakeBackupDetailsEncryptionWithPassphrase() throws AgentUnavaila TakeBackupCommand capturedCmd = cmdCaptor.getValue(); Map details = capturedCmd.getDetails(); - Assert.assertEquals("true", details.get("encryption")); - Assert.assertEquals("my-secret-passphrase", details.get("encryption_passphrase")); + Assert.assertEquals("true", details.get(TakeBackupCommand.DETAIL_ENCRYPTION)); + Assert.assertEquals("my-secret-passphrase", details.get(TakeBackupCommand.DETAIL_ENCRYPTION_PASSPHRASE)); overrideConfigValue(nasBackupProvider.NASBackupEncryptionEnabled, "false"); overrideConfigValue(nasBackupProvider.NASBackupEncryptionPassphrase, ""); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java index 5daf1daac8c6..74aeb4e8ca42 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java @@ -53,14 +53,6 @@ public class LibvirtTakeBackupCommandWrapper extends CommandWrapper { private static final Integer EXIT_CLEANUP_FAILED = 20; - // Detail keys exchanged via TakeBackupCommand.getDetails() — single source of truth so - // the management-server config wiring, this wrapper, and any future readers don't drift. - // (Per Copilot review on apache/cloudstack#12898: avoid scattering string literals.) - static final String DETAIL_COMPRESSION = "compression"; - static final String DETAIL_ENCRYPTION = "encryption"; - static final String DETAIL_ENCRYPTION_PASSPHRASE = "encryption_passphrase"; - static final String DETAIL_BANDWIDTH_LIMIT = "bandwidth_limit"; - static final String DETAIL_INTEGRITY_CHECK = "integrity_check"; @Override public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvirtComputingResource) { final String vmName = command.getVmName(); @@ -103,11 +95,11 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir File passphraseFile = null; Map details = command.getDetails(); if (details != null) { - if ("true".equals(details.get(DETAIL_COMPRESSION))) { + if ("true".equals(details.get(TakeBackupCommand.DETAIL_COMPRESSION))) { cmdArgs.add("-c"); } - if ("true".equals(details.get(DETAIL_ENCRYPTION))) { - String passphrase = details.get(DETAIL_ENCRYPTION_PASSPHRASE); + if ("true".equals(details.get(TakeBackupCommand.DETAIL_ENCRYPTION))) { + String passphrase = details.get(TakeBackupCommand.DETAIL_ENCRYPTION_PASSPHRASE); if (passphrase == null || passphrase.isEmpty()) { return new BackupAnswer(command, false, "Encryption is enabled but no passphrase was provided"); } @@ -128,11 +120,11 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir return new BackupAnswer(command, false, "Failed to create encryption passphrase file: " + e.getMessage()); } } - String bwLimit = details.get(DETAIL_BANDWIDTH_LIMIT); + String bwLimit = details.get(TakeBackupCommand.DETAIL_BANDWIDTH_LIMIT); if (bwLimit != null && !"0".equals(bwLimit)) { cmdArgs.add("-b"); cmdArgs.add(bwLimit); } - if ("true".equals(details.get(DETAIL_INTEGRITY_CHECK))) { + if ("true".equals(details.get(TakeBackupCommand.DETAIL_INTEGRITY_CHECK))) { cmdArgs.add("--verify"); } }