Skip to content

Commit 19944a4

Browse files
weizhouapacheweizhouapache
committed
kvm: delete primary-only RBD snapshots before the volume
Fixes apache#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.
1 parent b9a5977 commit 19944a4

2 files changed

Lines changed: 235 additions & 0 deletions

File tree

‎engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,7 @@ public AsyncCallFuture<VolumeApiResult> expungeVolumeAsync(VolumeInfo volume) {
450450
future.complete(result);
451451
return future;
452452
}
453+
deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete(vol);
453454
}
454455

455456
DeleteVolumeContext<VolumeApiResult> context = new DeleteVolumeContext<>(null, vo, future);
@@ -561,6 +562,32 @@ protected void deleteKvmSnapshotOnPrimary(SnapshotDataStoreVO snapshotDataStoreV
561562
snapshotApiService.deleteSnapshot(snapshotDataStoreVO.getSnapshotId(), null);
562563
}
563564

565+
private void deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete(VolumeVO vol) {
566+
if (!HypervisorType.KVM.equals(volDao.getHypervisorType(vol.getId()))) {
567+
return;
568+
}
569+
Long poolId = vol.getPoolId();
570+
if (poolId == null) {
571+
return;
572+
}
573+
StoragePoolVO pool = storagePoolDao.findById(poolId);
574+
if (pool == null || !StoragePoolType.RBD.equals(pool.getPoolType())) {
575+
return;
576+
}
577+
578+
List<SnapshotDataStoreVO> snapStoreVOs = _snapshotStoreDao.listAllByVolumeAndDataStore(vol.getId(), DataStoreRole.Primary);
579+
for (SnapshotDataStoreVO snapStoreVo : snapStoreVOs) {
580+
try {
581+
logger.debug("Deleting snapshot [{}] before deleting volume {} from RBD storage pool [{}], as it only exists on primary storage and " +
582+
"will otherwise be destroyed along with the volume.", snapStoreVo, vol, pool);
583+
deleteKvmSnapshotOnPrimary(snapStoreVo);
584+
} catch (Exception e) {
585+
logger.warn("Failed to delete snapshot [{}] before deleting volume {} from RBD storage pool [{}]. Its database record may remain " +
586+
"after the volume is deleted.", snapStoreVo, vol, pool, e);
587+
}
588+
}
589+
}
590+
564591
@Override
565592
public boolean cloneVolume(long volumeId, long baseVolId) {
566593
// TODO Auto-generated method stub

‎engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceImplRbdSnapshotCleanupTest.java‎

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.cloudstack.storage.volume;
20+
21+
import com.cloud.hypervisor.Hypervisor.HypervisorType;
22+
import com.cloud.storage.DataStoreRole;
23+
import com.cloud.storage.Storage;
24+
import com.cloud.storage.VolumeVO;
25+
import com.cloud.storage.dao.VolumeDao;
26+
import com.cloud.storage.snapshot.SnapshotApiService;
27+
28+
import java.util.Arrays;
29+
import java.util.Collections;
30+
import java.util.List;
31+
32+
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
33+
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
34+
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
35+
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
36+
import org.junit.Before;
37+
import org.junit.Test;
38+
import org.junit.runner.RunWith;
39+
import org.mockito.Mock;
40+
import org.mockito.Mockito;
41+
import org.mockito.junit.MockitoJUnitRunner;
42+
import org.springframework.test.util.ReflectionTestUtils;
43+
44+
/**
45+
* Tests for {@link VolumeServiceImpl#deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete(VolumeVO)}, invoked from
46+
* {@link VolumeServiceImpl#expungeVolumeAsync} before a Primary role volume is deleted.
47+
*/
48+
@RunWith(MockitoJUnitRunner.class)
49+
public class VolumeServiceImplRbdSnapshotCleanupTest {
50+
51+
private VolumeServiceImpl volumeServiceImplSpy;
52+
53+
@Mock
54+
private VolumeDao volumeDaoMock;
55+
56+
@Mock
57+
private PrimaryDataStoreDao primaryDataStoreDaoMock;
58+
59+
@Mock
60+
private SnapshotDataStoreDao snapshotDataStoreDaoMock;
61+
62+
@Mock
63+
private SnapshotApiService snapshotApiServiceMock;
64+
65+
@Mock
66+
private VolumeVO volumeVoMock;
67+
68+
private static final long VOLUME_ID = 83L;
69+
70+
@Before
71+
public void setup() {
72+
volumeServiceImplSpy = Mockito.spy(new VolumeServiceImpl());
73+
volumeServiceImplSpy.volDao = volumeDaoMock;
74+
volumeServiceImplSpy.storagePoolDao = primaryDataStoreDaoMock;
75+
volumeServiceImplSpy._snapshotStoreDao = snapshotDataStoreDaoMock;
76+
ReflectionTestUtils.setField(volumeServiceImplSpy, "snapshotApiService", snapshotApiServiceMock);
77+
78+
Mockito.doReturn(VOLUME_ID).when(volumeVoMock).getId();
79+
}
80+
81+
private void invoke() {
82+
ReflectionTestUtils.invokeMethod(volumeServiceImplSpy, "deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete", volumeVoMock);
83+
}
84+
85+
@Test
86+
public void skipsWhenPoolIdIsNull() {
87+
Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID);
88+
Mockito.doReturn(null).when(volumeVoMock).getPoolId();
89+
90+
invoke();
91+
92+
Mockito.verify(primaryDataStoreDaoMock, Mockito.never()).findById(Mockito.anyLong());
93+
Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listAllByVolumeAndDataStore(Mockito.anyLong(), Mockito.any());
94+
}
95+
96+
@Test
97+
public void skipsWhenHypervisorIsNotKvm() {
98+
Mockito.doReturn(HypervisorType.VMware).when(volumeDaoMock).getHypervisorType(VOLUME_ID);
99+
100+
invoke();
101+
102+
Mockito.verify(primaryDataStoreDaoMock, Mockito.never()).findById(Mockito.anyLong());
103+
Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listAllByVolumeAndDataStore(Mockito.anyLong(), Mockito.any());
104+
}
105+
106+
@Test
107+
public void skipsWhenPoolIsNotRbd() {
108+
long poolId = 5L;
109+
StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
110+
Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID);
111+
Mockito.doReturn(poolId).when(volumeVoMock).getPoolId();
112+
Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId);
113+
Mockito.doReturn(Storage.StoragePoolType.NetworkFilesystem).when(pool).getPoolType();
114+
115+
invoke();
116+
117+
Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listAllByVolumeAndDataStore(Mockito.anyLong(), Mockito.any());
118+
}
119+
120+
@Test
121+
public void skipsWhenNoSnapshotsOnPrimary() {
122+
long poolId = 5L;
123+
StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
124+
Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID);
125+
Mockito.doReturn(poolId).when(volumeVoMock).getPoolId();
126+
Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId);
127+
Mockito.doReturn(Storage.StoragePoolType.RBD).when(pool).getPoolType();
128+
Mockito.doReturn(Collections.emptyList()).when(snapshotDataStoreDaoMock).listAllByVolumeAndDataStore(VOLUME_ID, DataStoreRole.Primary);
129+
130+
invoke();
131+
132+
Mockito.verifyNoInteractions(snapshotApiServiceMock);
133+
}
134+
135+
@Test
136+
public void deletesEachPrimaryOnlySnapshotBeforeVolumeIsDeleted() {
137+
long poolId = 5L;
138+
StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
139+
Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID);
140+
Mockito.doReturn(poolId).when(volumeVoMock).getPoolId();
141+
Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId);
142+
Mockito.doReturn(Storage.StoragePoolType.RBD).when(pool).getPoolType();
143+
144+
SnapshotDataStoreVO snap1 = Mockito.mock(SnapshotDataStoreVO.class);
145+
Mockito.doReturn(11L).when(snap1).getSnapshotId();
146+
SnapshotDataStoreVO snap2 = Mockito.mock(SnapshotDataStoreVO.class);
147+
Mockito.doReturn(22L).when(snap2).getSnapshotId();
148+
List<SnapshotDataStoreVO> snapStoreVOs = Arrays.asList(snap1, snap2);
149+
Mockito.doReturn(snapStoreVOs).when(snapshotDataStoreDaoMock).listAllByVolumeAndDataStore(VOLUME_ID, DataStoreRole.Primary);
150+
151+
// Neither snapshot has an Image role sibling, so both should be deleted through the normal workflow.
152+
Mockito.doReturn(Collections.singletonList(snap1)).when(snapshotDataStoreDaoMock).findBySnapshotId(11L);
153+
Mockito.doReturn(Collections.singletonList(snap2)).when(snapshotDataStoreDaoMock).findBySnapshotId(22L);
154+
155+
invoke();
156+
157+
Mockito.verify(snapshotApiServiceMock).deleteSnapshot(11L, null);
158+
Mockito.verify(snapshotApiServiceMock).deleteSnapshot(22L, null);
159+
}
160+
161+
@Test
162+
public void keepsGoingWhenOneSnapshotDeleteFails() {
163+
long poolId = 5L;
164+
StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
165+
Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID);
166+
Mockito.doReturn(poolId).when(volumeVoMock).getPoolId();
167+
Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId);
168+
Mockito.doReturn(Storage.StoragePoolType.RBD).when(pool).getPoolType();
169+
170+
SnapshotDataStoreVO snap1 = Mockito.mock(SnapshotDataStoreVO.class);
171+
Mockito.doReturn(11L).when(snap1).getSnapshotId();
172+
SnapshotDataStoreVO snap2 = Mockito.mock(SnapshotDataStoreVO.class);
173+
Mockito.doReturn(22L).when(snap2).getSnapshotId();
174+
Mockito.doReturn(Arrays.asList(snap1, snap2)).when(snapshotDataStoreDaoMock).listAllByVolumeAndDataStore(VOLUME_ID, DataStoreRole.Primary);
175+
Mockito.doReturn(Collections.singletonList(snap1)).when(snapshotDataStoreDaoMock).findBySnapshotId(11L);
176+
Mockito.doReturn(Collections.singletonList(snap2)).when(snapshotDataStoreDaoMock).findBySnapshotId(22L);
177+
Mockito.doThrow(new RuntimeException("boom")).when(snapshotApiServiceMock).deleteSnapshot(11L, null);
178+
179+
invoke();
180+
181+
Mockito.verify(snapshotApiServiceMock).deleteSnapshot(11L, null);
182+
Mockito.verify(snapshotApiServiceMock).deleteSnapshot(22L, null);
183+
}
184+
185+
@Test
186+
public void removesOnlyTheReferenceWhenSnapshotHasAnImageCopy() {
187+
long poolId = 5L;
188+
StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
189+
Mockito.doReturn(HypervisorType.KVM).when(volumeDaoMock).getHypervisorType(VOLUME_ID);
190+
Mockito.doReturn(poolId).when(volumeVoMock).getPoolId();
191+
Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(poolId);
192+
Mockito.doReturn(Storage.StoragePoolType.RBD).when(pool).getPoolType();
193+
194+
SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class);
195+
Mockito.doReturn(11L).when(primaryRef).getSnapshotId();
196+
Mockito.doReturn(99L).when(primaryRef).getId();
197+
SnapshotDataStoreVO imageRef = Mockito.mock(SnapshotDataStoreVO.class);
198+
Mockito.doReturn(DataStoreRole.Image).when(imageRef).getRole();
199+
200+
Mockito.doReturn(Collections.singletonList(primaryRef)).when(snapshotDataStoreDaoMock).listAllByVolumeAndDataStore(VOLUME_ID, DataStoreRole.Primary);
201+
Mockito.doReturn(Arrays.asList(primaryRef, imageRef)).when(snapshotDataStoreDaoMock).findBySnapshotId(11L);
202+
203+
invoke();
204+
205+
Mockito.verify(snapshotDataStoreDaoMock).remove(99L);
206+
Mockito.verify(snapshotApiServiceMock, Mockito.never()).deleteSnapshot(Mockito.anyLong(), Mockito.any());
207+
}
208+
}

0 commit comments

Comments
 (0)