From b2297152878438dafc2e8dc92c18466124a6fa92 Mon Sep 17 00:00:00 2001 From: Ramgopal Nagaboina Date: Tue, 8 Sep 2026 22:59:34 -0400 Subject: [PATCH 1/2] snapshot: reject a duplicate snapshot name for a volume Two snapshots of the same volume with the same name map to the same file on the snapshot store, so creating the second overwrites the first, and later deleting either one removes the shared file and leaves the other snapshot pointing at nothing. Reject creating a snapshot when an active (non-destroyed) snapshot with the same name already exists for the volume. Auto-generated names already carry a timestamp so they are unaffected. Fixes: #13051 --- .../storage/snapshot/SnapshotManagerImpl.java | 6 +++ .../snapshot/SnapshotManagerImplTest.java | 44 +++++++++++++++++++ 2 files changed, 50 insertions(+) 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 0d6e9de509fa..3122dda697a6 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -1721,6 +1721,12 @@ public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName, if (snapshotName == null) snapshotName = vmDisplayName + "_" + volume.getName() + "_" + timeString; + for (SnapshotVO existingSnapshot : _snapshotDao.listByStatusNotIn(volumeId, Snapshot.State.Destroyed, Snapshot.State.Error)) { + if (snapshotName.equals(existingSnapshot.getName())) { + throw new InvalidParameterValueException(String.format("A snapshot with name [%s] already exists for volume %s.", snapshotName, volume)); + } + } + HypervisorType hypervisorType = HypervisorType.None; StoragePoolVO storagePool = _storagePoolDao.findById(volume.getDataStore().getId()); if (storagePool.getScope() == ScopeType.ZONE) { 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 32b103f39d3f..f5ad35cfb806 100644 --- a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java @@ -28,8 +28,13 @@ import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotResult; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService; import org.apache.cloudstack.framework.async.AsyncCallFuture; +import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory; +import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -45,10 +50,12 @@ import com.cloud.dc.dao.DataCenterDao; import com.cloud.event.ActionEventUtils; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceUnavailableException; import com.cloud.org.Grouping; import com.cloud.storage.DataStoreRole; +import com.cloud.storage.ScopeType; import com.cloud.storage.Snapshot; import com.cloud.storage.SnapshotVO; import com.cloud.storage.VolumeVO; @@ -59,8 +66,10 @@ import com.cloud.user.AccountManager; import com.cloud.user.AccountVO; import com.cloud.user.ResourceLimitService; +import com.cloud.user.User; import com.cloud.user.dao.AccountDao; import com.cloud.utils.Pair; +import com.cloud.vm.dao.UserVmDao; @RunWith(MockitoJUnitRunner.class) public class SnapshotManagerImplTest { @@ -86,9 +95,44 @@ public class SnapshotManagerImplTest { SnapshotZoneDao snapshotZoneDao; @Mock VolumeDao volumeDao; + @Mock + PrimaryDataStoreDao primaryDataStoreDao; + @Mock + VolumeDataFactory volFactory; + @Mock + UserVmDao userVmDao; @InjectMocks SnapshotManagerImpl snapshotManager = new SnapshotManagerImpl(); + @Test + public void testAllocSnapshotRejectsDuplicateNameForVolume() { + long volumeId = 1L; + CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class)); + try { + VolumeInfo volume = Mockito.mock(VolumeInfo.class); + Mockito.when(volFactory.getVolume(volumeId)).thenReturn(volume); + DataStore dataStore = Mockito.mock(DataStore.class); + Mockito.when(volume.getDataStore()).thenReturn(dataStore); + Mockito.when(dataStore.getId()).thenReturn(10L); + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.when(primaryDataStoreDao.findById(10L)).thenReturn(pool); + Mockito.when(pool.getScope()).thenReturn(ScopeType.ZONE); + Mockito.when(pool.getHypervisor()).thenReturn(HypervisorType.None); + Mockito.when(volume.getInstanceId()).thenReturn(null); + Mockito.when(volume.getAccountId()).thenReturn(2L); + + SnapshotVO existing = Mockito.mock(SnapshotVO.class); + Mockito.when(existing.getName()).thenReturn("dup"); + Mockito.when(snapshotDao.listByStatusNotIn(volumeId, Snapshot.State.Destroyed, Snapshot.State.Error)) + .thenReturn(List.of(existing)); + + Assert.assertThrows(InvalidParameterValueException.class, () -> + snapshotManager.allocSnapshot(volumeId, Snapshot.MANUAL_POLICY_ID, "dup", null)); + } finally { + CallContext.unregister(); + } + } + @Test public void testGetSnapshotZoneImageStoreValid() { final long snapshotId = 1L; From ca6075ea8aef8e5c1b0069833970239b0e327d37 Mon Sep 17 00:00:00 2001 From: Ramgopal Nagaboina Date: Thu, 10 Sep 2026 16:12:46 -0400 Subject: [PATCH 2/2] snapshot: look up a duplicate snapshot name by volume and name Query for an active snapshot with the given name on the volume instead of listing every snapshot of the volume and comparing names, as asked in review. --- .../java/com/cloud/storage/dao/SnapshotDao.java | 2 ++ .../com/cloud/storage/dao/SnapshotDaoImpl.java | 16 ++++++++++++++++ .../storage/snapshot/SnapshotManagerImpl.java | 6 ++---- .../snapshot/SnapshotManagerImplTest.java | 6 ++---- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java b/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java index 737e4a5a53ca..2659f41d5620 100755 --- a/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java +++ b/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java @@ -53,6 +53,8 @@ public interface SnapshotDao extends GenericDao, StateDao listByStatusNotIn(long volumeId, Snapshot.State... status); + SnapshotVO findByVolumeIdAndNameNotInStatus(long volumeId, String name, Snapshot.State... status); + /** * Retrieves a list of snapshots filtered by ids. * @param ids Snapshot ids. diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java b/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java index 238ae54e07f3..0f6576e996a8 100755 --- a/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java @@ -65,6 +65,7 @@ public class SnapshotDaoImpl extends GenericDaoBase implements private SearchBuilder InstanceIdSearch; private SearchBuilder StatusSearch; private SearchBuilder notInStatusSearch; + private SearchBuilder volumeIdNameNotInStatusSearch; private GenericSearchBuilder CountSnapshotsByAccount; @Inject ResourceTagDao _tagsDao; @@ -159,6 +160,12 @@ protected void init() { notInStatusSearch.and("status", notInStatusSearch.entity().getState(), SearchCriteria.Op.NOTIN); notInStatusSearch.done(); + volumeIdNameNotInStatusSearch = createSearchBuilder(); + volumeIdNameNotInStatusSearch.and("volumeId", volumeIdNameNotInStatusSearch.entity().getVolumeId(), SearchCriteria.Op.EQ); + volumeIdNameNotInStatusSearch.and("name", volumeIdNameNotInStatusSearch.entity().getName(), SearchCriteria.Op.EQ); + volumeIdNameNotInStatusSearch.and("status", volumeIdNameNotInStatusSearch.entity().getState(), SearchCriteria.Op.NOTIN); + volumeIdNameNotInStatusSearch.done(); + CountSnapshotsByAccount = createSearchBuilder(Long.class); CountSnapshotsByAccount.select(null, Func.COUNT, null); CountSnapshotsByAccount.and("account", CountSnapshotsByAccount.entity().getAccountId(), SearchCriteria.Op.EQ); @@ -295,6 +302,15 @@ public List listByStatusNotIn(long volumeId, Snapshot.State... statu return listBy(sc, null); } + @Override + public SnapshotVO findByVolumeIdAndNameNotInStatus(long volumeId, String name, Snapshot.State... status) { + SearchCriteria sc = volumeIdNameNotInStatusSearch.create(); + sc.setParameters("volumeId", volumeId); + sc.setParameters("name", name); + sc.setParameters("status", (Object[]) status); + return findOneBy(sc); + } + @Override public List searchByVolumes(List volumeIds) { if (CollectionUtils.isEmpty(volumeIds)) { 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 3122dda697a6..ed1af687c563 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -1721,10 +1721,8 @@ public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName, if (snapshotName == null) snapshotName = vmDisplayName + "_" + volume.getName() + "_" + timeString; - for (SnapshotVO existingSnapshot : _snapshotDao.listByStatusNotIn(volumeId, Snapshot.State.Destroyed, Snapshot.State.Error)) { - if (snapshotName.equals(existingSnapshot.getName())) { - throw new InvalidParameterValueException(String.format("A snapshot with name [%s] already exists for volume %s.", snapshotName, volume)); - } + if (_snapshotDao.findByVolumeIdAndNameNotInStatus(volumeId, snapshotName, Snapshot.State.Destroyed, Snapshot.State.Error) != null) { + throw new InvalidParameterValueException(String.format("A snapshot with name [%s] already exists for volume %s.", snapshotName, volume)); } HypervisorType hypervisorType = HypervisorType.None; 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 f5ad35cfb806..c8eb9cbc0e5e 100644 --- a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java @@ -121,10 +121,8 @@ public void testAllocSnapshotRejectsDuplicateNameForVolume() { Mockito.when(volume.getInstanceId()).thenReturn(null); Mockito.when(volume.getAccountId()).thenReturn(2L); - SnapshotVO existing = Mockito.mock(SnapshotVO.class); - Mockito.when(existing.getName()).thenReturn("dup"); - Mockito.when(snapshotDao.listByStatusNotIn(volumeId, Snapshot.State.Destroyed, Snapshot.State.Error)) - .thenReturn(List.of(existing)); + Mockito.when(snapshotDao.findByVolumeIdAndNameNotInStatus(volumeId, "dup", Snapshot.State.Destroyed, Snapshot.State.Error)) + .thenReturn(Mockito.mock(SnapshotVO.class)); Assert.assertThrows(InvalidParameterValueException.class, () -> snapshotManager.allocSnapshot(volumeId, Snapshot.MANUAL_POLICY_ID, "dup", null));