Fix: Volumes on lost local storage cannot be removed (#7594)

engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java

@@ -147,4 +147,6 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
      */
     List<VolumeVO> findByDiskOfferingId(long diskOfferingId);
     VolumeVO getInstanceRootVolume(long instanceId);
+
+    void updateAndRemoveVolume(VolumeVO volume);
 }

engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java

@@ -766,4 +766,15 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
         sc.setParameters("vType", Volume.Type.ROOT);
         return findOneBy(sc);
     }
+
+    @Override
+    public void updateAndRemoveVolume(VolumeVO volume) {
+        if (volume.getState() != Volume.State.Destroy) {
+            volume.setState(Volume.State.Destroy);
+            volume.setPoolId(null);
+            volume.setInstanceId(null);
+            update(volume.getId(), volume);
+            remove(volume.getId());
+        }
+    }
 }

server/src/main/java/com/cloud/resource/ResourceManagerImpl.java

@@ -38,6 +38,9 @@ import javax.naming.ConfigurationException;
 
 import com.cloud.exception.StorageConflictException;
 import com.cloud.exception.StorageUnavailableException;
+import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.dao.VolumeDao;
 import org.apache.cloudstack.annotation.AnnotationService;
 import org.apache.cloudstack.annotation.dao.AnnotationDao;
 import org.apache.cloudstack.api.ApiConstants;
@@ -294,6 +297,8 @@ public class ResourceManagerImpl extends ManagerBase implements ResourceManager,
     private UserVmDetailsDao userVmDetailsDao;
     @Inject
     private AnnotationDao annotationDao;
+    @Inject
+    private VolumeDao volumeDao;
 
     private final long _nodeId = ManagementServerNode.getManagementServerId();
 
@@ -979,6 +984,7 @@ public class ResourceManagerImpl extends ManagerBase implements ResourceManager,
                     final Long poolId = pool.getPoolId();
                     final StoragePoolVO storagePool = _storagePoolDao.findById(poolId);
                     if (storagePool.isLocal() && isForceDeleteStorage) {
+                        destroyLocalStoragePoolVolumes(poolId);
                         storagePool.setUuid(null);
                         storagePool.setClusterId(null);
                         _storagePoolDao.update(poolId, storagePool);
@@ -1011,6 +1017,27 @@ public class ResourceManagerImpl extends ManagerBase implements ResourceManager,
         return true;
     }
 
+    private void addVolumesToList(List<VolumeVO> volumes, List<VolumeVO> volumesToAdd) {
+        if (CollectionUtils.isNotEmpty(volumesToAdd)) {
+            volumes.addAll(volumesToAdd);
+        }
+    }
+
+    protected void destroyLocalStoragePoolVolumes(long poolId) {
+        List<VolumeVO> rootDisks = volumeDao.findByPoolId(poolId);
+        List<VolumeVO> dataVolumes = volumeDao.findByPoolId(poolId, Volume.Type.DATADISK);
+
+        List<VolumeVO> volumes = new ArrayList<>();
+        addVolumesToList(volumes, rootDisks);
+        addVolumesToList(volumes, dataVolumes);
+
+        if (CollectionUtils.isNotEmpty(volumes)) {
+            for (VolumeVO volume : volumes) {
+                volumeDao.updateAndRemoveVolume(volume);
+            }
+        }
+    }
+
     /**
      * Returns true if host can be deleted.</br>
      * A host can be deleted either if it is in Maintenance or "Degraded" state.

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

@@ -36,6 +36,7 @@ import javax.inject.Inject;
 
 import org.apache.cloudstack.api.ApiConstants.IoDriverPolicy;
 import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.InternalIdentity;
 import org.apache.cloudstack.api.ServerApiException;
 import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd;
 import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd;
@@ -88,6 +89,7 @@ import org.apache.cloudstack.storage.command.AttachAnswer;
 import org.apache.cloudstack.storage.command.AttachCommand;
 import org.apache.cloudstack.storage.command.DettachCommand;
 import org.apache.cloudstack.storage.command.TemplateOrVolumePostUploadCommand;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
 import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
@@ -273,6 +275,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
     @Inject
     private PrimaryDataStoreDao _storagePoolDao;
     @Inject
+    private ImageStoreDao imageStoreDao;
+    @Inject
     private DiskOfferingDao _diskOfferingDao;
     @Inject
     private ServiceOfferingDao _serviceOfferingDao;
@@ -1619,6 +1623,12 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
     }
 
     private void expungeVolumesInPrimaryOrSecondary(VolumeVO volume, DataStoreRole role) throws InterruptedException, ExecutionException {
+        if (!canAccessVolumeStore(volume, role)) {
+            s_logger.debug(String.format("Cannot access the storage pool with role: %s " +
+                            "for the volume: %s, skipping expunge from storage",
+                    role.name(), volume.getName()));
+            return;
+        }
         VolumeInfo volOnStorage = volFactory.getVolume(volume.getId(), role);
         if (volOnStorage != null) {
             s_logger.info("Expunging volume " + volume.getId() + " from " + role + " data store");
@@ -1638,6 +1648,17 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
             }
         }
     }
+
+    private boolean canAccessVolumeStore(VolumeVO volume, DataStoreRole role) {
+        if (volume == null) {
+            throw new CloudRuntimeException("No volume given, cannot check access to volume store");
+        }
+        InternalIdentity pool = role == DataStoreRole.Primary ?
+                _storagePoolDao.findById(volume.getPoolId()) :
+                imageStoreDao.findById(volume.getPoolId());
+        return pool != null;
+    }
+
     /**
      * Clean volumes cache entries (if they exist).
      */

server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java

@@ -34,9 +34,13 @@ import static org.mockito.Mockito.when;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.UUID;
 
+import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.dao.VolumeDao;
 import org.apache.cloudstack.api.command.admin.host.CancelHostAsDegradedCmd;
 import org.apache.cloudstack.api.command.admin.host.DeclareHostAsDegradedCmd;
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
@@ -98,6 +102,8 @@ public class ResourceManagerImplTest {
     private VMInstanceDao vmInstanceDao;
     @Mock
     private ConfigurationDao configurationDao;
+    @Mock
+    private VolumeDao volumeDao;
 
     @Spy
     @InjectMocks
@@ -119,6 +125,13 @@ public class ResourceManagerImplTest {
     @Mock
     private GetVncPortCommand getVncPortCommandVm2;
 
+    @Mock
+    private VolumeVO rootDisk1;
+    @Mock
+    private VolumeVO rootDisk2;
+    @Mock
+    private VolumeVO dataDisk;
+
     @Mock
     private Connection sshConnection;
 
@@ -138,6 +151,10 @@ public class ResourceManagerImplTest {
     private static String vm2VncAddress = "10.2.2.2";
     private static int vm2VncPort = 5901;
 
+    private static long poolId = 1L;
+    private List<VolumeVO> rootDisks;
+    private List<VolumeVO> dataDisks;
+
     @Before
     public void setup() throws Exception {
         MockitoAnnotations.initMocks(this);
@@ -179,6 +196,11 @@ public class ResourceManagerImplTest {
                 willReturn(new SSHCmdHelper.SSHCmdResult(0,"",""));
 
         when(configurationDao.getValue(ResourceManager.KvmSshToAgentEnabled.key())).thenReturn("true");
+
+        rootDisks = Arrays.asList(rootDisk1, rootDisk2);
+        dataDisks = Collections.singletonList(dataDisk);
+        when(volumeDao.findByPoolId(poolId)).thenReturn(rootDisks);
+        when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(dataDisks);
     }
 
     @Test
@@ -527,4 +549,33 @@ public class ResourceManagerImplTest {
         return new HostVO(1L, "host01", Host.Type.Routing, "192.168.1.1", "255.255.255.0", null, null, null, null, null, null, null, null, null, null, UUID.randomUUID().toString(),
                 hostStatus, "1.0", null, null, 1L, null, 0, 0, null, 0, null);
     }
+
+    @Test
+    public void testDestroyLocalStoragePoolVolumesBothRootDisksAndDataDisks() {
+        resourceManager.destroyLocalStoragePoolVolumes(poolId);
+        verify(volumeDao, times(rootDisks.size() + dataDisks.size()))
+                .updateAndRemoveVolume(any(VolumeVO.class));
+    }
+
+    @Test
+    public void testDestroyLocalStoragePoolVolumesOnlyRootDisks() {
+        when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(null);
+        resourceManager.destroyLocalStoragePoolVolumes(poolId);
+        verify(volumeDao, times(rootDisks.size())).updateAndRemoveVolume(any(VolumeVO.class));
+    }
+
+    @Test
+    public void testDestroyLocalStoragePoolVolumesOnlyDataDisks() {
+        when(volumeDao.findByPoolId(poolId)).thenReturn(null);
+        resourceManager.destroyLocalStoragePoolVolumes(poolId);
+        verify(volumeDao, times(dataDisks.size())).updateAndRemoveVolume(any(VolumeVO.class));
+    }
+
+    @Test
+    public void testDestroyLocalStoragePoolVolumesNoDisks() {
+        when(volumeDao.findByPoolId(poolId)).thenReturn(null);
+        when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(null);
+        resourceManager.destroyLocalStoragePoolVolumes(poolId);
+        verify(volumeDao, never()).updateAndRemoveVolume(any(VolumeVO.class));
+    }
 }

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

@@ -52,6 +52,8 @@ import org.apache.cloudstack.framework.jobs.AsyncJobExecutionContext;
 import org.apache.cloudstack.framework.jobs.AsyncJobManager;
 import org.apache.cloudstack.framework.jobs.dao.AsyncJobJoinMapDao;
 import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
 import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
 import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreDao;
@@ -143,6 +145,12 @@ public class VolumeApiServiceImplTest {
     @Mock
     private PrimaryDataStoreDao primaryDataStoreDaoMock;
     @Mock
+    private ImageStoreDao imageStoreDao;
+    @Mock
+    private ImageStoreVO imageStoreVO;
+    @Mock
+    private StoragePoolVO storagePoolVO;
+    @Mock
     private VMSnapshotDao _vmSnapshotDao;
     @Mock
     private AsyncJobManager _jobMgr;
@@ -214,6 +222,7 @@ public class VolumeApiServiceImplTest {
     private long volumeMockId = 12313l;
     private long vmInstanceMockId = 1123l;
     private long volumeSizeMock = 456789921939l;
+    private static long imageStoreId = 10L;
 
     private String projectMockUuid = "projectUuid";
     private long projecMockId = 13801801923810L;
@@ -239,6 +248,10 @@ public class VolumeApiServiceImplTest {
 
         Mockito.when(storagePoolMock.getId()).thenReturn(storagePoolMockId);
 
+        Mockito.when(volumeVoMock.getPoolId()).thenReturn(storagePoolMockId);
+        Mockito.when(imageStoreDao.findById(imageStoreId)).thenReturn(imageStoreVO);
+        Mockito.when(primaryDataStoreDaoMock.findById(storagePoolMockId)).thenReturn(storagePoolVO);
+
         volumeApiServiceImpl._gson = GsonHelper.getGsonLogger();
 
         // mock caller context
@@ -914,7 +927,7 @@ public class VolumeApiServiceImplTest {
     @Test
     public void expungeVolumesInSecondaryStorageIfNeededTestVolumeNotFoundInSecondaryStorage() throws InterruptedException, ExecutionException {
         Mockito.lenient().doReturn(asyncCallFutureVolumeapiResultMock).when(volumeServiceMock).expungeVolumeAsync(volumeInfoMock);
-        Mockito.doReturn(null).when(volumeDataFactoryMock).getVolume(volumeMockId, DataStoreRole.Image);
+        Mockito.lenient().doReturn(null).when(imageStoreDao).findById(imageStoreId);
         Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId, ResourceType.secondary_storage, volumeSizeMock);
         Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
         Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
@@ -933,6 +946,7 @@ public class VolumeApiServiceImplTest {
         Mockito.doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId, ResourceType.secondary_storage, volumeSizeMock);
         Mockito.doReturn(accountMockId).when(volumeInfoMock).getAccountId();
         Mockito.doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
+        Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
 
         volumeApiServiceImpl.expungeVolumesInSecondaryStorageIfNeeded(volumeVoMock);
 
@@ -948,6 +962,7 @@ public class VolumeApiServiceImplTest {
         Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId, ResourceType.secondary_storage, volumeSizeMock);
         Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
         Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
+        Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
 
         Mockito.doThrow(InterruptedException.class).when(asyncCallFutureVolumeapiResultMock).get();
 
@@ -962,6 +977,7 @@ public class VolumeApiServiceImplTest {
         Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId, ResourceType.secondary_storage, volumeSizeMock);
         Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
         Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
+        Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
 
         Mockito.doThrow(ExecutionException.class).when(asyncCallFutureVolumeapiResultMock).get();