This is an automated email from the ASF dual-hosted git repository.
DaanHoogland pushed a commit to branch 4.20
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.20 by this push:
new e742aadfc97 Reject creating a snapshot with a duplicate name for the
same volume (#14089)
e742aadfc97 is described below
commit e742aadfc97e84b65f4b24052f57f5ee0bd2c849
Author: The Kernel Nomad <[email protected]>
AuthorDate: Thu Oct 22 03:48:52 2026 -0400
Reject creating a snapshot with a duplicate name for the same volume
(#14089)
---
.../java/com/cloud/storage/dao/SnapshotDao.java | 2 +
.../com/cloud/storage/dao/SnapshotDaoImpl.java | 16 +++++
.../com/cloud/storage/dao/SnapshotDaoImplTest.java | 80 ++++++++++++++++++++++
.../storage/snapshot/SnapshotManagerImpl.java | 5 ++
.../storage/snapshot/SnapshotManagerImplTest.java | 78 +++++++++++++++++++++
5 files changed, 181 insertions(+)
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 737e4a5a53c..2659f41d562 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<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.
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 238ae54e07f..0f6576e996a 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<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 @@ public class SnapshotDaoImpl extends
GenericDaoBase<SnapshotVO, Long> implements
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 class SnapshotDaoImpl extends
GenericDaoBase<SnapshotVO, Long> implements
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)) {
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 00000000000..f4609bcc340
--- /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<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);
+ }
+}
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 0d6e9de509f..ce3cee8480a 100755
--- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
+++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
@@ -1718,9 +1718,14 @@ public class SnapshotManagerImpl extends
MutualExclusiveIdsManagerBase implement
if (vmInstance != null) {
vmDisplayName = vmInstance.getHostName();
}
+ boolean nameProvided = snapshotName != null;
if (snapshotName == null)
snapshotName = vmDisplayName + "_" + volume.getName() + "_" +
timeString;
+ if (nameProvided &&
_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 32b103f39d3..d99c67e2cf6 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.SnapshotInfo;
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.DataCenterVO;
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.Account;
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,78 @@ 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 testAllocSnapshotDoesNotCheckDuplicateForAutoGeneratedName() {
+ 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.lenient().when(dataStore.getId()).thenReturn(10L);
+ StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
+
Mockito.lenient().when(primaryDataStoreDao.findById(10L)).thenReturn(pool);
+ Mockito.lenient().when(pool.getScope()).thenReturn(ScopeType.ZONE);
+
Mockito.lenient().when(pool.getHypervisor()).thenReturn(HypervisorType.None);
+ Mockito.when(volume.getInstanceId()).thenReturn(null);
+ Mockito.when(volume.getAccountId()).thenReturn(2L);
+
+ // A recurring snapshot carries no caller-supplied name, so the
auto-generated name must not be checked.
+
Mockito.lenient().when(snapshotDao.findByVolumeIdAndNameNotInStatus(Mockito.eq(volumeId),
Mockito.anyString(),
+ Mockito.eq(Snapshot.State.Destroyed),
Mockito.eq(Snapshot.State.Error)))
+ .thenReturn(Mockito.mock(SnapshotVO.class));
+
+ try {
+ snapshotManager.allocSnapshot(volumeId,
Snapshot.MANUAL_POLICY_ID, null, null);
+ } catch (Exception e) {
+ // The rest of allocSnapshot is not stubbed here; this test
only asserts the duplicate-name guard is
+ // not applied to an auto-generated name.
+ }
+
+ Mockito.verify(snapshotDao,
Mockito.never()).findByVolumeIdAndNameNotInStatus(Mockito.eq(volumeId),
+ Mockito.anyString(), Mockito.eq(Snapshot.State.Destroyed),
Mockito.eq(Snapshot.State.Error));
+ } finally {
+ CallContext.unregister();
+ }
+ }
+
@Test
public void testGetSnapshotZoneImageStoreValid() {
final long snapshotId = 1L;
