Refuse snapshot operations cleanly when the volume is gone

revertSnapshot looked up the snapshot's volume with findById(), which
does not return removed volumes, and used it without a null check. Any
snapshot of a deleted volume therefore failed with an NPE on
VolumeVO.getState(). It now fails with an error saying the volume no
longer exists.

The same pattern elsewhere:
- copySnapshot without sourcezoneid took the source zone from the
  volume, found the same way. For a deleted volume it now falls back
  to the snapshot's own zone, which is the zone it was taken in.
- createVolume and createTemplate from a snapshot look the volume up
  including removed rows, but still failed with an NPE when the row
  itself is missing. They now refuse with a clear error.

Fixes #14644

Signed-off-by: Brad House <bhouse@nexthop.ai>

Author: bhouse-nexthop

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

@@ -901,6 +901,9 @@ public VolumeVO allocVolume(CreateVolumeCmd cmd) throws ResourceAllocationExcept
             }
 
             parentVolume = _volsDao.findByIdIncludingRemoved(snapshotCheck.getVolumeId());
+            if (parentVolume == null) {
+                throw new InvalidParameterValueException(String.format("Cannot create a volume from snapshot %s, because the volume it was taken from no longer exists", snapshotCheck.getUuid()));
+            }
 
             // Don't support creating templates from encrypted volumes (yet)
             if (parentVolume.getPassphraseId() != null) {

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

@@ -377,6 +377,9 @@ public Snapshot revertSnapshot(Long snapshotId) {
             throw new InvalidParameterValueException(String.format("The snapshot [%s] is part of a [%s] snapshots and cannot be reverted separately", snapshot, snapshot.getTypeDescription()));
         }
         VolumeVO volume = _volsDao.findById(snapshot.getVolumeId());
+        if (volume == null) {
+            throw new InvalidParameterValueException(String.format("Cannot revert snapshot %s, because its volume no longer exists", snapshot.getUuid()));
+        }
         if (volume.getState() != Volume.State.Ready) {
             throw new InvalidParameterValueException("The volume is not in Ready state.");
         }
@@ -2265,9 +2268,10 @@ protected Pair<SnapshotVO, Long> getCheckedSnapshotForCopy(final SnapshotVO snap
         if (snapshot.getLocationType() != null && !Snapshot.LocationType.SECONDARY.equals(snapshot.getLocationType()) && !useStorageReplication) {
             throw new InvalidParameterValueException("Snapshot is not backed up");
         }
-        Volume volume = _volsDao.findById(snapshot.getVolumeId());
         if (sourceZoneId == null) {
-            sourceZoneId = volume.getDataCenterId();
+            // The volume may have been removed since the snapshot was taken; the snapshot records the same zone.
+            Volume volume = _volsDao.findById(snapshot.getVolumeId());
+            sourceZoneId = volume != null ? volume.getDataCenterId() : snapshot.getDataCenterId();
         }
         if (CollectionUtils.isEmpty(destZoneIds)) {
                 throw new InvalidParameterValueException("Please specify valid destination zone(s).");

server/src/main/java/com/cloud/template/TemplateManagerImpl.java

@@ -2006,9 +2006,12 @@ public VMTemplateVO createPrivateTemplateRecord(CreateTemplateCmd cmd, Account t
             }
             // Volume could be removed so find including removed to record source template id.
             volume = _volumeDao.findByIdIncludingRemoved(snapshot.getVolumeId());
+            if (volume == null) {
+                throw new InvalidParameterValueException(String.format("Cannot create a Template from Snapshot %s, because the volume it was taken from no longer exists", snapshot.getUuid()));
+            }
 
             // Don't support creating templates from encrypted volumes (yet)
-            if (volume != null && volume.getPassphraseId() != null) {
+            if (volume.getPassphraseId() != null) {
                 throw new UnsupportedOperationException("Cannot create Templates from Snapshots of encrypted volumes");
             }
 

server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java

@@ -70,6 +70,7 @@
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
 import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
 import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreVO;
@@ -255,6 +256,9 @@ public class VolumeApiServiceImplTest {
     @Mock
     private SnapshotDao snapshotDaoMock;
 
+    @Mock
+    private SnapshotDataStoreDao snapshotDataStoreDaoMock;
+
     @Mock
     private SnapshotPolicyDetailsDao snapshotPolicyDetailsDao;
 
@@ -625,6 +629,26 @@ public void testNonEmptyGetVolumeNameFromCmd() {
         Assert.assertSame(volumeApiServiceImpl.getVolumeNameFromCommand(createVol), "abc");
     }
 
+    @Test
+    public void allocVolumeFromSnapshotWhoseVolumeRowIsGoneIsRefused() throws ResourceAllocationException {
+        long snapshotId = 42L;
+        SnapshotVO snapshot = Mockito.mock(SnapshotVO.class);
+        Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp);
+        Mockito.when(snapshot.getVolumeId()).thenReturn(volumeMockId);
+        Mockito.when(snapshotDaoMock.findById(snapshotId)).thenReturn(snapshot);
+        Mockito.when(volumeDaoMock.findByIdIncludingRemoved(volumeMockId)).thenReturn(null);
+        Mockito.when(createVol.getDisplayVolume()).thenReturn(null);
+        Mockito.when(createVol.getDiskOfferingId()).thenReturn(null);
+        Mockito.when(createVol.getSnapshotId()).thenReturn(snapshotId);
+
+        try {
+            volumeApiServiceImpl.allocVolume(createVol);
+            Assert.fail("Creating a volume from a snapshot whose volume row is gone must be refused");
+        } catch (InvalidParameterValueException e) {
+            Assert.assertTrue(e.getMessage(), e.getMessage().contains("no longer exists"));
+        }
+    }
+
     @Test
     public void testUpdateMissingRootDiskControllerWithNullChainInfo() {
         volumeApiServiceImpl.updateMissingRootDiskController(null, null);

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

@@ -447,11 +447,22 @@ public void testGetCheckedSnapshotForCopyDestNotSpecified() {
     public void testGetCheckedSnapshotForCopyDestContainsSource() {
         SnapshotVO snapshotVO = Mockito.mock(SnapshotVO.class);
         Mockito.when(snapshotVO.getState()).thenReturn(Snapshot.State.BackedUp);
-        Mockito.when(snapshotVO.getVolumeId()).thenReturn(1L);
-        Mockito.when(volumeDao.findById(Mockito.anyLong())).thenReturn(Mockito.mock(VolumeVO.class));
         snapshotManager.getCheckedSnapshotForCopy(snapshotVO, List.of(100L, 1L), 1L, false);
     }
 
+    @Test
+    public void testGetCheckedSnapshotForCopyVolumeRemoved() {
+        final Long zoneId = 1L;
+        SnapshotVO snapshotVO = Mockito.mock(SnapshotVO.class);
+        Mockito.when(snapshotVO.getState()).thenReturn(Snapshot.State.BackedUp);
+        Mockito.when(snapshotVO.getVolumeId()).thenReturn(1L);
+        Mockito.when(snapshotVO.getDataCenterId()).thenReturn(zoneId);
+        Mockito.when(volumeDao.findById(1L)).thenReturn(null);
+        Mockito.when(dataCenterDao.findById(zoneId)).thenReturn(Mockito.mock(DataCenterVO.class));
+        Pair<SnapshotVO, Long> result = snapshotManager.getCheckedSnapshotForCopy(snapshotVO, List.of(100L, 101L), null, false);
+        Assert.assertEquals(zoneId, result.second());
+    }
+
     @Test(expected = InvalidParameterValueException.class)
     public void testGetCheckedSnapshotForCopyNullSourceZone() {
         SnapshotVO snapshotVO = Mockito.mock(SnapshotVO.class);

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

@@ -344,6 +344,19 @@ public void testRevertSnapshotF1() {
         _snapshotMgr.revertSnapshot(TEST_SNAPSHOT_ID);
     }
 
+    // volume removed or its row gone
+    @Test
+    public void testRevertSnapshotVolumeGone() {
+        when(_volumeDao.findById(anyLong())).thenReturn(null);
+        try {
+            _snapshotMgr.revertSnapshot(TEST_SNAPSHOT_ID);
+            Assert.fail("Reverting a snapshot whose volume no longer exists must be refused");
+        } catch (InvalidParameterValueException e) {
+            Assert.assertTrue(e.getMessage(), e.getMessage().contains("no longer exists"));
+        }
+        Mockito.verify(snapshotStrategy, Mockito.never()).revertSnapshot(any());
+    }
+
     // vm on Xenserver, return null
     @Test
     public void testRevertSnapshotF2() {

server/src/test/java/com/cloud/template/TemplateManagerImplTest.java

@@ -101,6 +101,7 @@
 import org.springframework.core.type.filter.TypeFilter;
 
 import com.cloud.agent.AgentManager;
+import com.cloud.api.ApiDBUtils;
 import com.cloud.api.query.dao.SnapshotJoinDao;
 import com.cloud.api.query.dao.UserVmJoinDao;
 import com.cloud.dc.dao.DataCenterDao;
@@ -499,6 +500,11 @@ public void testCreatePrivateTemplateRecordForRegionStore() throws ResourceAlloc
         when(mockSnapshot.getVolumeId()).thenReturn(1L);
         when(mockSnapshot.getState()).thenReturn(Snapshot.State.BackedUp);
         when(mockSnapshot.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.XenServer);
+        VolumeVO mockVolume = mock(VolumeVO.class);
+        when(mockVolume.getPassphraseId()).thenReturn(null);
+        when(mockVolume.getTemplateId()).thenReturn(null);
+        when(mockVolume.getInstanceId()).thenReturn(null);
+        when(volumeDao.findByIdIncludingRemoved(1L)).thenReturn(mockVolume);
 
         GuestOSVO mockGuestOS = mock(GuestOSVO.class);
         when(guestOSDao.findById(anyLong())).thenReturn(mockGuestOS);
@@ -518,12 +524,34 @@ public VMTemplateVO answer(InvocationOnMock invocationOnMock) throws Throwable {
             }
         });
 
-        try (MockedConstruction<CheckedReservation> mockCheckedReservation = Mockito.mockConstruction(CheckedReservation.class)) {
+        try (MockedConstruction<CheckedReservation> mockCheckedReservation = Mockito.mockConstruction(CheckedReservation.class);
+             MockedStatic<ApiDBUtils> apiDBUtilsMock = Mockito.mockStatic(ApiDBUtils.class)) {
             VMTemplateVO template = templateManager.createPrivateTemplateRecord(mockCreateCmd, mockTemplateOwner);
             assertTrue("Template in a region store should have cross zones set", template.isCrossZones());
         }
     }
 
+    @Test
+    public void testCreatePrivateTemplateRecordSnapshotVolumeRowGone() throws ResourceAllocationException {
+        CreateTemplateCmd mockCreateCmd = mock(CreateTemplateCmd.class);
+        when(mockCreateCmd.getTemplateName()).thenReturn("test");
+        when(mockCreateCmd.getVolumeId()).thenReturn(null);
+        when(mockCreateCmd.getSnapshotId()).thenReturn(1L);
+
+        SnapshotVO mockSnapshot = mock(SnapshotVO.class);
+        when(snapshotDao.findById(1L)).thenReturn(mockSnapshot);
+        when(mockSnapshot.getVolumeId()).thenReturn(2L);
+        when(volumeDao.findByIdIncludingRemoved(2L)).thenReturn(null);
+
+        try {
+            templateManager.createPrivateTemplateRecord(mockCreateCmd, mock(Account.class));
+            fail("Creating a Template from a Snapshot whose volume row is gone must be refused");
+        } catch (InvalidParameterValueException e) {
+            assertTrue(e.getMessage(), e.getMessage().contains("no longer exists"));
+        }
+        Mockito.verify(vmTemplateDao, Mockito.never()).persist(any(VMTemplateVO.class));
+    }
+
     @Test
     public void testLinkUserDataToTemplate() {
         LinkUserDataToTemplateCmd cmd = Mockito.mock(LinkUserDataToTemplateCmd.class);