From f54062d45fada3a99cc6ba63dce2c05934a91224 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Jandre?= <48719461+JoaoJandre@users.noreply.github.com> Date: Mon, 1 Sep 2025 15:04:27 -0300 Subject: [PATCH 1/3] Avoid data loss --- .../snapshot/DefaultSnapshotStrategy.java | 6 ++++ .../vmsnapshot/DefaultVMSnapshotStrategy.java | 35 ++++++++++++++++--- .../vmsnapshot/VMSnapshotStrategyKVMTest.java | 6 ++++ .../vmsnapshot/VMSnapshotStrategyTest.java | 17 +++++++++ .../cloudstack/backup/NASBackupProvider.java | 7 ++++ .../backup/NASBackupProviderTest.java | 4 +++ 6 files changed, 70 insertions(+), 5 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index c1981941ac03..c6d89a870145 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -672,6 +672,12 @@ private StrategyPriority validateVmSnapshot(Snapshot snapshot) { } } + if (CollectionUtils.isNotEmpty(vmSnapshotDao.findByVmAndByType(volumeVO.getInstanceId(), VMSnapshot.Type.DiskAndMemory))) { + logger.debug("DefaultSnapshotStrategy cannot handle snapshot [{}] for volume [{}] as the volume is attached to a VM with disk-and-memory VM snapshots." + + "Restoring the volume snapshot will corrupt any newer disk-and-memory VM snapshots.", snapshot); + return StrategyPriority.CANT_HANDLE; + } + return StrategyPriority.DEFAULT; } diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java index 2d9b1e67e097..56a55fe8fca0 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java @@ -26,14 +26,19 @@ import javax.naming.ConfigurationException; import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.Snapshot; +import com.cloud.storage.dao.SnapshotDao; import com.cloud.vm.snapshot.VMSnapshotDetailsVO; import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao; +import org.apache.cloudstack.backup.BackupOfferingVO; +import org.apache.cloudstack.backup.dao.BackupOfferingDao; import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority; import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotOptions; import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotStrategy; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.to.VolumeObjectTO; +import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.StringUtils; import com.cloud.agent.AgentManager; @@ -106,6 +111,12 @@ public class DefaultVMSnapshotStrategy extends ManagerBase implements VMSnapshot @Inject VMSnapshotDetailsDao vmSnapshotDetailsDao; + @Inject + private BackupOfferingDao backupOfferingDao; + + @Inject + private SnapshotDao snapshotDao; + protected static final String KVM_FILE_BASED_STORAGE_SNAPSHOT = "kvmFileBasedStorageSnapshot"; protected static final String STORAGE_SNAPSHOT = "kvmStorageSnapshot"; @@ -479,24 +490,38 @@ public boolean deleteVMSnapshotFromDB(VMSnapshot vmSnapshot, boolean unmanage) { @Override public StrategyPriority canHandle(Long vmId, Long rootPoolId, boolean snapshotMemory) { UserVmVO vm = userVmDao.findById(vmId); + String cantHandleLog = String.format("Default VM snapshot cannot handle VM snapshot for [%s]", vm); if (State.Running.equals(vm.getState()) && !snapshotMemory) { - logger.debug("Default VM snapshot strategy cannot handle VM snapshot for [{}] as it is running and its memory will not be affected.", vm); + logger.debug("{} as it is running and its memory will not be affected.", cantHandleLog, vm); return StrategyPriority.CANT_HANDLE; } if (vmHasKvmDiskOnlySnapshot(vm)) { - logger.debug("Default VM snapshot strategy cannot handle VM snapshot for [{}] as it has a disk-only VM snapshot using kvmFileBasedStorageSnapshot strategy." + - "These two strategies are not compatible, as reverting a disk-only VM snapshot will erase newer disk-and-memory VM snapshots.", vm); + logger.debug("{} as it is not compatible with disk-only VM snapshot on KVM. As disk-and-memory snapshots use internal snapshots and disk-only VM snapshots use" + + " external snapshots. When restoring external snapshots, any newer internal snapshots are lost.", cantHandleLog); return StrategyPriority.CANT_HANDLE; } List volumes = volumeDao.findByInstance(vmId); for (VolumeVO volume : volumes) { if (volume.getFormat() != ImageFormat.QCOW2) { - logger.debug("Default VM snapshot strategy cannot handle VM snapshot for [{}] as it has a volume [{}] that is not in the QCOW2 format.", vm, volume); + logger.debug("{} as it has a volume [{}] that is not in the QCOW2 format.", cantHandleLog, vm, volume); + return StrategyPriority.CANT_HANDLE; + } + + if (CollectionUtils.isNotEmpty(snapshotDao.listByVolumeIdAndTypeNotInAndStateNotRemoved(volume.getId(), Snapshot.Type.GROUP))) { + logger.debug("{} as it has a volume [{}] with volume snapshots. As disk-and-memory snapshots use internal snapshots and volume snapshots use external" + + " snapshots. When restoring external snapshots, any newer internal snapshots are lost.", cantHandleLog, volume); return StrategyPriority.CANT_HANDLE; } } + + BackupOfferingVO backupOffering = backupOfferingDao.findById(vm.getBackupOfferingId()); + if (backupOffering != null && "nas".equals(backupOffering.getProvider())) { + logger.debug("{} as the VM has a backup offering for a provider that is not supported.", cantHandleLog); + return StrategyPriority.CANT_HANDLE; + } + return StrategyPriority.DEFAULT; } @@ -507,7 +532,7 @@ private boolean vmHasKvmDiskOnlySnapshot(UserVm vm) { for (VMSnapshotVO vmSnapshotVO : vmSnapshotDao.findByVmAndByType(vm.getId(), VMSnapshot.Type.Disk)) { List vmSnapshotDetails = vmSnapshotDetailsDao.listDetails(vmSnapshotVO.getId()); - if (vmSnapshotDetails.stream().anyMatch(vmSnapshotDetailsVO -> vmSnapshotDetailsVO.getName().equals(KVM_FILE_BASED_STORAGE_SNAPSHOT))) { + if (vmSnapshotDetails.stream().anyMatch(detailsVO -> KVM_FILE_BASED_STORAGE_SNAPSHOT.equals(detailsVO.getName()) || STORAGE_SNAPSHOT.equals(detailsVO.getName()))) { return true; } } diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java index 609a1225118a..dbbe0197bd6b 100644 --- a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java +++ b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java @@ -29,6 +29,7 @@ import javax.inject.Inject; +import org.apache.cloudstack.backup.dao.BackupOfferingDao; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProviderManager; @@ -431,5 +432,10 @@ public DataStoreProviderManager manager() { public VMSnapshotDetailsDao vmSnapshotDetailsDao () { return Mockito.mock(VMSnapshotDetailsDao.class); } + + @Bean + public BackupOfferingDao backupOfferingDao() { + return Mockito.mock(BackupOfferingDao.class); + } } } diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java index 829ca5ade39b..e2bb508249a8 100644 --- a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java +++ b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java @@ -25,7 +25,9 @@ import javax.inject.Inject; +import com.cloud.storage.dao.SnapshotDao; import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao; +import org.apache.cloudstack.backup.dao.BackupOfferingDao; import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotStrategy; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; @@ -318,5 +320,20 @@ public HostDao hostDao() { public PrimaryDataStoreDao primaryDataStoreDao() { return Mockito.mock(PrimaryDataStoreDao.class); } + + @Bean + public BackupOfferingDao backupOfferingDao() { + return Mockito.mock(BackupOfferingDao.class); + } + + @Bean + public VMSnapshotDetailsDao VMSnapshotDetailsDao() { + return Mockito.mock(VMSnapshotDetailsDao.class); + } + + @Bean + public SnapshotDao snapshotDao() { + return Mockito.mock(SnapshotDao.class); + } } } 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 e5f98ad291be..3e2939593f72 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 @@ -53,6 +53,7 @@ import org.apache.cloudstack.framework.config.Configurable; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.commons.collections4.CollectionUtils; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; @@ -165,6 +166,12 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce throw new CloudRuntimeException("No valid backup repository found for the VM, please check the attached backup offering"); } + if (CollectionUtils.isNotEmpty(vmSnapshotDao.findByVmAndByType(vm.getId(), VMSnapshot.Type.DiskAndMemory))) { + logger.debug("NAS backup provider cannot take backups of a VM [{}] with disk-and-memory VM snapshots. Restoring the backup will corrupt any newer disk-and-memory " + + "VM snapshots.", vm); + throw new CloudRuntimeException(String.format("Cannot take backup of VM [%s] as it has disk-and-memory VM snapshots.", vm.getUuid())); + } + final Date creationDate = new Date(); final String backupPath = String.format("%s/%s", vm.getInstanceName(), new SimpleDateFormat("yyyy.MM.dd.HH.mm.ss").format(creationDate)); 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 d6f29dc1aac1..004c4380ff61 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 @@ -23,6 +23,7 @@ import java.util.List; import java.util.Optional; +import com.cloud.vm.snapshot.dao.VMSnapshotDao; import org.apache.cloudstack.backup.dao.BackupDao; import org.apache.cloudstack.backup.dao.BackupRepositoryDao; import org.apache.cloudstack.backup.dao.BackupOfferingDao; @@ -84,6 +85,9 @@ public class NASBackupProviderTest { @Mock private ResourceManager resourceManager; + @Mock + private VMSnapshotDao vmSnapshotDaoMock; + @Test public void testDeleteBackup() throws OperationTimedoutException, AgentUnavailableException { Long hostId = 1L; From 8938966ccabd986359543c00fef1a0b4d01507d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Jandre?= <48719461+JoaoJandre@users.noreply.github.com> Date: Tue, 2 Sep 2025 14:37:11 -0300 Subject: [PATCH 2/3] address review --- .../cloudstack/backup/BackupProvider.java | 4 ++++ .../apache/cloudstack/backup/BackupService.java | 7 +++++++ .../vmsnapshot/DefaultVMSnapshotStrategy.java | 17 ++++++++++++++--- .../cloudstack/backup/NASBackupProvider.java | 5 +++++ 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java b/api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java index 1eb36f895565..839911f0dacc 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java @@ -122,6 +122,10 @@ public interface BackupProvider { */ boolean supportsInstanceFromBackup(); + default boolean supportsMemoryVmSnapshot() { + return true; + } + /** * Returns the backup storage usage (Used, Total) for a backup provider * @param zoneId the zone for which to return metrics diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupService.java b/api/src/main/java/org/apache/cloudstack/backup/BackupService.java index d4beb629fe0f..3ba2978c0fa5 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupService.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupService.java @@ -34,4 +34,11 @@ public interface BackupService { * @return backup provider */ BackupProvider getBackupProvider(final Long zoneId); + + /** + * Find backup provider by name + * @param name backup provider name + * @return backup provider + */ + BackupProvider getBackupProvider(final String name); } diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java index 56a55fe8fca0..df78678afb05 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java @@ -30,7 +30,9 @@ import com.cloud.storage.dao.SnapshotDao; import com.cloud.vm.snapshot.VMSnapshotDetailsVO; import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao; +import org.apache.cloudstack.backup.BackupManager; import org.apache.cloudstack.backup.BackupOfferingVO; +import org.apache.cloudstack.backup.BackupProvider; import org.apache.cloudstack.backup.dao.BackupOfferingDao; import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority; import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotOptions; @@ -109,7 +111,10 @@ public class DefaultVMSnapshotStrategy extends ManagerBase implements VMSnapshot PrimaryDataStoreDao primaryDataStoreDao; @Inject - VMSnapshotDetailsDao vmSnapshotDetailsDao; + private VMSnapshotDetailsDao vmSnapshotDetailsDao; + + @Inject + private BackupManager backupManager; @Inject private BackupOfferingDao backupOfferingDao; @@ -516,8 +521,14 @@ public StrategyPriority canHandle(Long vmId, Long rootPoolId, boolean snapshotMe } } - BackupOfferingVO backupOffering = backupOfferingDao.findById(vm.getBackupOfferingId()); - if (backupOffering != null && "nas".equals(backupOffering.getProvider())) { + + BackupOfferingVO backupOfferingVO = backupOfferingDao.findById(vm.getBackupOfferingId()); + if (backupOfferingVO == null) { + return StrategyPriority.DEFAULT; + } + + BackupProvider provider = backupManager.getBackupProvider(backupOfferingVO.getProvider()); + if (!provider.supportsMemoryVmSnapshot()) { logger.debug("{} as the VM has a backup offering for a provider that is not supported.", cantHandleLog); return StrategyPriority.CANT_HANDLE; } 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 3e2939593f72..074139c5f033 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 @@ -471,6 +471,11 @@ public boolean supportsInstanceFromBackup() { return true; } + @Override + public boolean supportsMemoryVmSnapshot() { + return false; + } + @Override public Pair getBackupStorageStats(Long zoneId) { final List repositories = backupRepositoryDao.listByZoneAndProvider(zoneId, getName()); From 9f9e5f2710356179f18f2813b4dbc7c121466823 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Jandre?= <48719461+JoaoJandre@users.noreply.github.com> Date: Wed, 10 Sep 2025 16:34:59 -0300 Subject: [PATCH 3/3] fix tests --- .../storage/vmsnapshot/VMSnapshotStrategyKVMTest.java | 6 ++++++ .../storage/vmsnapshot/VMSnapshotStrategyTest.java | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java index dbbe0197bd6b..02c17f8f3bd5 100644 --- a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java +++ b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java @@ -29,6 +29,7 @@ import javax.inject.Inject; +import org.apache.cloudstack.backup.BackupManager; import org.apache.cloudstack.backup.dao.BackupOfferingDao; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; @@ -437,5 +438,10 @@ public VMSnapshotDetailsDao vmSnapshotDetailsDao () { public BackupOfferingDao backupOfferingDao() { return Mockito.mock(BackupOfferingDao.class); } + + @Bean + public BackupManager backupManager() { + return Mockito.mock(BackupManager.class); + } } } diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java index e2bb508249a8..a20b52fac2dd 100644 --- a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java +++ b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java @@ -27,6 +27,7 @@ import com.cloud.storage.dao.SnapshotDao; import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao; +import org.apache.cloudstack.backup.BackupManager; import org.apache.cloudstack.backup.dao.BackupOfferingDao; import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotStrategy; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; @@ -335,5 +336,10 @@ public VMSnapshotDetailsDao VMSnapshotDetailsDao() { public SnapshotDao snapshotDao() { return Mockito.mock(SnapshotDao.class); } + + @Bean + public BackupManager backupManager() { + return Mockito.mock(BackupManager.class); + } } }