server: fix for respecting secondary storage threshold limit (#3480)

Retrieval of an image store using ImageStoreProviderManager has been refactored by introducing three different methods,
DataStore getRandomImageStore(List<DataStore> imageStores);
To get an image store for reading purpose. Threshold capacity check will not be used here.
DataStore getImageStoreWithFreeCapacity(List<DataStore> imageStores);
To get an image store for reading purpose. Threshold capacity check will be used here and the store with max free space will be returned. If no store with filled storage less than the threshold is found, the NULL value will be returned.
List<DataStore> listImageStoresWithFreeCapacity(List<DataStore> imageStores);
To get a list of image stores for writing purpose which fulfills threshold capacity check.

Correspondingly DataStoreManager methods have been refactored to return similar values for a given zone.

Fixes #3287 - NULL value will be returned when secondary storage is needed for writing but there is not store with free space.
Fixes #3041 - Rather than returning random secondary storage for writing, storage with max. free space will be returned.
Fixes #3478 - For migration on VMware, all writable secondary storage will be mounted while preparation.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

Author: shwstppr

engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreManager.java

@@ -33,7 +33,11 @@ public interface DataStoreManager {
 
     List<DataStore> getImageStoresByScope(ZoneScope scope);
 
-    DataStore getImageStore(long zoneId);
+    DataStore getRandomImageStore(long zoneId);
+
+    DataStore getImageStoreWithFreeCapacity(long zoneId);
+
+    List<DataStore> listImageStoresWithFreeCapacity(long zoneId);
 
     List<DataStore> getImageCacheStores(Scope scope);
 

engine/storage/cache/src/main/java/org/apache/cloudstack/storage/cache/allocator/StorageCacheRandomAllocator.java

@@ -62,7 +62,7 @@ public DataStore getCacheStore(Scope scope) {
             return null;
         }
 
-        return imageStoreMgr.getImageStore(cacheStores);
+        return imageStoreMgr.getImageStoreWithFreeCapacity(cacheStores);
     }
 
     @Override
@@ -88,6 +88,6 @@ public DataStore getCacheStore(DataObject data, Scope scope) {
                 }
             }
         }
-        return imageStoreMgr.getImageStore(cacheStores);
+        return imageStoreMgr.getImageStoreWithFreeCapacity(cacheStores);
     }
 }

engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java

@@ -328,8 +328,8 @@ protected Answer copyVolumeBetweenPools(DataObject srcData, DataObject destData)
         if (cacheStore == null) {
             // need to find a nfs or cifs image store, assuming that can't copy volume
             // directly to s3
-            ImageStoreEntity imageStore = (ImageStoreEntity)dataStoreMgr.getImageStore(destScope.getScopeId());
-            if (!imageStore.getProtocol().equalsIgnoreCase("nfs") && !imageStore.getProtocol().equalsIgnoreCase("cifs")) {
+            ImageStoreEntity imageStore = (ImageStoreEntity)dataStoreMgr.getImageStoreWithFreeCapacity(destScope.getScopeId());
+            if (imageStore == null || !imageStore.getProtocol().equalsIgnoreCase("nfs") && !imageStore.getProtocol().equalsIgnoreCase("cifs")) {
                 s_logger.debug("can't find a nfs (or cifs) image store to satisfy the need for a staging store");
                 return null;
             }

engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java

@@ -24,18 +24,17 @@
 
 import javax.inject.Inject;
 
-import com.cloud.storage.ScopeType;
-import com.cloud.storage.Storage;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
 import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
 import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
-import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
 import org.apache.cloudstack.storage.command.CopyCommand;
 import org.apache.cloudstack.storage.datastore.DataStoreManagerImpl;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
 import org.apache.cloudstack.storage.to.TemplateObjectTO;
+import org.apache.commons.collections.MapUtils;
 import org.apache.commons.lang.StringUtils;
 import org.apache.log4j.Logger;
 
@@ -47,6 +46,8 @@
 import com.cloud.host.Host;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.ScopeType;
+import com.cloud.storage.Storage;
 import com.cloud.storage.Storage.StoragePoolType;
 import com.cloud.storage.StorageManager;
 import com.cloud.storage.StoragePool;
@@ -56,7 +57,6 @@
 import com.cloud.storage.dao.VMTemplatePoolDao;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.vm.VirtualMachineManager;
-import org.apache.commons.collections.MapUtils;
 
 /**
  * Extends {@link StorageSystemDataMotionStrategy}, allowing KVM hosts to migrate VMs with the ROOT volume on a non managed local storage pool.
@@ -197,19 +197,21 @@ protected void copyTemplateToTargetFilesystemStorageIfNeeded(VolumeInfo srcVolum
             Host destHost) {
         VMTemplateStoragePoolVO sourceVolumeTemplateStoragePoolVO = vmTemplatePoolDao.findByPoolTemplate(destStoragePool.getId(), srcVolumeInfo.getTemplateId());
         if (sourceVolumeTemplateStoragePoolVO == null && destStoragePool.getPoolType() == StoragePoolType.Filesystem) {
-            DataStore sourceTemplateDataStore = dataStoreManagerImpl.getImageStore(srcVolumeInfo.getDataCenterId());
-            TemplateInfo sourceTemplateInfo = templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(), sourceTemplateDataStore);
-            TemplateObjectTO sourceTemplate = new TemplateObjectTO(sourceTemplateInfo);
+            DataStore sourceTemplateDataStore = dataStoreManagerImpl.getRandomImageStore(srcVolumeInfo.getDataCenterId());
+            if (sourceTemplateDataStore != null) {
+                TemplateInfo sourceTemplateInfo = templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(), sourceTemplateDataStore);
+                TemplateObjectTO sourceTemplate = new TemplateObjectTO(sourceTemplateInfo);
 
-            LOGGER.debug(String.format("Could not find template [id=%s, name=%s] on the storage pool [id=%s]; copying the template to the target storage pool.",
-                    srcVolumeInfo.getTemplateId(), sourceTemplateInfo.getName(), destDataStore.getId()));
+                LOGGER.debug(String.format("Could not find template [id=%s, name=%s] on the storage pool [id=%s]; copying the template to the target storage pool.",
+                        srcVolumeInfo.getTemplateId(), sourceTemplateInfo.getName(), destDataStore.getId()));
 
-            TemplateInfo destTemplateInfo = templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(), destDataStore);
-            final TemplateObjectTO destTemplate = new TemplateObjectTO(destTemplateInfo);
-            Answer copyCommandAnswer = sendCopyCommand(destHost, sourceTemplate, destTemplate, destDataStore);
+                TemplateInfo destTemplateInfo = templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(), destDataStore);
+                final TemplateObjectTO destTemplate = new TemplateObjectTO(destTemplateInfo);
+                Answer copyCommandAnswer = sendCopyCommand(destHost, sourceTemplate, destTemplate, destDataStore);
 
-            if (copyCommandAnswer != null && copyCommandAnswer.getResult()) {
-                updateTemplateReferenceIfSuccessfulCopy(srcVolumeInfo, srcStoragePool, destTemplateInfo, destDataStore);
+                if (copyCommandAnswer != null && copyCommandAnswer.getResult()) {
+                    updateTemplateReferenceIfSuccessfulCopy(srcVolumeInfo, srcStoragePool, destTemplateInfo, destDataStore);
+                }
             }
         }
     }

engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageSystemDataMotionTest.java

@@ -18,13 +18,12 @@
  */
 package org.apache.cloudstack.storage.motion;
 
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.when;
+
 import java.util.HashMap;
 import java.util.Map;
 
-import com.cloud.host.Host;
-import com.cloud.hypervisor.Hypervisor;
-import com.cloud.storage.ScopeType;
-import com.cloud.storage.Storage;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
 import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
 import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
@@ -58,21 +57,22 @@
 import com.cloud.exception.AgentUnavailableException;
 import com.cloud.exception.CloudException;
 import com.cloud.exception.OperationTimedoutException;
+import com.cloud.host.Host;
 import com.cloud.host.HostVO;
+import com.cloud.hypervisor.Hypervisor;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.storage.DataStoreRole;
-import com.cloud.storage.StoragePool;
-import com.cloud.storage.VMTemplateStoragePoolVO;
+import com.cloud.storage.ScopeType;
+import com.cloud.storage.Storage;
 import com.cloud.storage.Storage.ImageFormat;
 import com.cloud.storage.Storage.StoragePoolType;
+import com.cloud.storage.StoragePool;
+import com.cloud.storage.VMTemplateStoragePoolVO;
 import com.cloud.storage.dao.DiskOfferingDao;
 import com.cloud.storage.dao.VMTemplatePoolDao;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.vm.VirtualMachineManager;
 
-import static org.junit.Assert.assertEquals;
-import static org.mockito.Mockito.when;
-
 @RunWith(MockitoJUnitRunner.class)
 public class KvmNonManagedStorageSystemDataMotionTest {
 
@@ -353,7 +353,7 @@ private void configureAndTestcopyTemplateToTargetStorageIfNeeded(VMTemplateStora
         Mockito.when(sourceTemplateInfo.getHypervisorType()).thenReturn(HypervisorType.KVM);
 
         Mockito.when(vmTemplatePoolDao.findByPoolTemplate(Mockito.anyLong(), Mockito.anyLong())).thenReturn(vmTemplateStoragePoolVO);
-        Mockito.when(dataStoreManagerImpl.getImageStore(Mockito.anyLong())).thenReturn(sourceTemplateDataStore);
+        Mockito.when(dataStoreManagerImpl.getRandomImageStore(Mockito.anyLong())).thenReturn(sourceTemplateDataStore);
         Mockito.when(templateDataFactory.getTemplate(Mockito.anyLong(), Mockito.eq(sourceTemplateDataStore))).thenReturn(sourceTemplateInfo);
         Mockito.when(templateDataFactory.getTemplate(Mockito.anyLong(), Mockito.eq(destDataStore))).thenReturn(sourceTemplateInfo);
         kvmNonManagedStorageDataMotionStrategy.copyTemplateToTargetFilesystemStorageIfNeeded(srcVolumeInfo, srcStoragePool, destDataStore, destStoragePool, destHost);
@@ -362,7 +362,7 @@ private void configureAndTestcopyTemplateToTargetStorageIfNeeded(VMTemplateStora
 
         InOrder verifyInOrder = Mockito.inOrder(vmTemplatePoolDao, dataStoreManagerImpl, templateDataFactory, kvmNonManagedStorageDataMotionStrategy);
         verifyInOrder.verify(vmTemplatePoolDao, Mockito.times(1)).findByPoolTemplate(Mockito.anyLong(), Mockito.anyLong());
-        verifyInOrder.verify(dataStoreManagerImpl, Mockito.times(times)).getImageStore(Mockito.anyLong());
+        verifyInOrder.verify(dataStoreManagerImpl, Mockito.times(times)).getRandomImageStore(Mockito.anyLong());
         verifyInOrder.verify(templateDataFactory, Mockito.times(times)).getTemplate(Mockito.anyLong(), Mockito.eq(sourceTemplateDataStore));
         verifyInOrder.verify(templateDataFactory, Mockito.times(times)).getTemplate(Mockito.anyLong(), Mockito.eq(destDataStore));
         verifyInOrder.verify(kvmNonManagedStorageDataMotionStrategy, Mockito.times(times)).sendCopyCommand(Mockito.eq(destHost), Mockito.any(TemplateObjectTO.class),

engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/manager/ImageStoreProviderManagerImpl.java

@@ -20,17 +20,14 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
 
-import org.apache.log4j.Logger;
-import org.springframework.stereotype.Component;
-
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProviderManager;
 import org.apache.cloudstack.engine.subsystem.api.storage.ImageStoreProvider;
@@ -42,6 +39,8 @@
 import org.apache.cloudstack.storage.image.datastore.ImageStoreEntity;
 import org.apache.cloudstack.storage.image.datastore.ImageStoreProviderManager;
 import org.apache.cloudstack.storage.image.store.ImageStoreImpl;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
 
 import com.cloud.server.StatsCollector;
 import com.cloud.storage.ScopeType;
@@ -144,19 +143,56 @@ public List<DataStore> listImageCacheStores(Scope scope) {
     }
 
     @Override
-    public DataStore getImageStore(List<DataStore> imageStores) {
+    public DataStore getRandomImageStore(List<DataStore> imageStores) {
+        if (imageStores.size() > 1) {
+            Collections.shuffle(imageStores);
+        }
+        return imageStores.get(0);
+    }
+
+    @Override
+    public DataStore getImageStoreWithFreeCapacity(List<DataStore> imageStores) {
         if (imageStores.size() > 1) {
-            Collections.shuffle(imageStores); // Randomize image store list.
-            Iterator<DataStore> i = imageStores.iterator();
-            DataStore imageStore = null;
-            while(i.hasNext()) {
-                imageStore = i.next();
+            imageStores.sort(new Comparator<DataStore>() { // Sort data stores based on free capacity
+                @Override
+                public int compare(DataStore store1, DataStore store2) {
+                    return Long.compare(_statsCollector.imageStoreCurrentFreeCapacity(store1),
+                            _statsCollector.imageStoreCurrentFreeCapacity(store2));
+                }
+            });
+            for (DataStore imageStore : imageStores) {
                 // Return image store if used percentage is less then threshold value i.e. 90%.
                 if (_statsCollector.imageStoreHasEnoughCapacity(imageStore)) {
                     return imageStore;
                 }
             }
+        } else if (imageStores.size() == 1) {
+            if (_statsCollector.imageStoreHasEnoughCapacity(imageStores.get(0))) {
+                return imageStores.get(0);
+            }
         }
-        return imageStores.get(0);
+
+        // No store with space found
+        s_logger.error(String.format("Can't find an image storage in zone with less than %d usage",
+                Math.round(_statsCollector.getImageStoreCapacityThreshold()*100)));
+        return null;
+    }
+
+    @Override
+    public List<DataStore> listImageStoresWithFreeCapacity(List<DataStore> imageStores) {
+        List<DataStore> stores = new ArrayList<>();
+        for (DataStore imageStore : imageStores) {
+            // Return image store if used percentage is less then threshold value i.e. 90%.
+            if (_statsCollector.imageStoreHasEnoughCapacity(imageStore)) {
+                stores.add(imageStore);
+            }
+        }
+
+        // No store with space found
+        if (stores.isEmpty()) {
+            s_logger.error(String.format("Can't find image storage in zone with less than %d usage",
+                    Math.round(_statsCollector.getImageStoreCapacityThreshold() * 100)));
+        }
+        return stores;
     }
 }

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java

@@ -240,7 +240,7 @@ private DataStore findSnapshotImageStore(SnapshotInfo snapshot) {
             fullSnapshot = snapshotFullBackup;
         }
         if (fullSnapshot) {
-            return dataStoreMgr.getImageStore(snapshot.getDataCenterId());
+            return dataStoreMgr.getImageStoreWithFreeCapacity(snapshot.getDataCenterId());
         } else {
             SnapshotInfo parentSnapshot = snapshot.getParent();
             // Note that DataStore information in parentSnapshot is for primary
@@ -251,7 +251,7 @@ private DataStore findSnapshotImageStore(SnapshotInfo snapshot) {
                 parentSnapshotOnBackupStore = _snapshotStoreDao.findBySnapshot(parentSnapshot.getId(), DataStoreRole.Image);
             }
             if (parentSnapshotOnBackupStore == null) {
-                return dataStoreMgr.getImageStore(snapshot.getDataCenterId());
+                return dataStoreMgr.getImageStoreWithFreeCapacity(snapshot.getDataCenterId());
             }
             return dataStoreMgr.getDataStore(parentSnapshotOnBackupStore.getDataStoreId(), parentSnapshotOnBackupStore.getRole());
         }

engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/DataStoreManagerImpl.java

@@ -73,12 +73,30 @@ public List<DataStore> getImageStoresByScope(ZoneScope scope) {
     }
 
     @Override
-    public DataStore getImageStore(long zoneId) {
+    public DataStore getRandomImageStore(long zoneId) {
         List<DataStore> stores = getImageStoresByScope(new ZoneScope(zoneId));
         if (stores == null || stores.size() == 0) {
             return null;
         }
-        return imageDataStoreMgr.getImageStore(stores);
+        return imageDataStoreMgr.getRandomImageStore(stores);
+    }
+
+    @Override
+    public DataStore getImageStoreWithFreeCapacity(long zoneId) {
+        List<DataStore> stores = getImageStoresByScope(new ZoneScope(zoneId));
+        if (stores == null || stores.size() == 0) {
+            return null;
+        }
+        return imageDataStoreMgr.getImageStoreWithFreeCapacity(stores);
+    }
+
+    @Override
+    public List<DataStore> listImageStoresWithFreeCapacity(long zoneId) {
+        List<DataStore> stores = getImageStoresByScope(new ZoneScope(zoneId));
+        if (stores == null || stores.size() == 0) {
+            return null;
+        }
+        return imageDataStoreMgr.listImageStoresWithFreeCapacity(stores);
     }
 
     @Override
@@ -110,7 +128,7 @@ public DataStore getImageCacheStore(long zoneId) {
         if (stores == null || stores.size() == 0) {
             return null;
         }
-        return imageDataStoreMgr.getImageStore(stores);
+        return imageDataStoreMgr.getImageStoreWithFreeCapacity(stores);
     }
 
     @Override

engine/storage/src/main/java/org/apache/cloudstack/storage/image/datastore/ImageStoreProviderManager.java

@@ -42,5 +42,38 @@ public interface ImageStoreProviderManager {
 
     boolean registerDriver(String uuid, ImageStoreDriver driver);
 
-    DataStore getImageStore(List<DataStore> imageStores);
+    /**
+     * Return a random DataStore from the a list of DataStores.
+     *
+     * @param imageStores the list of image stores from which a random store
+     *                    to be returned
+     * @return            random DataStore
+     */
+    DataStore getRandomImageStore(List<DataStore> imageStores);
+
+    /**
+     * Return a DataStore which has free capacity. Stores will be sorted
+     * based on their free space and capacity check will be done based on
+     * the predefined threshold value. If a store is full beyond the
+     * threshold it won't be considered for response. First store in the
+     * sorted list free capacity will be returned. When there is no store
+     * with free capacity in the list a null value will be returned.
+     *
+     * @param imageStores the list of image stores from which stores with free
+     *                    capacity stores to be returned
+     * @return            the DataStore which has free capacity
+     */
+    DataStore getImageStoreWithFreeCapacity(List<DataStore> imageStores);
+
+    /**
+     * Return a list of DataStore which have free capacity. Free capacity check
+     * will be done based on the predefined threshold value. If a store is full
+     * beyond the threshold it won't be considered for response. An empty list
+     * will be returned when no store in the parameter list has free capacity.
+     *
+     * @param imageStores the list of image stores from which stores with free
+     *                    capacity stores to be returned
+     * @return            the list of DataStore which have free capacity
+     */
+    List<DataStore> listImageStoresWithFreeCapacity(List<DataStore> imageStores);
 }

plugins/hypervisors/hyperv/src/main/java/com/cloud/hypervisor/hyperv/manager/HypervManagerImpl.java

@@ -137,7 +137,7 @@ public String prepareSecondaryStorageStore(long zoneId) {
 
     private String getSecondaryStorageStoreUrl(long zoneId) {
         String secUrl = null;
-        DataStore secStore = _dataStoreMgr.getImageStore(zoneId);
+        DataStore secStore = _dataStoreMgr.getImageStoreWithFreeCapacity(zoneId);
         if (secStore != null) {
             secUrl = secStore.getUri();
         }