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/engine/schema/src/test/java/com/cloud/storage/dao/SnapshotDaoImplTest.java b/engine/schema/src/test/java/com/cloud/storage/dao/SnapshotDaoImplTest.java new file mode 100644 index 000000000000..f4609bcc3403 --- /dev/null +++ b/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 volumeIdNameNotInStatusSearchMock; + + @Mock + SearchCriteria 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); + } +} 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..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,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) { 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..c8eb9cbc0e5e 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,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;