Reject creating a snapshot with a duplicate name for the same volume

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

@@ -53,6 +53,8 @@ public interface SnapshotDao extends GenericDao<SnapshotVO, Long>, StateDao<Snap
 
     List<SnapshotVO> 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.

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

@@ -65,6 +65,7 @@ public class SnapshotDaoImpl extends GenericDaoBase<SnapshotVO, Long> implements
     private SearchBuilder<SnapshotVO> InstanceIdSearch;
     private SearchBuilder<SnapshotVO> StatusSearch;
     private SearchBuilder<SnapshotVO> notInStatusSearch;
+    private SearchBuilder<SnapshotVO> volumeIdNameNotInStatusSearch;
     private GenericSearchBuilder<SnapshotVO, Long> 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<SnapshotVO> listByStatusNotIn(long volumeId, Snapshot.State... statu
         return listBy(sc, null);
     }
 
+    @Override
+    public SnapshotVO findByVolumeIdAndNameNotInStatus(long volumeId, String name, Snapshot.State... status) {
+        SearchCriteria<SnapshotVO> sc = volumeIdNameNotInStatusSearch.create();
+        sc.setParameters("volumeId", volumeId);
+        sc.setParameters("name", name);
+        sc.setParameters("status", (Object[]) status);
+        return findOneBy(sc);
+    }
+
     @Override
     public List<SnapshotVO> searchByVolumes(List<Long> volumeIds) {
         if (CollectionUtils.isEmpty(volumeIds)) {

engine/schema/src/test/java/com/cloud/storage/dao/SnapshotDaoImplTest.java

@@ -0,0 +1,80 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.storage.dao;
+
+import java.lang.reflect.Field;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.storage.Snapshot;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SnapshotDaoImplTest {
+
+    @Mock
+    SearchBuilder<SnapshotVO> volumeIdNameNotInStatusSearchMock;
+
+    @Mock
+    SearchCriteria<SnapshotVO> searchCriteriaMock;
+
+    @Spy
+    SnapshotDaoImpl snapshotDao = new SnapshotDaoImpl();
+
+    @Before
+    public void setUp() throws Exception {
+        // init() wires up several search builders that join other DAOs, which are not available in a
+        // unit test; inject only the search builder this method uses and stub it to return a criteria.
+        Field searchField = SnapshotDaoImpl.class.getDeclaredField("volumeIdNameNotInStatusSearch");
+        searchField.setAccessible(true);
+        searchField.set(snapshotDao, volumeIdNameNotInStatusSearchMock);
+        Mockito.lenient().doReturn(searchCriteriaMock).when(volumeIdNameNotInStatusSearchMock).create();
+    }
+
+    @Test
+    public void testFindByVolumeIdAndNameNotInStatusReturnsTheMatch() {
+        SnapshotVO expected = Mockito.mock(SnapshotVO.class);
+        Mockito.doReturn(expected).when(snapshotDao).findOneBy(Mockito.any(SearchCriteria.class));
+
+        SnapshotVO result = snapshotDao.findByVolumeIdAndNameNotInStatus(5L, "snap-1", Snapshot.State.Destroyed);
+
+        Assert.assertSame(expected, result);
+        Mockito.verify(searchCriteriaMock).setParameters("volumeId", 5L);
+        Mockito.verify(searchCriteriaMock).setParameters("name", "snap-1");
+        Mockito.verify(snapshotDao).findOneBy(searchCriteriaMock);
+    }
+
+    @Test
+    public void testFindByVolumeIdAndNameNotInStatusReturnsNullWhenNoMatch() {
+        Mockito.doReturn(null).when(snapshotDao).findOneBy(Mockito.any(SearchCriteria.class));
+
+        SnapshotVO result = snapshotDao.findByVolumeIdAndNameNotInStatus(5L, "snap-1");
+
+        Assert.assertNull(result);
+        Mockito.verify(searchCriteriaMock).setParameters("volumeId", 5L);
+        Mockito.verify(snapshotDao).findOneBy(searchCriteriaMock);
+    }
+}

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

@@ -1721,6 +1721,10 @@ public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName,
         if (snapshotName == null)
             snapshotName = vmDisplayName + "_" + volume.getName() + "_" + timeString;
 
+        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;
         StoragePoolVO storagePool = _storagePoolDao.findById(volume.getDataStore().getId());
         if (storagePool.getScope() == ScopeType.ZONE) {

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,42 @@ 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);
+
+            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));
+        } finally {
+            CallContext.unregister();
+        }
+    }
+
     @Test
     public void testGetSnapshotZoneImageStoreValid() {
         final long snapshotId = 1L;