diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 9b2f4f831d3e..7e07f34db0da 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/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) { diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index e7ba9d87febd..f9da9a3a39e4 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/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 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)."); diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 13c47e83bfc5..a7295b26b4fe 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/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"); } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index b806eb4afe5c..2aafac535ac2 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/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); diff --git a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java b/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java index d406753a95ac..7f82387c0eeb 100644 --- a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java +++ b/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 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); diff --git a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerTest.java b/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerTest.java index 8eee0ed66261..87bd5d8b5350 100755 --- a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerTest.java +++ b/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() { diff --git a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java index b1135e07562d..765edf9d23d5 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/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 mockCheckedReservation = Mockito.mockConstruction(CheckedReservation.class)) { + try (MockedConstruction mockCheckedReservation = Mockito.mockConstruction(CheckedReservation.class); + MockedStatic 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);