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

Author: rootnomad

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

@@ -2042,6 +2042,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) {

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

@@ -24,7 +24,9 @@
 import com.cloud.exception.PermissionDeniedException;
 import com.cloud.exception.ResourceUnavailableException;
 import com.cloud.org.Grouping;
+import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.ScopeType;
 import com.cloud.storage.Snapshot;
 import com.cloud.storage.Storage.StoragePoolType;
 import com.cloud.storage.SnapshotPolicyVO;
@@ -41,6 +43,7 @@
 import com.cloud.user.ResourceLimitService;
 import com.cloud.user.User;
 import com.cloud.user.dao.AccountDao;
+import com.cloud.vm.dao.UserVmDao;
 import com.cloud.utils.Pair;
 
 import com.cloud.utils.db.SearchBuilder;
@@ -56,6 +59,8 @@
 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.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;
@@ -110,6 +115,10 @@ public class SnapshotManagerImplTest {
     TaggedResourceService taggedResourceService;
     @Mock
     PrimaryDataStoreDao primaryDataStoreDao;
+    @Mock
+    VolumeDataFactory volFactory;
+    @Mock
+    UserVmDao userVmDao;
     @InjectMocks
     SnapshotManagerImpl snapshotManager = new SnapshotManagerImpl();
 
@@ -669,4 +678,29 @@ public void testRemoveClvmPrimarySnapshotStoreRefIfNeeded_NonClvmPool() {
         Mockito.verify(snapshotStoreDao, Mockito.never()).removeBySnapshotStore(
                 Mockito.anyLong(), Mockito.anyLong(), Mockito.any());
     }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testAllocSnapshotRejectsDuplicateNameForVolume() throws Exception {
+        long volumeId = 1L;
+        CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
+
+        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));
+
+        snapshotManager.allocSnapshot(volumeId, Snapshot.MANUAL_POLICY_ID, "dup", null);
+    }
 }