Address review comments

Author: Pearl Dsilva

api/src/main/java/org/apache/cloudstack/api/command/admin/storage/ListImageStoresCmd.java

@@ -51,7 +51,7 @@ public class ListImageStoresCmd extends BaseListCmd {
     @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = ImageStoreResponse.class, description = "the ID of the storage pool")
     private Long id;
 
-    @Parameter(name = ApiConstants.READ_ONLY, type = CommandType.BOOLEAN, entityType = ImageStoreResponse.class, description = "read-only status of the image store")
+    @Parameter(name = ApiConstants.READ_ONLY, type = CommandType.BOOLEAN, entityType = ImageStoreResponse.class, description = "read-only status of the image store", since = "4.15.0")
     private Boolean readonly;
 
     /////////////////////////////////////////////////////

api/src/main/java/org/apache/cloudstack/api/command/admin/storage/MigrateSecondaryStorageDataCmd.java

@@ -37,11 +37,11 @@
         responseObject = MigrationResponse.class,
         requestHasSensitiveInfo = false,
         responseHasSensitiveInfo = false,
-        since = "4.14.0",
+        since = "4.15.0",
         authorized = {RoleType.Admin})
 public class MigrateSecondaryStorageDataCmd extends BaseAsyncCmd {
 
-    public static final Logger s_logger = Logger.getLogger(MigrateSecondaryStorageDataCmd.class.getName());
+    public static final Logger LOGGER = Logger.getLogger(MigrateSecondaryStorageDataCmd.class.getName());
 
     public static final String APINAME = "migrateSecondaryStorageData";
 

api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateImageStoreCmd.java

@@ -30,7 +30,7 @@
 import com.cloud.storage.ImageStore;
 
 @APICommand(name = UpdateImageStoreCmd.APINAME, description = "Updates image store read-only status", responseObject = ImageStoreResponse.class, entityType = {ImageStore.class},
-        requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+        requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.15.0")
 public class UpdateImageStoreCmd extends BaseCmd {
     private static final Logger LOG = Logger.getLogger(UpdateImageStoreCmd.class.getName());
     public static final String APINAME = "updateImageStore";

engine/components-api/src/main/java/com/cloud/storage/StorageManager.java

@@ -112,6 +112,9 @@ public interface StorageManager extends StorageService {
     ConfigKey<Integer> PRIMARY_STORAGE_DOWNLOAD_WAIT = new ConfigKey<Integer>("Storage", Integer.class, "primary.storage.download.wait", "10800",
             "In second, timeout for download template to primary storage", false);
 
+    ConfigKey<Integer>  SecStorageMaxMigrateSessions = new ConfigKey<Integer>("Advanced", Integer.class, "secstorage.max.migrate.sessions", "2",
+            "The max number of concurrent copy command execution sessions that an SSVM can handle", true, ConfigKey.Scope.Global);
+
     /**
      * Returns a comma separated list of tags for the specified storage pool
      * @param poolId

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java

@@ -43,7 +43,6 @@
 import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO;
 import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreVO;
-import org.apache.log4j.Logger;
 
 import com.cloud.host.HostVO;
 import com.cloud.host.Status;
@@ -82,13 +81,11 @@ public class DataMigrationUtility {
     @Inject
     SnapshotDao snapshotDao;
 
-    private static final Logger s_logger = Logger.getLogger(DataMigrationUtility.class);
-
     /** This function verifies if the given image store comprises of data objects that are not in either the "Ready" or
      * "Allocated" state - in such a case, if the migration policy is complete, the migration is terminated
      */
-    private boolean filesReady(Long srcDataStoreId) {
-        String[] validStates = new String[]{"Ready", "Allocated"};
+    private boolean filesReadyToMigrate(Long srcDataStoreId) {
+        String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
         boolean isReady = true;
         List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
         for (TemplateDataStoreVO template : templates) {
@@ -107,7 +104,7 @@ private boolean filesReady(Long srcDataStoreId) {
 
     protected void checkIfCompleteMigrationPossible(ImageStoreService.MigrationPolicy policy, Long srcDataStoreId) {
         if (policy == ImageStoreService.MigrationPolicy.COMPLETE) {
-            if (!filesReady(srcDataStoreId)) {
+            if (!filesReadyToMigrate(srcDataStoreId)) {
                 throw new CloudRuntimeException("Complete migration failed as there are data objects which are not Ready");
             }
         }
@@ -147,9 +144,9 @@ public int compare(Map.Entry<Long, Pair<Long, Long>> e1, Map.Entry<Long, Pair<Lo
 
     protected List<DataObject> getSortedValidSourcesList(DataStore srcDataStore, Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains) {
         List<DataObject> files = new ArrayList<>();
-        files.addAll(getAllValidTemplates(srcDataStore));
-        files.addAll(getAllValidSnapshotsAndChains(srcDataStore, snapshotChains));
-        files.addAll(getAllValidVolumes(srcDataStore));
+        files.addAll(getAllReadyTemplates(srcDataStore));
+        files.addAll(getAllReadySnapshotsAndChains(srcDataStore, snapshotChains));
+        files.addAll(getAllReadyVolumes(srcDataStore));
 
         files = sortFilesOnSize(files, snapshotChains);
 
@@ -168,14 +165,13 @@ public int compare(DataObject o1, DataObject o2) {
                 if (o2 instanceof  SnapshotInfo) {
                     size2 = snapshotChains.get(o2).second();
                 }
-                return (int) (size2 - size1);
+                return size2 > size1 ? 1 : -1;
             }
         });
         return files;
     }
 
-    // Gets list of all valid templates, i.e, templates in "Ready" state for migration
-    protected List<DataObject> getAllValidTemplates(DataStore srcDataStore) {
+    protected List<DataObject> getAllReadyTemplates(DataStore srcDataStore) {
 
         List<DataObject> files = new LinkedList<>();
         List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStore.getId());
@@ -192,7 +188,7 @@ protected List<DataObject> getAllValidTemplates(DataStore srcDataStore) {
      * for each parent snapshot and the cumulative size of the chain - this is done to ensure that all the snapshots in a chain
      * are migrated to the same datastore
      */
-    protected List<DataObject> getAllValidSnapshotsAndChains(DataStore srcDataStore, Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains) {
+    protected List<DataObject> getAllReadySnapshotsAndChains(DataStore srcDataStore, Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains) {
         List<SnapshotInfo> files = new LinkedList<>();
         List<SnapshotDataStoreVO> snapshots = snapshotDataStoreDao.listByStoreId(srcDataStore.getId(), DataStoreRole.Image);
         for (SnapshotDataStoreVO snapshot : snapshots) {
@@ -219,7 +215,6 @@ protected List<DataObject> getAllValidSnapshotsAndChains(DataStore srcDataStore,
         return (List<DataObject>) (List<?>) files;
     }
 
-    // Finds the cumulative file size for all data objects in the chain
     protected Long getSizeForChain(List<SnapshotInfo> chain) {
         Long size = 0L;
         for (SnapshotInfo snapshot : chain) {
@@ -228,8 +223,8 @@ protected Long getSizeForChain(List<SnapshotInfo> chain) {
         return size;
     }
 
-    // Returns a list of volumes that are in "Ready" state
-    protected List<DataObject> getAllValidVolumes(DataStore srcDataStore) {
+
+    protected List<DataObject> getAllReadyVolumes(DataStore srcDataStore) {
         List<DataObject> files = new LinkedList<>();
         List<VolumeDataStoreVO> volumes = volumeDataStoreDao.listByStoreId(srcDataStore.getId());
         for (VolumeDataStoreVO volume : volumes) {

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java

@@ -56,14 +56,13 @@
 import org.apache.commons.math3.stat.descriptive.moment.StandardDeviation;
 import org.apache.log4j.Logger;
 
-import com.cloud.configuration.Config;
 import com.cloud.server.StatsCollector;
 import com.cloud.storage.DataStoreRole;
 import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.StorageManager;
 import com.cloud.storage.StorageService;
 import com.cloud.storage.StorageStats;
 import com.cloud.storage.dao.SnapshotDao;
-import com.cloud.utils.NumbersUtil;
 import com.cloud.utils.Pair;
 import com.cloud.utils.StringUtils;
 import com.cloud.utils.component.ManagerBase;
@@ -133,7 +132,7 @@ public boolean offer(T task) {
 
     @Override
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
-        numConcurrentCopyTasksPerSSVM = NumbersUtil.parseInt(configDao.getValue(Config.SecStorageMaxMigrateSessions.key()), 2);
+        numConcurrentCopyTasksPerSSVM = StorageManager.SecStorageMaxMigrateSessions.value();
         return true;
     }
 
@@ -144,25 +143,18 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
         String message = null;
 
         migrationHelper.checkIfCompleteMigrationPossible(migrationPolicy, srcDataStoreId);
-
         DataStore srcDatastore = dataStoreManager.getDataStore(srcDataStoreId, DataStoreRole.Image);
         Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains = new HashMap<>();
         files = migrationHelper.getSortedValidSourcesList(srcDatastore, snapshotChains);
 
         if (files.isEmpty()) {
             return new MigrationResponse("No files in Image store "+srcDatastore.getId()+ " to migrate", migrationPolicy.toString(), true);
         }
-
-        // Create capacity class with free and total space, maybe id of ds too and use that as the value
         Map<Long, Pair<Long, Long>> storageCapacities = new Hashtable<>();
-
         for (Long storeId : destDatastores) {
             storageCapacities.put(storeId, new Pair<>(null, null));
         }
         storageCapacities.put(srcDataStoreId, new Pair<>(null, null));
-
-        // If the migration policy is to completely migrate data from the given source Image Store, then set it's state
-        // to readonly
         if (migrationPolicy == MigrationPolicy.COMPLETE) {
             s_logger.debug("Setting source image store "+srcDatastore.getId()+ " to read-only");
             storageService.updateImageStoreStatus(srcDataStoreId, true);
@@ -172,8 +164,8 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
         double meanstddev = getStandardDeviation(storageCapacities);
         double threshold = ImageStoreImbalanceThreshold.value();
         MigrationResponse response = null;
-
-        ThreadPoolExecutor executor = new ThreadPoolExecutor(numConcurrentCopyTasksPerSSVM , numConcurrentCopyTasksPerSSVM, 30, TimeUnit.MINUTES, new MigrateBlockingQueue<>(numConcurrentCopyTasksPerSSVM));
+        ThreadPoolExecutor executor = new ThreadPoolExecutor(numConcurrentCopyTasksPerSSVM , numConcurrentCopyTasksPerSSVM, 30,
+                TimeUnit.MINUTES, new MigrateBlockingQueue<>(numConcurrentCopyTasksPerSSVM));
         Date start = new Date();
         if (meanstddev < threshold && migrationPolicy == MigrationPolicy.BALANCE) {
             s_logger.debug("mean std deviation of the image stores is below threshold, no migration required");
@@ -188,28 +180,14 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
                 chosenFileForMigration = files.remove(0);
             }
 
-            // Choose datastore with maximum free capacity as the destination datastore for migration
             storageCapacities = getStorageCapacities(storageCapacities);
             List<Long> orderedDS = migrationHelper.sortDataStores(storageCapacities);
             Long destDatastoreId = orderedDS.get(0);
 
-            // If there aren't anymore files available for migration or no valid Image stores available for migration
-            // end the migration process
             if (chosenFileForMigration == null || destDatastoreId == null || destDatastoreId == srcDatastore.getId()) {
-                if (destDatastoreId == srcDatastore.getId() && !files.isEmpty() ) {
-                    if (migrationPolicy == MigrationPolicy.BALANCE) {
-                        s_logger.debug("Migration completed : data stores have been balanced ");
-                        message = "Image stores have been balanced";
-                        success = true;
-                    } else {
-                        message = "Files not completely migrated from "+ srcDatastore.getId() +
-                                " If you want to continue using the Image Store, please change the read-only status using 'update imagestore' command";
-                        success = false;
-                    }
-                } else {
-                    message = "Migration completed";
-                    success = true;
-                }
+                Pair<String, Boolean> result = migrateCompleted(destDatastoreId, srcDatastore, files, migrationPolicy);
+                message = result.first();
+                success = result.second();
                 break;
             }
 
@@ -218,24 +196,8 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
                 continue;
             }
 
-            // If there is a benefit in migration of the chosen file to the destination store, then proceed with migration
             if (shouldMigrate(chosenFileForMigration, srcDatastore.getId(), destDatastoreId, storageCapacities, snapshotChains, migrationPolicy)) {
-                Long fileSize = migrationHelper.getFileSize(chosenFileForMigration, snapshotChains);
-                storageCapacities = assumeMigrate(storageCapacities, srcDatastore.getId(), destDatastoreId, fileSize);
-                long activeSsvms = migrationHelper.activeSSVMCount(srcDatastore);
-                long totalJobs = activeSsvms * numConcurrentCopyTasksPerSSVM;
-                // Increase thread pool size with increase in number of SSVMs
-                if ( totalJobs > executor.getCorePoolSize()) {
-                    executor.setMaximumPoolSize((int) (totalJobs));
-                    executor.setCorePoolSize((int) (totalJobs));
-                }
-
-                MigrateDataTask task = new MigrateDataTask(chosenFileForMigration, srcDatastore, dataStoreManager.getDataStore(destDatastoreId, DataStoreRole.Image));
-                if (chosenFileForMigration instanceof SnapshotInfo ) {
-                    task.setSnapshotChains(snapshotChains);
-                }
-                futures.add((executor.submit(task)));
-                s_logger.debug("Migration of file  " + chosenFileForMigration.getId() + " is initiated");
+                migrateAway(chosenFileForMigration, storageCapacities, snapshotChains, srcDatastore, destDatastoreId, executor, futures);
             } else {
                 if (migrationPolicy == MigrationPolicy.BALANCE) {
                     message = "Migration completed and has successfully balanced the data objects among stores:  " + StringUtils.join(storageCapacities.keySet(), ",");
@@ -247,11 +209,50 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
             }
         }
         Date end = new Date();
-        // Migrate snapshots created during the migration process
         handleSnapshotMigration(srcDataStoreId, start, end, migrationPolicy, futures, storageCapacities, executor);
         return handleResponse(futures, migrationPolicy, message, success);
     }
 
+    protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy) {
+        String message = "";
+        boolean success = true;
+        if (destDatastoreId == srcDatastore.getId() && !files.isEmpty() ) {
+            if (migrationPolicy == MigrationPolicy.BALANCE) {
+                s_logger.debug("Migration completed : data stores have been balanced ");
+                message = "Image stores have been balanced";
+                success = true;
+            } else {
+                message = "Files not completely migrated from "+ srcDatastore.getId() +
+                        " If you want to continue using the Image Store, please change the read-only status using 'update imagestore' command";
+                success = false;
+            }
+        } else {
+            message = "Migration completed";
+        }
+        return new Pair<String, Boolean>(message, success);
+    }
+
+    protected void migrateAway(DataObject chosenFileForMigration, Map<Long, Pair<Long, Long>> storageCapacities,
+                               Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains, DataStore srcDatastore, Long destDatastoreId, ThreadPoolExecutor executor,
+    List<Future<AsyncCallFuture<DataObjectResult>>> futures) {
+        Long fileSize = migrationHelper.getFileSize(chosenFileForMigration, snapshotChains);
+        storageCapacities = assumeMigrate(storageCapacities, srcDatastore.getId(), destDatastoreId, fileSize);
+        long activeSsvms = migrationHelper.activeSSVMCount(srcDatastore);
+        long totalJobs = activeSsvms * numConcurrentCopyTasksPerSSVM;
+        // Increase thread pool size with increase in number of SSVMs
+        if ( totalJobs > executor.getCorePoolSize()) {
+            executor.setMaximumPoolSize((int) (totalJobs));
+            executor.setCorePoolSize((int) (totalJobs));
+        }
+
+        MigrateDataTask task = new MigrateDataTask(chosenFileForMigration, srcDatastore, dataStoreManager.getDataStore(destDatastoreId, DataStoreRole.Image));
+        if (chosenFileForMigration instanceof SnapshotInfo ) {
+            task.setSnapshotChains(snapshotChains);
+        }
+        futures.add((executor.submit(task)));
+        s_logger.debug("Migration of file  " + chosenFileForMigration.getId() + " is initiated");
+    }
+
 
 
     private MigrationResponse handleResponse(List<Future<AsyncCallFuture<DataObjectResult>>> futures, MigrationPolicy migrationPolicy, String message, boolean success) {
@@ -271,7 +272,7 @@ private MigrationResponse handleResponse(List<Future<AsyncCallFuture<DataObjectR
         return new MigrationResponse(message, migrationPolicy.toString(), success);
     }
 
-    private  void handleSnapshotMigration(Long srcDataStoreId, Date start, Date end, MigrationPolicy policy,
+    private void handleSnapshotMigration(Long srcDataStoreId, Date start, Date end, MigrationPolicy policy,
                                           List<Future<AsyncCallFuture<DataObjectResult>>> futures, Map<Long, Pair<Long, Long>> storageCapacities, ThreadPoolExecutor executor) {
         DataStore srcDatastore = dataStoreManager.getDataStore(srcDataStoreId, DataStoreRole.Image);
         List<SnapshotDataStoreVO> snaps = snapshotDataStoreDao.findSnapshots(srcDataStoreId, start, end);