Block use of internal and external snapshots on KVM

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

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);
 }

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;
     }
 

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java

@@ -26,14 +26,21 @@
 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.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;
 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;
@@ -104,7 +111,16 @@ public class DefaultVMSnapshotStrategy extends ManagerBase implements VMSnapshot
     PrimaryDataStoreDao primaryDataStoreDao;
 
     @Inject
-    VMSnapshotDetailsDao vmSnapshotDetailsDao;
+    private VMSnapshotDetailsDao vmSnapshotDetailsDao;
+
+    @Inject
+    private BackupManager backupManager;
+
+    @Inject
+    private BackupOfferingDao backupOfferingDao;
+
+    @Inject
+    private SnapshotDao snapshotDao;
 
     protected static final String KVM_FILE_BASED_STORAGE_SNAPSHOT = "kvmFileBasedStorageSnapshot";
 
@@ -479,24 +495,44 @@ 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<VolumeVO> 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 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;
+        }
+
         return StrategyPriority.DEFAULT;
     }
 
@@ -507,7 +543,7 @@ private boolean vmHasKvmDiskOnlySnapshot(UserVm vm) {
 
         for (VMSnapshotVO vmSnapshotVO : vmSnapshotDao.findByVmAndByType(vm.getId(), VMSnapshot.Type.Disk)) {
             List<VMSnapshotDetailsVO> 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;
             }
         }

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);
+        }
     }
 }

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);
+        }
     }
 }

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<Boolean, Backup> 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));
@@ -464,6 +471,11 @@ public boolean supportsInstanceFromBackup() {
         return true;
     }
 
+    @Override
+    public boolean supportsMemoryVmSnapshot() {
+        return false;
+    }
+
     @Override
     public Pair<Long, Long> getBackupStorageStats(Long zoneId) {
         final List<BackupRepository> repositories = backupRepositoryDao.listByZoneAndProvider(zoneId, getName());

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;