Skip to content

Commit 77515d1

Browse files
Damans227Damans227
committed
Clean up primary-only snapshot in StorageManagerImpl scavenger instead
1 parent e580af8 commit 77515d1

4 files changed

Lines changed: 95 additions & 92 deletions

File tree

‎server/src/main/java/com/cloud/storage/StorageManagerImpl.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2145,6 +2145,7 @@ public void cleanupStorage(boolean recurring) {
21452145
}
21462146

21472147
try {
2148+
cleanupSnapshotRecordsInPrimaryStorageOnly(vol);
21482149
VolumeInfo volumeInfo = volFactory.getVolume(vol.getId());
21492150
if (volumeInfo != null) {
21502151
volService.ensureVolumeIsExpungeReady(vol.getId());
@@ -2292,6 +2293,34 @@ private void deleteSnapshot(SnapshotVO snapshot, SnapshotDataStoreVO snapshotDat
22922293
}
22932294
}
22942295

2296+
/**
2297+
* Cleans up snapshot DB records for snapshots that exist only on primary storage (no secondary copy).
2298+
* This handles the case where snapshot.backup.to.secondary=false and the volume
2299+
* is being deleted during VM expunge — the RBD snapshots will be destroyed along with the image,
2300+
* so the DB records need to be cleaned up to avoid orphaned entries.
2301+
*/
2302+
protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) {
2303+
List<SnapshotVO> snapshots = _snapshotDao.listByVolumeId(volume.getId());
2304+
if (CollectionUtils.isEmpty(snapshots)) {
2305+
return;
2306+
}
2307+
for (SnapshotVO snapshot : snapshots) {
2308+
if (Snapshot.State.Destroyed.equals(snapshot.getState())) {
2309+
continue;
2310+
}
2311+
List<SnapshotDataStoreVO> primaryRefs = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary);
2312+
List<SnapshotDataStoreVO> secondaryRefs = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Image);
2313+
if (CollectionUtils.isNotEmpty(primaryRefs) && CollectionUtils.isEmpty(secondaryRefs)) {
2314+
logger.info("Cleaning up snapshot {} (primary-only, no secondary copy) as volume {} is being deleted", snapshot, volume);
2315+
for (SnapshotDataStoreVO ref : primaryRefs) {
2316+
_snapshotStoreDao.expunge(ref.getId());
2317+
}
2318+
snapshot.setState(Snapshot.State.Destroyed);
2319+
_snapshotDao.update(snapshot.getId(), snapshot);
2320+
}
2321+
}
2322+
}
2323+
22952324
private void removeSnapshotsInErrorStatus() {
22962325
List<SnapshotVO> snapshotsInErrorStatus = _snapshotDao.listAllByStatusIncludingRemoved(Snapshot.State.Error);
22972326
for (SnapshotVO snapshotVO : snapshotsInErrorStatus) {

‎server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java‎

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1640,7 +1640,6 @@ public boolean deleteVolume(long volumeId, Account caller) throws ConcurrentOper
16401640

16411641
private boolean deleteVolumeFromStorage(VolumeVO volume, Account caller) throws ConcurrentOperationException {
16421642
try {
1643-
cleanupSnapshotRecordsInPrimaryStorageOnly(volume);
16441643
expungeVolumesInPrimaryStorageIfNeeded(volume);
16451644
expungeVolumesInSecondaryStorageIfNeeded(volume);
16461645
cleanVolumesCache(volume);
@@ -1651,34 +1650,6 @@ private boolean deleteVolumeFromStorage(VolumeVO volume, Account caller) throws
16511650
}
16521651
}
16531652

1654-
/**
1655-
* Cleans up snapshot DB records for snapshots that exist only on primary storage (no secondary copy).
1656-
* This handles the case where snapshot.backup.to.secondary=false and the volume
1657-
* is being deleted during VM expunge; the RBD snapshots will be destroyed along with the image,
1658-
* so the DB records need to be cleaned up to avoid orphaned entries.
1659-
*/
1660-
protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) {
1661-
List<SnapshotVO> snapshots = _snapshotDao.listByVolumeId(volume.getId());
1662-
if (CollectionUtils.isEmpty(snapshots)) {
1663-
return;
1664-
}
1665-
for (SnapshotVO snapshot : snapshots) {
1666-
if (Snapshot.State.Destroyed.equals(snapshot.getState())) {
1667-
continue;
1668-
}
1669-
List<SnapshotDataStoreVO> primaryRefs = _snapshotDataStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary);
1670-
List<SnapshotDataStoreVO> secondaryRefs = _snapshotDataStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Image);
1671-
if (CollectionUtils.isNotEmpty(primaryRefs) && CollectionUtils.isEmpty(secondaryRefs)) {
1672-
logger.info("Cleaning up snapshot {} (primary-only, no secondary copy) as volume {} is being deleted", snapshot, volume);
1673-
for (SnapshotDataStoreVO ref : primaryRefs) {
1674-
_snapshotDataStoreDao.expunge(ref.getId());
1675-
}
1676-
snapshot.setState(Snapshot.State.Destroyed);
1677-
_snapshotDao.update(snapshot.getId(), snapshot);
1678-
}
1679-
}
1680-
}
1681-
16821653
/**
16831654
* Retrieves and validates the volume for the {@link #deleteVolume(long, Account)} method. The following validation are executed.
16841655
* <ul>

‎server/src/test/java/com/cloud/storage/StorageManagerImplTest.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import java.lang.reflect.Field;
2020
import java.util.ArrayList;
21+
import java.util.Collections;
2122
import java.util.Arrays;
2223
import java.util.HashMap;
2324
import java.util.List;
@@ -47,6 +48,8 @@
4748
import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao;
4849
import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO;
4950
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
51+
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
52+
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
5053
import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailVO;
5154
import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
5255
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
@@ -85,6 +88,7 @@
8588
import com.cloud.host.Host;
8689
import com.cloud.hypervisor.Hypervisor.HypervisorType;
8790
import com.cloud.hypervisor.HypervisorGuruManager;
91+
import com.cloud.storage.dao.SnapshotDao;
8892
import com.cloud.storage.dao.VolumeDao;
8993
import com.cloud.user.AccountManagerImpl;
9094
import com.cloud.utils.Pair;
@@ -129,6 +133,10 @@ public class StorageManagerImplTest {
129133
AccountManagerImpl accountMgr;
130134
@Mock
131135
StoragePoolDetailsDao storagePoolDetailsDao;
136+
@Mock
137+
SnapshotDao snapshotDao;
138+
@Mock
139+
SnapshotDataStoreDao snapshotStoreDao;
132140

133141
@Mock
134142
ClusterDao clusterDao;
@@ -1716,4 +1724,62 @@ public void testDiscoverObjectStoreInitializationFailure() {
17161724

17171725
storageManagerImpl.discoverObjectStore(name, url, size, providerName, details);
17181726
}
1727+
1728+
@Test
1729+
public void testCleanupSnapshotRecordsInPrimaryStorageOnly() {
1730+
VolumeVO volume = Mockito.mock(VolumeVO.class);
1731+
Mockito.when(volume.getId()).thenReturn(1L);
1732+
1733+
SnapshotVO snapshot = Mockito.mock(SnapshotVO.class);
1734+
Mockito.when(snapshot.getId()).thenReturn(10L);
1735+
Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp);
1736+
Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot));
1737+
1738+
SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class);
1739+
Mockito.when(primaryRef.getId()).thenReturn(100L);
1740+
Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef));
1741+
Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(Collections.emptyList());
1742+
1743+
storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume);
1744+
1745+
Mockito.verify(snapshotStoreDao).expunge(100L);
1746+
Mockito.verify(snapshot).setState(Snapshot.State.Destroyed);
1747+
Mockito.verify(snapshotDao).update(10L, snapshot);
1748+
}
1749+
1750+
@Test
1751+
public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsWhenSecondaryExists() {
1752+
VolumeVO volume = Mockito.mock(VolumeVO.class);
1753+
Mockito.when(volume.getId()).thenReturn(1L);
1754+
1755+
SnapshotVO snapshot = Mockito.mock(SnapshotVO.class);
1756+
Mockito.when(snapshot.getId()).thenReturn(10L);
1757+
Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp);
1758+
Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot));
1759+
1760+
SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class);
1761+
SnapshotDataStoreVO secondaryRef = Mockito.mock(SnapshotDataStoreVO.class);
1762+
Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef));
1763+
Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(List.of(secondaryRef));
1764+
1765+
storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume);
1766+
1767+
Mockito.verify(snapshotStoreDao, Mockito.never()).expunge(Mockito.anyLong());
1768+
Mockito.verify(snapshotDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any(SnapshotVO.class));
1769+
}
1770+
1771+
@Test
1772+
public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsDestroyedSnapshots() {
1773+
VolumeVO volume = Mockito.mock(VolumeVO.class);
1774+
Mockito.when(volume.getId()).thenReturn(1L);
1775+
1776+
SnapshotVO snapshot = Mockito.mock(SnapshotVO.class);
1777+
Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.Destroyed);
1778+
Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot));
1779+
1780+
storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume);
1781+
1782+
Mockito.verify(snapshotStoreDao, Mockito.never()).listBySnapshotAndDataStoreRole(Mockito.anyLong(), Mockito.any());
1783+
Mockito.verify(snapshotStoreDao, Mockito.never()).expunge(Mockito.anyLong());
1784+
}
17191785
}

‎server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java‎

Lines changed: 0 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,6 @@
122122
import com.cloud.storage.Volume.Type;
123123
import com.cloud.storage.dao.DiskOfferingDao;
124124
import com.cloud.storage.dao.SnapshotDao;
125-
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
126-
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
127125
import com.cloud.storage.dao.StoragePoolTagsDao;
128126
import com.cloud.storage.dao.VMTemplateDao;
129127
import com.cloud.storage.dao.VolumeDao;
@@ -255,9 +253,6 @@ public class VolumeApiServiceImplTest {
255253
@Mock
256254
private SnapshotDao snapshotDaoMock;
257255

258-
@Mock
259-
private SnapshotDataStoreDao snapshotDataStoreDaoMock;
260-
261256
@Mock
262257
private SnapshotPolicyDetailsDao snapshotPolicyDetailsDao;
263258

@@ -2294,62 +2289,4 @@ private List<VMSnapshotVO> generateVmSnapshotVoList(VMSnapshot.Type t1, VMSnapsh
22942289
return List.of(mock1, mock2);
22952290
}
22962291

2297-
@Test
2298-
public void testCleanupSnapshotRecordsInPrimaryStorageOnly() {
2299-
VolumeVO volume = Mockito.mock(VolumeVO.class);
2300-
Mockito.when(volume.getId()).thenReturn(1L);
2301-
2302-
SnapshotVO snapshot = Mockito.mock(SnapshotVO.class);
2303-
Mockito.when(snapshot.getId()).thenReturn(10L);
2304-
Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp);
2305-
Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot));
2306-
2307-
SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class);
2308-
Mockito.when(primaryRef.getId()).thenReturn(100L);
2309-
Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef));
2310-
Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(Collections.emptyList());
2311-
2312-
volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume);
2313-
2314-
Mockito.verify(snapshotDataStoreDaoMock).expunge(100L);
2315-
Mockito.verify(snapshot).setState(Snapshot.State.Destroyed);
2316-
Mockito.verify(snapshotDaoMock).update(10L, snapshot);
2317-
}
2318-
2319-
@Test
2320-
public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsWhenSecondaryExists() {
2321-
VolumeVO volume = Mockito.mock(VolumeVO.class);
2322-
Mockito.when(volume.getId()).thenReturn(1L);
2323-
2324-
SnapshotVO snapshot = Mockito.mock(SnapshotVO.class);
2325-
Mockito.when(snapshot.getId()).thenReturn(10L);
2326-
Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp);
2327-
Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot));
2328-
2329-
SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class);
2330-
SnapshotDataStoreVO secondaryRef = Mockito.mock(SnapshotDataStoreVO.class);
2331-
Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef));
2332-
Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(List.of(secondaryRef));
2333-
2334-
volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume);
2335-
2336-
Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).expunge(Mockito.anyLong());
2337-
Mockito.verify(snapshotDaoMock, Mockito.never()).update(Mockito.anyLong(), Mockito.any(SnapshotVO.class));
2338-
}
2339-
2340-
@Test
2341-
public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsDestroyedSnapshots() {
2342-
VolumeVO volume = Mockito.mock(VolumeVO.class);
2343-
Mockito.when(volume.getId()).thenReturn(1L);
2344-
2345-
SnapshotVO snapshot = Mockito.mock(SnapshotVO.class);
2346-
Mockito.when(snapshot.getId()).thenReturn(10L);
2347-
Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.Destroyed);
2348-
Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot));
2349-
2350-
volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume);
2351-
2352-
Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listBySnapshotAndDataStoreRole(Mockito.anyLong(), Mockito.any());
2353-
Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).expunge(Mockito.anyLong());
2354-
}
23552292
}

0 commit comments

Comments
 (0)