From f7204210698f0b1ee12ec17d15020a2c0f4b6995 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Thu, 10 Sep 2026 17:07:36 +0200 Subject: [PATCH] kvm: delete primary-only RBD snapshots before the volume Fixes apache/cloudstack#12002. On Ceph/RBD primary storage, deleting a volume also destroys any of its remaining snapshots: LibvirtStorageAdaptor#deleteVol passes VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS for RBD pools, so libvirt unprotects and removes them along with the image. Run that same snapshots cleanup earlier: for a KVM + RBD volume, delete its primary-only snapshots before sending the volume delete command, while they can still be found and removed cleanly. This is the choke point shared by both the standalone DeleteVolume API and VM destroy/expunge, so it covers both affected flows. The existing post-delete cleanup in deleteVolumeCallback stays in place as a no-op safety net for anything this pass didn't find. Verified live on a KVM + Ceph environment (RBD primary storage, snapshot.backup.to.secondary=false): destroying a VM with a primary-only snapshot now deletes it cleanly. --- .../storage/volume/VolumeServiceImpl.java | 27 +++ ...lumeServiceImplRbdSnapshotCleanupTest.java | 208 ++++++++++++++++++ 2 files changed, 235 insertions(+) create mode 100644 engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceImplRbdSnapshotCleanupTest.java diff --git a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java index 58807bdc6a65..f1133d30a154 100644 --- a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java +++ b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java @@ -450,6 +450,7 @@ public AsyncCallFuture expungeVolumeAsync(VolumeInfo volume) { future.complete(result); return future; } + deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete(vol); } DeleteVolumeContext context = new DeleteVolumeContext<>(null, vo, future); @@ -561,6 +562,32 @@ protected void deleteKvmSnapshotOnPrimary(SnapshotDataStoreVO snapshotDataStoreV snapshotApiService.deleteSnapshot(snapshotDataStoreVO.getSnapshotId(), null); } + private void deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete(VolumeVO vol) { + if (!HypervisorType.KVM.equals(volDao.getHypervisorType(vol.getId()))) { + return; + } + Long poolId = vol.getPoolId(); + if (poolId == null) { + return; + } + StoragePoolVO pool = storagePoolDao.findById(poolId); + if (pool == null || !StoragePoolType.RBD.equals(pool.getPoolType())) { + return; + } + + List snapStoreVOs = _snapshotStoreDao.listAllByVolumeAndDataStore(vol.getId(), DataStoreRole.Primary); + for (SnapshotDataStoreVO snapStoreVo : snapStoreVOs) { + try { + logger.debug("Deleting snapshot [{}] before deleting volume {} from RBD storage pool [{}], as it only exists on primary storage and " + + "will otherwise be destroyed along with the volume.", snapStoreVo, vol, pool); + deleteKvmSnapshotOnPrimary(snapStoreVo); + } catch (Exception e) { + logger.warn("Failed to delete snapshot [{}] before deleting volume {} from RBD storage pool [{}]. Its database record may remain " + + "after the volume is deleted.", snapStoreVo, vol, pool, e); + } + } + } + @Override public boolean cloneVolume(long volumeId, long baseVolId) { // TODO Auto-generated method stub diff --git a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceImplRbdSnapshotCleanupTest.java b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceImplRbdSnapshotCleanupTest.java new file mode 100644 index 000000000000..e594131f8934 --- /dev/null +++ b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceImplRbdSnapshotCleanupTest.java @@ -0,0 +1,208 @@ +/* + * 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 org.apache.cloudstack.storage.volume; + +import com.cloud.hypervisor.Hypervisor.HypervisorType; +import com.cloud.storage.DataStoreRole; +import com.cloud.storage.Storage; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.storage.snapshot.SnapshotApiService; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +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.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.test.util.ReflectionTestUtils; + +/** + * Tests for {@link VolumeServiceImpl#deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete(VolumeVO)}, invoked from + * {@link VolumeServiceImpl#expungeVolumeAsync} before a Primary role volume is deleted. + */ +@RunWith(MockitoJUnitRunner.class) +public class VolumeServiceImplRbdSnapshotCleanupTest { + + private VolumeServiceImpl volumeServiceImplSpy; + + @Mock + private VolumeDao volumeDaoMock; + + @Mock + private PrimaryDataStoreDao primaryDataStoreDaoMock; + + @Mock + private SnapshotDataStoreDao snapshotDataStoreDaoMock; + + @Mock + private SnapshotApiService snapshotApiServiceMock; + + @Mock + private VolumeVO volumeVoMock; + + private static final long VOLUME_ID = 83L; + + @Before + public void setup() { + volumeServiceImplSpy = Mockito.spy(new VolumeServiceImpl()); + volumeServiceImplSpy.volDao = volumeDaoMock; + volumeServiceImplSpy.storagePoolDao = primaryDataStoreDaoMock; + volumeServiceImplSpy._snapshotStoreDao = snapshotDataStoreDaoMock; + ReflectionTestUtils.setField(volumeServiceImplSpy, "snapshotApiService", snapshotApiServiceMock); + + Mockito.doReturn(VOLUME_ID).when(volumeVoMock).getId(); + } + + private void invoke() { + ReflectionTestUtils.invokeMethod(volumeServiceImplSpy, "deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete", volumeVoMock); + } + + @Test + public void skipsWhenPoolIdIsNull() { + Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID); + Mockito.doReturn(null).when(volumeVoMock).getPoolId(); + + invoke(); + + Mockito.verify(primaryDataStoreDaoMock, Mockito.never()).findById(Mockito.anyLong()); + Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listAllByVolumeAndDataStore(Mockito.anyLong(), Mockito.any()); + } + + @Test + public void skipsWhenHypervisorIsNotKvm() { + Mockito.doReturn(HypervisorType.VMware).when(volumeDaoMock).getHypervisorType(VOLUME_ID); + + invoke(); + + Mockito.verify(primaryDataStoreDaoMock, Mockito.never()).findById(Mockito.anyLong()); + Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listAllByVolumeAndDataStore(Mockito.anyLong(), Mockito.any()); + } + + @Test + public void skipsWhenPoolIsNotRbd() { + long poolId = 5L; + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID); + Mockito.doReturn(poolId).when(volumeVoMock).getPoolId(); + Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId); + Mockito.doReturn(Storage.StoragePoolType.NetworkFilesystem).when(pool).getPoolType(); + + invoke(); + + Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listAllByVolumeAndDataStore(Mockito.anyLong(), Mockito.any()); + } + + @Test + public void skipsWhenNoSnapshotsOnPrimary() { + long poolId = 5L; + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID); + Mockito.doReturn(poolId).when(volumeVoMock).getPoolId(); + Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId); + Mockito.doReturn(Storage.StoragePoolType.RBD).when(pool).getPoolType(); + Mockito.doReturn(Collections.emptyList()).when(snapshotDataStoreDaoMock).listAllByVolumeAndDataStore(VOLUME_ID, DataStoreRole.Primary); + + invoke(); + + Mockito.verifyNoInteractions(snapshotApiServiceMock); + } + + @Test + public void deletesEachPrimaryOnlySnapshotBeforeVolumeIsDeleted() { + long poolId = 5L; + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID); + Mockito.doReturn(poolId).when(volumeVoMock).getPoolId(); + Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId); + Mockito.doReturn(Storage.StoragePoolType.RBD).when(pool).getPoolType(); + + SnapshotDataStoreVO snap1 = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.doReturn(11L).when(snap1).getSnapshotId(); + SnapshotDataStoreVO snap2 = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.doReturn(22L).when(snap2).getSnapshotId(); + List snapStoreVOs = Arrays.asList(snap1, snap2); + Mockito.doReturn(snapStoreVOs).when(snapshotDataStoreDaoMock).listAllByVolumeAndDataStore(VOLUME_ID, DataStoreRole.Primary); + + // Neither snapshot has an Image role sibling, so both should be deleted through the normal workflow. + Mockito.doReturn(Collections.singletonList(snap1)).when(snapshotDataStoreDaoMock).findBySnapshotId(11L); + Mockito.doReturn(Collections.singletonList(snap2)).when(snapshotDataStoreDaoMock).findBySnapshotId(22L); + + invoke(); + + Mockito.verify(snapshotApiServiceMock).deleteSnapshot(11L, null); + Mockito.verify(snapshotApiServiceMock).deleteSnapshot(22L, null); + } + + @Test + public void keepsGoingWhenOneSnapshotDeleteFails() { + long poolId = 5L; + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID); + Mockito.doReturn(poolId).when(volumeVoMock).getPoolId(); + Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId); + Mockito.doReturn(Storage.StoragePoolType.RBD).when(pool).getPoolType(); + + SnapshotDataStoreVO snap1 = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.doReturn(11L).when(snap1).getSnapshotId(); + SnapshotDataStoreVO snap2 = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.doReturn(22L).when(snap2).getSnapshotId(); + Mockito.doReturn(Arrays.asList(snap1, snap2)).when(snapshotDataStoreDaoMock).listAllByVolumeAndDataStore(VOLUME_ID, DataStoreRole.Primary); + Mockito.doReturn(Collections.singletonList(snap1)).when(snapshotDataStoreDaoMock).findBySnapshotId(11L); + Mockito.doReturn(Collections.singletonList(snap2)).when(snapshotDataStoreDaoMock).findBySnapshotId(22L); + Mockito.doThrow(new RuntimeException("boom")).when(snapshotApiServiceMock).deleteSnapshot(11L, null); + + invoke(); + + Mockito.verify(snapshotApiServiceMock).deleteSnapshot(11L, null); + Mockito.verify(snapshotApiServiceMock).deleteSnapshot(22L, null); + } + + @Test + public void removesOnlyTheReferenceWhenSnapshotHasAnImageCopy() { + long poolId = 5L; + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID); + Mockito.doReturn(poolId).when(volumeVoMock).getPoolId(); + Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId); + Mockito.doReturn(Storage.StoragePoolType.RBD).when(pool).getPoolType(); + + SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.doReturn(11L).when(primaryRef).getSnapshotId(); + Mockito.doReturn(99L).when(primaryRef).getId(); + SnapshotDataStoreVO imageRef = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.doReturn(DataStoreRole.Image).when(imageRef).getRole(); + + Mockito.doReturn(Collections.singletonList(primaryRef)).when(snapshotDataStoreDaoMock).listAllByVolumeAndDataStore(VOLUME_ID, DataStoreRole.Primary); + Mockito.doReturn(Arrays.asList(primaryRef, imageRef)).when(snapshotDataStoreDaoMock).findBySnapshotId(11L); + + invoke(); + + Mockito.verify(snapshotDataStoreDaoMock).remove(99L); + Mockito.verify(snapshotApiServiceMock, Mockito.never()).deleteSnapshot(Mockito.anyLong(), Mockito.any()); + } +}