kvm: delete primary-only RBD snapshots before the volume

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

@@ -450,6 +450,7 @@ public AsyncCallFuture<VolumeApiResult> expungeVolumeAsync(VolumeInfo volume) {
                 future.complete(result);
                 return future;
             }
+            deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete(vol);
         }
 
         DeleteVolumeContext<VolumeApiResult> 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<SnapshotDataStoreVO> 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

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<SnapshotDataStoreVO> 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());
+    }
+}