address review comments

Author: DaanHoogland

engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java

@@ -45,6 +45,8 @@ public interface SnapshotDao extends GenericDao<SnapshotVO, Long>, StateDao<Snap
 
     List<SnapshotVO> listByStatus(long volumeId, Snapshot.State... status);
 
+    List<SnapshotVO> listByStatus(Filter filter, long volumeId, Snapshot.State... status);
+
     List<SnapshotVO> listAllByStatus(Snapshot.State... status);
 
     List<SnapshotVO> listAllByStatusIncludingRemoved(Snapshot.State... status);

engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java

@@ -255,10 +255,15 @@ public List<SnapshotVO> listByInstanceId(long instanceId, Snapshot.State... stat
 
     @Override
     public List<SnapshotVO> listByStatus(long volumeId, Snapshot.State... status) {
+        return listByStatus(null, volumeId, status);
+    }
+
+    @Override
+    public List<SnapshotVO> listByStatus(Filter filter, long volumeId, Snapshot.State... status) {
         SearchCriteria<SnapshotVO> sc = StatusSearch.create();
         sc.setParameters(VOLUME_ID, volumeId);
         sc.setParameters(STATUS, (Object[])status);
-        return listBy(sc, null);
+        return listBy(sc, filter);
     }
 
     @Override

engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql

@@ -651,3 +651,6 @@ WHERE `name`='user.vm.readonly.details' AND `value` IS NOT NULL;
 -- usage records introduced in 4.22.1 (cumulative and per-VM) can coexist. See #13399.
 CALL `cloud_usage`.`IDEMPOTENT_DROP_INDEX`('id', 'cloud_usage.usage_volume');
 CALL `cloud_usage`.`IDEMPOTENT_ADD_UNIQUE_INDEX`('cloud_usage.usage_volume', 'id', '(volume_id ASC, created ASC, vm_id ASC)');
+
+-- Index event lookups by resource, e.g. the latest snapshot outcome events of a volume
+CALL `cloud`.`IDEMPOTENT_ADD_KEY`('i_event__resource_id__resource_type__type', 'cloud.event', '(`resource_id`, `resource_type`, `type`, `created`)');

server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java

@@ -57,15 +57,15 @@ public interface SnapshotManager extends Configurable {
 
     ConfigKey<Integer> SnapshotRecurringMaxFailures = new ConfigKey<>(Integer.class, "snapshot.recurring.max.failures", "Snapshots", "3",
             "Maximum number of consecutive failed attempts allowed for a recurring snapshot before it is left until its next regularly scheduled run and a failure event is logged. Set to 0 to retry indefinitely.",
-            true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
+            true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone), null);
 
     ConfigKey<Integer> SnapshotRecurringRetryInterval = new ConfigKey<>(Integer.class, "snapshot.recurring.retry.interval", "Snapshots", "300",
             "Time in seconds to wait before retrying a failed recurring snapshot attempt.",
-            true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
+            true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone), null);
 
     ConfigKey<Boolean> SnapshotSkipIfVmNotRunning = new ConfigKey<>(Boolean.class, "snapshot.skip.if.vm.not.running", "Snapshots", "false",
             "For recurring snapshots, skip taking a new snapshot of a volume (and log a skipped event instead) when the VM it is attached to has not been running since the last snapshot was taken, since nothing on the volume could have changed.",
-            true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
+            true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Zone), null);
 
     public static final ConfigKey<Boolean> VmStorageSnapshotKvm = new ConfigKey<>(Boolean.class, "kvm.vmstoragesnapshot.enabled", "Snapshots", "true", "For live snapshot of virtual machine instance on KVM hypervisor without memory. Requires qemu version 1.6+ (on NFS or Local file system) and qemu-guest-agent installed on guest VM", true, ConfigKey.Scope.Global, null);
 

server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java

@@ -499,8 +499,16 @@ protected boolean shouldSkipUnchangedVolumeSnapshot(final VolumeVO volume) {
             return false;
         }
 
+        // The volume may have been written to on another instance and attached to this one after the last snapshot.
+        // Volumes allocated together with their instance have no attach time and have been on it since creation.
+        final Date attached = volume.getAttached();
+        if (attached != null && !attached.before(lastSnapshot.getCreated())) {
+            return false;
+        }
+
+        // Only a confirmed powered off state proves the instance has not been running; PowerUnknown does not.
         final VMInstanceVO vm = vmInstanceDao.findById(instanceId);
-        if (vm == null || vm.getPowerState() == VirtualMachine.PowerState.PowerOn) {
+        if (vm == null || vm.getPowerState() != VirtualMachine.PowerState.PowerOff) {
             return false;
         }
 
@@ -510,9 +518,13 @@ protected boolean shouldSkipUnchangedVolumeSnapshot(final VolumeVO volume) {
         return poweredOffSince != null && poweredOffSince.before(lastSnapshot.getCreated());
     }
 
+    /**
+     * Returns the most recent successfully taken snapshot of the volume. Snapshots left in Allocated or Error
+     * state by failed attempts are not a usable baseline and are ignored.
+     */
     protected SnapshotVO findLastSnapshot(final long volumeId) {
         final Filter filter = new Filter(SnapshotVO.class, "created", false, 0L, 1L);
-        final List<SnapshotVO> snapshots = _snapshotDao.listByVolumeId(filter, volumeId);
+        final List<SnapshotVO> snapshots = _snapshotDao.listByStatus(filter, volumeId, Snapshot.State.BackedUp);
         return (snapshots == null || snapshots.isEmpty()) ? null : snapshots.get(0);
     }
 

server/src/test/java/com/cloud/storage/snapshot/SnapshotSchedulerImplTest.java

@@ -688,6 +688,52 @@ public void shouldSkipUnchangedVolumeSnapshotTestVmOffSinceAfterLastSnapshotRetu
         Assert.assertFalse(result);
     }
 
+    @Test
+    public void shouldSkipUnchangedVolumeSnapshotTestVmPowerUnknownReturnsFalse() {
+        stubSkipConfigEnabled();
+        doReturn(5L).when(volumeVoMock).getInstanceId();
+        doReturn(1L).when(volumeVoMock).getId();
+        doReturn(snapshotVoMock).when(snapshotSchedulerImplSpy).findLastSnapshot(Mockito.anyLong());
+        doReturn(vmInstanceVoMock).when(vmInstanceDaoMock).findById(5L);
+        doReturn(VirtualMachine.PowerState.PowerUnknown).when(vmInstanceVoMock).getPowerState();
+
+        boolean result = snapshotSchedulerImplSpy.shouldSkipUnchangedVolumeSnapshot(volumeVoMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void shouldSkipUnchangedVolumeSnapshotTestAttachedAfterLastSnapshotReturnsFalse() {
+        stubSkipConfigEnabled();
+        doReturn(5L).when(volumeVoMock).getInstanceId();
+        doReturn(1L).when(volumeVoMock).getId();
+        doReturn(new Date(3000L)).when(volumeVoMock).getAttached();
+        doReturn(new Date(2000L)).when(snapshotVoMock).getCreated();
+        doReturn(snapshotVoMock).when(snapshotSchedulerImplSpy).findLastSnapshot(Mockito.anyLong());
+
+        boolean result = snapshotSchedulerImplSpy.shouldSkipUnchangedVolumeSnapshot(volumeVoMock);
+
+        Assert.assertFalse(result);
+        Mockito.verifyNoInteractions(vmInstanceDaoMock);
+    }
+
+    @Test
+    public void shouldSkipUnchangedVolumeSnapshotTestAttachedBeforeLastSnapshotAndVmOffReturnsTrue() {
+        stubSkipConfigEnabled();
+        doReturn(5L).when(volumeVoMock).getInstanceId();
+        doReturn(1L).when(volumeVoMock).getId();
+        doReturn(new Date(500L)).when(volumeVoMock).getAttached();
+        doReturn(new Date(2000L)).when(snapshotVoMock).getCreated();
+        doReturn(snapshotVoMock).when(snapshotSchedulerImplSpy).findLastSnapshot(Mockito.anyLong());
+        doReturn(vmInstanceVoMock).when(vmInstanceDaoMock).findById(5L);
+        doReturn(VirtualMachine.PowerState.PowerOff).when(vmInstanceVoMock).getPowerState();
+        doReturn(new Date(1000L)).when(vmInstanceVoMock).getPowerStateUpdateTime();
+
+        boolean result = snapshotSchedulerImplSpy.shouldSkipUnchangedVolumeSnapshot(volumeVoMock);
+
+        Assert.assertTrue(result);
+    }
+
     private void stubSkipConfigEnabled() {
         doReturn(true).when(snapshotSchedulerImplSpy).getScopedConfigValue(Mockito.eq(SnapshotManager.SnapshotSkipIfVmNotRunning), Mockito.any(), Mockito.any());
         doReturn(1L).when(volumeVoMock).getAccountId();
@@ -734,7 +780,7 @@ public void skipAndRescheduleSnapshotTestCouldNotAcquireLockDoesNothing() {
 
     @Test
     public void findLastSnapshotTestReturnsMostRecentSnapshot() {
-        doReturn(List.of(snapshotVoMock)).when(snapshotDaoMock).listByVolumeId(Mockito.any(), Mockito.eq(1L));
+        doReturn(List.of(snapshotVoMock)).when(snapshotDaoMock).listByStatus(Mockito.any(), Mockito.eq(1L), Mockito.eq(Snapshot.State.BackedUp));
 
         SnapshotVO result = snapshotSchedulerImplSpy.findLastSnapshot(1L);
 
@@ -743,7 +789,7 @@ public void findLastSnapshotTestReturnsMostRecentSnapshot() {
 
     @Test
     public void findLastSnapshotTestNoSnapshotsReturnsNull() {
-        doReturn(Collections.emptyList()).when(snapshotDaoMock).listByVolumeId(Mockito.any(), Mockito.eq(1L));
+        doReturn(Collections.emptyList()).when(snapshotDaoMock).listByStatus(Mockito.any(), Mockito.eq(1L), Mockito.eq(Snapshot.State.BackedUp));
 
         SnapshotVO result = snapshotSchedulerImplSpy.findLastSnapshot(1L);