weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2035455775
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -138,6 +144,14 @@ public interface BackupManager extends BackupService,
Configurable, PluggableSer
ConfigKey.Scope.Global,
null);
+ ConfigKey<Float> BackupStorageCapacityThreshold = new ConfigKey<>("Alert",
Float.class,
Review Comment:
do we need to a setting to disable threshold ?
##########
api/src/main/java/com/cloud/vm/VirtualMachine.java:
##########
@@ -128,7 +128,6 @@ public static StateMachine2<State, VirtualMachine.Event,
VirtualMachine> getStat
s_fsm.addTransition(new Transition<State, Event>(State.Error,
VirtualMachine.Event.DestroyRequested, State.Expunging, null));
s_fsm.addTransition(new Transition<State, Event>(State.Error,
VirtualMachine.Event.ExpungeOperation, State.Expunging, null));
s_fsm.addTransition(new Transition<State, Event>(State.Stopped,
Event.RestoringRequested, State.Restoring, null));
- s_fsm.addTransition(new Transition<State, Event>(State.Expunging,
Event.RestoringRequested, State.Restoring, null));
Review Comment:
makes sense
is it needed for 4.20 ? (a trivial issue as I can see)
##########
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java:
##########
@@ -1224,4 +1224,14 @@ public Map<String, Long>
getNameIdMapForVmIds(Collection<Long> ids) {
return vms.stream()
.collect(Collectors.toMap(VMInstanceVO::getInstanceName,
VMInstanceVO::getId));
}
+
+ @Override
+ public List<VMInstanceVO> listByIds(List<Long> ids) {
Review Comment:
1. `UserVmDaoImpl` has the same method
2. in case of `listIncludingRemovedBy` (not `listBy`), would it better to
rename the method ?
##########
engine/schema/src/main/java/org/apache/cloudstack/backup/BackupDetailVO.java:
##########
@@ -0,0 +1,98 @@
+// 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.backup;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+
+import org.apache.cloudstack.api.ResourceDetail;
+
+@Entity
+@Table(name = "backup_details")
+public class BackupDetailVO implements ResourceDetail {
+ @Id
+ @GeneratedValue(strategy = GenerationType.IDENTITY)
+ @Column(name = "id")
+ private long id;
+
+ @Column(name = "backup_id")
+ private long resourceId;
+
+ @Column(name = "name")
+ private String name;
+
+ @Column(name = "value", length = 1024)
Review Comment:
is 1024 big enough ?
##########
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##########
@@ -102,6 +111,18 @@ public class BackupResponse extends BaseResponse {
@Param(description = "zone name")
private String zone;
+ @SerializedName(ApiConstants.VM_DETAILS)
+ @Param(description = "Lists the vm specific details for the backup", since
= "4.21.0")
+ private Map<String, String> vmDetails;
+
+ @SerializedName(ApiConstants.INTERVAL_TYPE)
+ @Param(description = "the interval type of the backup schedule", since =
"4.21.0")
+ private String intervalType;
+
+ @SerializedName(ApiConstants.BACKUP_VM_OFFERING_REMOVED)
Review Comment:
is it used somewhere, except in the response ?
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java:
##########
@@ -147,6 +148,13 @@ public class DeployVMCmd extends
BaseAsyncCreateCustomIdCmd implements SecurityG
since = "4.4")
private Long rootdisksize;
+ @Parameter(name = ApiConstants.DATADISKS_DETAILS,
+ type = CommandType.MAP,
+ since = "4.21.0",
+ description = "Disk offering details for creating multiple data
volumes. Mutually exclusibe with diskOfferingId." +
+ " Example:
datadisksdetails[0].diskofferingid=1&datadisksdetails[0].size=10&datadisksdetails[0].miniops=100&datadisksdetails[0].maxiops=200")
Review Comment:
is `diskofferingid` a id (long) or uuid (string)?
in case of uuid, it would be good to change the value `1` to a uuid string
in this example
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/CloudOrchestrator.java:
##########
@@ -225,6 +226,7 @@ public VirtualMachineEntity createVirtualMachine(String id,
String owner, String
DiskOfferingInfo dataDiskOfferingInfo = new DiskOfferingInfo();
dataDiskOfferingInfo.setDiskOffering(diskOffering);
+ dataDiskOfferingInfo.setDeviceId(1L);
Review Comment:
if the vm is created from an ISO (not template), is the data disk offering
used for root volume ?
did not test yet
##########
api/src/main/java/org/apache/cloudstack/api/response/ObjectStoreResponse.java:
##########
@@ -44,6 +46,10 @@ public class ObjectStoreResponse extends
BaseResponseWithAnnotations {
@Param(description = "the total size of the object store")
private Long storageTotal;
+ @SerializedName("storageallocated")
+ @Param(description = "the object store currently allocated size")
Review Comment:
`the allocated size of the oject store` ?
##########
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java:
##########
@@ -220,6 +220,14 @@ public interface StorageManager extends StorageService {
"storage.pool.host.connect.workers", "1",
"Number of worker threads to be used to connect hosts to a primary
storage", true);
+ ConfigKey<Float> ObjectStorageCapacityThreshold = new ConfigKey<>("Alert",
Float.class,
+ "zone.objectStorage.capacity.notificationthreshold",
+ "0.75",
+ "Percentage (as a value between 0 and 1) of object storage
utilization above which alerts will be sent about low storage available.",
+ true,
+ ConfigKey.Scope.Global,
Review Comment:
is it a global setting or zone setting ?
the key has `zone`, but the scope is `Global`
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java:
##########
@@ -147,6 +148,13 @@ public class DeployVMCmd extends
BaseAsyncCreateCustomIdCmd implements SecurityG
since = "4.4")
private Long rootdisksize;
+ @Parameter(name = ApiConstants.DATADISKS_DETAILS,
+ type = CommandType.MAP,
+ since = "4.21.0",
+ description = "Disk offering details for creating multiple data
volumes. Mutually exclusibe with diskOfferingId." +
Review Comment:
exclusibe -> exclusive
##########
engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql:
##########
@@ -37,3 +33,38 @@ WHERE rp.rule = 'quotaStatement'
AND NOT EXISTS(SELECT 1 FROM cloud.role_permissions rp_ WHERE rp.role_id =
rp_.role_id AND rp_.rule = 'quotaCreditsList');
CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.host', 'last_mgmt_server_id',
'bigint unsigned DEFAULT NULL COMMENT "last management server this host is
connected to" AFTER `mgmt_server_id`');
+
+-- Add column max_backup to backup_schedule table
+ALTER TABLE `cloud`.`backup_schedule` ADD COLUMN `max_backups` int(8) default
NULL COMMENT 'maximum number of backups to maintain';
Review Comment:
call `IDEMPOTENT_ADD_COLUMN` here ?
##########
engine/schema/src/main/java/org/apache/cloudstack/backup/BackupDetailVO.java:
##########
@@ -0,0 +1,98 @@
+// 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.backup;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+
+import org.apache.cloudstack.api.ResourceDetail;
+
+@Entity
+@Table(name = "backup_details")
+public class BackupDetailVO implements ResourceDetail {
+ @Id
+ @GeneratedValue(strategy = GenerationType.IDENTITY)
+ @Column(name = "id")
+ private long id;
+
+ @Column(name = "backup_id")
+ private long resourceId;
+
+ @Column(name = "name")
+ private String name;
+
+ @Column(name = "value", length = 1024)
+ private String value;
+
+ @Column(name = "display")
+ private boolean display = true;
+
+ public BackupDetailVO() {
+ }
+
+ public BackupDetailVO(long templateId, String name, String value, boolean
display) {
Review Comment:
templateId or backupId ?
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/resource/ListCapacityCmd.java:
##########
@@ -134,6 +134,8 @@ public int compare(CapacityResponse resp1, CapacityResponse
resp2) {
int res = resp1.getZoneName().compareTo(resp2.getZoneName());
if (res != 0) {
return res;
+ } else if (getSortBy() != null) {
Review Comment:
makes sense
will you create a separated pr for 4.19/4.20 ? @abh1sar
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmd.java:
##########
@@ -56,6 +56,9 @@ public class AddObjectStoragePoolCmd extends BaseCmd {
@Parameter(name = ApiConstants.TAGS, type = CommandType.STRING,
description = "the tags for the storage pool")
private String tags;
+ @Parameter(name = ApiConstants.SIZE, type = CommandType.LONG, description
= "the total size of the object store in GiB. Used for tracking capacity and
sending alerts", since = "4.21")
Review Comment:
@abh1sar
does it mean that CloudStack does not get the actual capacity ?
It relies on the admins to pass correct value, right ?
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vm/CreateVMFromBackupCmd.java:
##########
@@ -0,0 +1,127 @@
+// 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.user.vm;
+
+import javax.inject.Inject;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ResponseObject;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.command.user.UserCmd;
+import org.apache.cloudstack.api.response.BackupResponse;
+import org.apache.cloudstack.api.response.UserVmResponse;
+import org.apache.cloudstack.backup.BackupManager;
+
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.InsufficientServerCapacityException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.uservm.UserVm;
+import com.cloud.vm.VirtualMachine;
+
+@APICommand(name = "createVMFromBackup",
+ description = "Creates and automatically starts a VM from a backup.",
Review Comment:
DeployVMCmd has a parameter `start`, is it honored by this command ?
##########
engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupDaoImpl.java:
##########
@@ -158,6 +196,27 @@ public void removeExistingBackups(Long zoneId, Long vmId) {
expunge(sc);
}
+ @Override
+ public BackupVO persist(BackupVO backup) {
+ return Transaction.execute((TransactionCallback<BackupVO>) status -> {
+ BackupVO backupDb = super.persist(backup);
+ saveDetails(backup);
+ loadDetails(backupDb);
+ return backupDb;
+ });
+ }
+
+ @Override
+ public boolean update(Long id, BackupVO backup) {
+ return Transaction.execute((TransactionCallback<Boolean>) status -> {
+ boolean result = super.update(id, backup);
+ if (result == true) {
Review Comment:
`== true` is not needed
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
