You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/05/08 10:55:59 UTC
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4053: Secondary Storage Usage Improvements
DaanHoogland commented on a change in pull request #4053:
URL: https://github.com/apache/cloudstack/pull/4053#discussion_r422041219
##########
File path: api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
##########
@@ -777,6 +780,7 @@
public static final String EXITCODE = "exitcode";
public static final String TARGET_ID = "targetid";
public static final String FILES = "files";
+ public static final String FROM = "from";
Review comment:
how is this used in the API? I could imagine a name like srcStore or srcImage would be more appropriate.
##########
File path: api/src/main/java/com/cloud/storage/ImageStoreService.java
##########
@@ -0,0 +1,29 @@
+// 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 com.cloud.storage;
Review comment:
new classes should go into org.apache.cloudstack.*
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/storage/MigrateSecondaryStorageDataCmd.java
##########
@@ -0,0 +1,124 @@
+// 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.api.command.admin.storage;
+
+import java.util.List;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ImageStoreResponse;
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.storage.ImageStore;
+import com.cloud.utils.StringUtils;
+
+@APICommand(name = MigrateSecondaryStorageDataCmd.APINAME,
+ description = "migrates data objects from one secondary storage to destination image store(s)",
+ responseObject = MigrationResponse.class,
+ entityType = {ImageStore.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.14.0",
+ authorized = {RoleType.Admin})
+public class MigrateSecondaryStorageDataCmd extends BaseAsyncCmd {
+
+ public static final Logger s_logger = Logger.getLogger(MigrateSecondaryStorageDataCmd.class.getName());
+
+ public static final String APINAME = "migrateSecondaryStorageData";
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.FROM,
+ type = CommandType.UUID,
+ entityType = ImageStoreResponse.class,
+ description = "id of the image store from where the data is to be migrated",
+ required = true)
+ private Long id;
+
+ @Parameter(name = ApiConstants.MIGRATE_TO,
+ type = CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = ImageStoreResponse.class,
+ description = "id of the destination secondary storage pool to which the templates are to be migrated to",
+ required = true)
+ private List<Long> migrateTo;
Review comment:
I think these should be called `sourcePool` and `destinationPools`. In light of conventions `srcpool` and destpools` are best, I think.
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/storage/MigrateSecondaryStorageDataCmd.java
##########
@@ -0,0 +1,124 @@
+// 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.api.command.admin.storage;
+
+import java.util.List;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ImageStoreResponse;
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.storage.ImageStore;
+import com.cloud.utils.StringUtils;
+
+@APICommand(name = MigrateSecondaryStorageDataCmd.APINAME,
+ description = "migrates data objects from one secondary storage to destination image store(s)",
+ responseObject = MigrationResponse.class,
+ entityType = {ImageStore.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.14.0",
+ authorized = {RoleType.Admin})
+public class MigrateSecondaryStorageDataCmd extends BaseAsyncCmd {
+
+ public static final Logger s_logger = Logger.getLogger(MigrateSecondaryStorageDataCmd.class.getName());
+
+ public static final String APINAME = "migrateSecondaryStorageData";
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.FROM,
+ type = CommandType.UUID,
+ entityType = ImageStoreResponse.class,
+ description = "id of the image store from where the data is to be migrated",
+ required = true)
+ private Long id;
+
+ @Parameter(name = ApiConstants.MIGRATE_TO,
+ type = CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = ImageStoreResponse.class,
+ description = "id of the destination secondary storage pool to which the templates are to be migrated to",
+ required = true)
+ private List<Long> migrateTo;
+
+ @Parameter(name = ApiConstants.MIGRATION_TYPE,
+ type = CommandType.STRING,
+ description = "Balance: if you want data to be distributed evenly among the destination stores, " +
+ "Complete: If you want to migrate the entire data from source image store to the destination store(s)",
+ required = true)
Review comment:
why is this required, isn't `Complete` a reasonable default? making this required lays unnecessary burdon on the user.
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/storage/MigrateSecondaryStorageDataCmd.java
##########
@@ -0,0 +1,124 @@
+// 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.api.command.admin.storage;
+
+import java.util.List;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ImageStoreResponse;
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.storage.ImageStore;
+import com.cloud.utils.StringUtils;
+
+@APICommand(name = MigrateSecondaryStorageDataCmd.APINAME,
+ description = "migrates data objects from one secondary storage to destination image store(s)",
+ responseObject = MigrationResponse.class,
+ entityType = {ImageStore.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.14.0",
+ authorized = {RoleType.Admin})
+public class MigrateSecondaryStorageDataCmd extends BaseAsyncCmd {
+
+ public static final Logger s_logger = Logger.getLogger(MigrateSecondaryStorageDataCmd.class.getName());
+
+ public static final String APINAME = "migrateSecondaryStorageData";
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.FROM,
+ type = CommandType.UUID,
+ entityType = ImageStoreResponse.class,
+ description = "id of the image store from where the data is to be migrated",
+ required = true)
+ private Long id;
+
+ @Parameter(name = ApiConstants.MIGRATE_TO,
+ type = CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = ImageStoreResponse.class,
+ description = "id of the destination secondary storage pool to which the templates are to be migrated to",
Review comment:
`id(s) of the destination storage pool(s)` and remove the last `to` from the sentence
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateImageStoreCmd.java
##########
@@ -0,0 +1,93 @@
+// 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.api.command.admin.storage;
+
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ImageStoreResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+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)
+public class UpdateImageStoreCmd extends BaseCmd {
+ private static final Logger LOG = Logger.getLogger(UpdateImageStoreCmd.class.getName());
+ public static final String APINAME = "updateImageStore";
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+ @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = ImageStoreResponse.class, required = true, description = "Image Store UUID")
+ private Long id;
+
+ @Parameter(name = ApiConstants.READ_ONLY, type = CommandType.BOOLEAN, required = true, description = "If set to true, it designates the corresponding image store to read-only")
Review comment:
can you expand on the usefullness of a store being read-only in this - or the API description? I.E. something like (in better English): "useful for deprecating image stores during storage migrations"
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/storage/MigrateSecondaryStorageDataCmd.java
##########
@@ -0,0 +1,124 @@
+// 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.api.command.admin.storage;
+
+import java.util.List;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ImageStoreResponse;
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.storage.ImageStore;
+import com.cloud.utils.StringUtils;
+
+@APICommand(name = MigrateSecondaryStorageDataCmd.APINAME,
+ description = "migrates data objects from one secondary storage to destination image store(s)",
+ responseObject = MigrationResponse.class,
+ entityType = {ImageStore.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.14.0",
+ authorized = {RoleType.Admin})
+public class MigrateSecondaryStorageDataCmd extends BaseAsyncCmd {
+
+ public static final Logger s_logger = Logger.getLogger(MigrateSecondaryStorageDataCmd.class.getName());
+
+ public static final String APINAME = "migrateSecondaryStorageData";
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.FROM,
+ type = CommandType.UUID,
+ entityType = ImageStoreResponse.class,
+ description = "id of the image store from where the data is to be migrated",
+ required = true)
+ private Long id;
+
+ @Parameter(name = ApiConstants.MIGRATE_TO,
+ type = CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = ImageStoreResponse.class,
+ description = "id of the destination secondary storage pool to which the templates are to be migrated to",
+ required = true)
+ private List<Long> migrateTo;
+
+ @Parameter(name = ApiConstants.MIGRATION_TYPE,
+ type = CommandType.STRING,
+ description = "Balance: if you want data to be distributed evenly among the destination stores, " +
+ "Complete: If you want to migrate the entire data from source image store to the destination store(s)",
+ required = true)
+ private String migrationType;
+
+ /////////////////////////////////////////////////////
+ /////////////////// Accessors ///////////////////////
+ /////////////////////////////////////////////////////
+
+ public Long getId() {
+ return id;
+ }
+
+ public List<Long> getMigrateTo() {
+ return migrateTo;
+ }
+
+ public String getMigrationType() {
+ return migrationType;
+ }
+
+ @Override
+ public String getEventType() {
+ return EventTypes.EVENT_FILE_MIGRATE;
+ }
+
+ @Override
+ public String getEventDescription() {
+ return "Attempting to migrate files/data objects " + "from : " + this.getId() + " to: " + StringUtils.join(getMigrateTo(), ",");
+ }
+
+ @Override
+ public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException {
Review comment:
Why throw so many different exceptions? we can catch most of them and give nice consise error messages in a ServerApiException, right?
Most of these are Runtime exception.
Mentioning them here is also redundant as they are declared in BaseCmd as well.
##########
File path: api/src/main/java/org/apache/cloudstack/api/response/MigrationResponse.java
##########
@@ -0,0 +1,73 @@
+// 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.api.response;
+
+import org.apache.cloudstack.api.BaseResponse;
+import org.apache.cloudstack.api.EntityReference;
+
+import com.cloud.serializer.Param;
+import com.cloud.storage.ImageStore;
+import com.google.gson.annotations.SerializedName;
+
+@EntityReference(value = ImageStore.class)
+public class MigrationResponse extends BaseResponse {
+ @SerializedName("message")
+ @Param(description = "Response message")
Review comment:
A user might wonder what this means: "Where does the messsage come from and what will it signify for me?" Can you expand on that, please?
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateImageStoreCmd.java
##########
@@ -0,0 +1,93 @@
+// 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.api.command.admin.storage;
+
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ImageStoreResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+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)
+public class UpdateImageStoreCmd extends BaseCmd {
+ private static final Logger LOG = Logger.getLogger(UpdateImageStoreCmd.class.getName());
+ public static final String APINAME = "updateImageStore";
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+ @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = ImageStoreResponse.class, required = true, description = "Image Store UUID")
+ private Long id;
+
+ @Parameter(name = ApiConstants.READ_ONLY, type = CommandType.BOOLEAN, required = true, description = "If set to true, it designates the corresponding image store to read-only")
+ private Boolean readonly;
+
+ /////////////////////////////////////////////////////
+ /////////////////// Accessors ///////////////////////
+ /////////////////////////////////////////////////////
+
+ public Long getId() {
+ return id;
+ }
+
+ public Boolean getReadonly() {
+ return readonly;
+ }
+
+ /////////////////////////////////////////////////////
+ /////////////// API Implementation///////////////////
+ /////////////////////////////////////////////////////
+
+ @Override
+ public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException {
Review comment:
same as above in `MigrateSecondaryStorageDataCmd`
##########
File path: engine/schema/src/main/java/com/cloud/secstorage/CommandExecLogDaoImpl.java
##########
@@ -43,4 +48,15 @@ public void expungeExpiredRecords(Date cutTime) {
sc.setParameters("created", cutTime);
expunge(sc);
}
+
+ @Override
+ public Integer getCopyCmdCountForSSVM(Long id) {
+ SearchCriteria<CommandExecLogVO> sc = CommandSearch.create();
+ sc.setParameters("host_id", id);
+ sc.setParameters("command_name", "CopyCommand");
+ List<CommandExecLogVO> copyCmds = customSearch(sc, null);
+ return copyCmds.size();
+ }
+
+
Review comment:
?
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -141,7 +141,6 @@
import com.cloud.vm.dao.UserVmDao;
public class VolumeOrchestrator extends ManagerBase implements VolumeOrchestrationService, Configurable {
-
Review comment:
makes sense but will lead possibly to unecessary conflicts during rebases, which are inevitable for this PR
##########
File path: engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/ImageStoreDaoImpl.java
##########
@@ -91,6 +91,20 @@ public ImageStoreVO findByName(String name) {
return listBy(sc);
}
+ @Override
+ public List<ImageStoreVO> findByScopeExcludingReadOnly(ZoneScope scope) {
+ SearchCriteria<ImageStoreVO> sc = createSearchCriteria();
+ sc.addAnd("role", SearchCriteria.Op.EQ, DataStoreRole.Image);
+ if (scope.getScopeId() != null) {
+ SearchCriteria<ImageStoreVO> scc = createSearchCriteria();
+ scc.addOr("scope", SearchCriteria.Op.EQ, ScopeType.REGION);
+ scc.addOr("dcId", SearchCriteria.Op.EQ, scope.getScopeId());
+ sc.addAnd("scope", SearchCriteria.Op.SC, scc);
+ sc.addAnd("readonly", SearchCriteria.Op.EQ, Boolean.FALSE);
Review comment:
so this line could be surrounded by `if (readonlyExcludedFlag == true) {}`
##########
File path: engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/ImageStoreDao.java
##########
@@ -31,6 +31,8 @@
List<ImageStoreVO> findByScope(ZoneScope scope);
+ List<ImageStoreVO> findByScopeExcludingReadOnly(ZoneScope scope);
Review comment:
this method name is confusing: `findByScope` but than the scope is allready set to `ZoneScope`? Wouldn't `findByZone` be better?
Also the method above could probably just be extended with a flag for `excludeReadOnly`.
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -0,0 +1,629 @@
+// 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.engine.orchestration;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.LinkedHashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService.DataObjectResult;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
+import org.apache.cloudstack.framework.async.AsyncCallFuture;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.jobs.AsyncJobManager;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
+import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
+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.commons.math3.stat.descriptive.moment.Mean;
+import org.apache.commons.math3.stat.descriptive.moment.StandardDeviation;
+import org.apache.log4j.Logger;
+
+import com.cloud.configuration.Config;
+import com.cloud.host.HostVO;
+import com.cloud.host.Status;
+import com.cloud.host.dao.HostDao;
+import com.cloud.server.StatsCollector;
+import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.ImageStoreService.MigrationPolicy;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.StorageService;
+import com.cloud.storage.StorageStats;
+import com.cloud.storage.VMTemplateVO;
+import com.cloud.storage.dao.SnapshotDao;
+import com.cloud.storage.dao.VMTemplateDao;
+import com.cloud.utils.NumbersUtil;
+import com.cloud.utils.Pair;
+import com.cloud.utils.StringUtils;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.SecondaryStorageVm;
+import com.cloud.vm.SecondaryStorageVmVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.SecondaryStorageVmDao;
+
+public class StorageOrchestrator extends ManagerBase implements StorageOrchestrationService, Configurable {
+
+ private static final Logger s_logger = Logger.getLogger(StorageOrchestrator.class);
+ @Inject
+ TemplateDataStoreDao templateDataStoreDao;
+ @Inject
+ SnapshotDataStoreDao snapshotDataStoreDao;
+ @Inject
+ VolumeDataStoreDao volumeDataStoreDao;
+ @Inject
+ VolumeDataFactory volumeFactory;
+ @Inject
+ VMTemplateDao templateDao;
+ @Inject
+ TemplateDataFactory templateFactory;
+ @Inject
+ SnapshotDao snapshotDao;
+ @Inject
+ SnapshotDataFactory snapshotFactory;
+ @Inject
+ DataStoreManager dataStoreManager;
+ @Inject
+ ImageStoreDao imageStoreDao;
+ @Inject
+ StatsCollector statsCollector;
+ @Inject
+ public StorageService storageService;
+ @Inject
+ SecondaryStorageVmDao secStorageVmDao;
+ @Inject
+ ConfigurationDao configDao;
+ @Inject
+ HostDao hostDao;
+ @Inject
+ private AsyncJobManager jobMgr;
+ @Inject
+ private SecondaryStorageService secStgSrv;
+
+ ConfigKey<Double> ImageStoreImbalanceThreshold = new ConfigKey<>("Advanced", Double.class,
+ "image.store.imbalance.threshold",
+ "0.1",
+ "The storage imbalance threshold that is compared with the standard deviation percentage for a storage utilization metric. " +
+ "The value is a percentage in decimal format.",
+ true, ConfigKey.Scope.Global);
+
+ Integer numConcurrentCopyTasksPerSSVM = 2;
+
+ private double imageStoreCapacityThreshold = 0.90;
+
+ @Override
+ public String getConfigComponentName() {
+ return StorageOrchestrationService.class.getName();
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return new ConfigKey<?>[]{ImageStoreImbalanceThreshold};
+ }
+
+ static class MigrateBlockingQueue<T> extends ArrayBlockingQueue<T> {
+
+ MigrateBlockingQueue(int size) {
+ super(size);
+ }
+
+ public boolean offer(T task) {
+ try {
+ this.put(task);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ return true;
+ }
+ }
+
+ @Override
+ public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+ numConcurrentCopyTasksPerSSVM = NumbersUtil.parseInt(configDao.getValue(Config.SecStorageMaxMigrateSessions.key()), 2);
+ return true;
+ }
+
+ @Override
+ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy) {
Review comment:
this method is way too long. please modularise it into byte size chunks and factor it out to a worker - or utility class?
I can not set a rule but in my opinion any method larger than 20 line or any class larger than 300 lines is probably not separating concerns.
every comment in this method should probably be converted to a good method name and extracted, and so should many top level blocks (if-, for- and while statements
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -0,0 +1,629 @@
+// 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.engine.orchestration;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.LinkedHashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService.DataObjectResult;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
+import org.apache.cloudstack.framework.async.AsyncCallFuture;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.jobs.AsyncJobManager;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
+import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
+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.commons.math3.stat.descriptive.moment.Mean;
+import org.apache.commons.math3.stat.descriptive.moment.StandardDeviation;
+import org.apache.log4j.Logger;
+
+import com.cloud.configuration.Config;
+import com.cloud.host.HostVO;
+import com.cloud.host.Status;
+import com.cloud.host.dao.HostDao;
+import com.cloud.server.StatsCollector;
+import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.ImageStoreService.MigrationPolicy;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.StorageService;
+import com.cloud.storage.StorageStats;
+import com.cloud.storage.VMTemplateVO;
+import com.cloud.storage.dao.SnapshotDao;
+import com.cloud.storage.dao.VMTemplateDao;
+import com.cloud.utils.NumbersUtil;
+import com.cloud.utils.Pair;
+import com.cloud.utils.StringUtils;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.SecondaryStorageVm;
+import com.cloud.vm.SecondaryStorageVmVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.SecondaryStorageVmDao;
+
+public class StorageOrchestrator extends ManagerBase implements StorageOrchestrationService, Configurable {
+
+ private static final Logger s_logger = Logger.getLogger(StorageOrchestrator.class);
+ @Inject
+ TemplateDataStoreDao templateDataStoreDao;
+ @Inject
+ SnapshotDataStoreDao snapshotDataStoreDao;
+ @Inject
+ VolumeDataStoreDao volumeDataStoreDao;
+ @Inject
+ VolumeDataFactory volumeFactory;
+ @Inject
+ VMTemplateDao templateDao;
+ @Inject
+ TemplateDataFactory templateFactory;
+ @Inject
+ SnapshotDao snapshotDao;
+ @Inject
+ SnapshotDataFactory snapshotFactory;
+ @Inject
+ DataStoreManager dataStoreManager;
+ @Inject
+ ImageStoreDao imageStoreDao;
+ @Inject
+ StatsCollector statsCollector;
+ @Inject
+ public StorageService storageService;
+ @Inject
+ SecondaryStorageVmDao secStorageVmDao;
+ @Inject
+ ConfigurationDao configDao;
+ @Inject
+ HostDao hostDao;
+ @Inject
+ private AsyncJobManager jobMgr;
+ @Inject
+ private SecondaryStorageService secStgSrv;
+
+ ConfigKey<Double> ImageStoreImbalanceThreshold = new ConfigKey<>("Advanced", Double.class,
+ "image.store.imbalance.threshold",
+ "0.1",
+ "The storage imbalance threshold that is compared with the standard deviation percentage for a storage utilization metric. " +
+ "The value is a percentage in decimal format.",
+ true, ConfigKey.Scope.Global);
+
+ Integer numConcurrentCopyTasksPerSSVM = 2;
+
+ private double imageStoreCapacityThreshold = 0.90;
+
+ @Override
+ public String getConfigComponentName() {
+ return StorageOrchestrationService.class.getName();
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return new ConfigKey<?>[]{ImageStoreImbalanceThreshold};
+ }
+
+ static class MigrateBlockingQueue<T> extends ArrayBlockingQueue<T> {
+
+ MigrateBlockingQueue(int size) {
+ super(size);
+ }
+
+ public boolean offer(T task) {
+ try {
+ this.put(task);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ return true;
+ }
+ }
+
+ @Override
+ public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+ numConcurrentCopyTasksPerSSVM = NumbersUtil.parseInt(configDao.getValue(Config.SecStorageMaxMigrateSessions.key()), 2);
+ return true;
+ }
+
+ @Override
+ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy) {
+ List<DataObject> files = new LinkedList<>();
+ int successCount = 0;
+ boolean success = true;
+ String message = null;
+
+ if (migrationPolicy == MigrationPolicy.COMPLETE) {
+ if (!filesReady(srcDataStoreId)) {
+ throw new CloudRuntimeException("Complete migration failed as there are data objects which are not Ready");
+ }
+ }
+
+ DataStore srcDatastore = dataStoreManager.getDataStore(srcDataStoreId, DataStoreRole.Image);
+ Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains = new HashMap<>();
+ files.addAll(getAllValidTemplates(srcDatastore));
+ files.addAll(getAllValidSnapshotChains(srcDatastore, snapshotChains));
+ files.addAll(getAllValidVolumes(srcDatastore));
Review comment:
or: `prepareSourcesList(...)`
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -0,0 +1,629 @@
+// 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.engine.orchestration;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.LinkedHashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService.DataObjectResult;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
+import org.apache.cloudstack.framework.async.AsyncCallFuture;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.jobs.AsyncJobManager;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
+import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
+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.commons.math3.stat.descriptive.moment.Mean;
+import org.apache.commons.math3.stat.descriptive.moment.StandardDeviation;
+import org.apache.log4j.Logger;
+
+import com.cloud.configuration.Config;
+import com.cloud.host.HostVO;
+import com.cloud.host.Status;
+import com.cloud.host.dao.HostDao;
+import com.cloud.server.StatsCollector;
+import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.ImageStoreService.MigrationPolicy;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.StorageService;
+import com.cloud.storage.StorageStats;
+import com.cloud.storage.VMTemplateVO;
+import com.cloud.storage.dao.SnapshotDao;
+import com.cloud.storage.dao.VMTemplateDao;
+import com.cloud.utils.NumbersUtil;
+import com.cloud.utils.Pair;
+import com.cloud.utils.StringUtils;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.SecondaryStorageVm;
+import com.cloud.vm.SecondaryStorageVmVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.SecondaryStorageVmDao;
+
+public class StorageOrchestrator extends ManagerBase implements StorageOrchestrationService, Configurable {
+
+ private static final Logger s_logger = Logger.getLogger(StorageOrchestrator.class);
+ @Inject
+ TemplateDataStoreDao templateDataStoreDao;
+ @Inject
+ SnapshotDataStoreDao snapshotDataStoreDao;
+ @Inject
+ VolumeDataStoreDao volumeDataStoreDao;
+ @Inject
+ VolumeDataFactory volumeFactory;
+ @Inject
+ VMTemplateDao templateDao;
+ @Inject
+ TemplateDataFactory templateFactory;
+ @Inject
+ SnapshotDao snapshotDao;
+ @Inject
+ SnapshotDataFactory snapshotFactory;
+ @Inject
+ DataStoreManager dataStoreManager;
+ @Inject
+ ImageStoreDao imageStoreDao;
+ @Inject
+ StatsCollector statsCollector;
+ @Inject
+ public StorageService storageService;
+ @Inject
+ SecondaryStorageVmDao secStorageVmDao;
+ @Inject
+ ConfigurationDao configDao;
+ @Inject
+ HostDao hostDao;
+ @Inject
+ private AsyncJobManager jobMgr;
+ @Inject
+ private SecondaryStorageService secStgSrv;
+
+ ConfigKey<Double> ImageStoreImbalanceThreshold = new ConfigKey<>("Advanced", Double.class,
+ "image.store.imbalance.threshold",
+ "0.1",
+ "The storage imbalance threshold that is compared with the standard deviation percentage for a storage utilization metric. " +
+ "The value is a percentage in decimal format.",
+ true, ConfigKey.Scope.Global);
+
+ Integer numConcurrentCopyTasksPerSSVM = 2;
+
+ private double imageStoreCapacityThreshold = 0.90;
+
+ @Override
+ public String getConfigComponentName() {
+ return StorageOrchestrationService.class.getName();
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return new ConfigKey<?>[]{ImageStoreImbalanceThreshold};
+ }
+
+ static class MigrateBlockingQueue<T> extends ArrayBlockingQueue<T> {
+
+ MigrateBlockingQueue(int size) {
+ super(size);
+ }
+
+ public boolean offer(T task) {
+ try {
+ this.put(task);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ return true;
+ }
+ }
+
+ @Override
+ public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+ numConcurrentCopyTasksPerSSVM = NumbersUtil.parseInt(configDao.getValue(Config.SecStorageMaxMigrateSessions.key()), 2);
+ return true;
+ }
+
+ @Override
+ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy) {
+ List<DataObject> files = new LinkedList<>();
+ int successCount = 0;
+ boolean success = true;
+ String message = null;
+
+ if (migrationPolicy == MigrationPolicy.COMPLETE) {
+ if (!filesReady(srcDataStoreId)) {
+ throw new CloudRuntimeException("Complete migration failed as there are data objects which are not Ready");
+ }
+ }
Review comment:
example extraction: above code could go in `checkCompleteMigrationFor ReadyVolumesOnly(...)`
##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
##########
@@ -39,9 +33,14 @@
import org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
import org.apache.cloudstack.framework.async.AsyncCompletionCallback;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
import com.cloud.agent.api.to.VirtualMachineTO;
import com.cloud.host.Host;
+import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.dao.VolumeDao;
Review comment:
unecessary conlict potential
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -0,0 +1,629 @@
+// 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.engine.orchestration;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.LinkedHashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService.DataObjectResult;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
+import org.apache.cloudstack.framework.async.AsyncCallFuture;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.jobs.AsyncJobManager;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
+import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
+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.commons.math3.stat.descriptive.moment.Mean;
+import org.apache.commons.math3.stat.descriptive.moment.StandardDeviation;
+import org.apache.log4j.Logger;
+
+import com.cloud.configuration.Config;
+import com.cloud.host.HostVO;
+import com.cloud.host.Status;
+import com.cloud.host.dao.HostDao;
+import com.cloud.server.StatsCollector;
+import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.ImageStoreService.MigrationPolicy;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.StorageService;
+import com.cloud.storage.StorageStats;
+import com.cloud.storage.VMTemplateVO;
+import com.cloud.storage.dao.SnapshotDao;
+import com.cloud.storage.dao.VMTemplateDao;
+import com.cloud.utils.NumbersUtil;
+import com.cloud.utils.Pair;
+import com.cloud.utils.StringUtils;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.SecondaryStorageVm;
+import com.cloud.vm.SecondaryStorageVmVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.SecondaryStorageVmDao;
+
+public class StorageOrchestrator extends ManagerBase implements StorageOrchestrationService, Configurable {
+
+ private static final Logger s_logger = Logger.getLogger(StorageOrchestrator.class);
+ @Inject
+ TemplateDataStoreDao templateDataStoreDao;
+ @Inject
+ SnapshotDataStoreDao snapshotDataStoreDao;
+ @Inject
+ VolumeDataStoreDao volumeDataStoreDao;
+ @Inject
+ VolumeDataFactory volumeFactory;
+ @Inject
+ VMTemplateDao templateDao;
+ @Inject
+ TemplateDataFactory templateFactory;
+ @Inject
+ SnapshotDao snapshotDao;
+ @Inject
+ SnapshotDataFactory snapshotFactory;
+ @Inject
+ DataStoreManager dataStoreManager;
+ @Inject
+ ImageStoreDao imageStoreDao;
+ @Inject
+ StatsCollector statsCollector;
+ @Inject
+ public StorageService storageService;
+ @Inject
+ SecondaryStorageVmDao secStorageVmDao;
+ @Inject
+ ConfigurationDao configDao;
+ @Inject
+ HostDao hostDao;
+ @Inject
+ private AsyncJobManager jobMgr;
+ @Inject
+ private SecondaryStorageService secStgSrv;
+
+ ConfigKey<Double> ImageStoreImbalanceThreshold = new ConfigKey<>("Advanced", Double.class,
+ "image.store.imbalance.threshold",
+ "0.1",
+ "The storage imbalance threshold that is compared with the standard deviation percentage for a storage utilization metric. " +
+ "The value is a percentage in decimal format.",
+ true, ConfigKey.Scope.Global);
+
+ Integer numConcurrentCopyTasksPerSSVM = 2;
+
+ private double imageStoreCapacityThreshold = 0.90;
+
+ @Override
+ public String getConfigComponentName() {
+ return StorageOrchestrationService.class.getName();
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return new ConfigKey<?>[]{ImageStoreImbalanceThreshold};
+ }
+
+ static class MigrateBlockingQueue<T> extends ArrayBlockingQueue<T> {
+
+ MigrateBlockingQueue(int size) {
+ super(size);
+ }
+
+ public boolean offer(T task) {
+ try {
+ this.put(task);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ return true;
+ }
+ }
+
+ @Override
+ public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+ numConcurrentCopyTasksPerSSVM = NumbersUtil.parseInt(configDao.getValue(Config.SecStorageMaxMigrateSessions.key()), 2);
+ return true;
+ }
+
+ @Override
+ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy) {
+ List<DataObject> files = new LinkedList<>();
+ int successCount = 0;
+ boolean success = true;
+ String message = null;
+
+ if (migrationPolicy == MigrationPolicy.COMPLETE) {
+ if (!filesReady(srcDataStoreId)) {
+ throw new CloudRuntimeException("Complete migration failed as there are data objects which are not Ready");
+ }
+ }
+
+ DataStore srcDatastore = dataStoreManager.getDataStore(srcDataStoreId, DataStoreRole.Image);
+ Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains = new HashMap<>();
+ files.addAll(getAllValidTemplates(srcDatastore));
+ files.addAll(getAllValidSnapshotChains(srcDatastore, snapshotChains));
+ files.addAll(getAllValidVolumes(srcDatastore));
+
+ Collections.sort(files, new Comparator<DataObject>() {
+ @Override
+ public int compare(DataObject o1, DataObject o2) {
+ Long size1 = o1.getSize();
+ Long size2 = o2.getSize();
+ if (o1 instanceof SnapshotInfo) {
+ size1 = snapshotChains.get(o1).second();
+ }
+ if (o2 instanceof SnapshotInfo) {
+ size2 = snapshotChains.get(o2).second();
+ }
+ //return o2.getSize() > o1.getSize() ? 1 : -1;
+ return size2 > size1 ? 1 : -1;
+ }
+ });
Review comment:
`sortSources(...), should probably be called from `prepareSourcesList()`
etc.
etc.
hope you get my drift. The code looks good but this is about maintainability. happy to discuss.
##########
File path: engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/ObjectInDataStoreStateMachine.java
##########
@@ -49,8 +50,12 @@ public String getDescription() {
DestroyRequested,
OperationSuccessed,
OperationFailed,
+ // Added as volume converts migrationrequested to copyrequested - VolumeObject.java
Review comment:
I don't understand this comment, should it still be here or was it only there as development aid?
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -0,0 +1,629 @@
+// 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.engine.orchestration;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.LinkedHashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+
+import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService;
+import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService.DataObjectResult;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
+import org.apache.cloudstack.framework.async.AsyncCallFuture;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.jobs.AsyncJobManager;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
+import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
+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.commons.math3.stat.descriptive.moment.Mean;
+import org.apache.commons.math3.stat.descriptive.moment.StandardDeviation;
+import org.apache.log4j.Logger;
+
+import com.cloud.configuration.Config;
+import com.cloud.host.HostVO;
+import com.cloud.host.Status;
+import com.cloud.host.dao.HostDao;
+import com.cloud.server.StatsCollector;
+import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.ImageStoreService.MigrationPolicy;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.StorageService;
+import com.cloud.storage.StorageStats;
+import com.cloud.storage.VMTemplateVO;
+import com.cloud.storage.dao.SnapshotDao;
+import com.cloud.storage.dao.VMTemplateDao;
+import com.cloud.utils.NumbersUtil;
+import com.cloud.utils.Pair;
+import com.cloud.utils.StringUtils;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.SecondaryStorageVm;
+import com.cloud.vm.SecondaryStorageVmVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.SecondaryStorageVmDao;
+
+public class StorageOrchestrator extends ManagerBase implements StorageOrchestrationService, Configurable {
+
+ private static final Logger s_logger = Logger.getLogger(StorageOrchestrator.class);
+ @Inject
+ TemplateDataStoreDao templateDataStoreDao;
+ @Inject
+ SnapshotDataStoreDao snapshotDataStoreDao;
+ @Inject
+ VolumeDataStoreDao volumeDataStoreDao;
+ @Inject
+ VolumeDataFactory volumeFactory;
+ @Inject
+ VMTemplateDao templateDao;
+ @Inject
+ TemplateDataFactory templateFactory;
+ @Inject
+ SnapshotDao snapshotDao;
+ @Inject
+ SnapshotDataFactory snapshotFactory;
+ @Inject
+ DataStoreManager dataStoreManager;
+ @Inject
+ ImageStoreDao imageStoreDao;
+ @Inject
+ StatsCollector statsCollector;
+ @Inject
+ public StorageService storageService;
+ @Inject
+ SecondaryStorageVmDao secStorageVmDao;
+ @Inject
+ ConfigurationDao configDao;
+ @Inject
+ HostDao hostDao;
+ @Inject
+ private AsyncJobManager jobMgr;
+ @Inject
+ private SecondaryStorageService secStgSrv;
+
+ ConfigKey<Double> ImageStoreImbalanceThreshold = new ConfigKey<>("Advanced", Double.class,
+ "image.store.imbalance.threshold",
+ "0.1",
+ "The storage imbalance threshold that is compared with the standard deviation percentage for a storage utilization metric. " +
+ "The value is a percentage in decimal format.",
+ true, ConfigKey.Scope.Global);
+
+ Integer numConcurrentCopyTasksPerSSVM = 2;
+
+ private double imageStoreCapacityThreshold = 0.90;
+
+ @Override
+ public String getConfigComponentName() {
+ return StorageOrchestrationService.class.getName();
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return new ConfigKey<?>[]{ImageStoreImbalanceThreshold};
+ }
+
+ static class MigrateBlockingQueue<T> extends ArrayBlockingQueue<T> {
+
+ MigrateBlockingQueue(int size) {
+ super(size);
+ }
+
+ public boolean offer(T task) {
+ try {
+ this.put(task);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ return true;
+ }
+ }
+
+ @Override
+ public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+ numConcurrentCopyTasksPerSSVM = NumbersUtil.parseInt(configDao.getValue(Config.SecStorageMaxMigrateSessions.key()), 2);
+ return true;
+ }
+
+ @Override
+ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy) {
+ List<DataObject> files = new LinkedList<>();
+ int successCount = 0;
+ boolean success = true;
+ String message = null;
+
+ if (migrationPolicy == MigrationPolicy.COMPLETE) {
+ if (!filesReady(srcDataStoreId)) {
+ throw new CloudRuntimeException("Complete migration failed as there are data objects which are not Ready");
+ }
+ }
+
+ DataStore srcDatastore = dataStoreManager.getDataStore(srcDataStoreId, DataStoreRole.Image);
+ Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains = new HashMap<>();
+ files.addAll(getAllValidTemplates(srcDatastore));
+ files.addAll(getAllValidSnapshotChains(srcDatastore, snapshotChains));
+ files.addAll(getAllValidVolumes(srcDatastore));
+
+ Collections.sort(files, new Comparator<DataObject>() {
+ @Override
+ public int compare(DataObject o1, DataObject o2) {
+ Long size1 = o1.getSize();
+ Long size2 = o2.getSize();
+ if (o1 instanceof SnapshotInfo) {
+ size1 = snapshotChains.get(o1).second();
+ }
+ if (o2 instanceof SnapshotInfo) {
+ size2 = snapshotChains.get(o2).second();
+ }
+ //return o2.getSize() > o1.getSize() ? 1 : -1;
+ return size2 > size1 ? 1 : -1;
Review comment:
so never 0? Can we return size2 - size1?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org