JoaoJandre commented on code in PR #10140: URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2205604506
########## engine/schema/src/main/java/org/apache/cloudstack/backup/BackupVO.java: ########## @@ -240,10 +268,32 @@ public void setBackedUpVolumes(String backedUpVolumes) { this.backedUpVolumes = backedUpVolumes; } + @Override + public Map<String, String> getDetails() { + return details; + } + + public void setDetail(String name, String value) { + assert (details != null) : "Did you forget to load the details?"; Review Comment: Using an if is better here. ########## api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java: ########## @@ -110,9 +111,28 @@ public interface BackupProvider { /** * This method should TODO Review Comment: Please describe what this method should do. ########## api/src/main/java/org/apache/cloudstack/api/command/admin/vm/CreateVMFromBackupCmdByAdmin.java: ########## @@ -0,0 +1,55 @@ +// 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.vm; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ResponseObject; +import org.apache.cloudstack.api.command.admin.AdminCmd; +import org.apache.cloudstack.api.command.user.vm.CreateVMFromBackupCmd; +import org.apache.cloudstack.api.response.ClusterResponse; +import org.apache.cloudstack.api.response.PodResponse; +import org.apache.cloudstack.api.response.UserVmResponse; + +import com.cloud.vm.VirtualMachine; + +@APICommand(name = "createVMFromBackup", + description = "Creates and automatically starts a VM from a backup.", + responseObject = UserVmResponse.class, + responseView = ResponseObject.ResponseView.Full, + entityType = {VirtualMachine.class}, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = true, + since = "4.21.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class CreateVMFromBackupCmdByAdmin extends CreateVMFromBackupCmd implements AdminCmd { + + @Parameter(name = ApiConstants.POD_ID, type = CommandType.UUID, entityType = PodResponse.class, description = "destination Pod ID to deploy the VM to - parameter available for root admin only", since = "4.13") + private Long podId; + + @Parameter(name = ApiConstants.CLUSTER_ID, type = CommandType.UUID, entityType = ClusterResponse.class, description = "destination Cluster ID to deploy the VM to - parameter available for root admin only", since = "4.13") Review Comment: ```suggestion @Parameter(name = ApiConstants.CLUSTER_ID, type = CommandType.UUID, entityType = ClusterResponse.class, description = "destination Cluster ID to deploy the VM to - parameter available for root admin only", since = "4.21") ``` ########## api/src/main/java/org/apache/cloudstack/api/command/user/vm/CreateVMFromBackupCmd.java: ########## @@ -0,0 +1,159 @@ +// 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.ACL; +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.response.BackupResponse; +import org.apache.cloudstack.api.response.ServiceOfferingResponse; +import org.apache.cloudstack.api.response.TemplateResponse; +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.", + responseObject = UserVmResponse.class, + responseView = ResponseObject.ResponseView.Restricted, + entityType = {VirtualMachine.class}, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = true, + since = "4.21.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class CreateVMFromBackupCmd extends BaseDeployVMCmd { + + @Inject + BackupManager backupManager; Review Comment: Is this used in this cmd? ########## api/src/main/java/org/apache/cloudstack/api/command/user/backup/ListBackupsCmd.java: ########## @@ -87,10 +107,22 @@ public Long getVmId() { return vmId; } + public String getName() { + return name; + } + + public Long getBackupOfferingId() { + return backupOfferingId; Review Comment: are these methods used anywhere? ########## api/src/main/java/com/cloud/offering/DiskOfferingInfo.java: ########## @@ -23,6 +23,7 @@ public class DiskOfferingInfo { private Long _size; private Long _minIops; private Long _maxIops; + private Long _deviceId; Review Comment: A disk offering should not have a deviceId tied to it, don't you agree? ########## api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java: ########## @@ -110,9 +111,28 @@ public interface BackupProvider { /** * This method should TODO + * * @param restorePoint the restore point to create a backup for - * @param vm The machine for which to create a backup - * @param metric the metric object to update with the new backup data + * @param vm The machine for which to create a backup + */ + Backup createNewBackupEntryForRestorePoint(Backup.RestorePoint restorePoint, VirtualMachine vm); + + /** + * Returns if the backup provider supports creating new instance from backup */ - Backup createNewBackupEntryForRestorePoint(Backup.RestorePoint restorePoint, VirtualMachine vm, Backup.Metric metric); + boolean supportsInstanceFromBackup(); + + /** + * Returns the backup storage usage (Used, Total) for a backup provider + * @param zoneId the zone for which to return metrics + * @return a pair of Used size and Total size for the backup storage + */ + Pair<Long, Long> getBackupStorageStats(Long zoneId); Review Comment: How is this related to VM creation from backup? ########## api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java: ########## @@ -110,9 +111,28 @@ public interface BackupProvider { /** * This method should TODO + * * @param restorePoint the restore point to create a backup for - * @param vm The machine for which to create a backup - * @param metric the metric object to update with the new backup data + * @param vm The machine for which to create a backup + */ + Backup createNewBackupEntryForRestorePoint(Backup.RestorePoint restorePoint, VirtualMachine vm); + + /** + * Returns if the backup provider supports creating new instance from backup */ - Backup createNewBackupEntryForRestorePoint(Backup.RestorePoint restorePoint, VirtualMachine vm, Backup.Metric metric); + boolean supportsInstanceFromBackup(); + + /** + * Returns the backup storage usage (Used, Total) for a backup provider + * @param zoneId the zone for which to return metrics + * @return a pair of Used size and Total size for the backup storage + */ + Pair<Long, Long> getBackupStorageStats(Long zoneId); + + /** + * Gets the backup storage usage (Used, Total) from the plugin and stores it in db + * @param zoneId the zone for which to return metrics + */ + void syncBackupStorageStats(Long zoneId); Review Comment: How is this related to VM creation from backup? ########## api/src/main/java/org/apache/cloudstack/api/command/user/backup/CreateBackupCmd.java: ########## @@ -62,13 +62,34 @@ public class CreateBackupCmd extends BaseAsyncCreateCmd { description = "ID of the VM") private Long vmId; + @Parameter(name = ApiConstants.NAME, + type = CommandType.STRING, + description = "the name of the backup", + since = "4.21.0") + private String name; + + @Parameter(name = ApiConstants.DESCRIPTION, + type = CommandType.STRING, + description = "the description for the backup", + since = "4.21.0") + private String description; + @Parameter(name = ApiConstants.SCHEDULE_ID, type = CommandType.LONG, entityType = BackupScheduleResponse.class, description = "backup schedule ID of the VM, if this is null, it indicates that it is a manual backup.", since = "4.21.0") private Long scheduleId; + @Parameter(name = ApiConstants.QUIESCE_VM, + type = CommandType.BOOLEAN, + required = false, + description = "Quiesce the instance before checkpointing the disks for backup. Applicable only to NAS backup provider. " + + "The filesystem is frozen before the backup starts and thawed immediately after. " + + "Requires the instance to have the QEMU Guest Agent installed and running.", + since = "4.21.0") + private Boolean quiesceVM; Review Comment: Shouldn't this change be in a different PR? ########## api/src/main/java/org/apache/cloudstack/api/command/admin/vm/CreateVMFromBackupCmdByAdmin.java: ########## @@ -0,0 +1,55 @@ +// 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.vm; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ResponseObject; +import org.apache.cloudstack.api.command.admin.AdminCmd; +import org.apache.cloudstack.api.command.user.vm.CreateVMFromBackupCmd; +import org.apache.cloudstack.api.response.ClusterResponse; +import org.apache.cloudstack.api.response.PodResponse; +import org.apache.cloudstack.api.response.UserVmResponse; + +import com.cloud.vm.VirtualMachine; + +@APICommand(name = "createVMFromBackup", + description = "Creates and automatically starts a VM from a backup.", + responseObject = UserVmResponse.class, + responseView = ResponseObject.ResponseView.Full, + entityType = {VirtualMachine.class}, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = true, + since = "4.21.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class CreateVMFromBackupCmdByAdmin extends CreateVMFromBackupCmd implements AdminCmd { + + @Parameter(name = ApiConstants.POD_ID, type = CommandType.UUID, entityType = PodResponse.class, description = "destination Pod ID to deploy the VM to - parameter available for root admin only", since = "4.13") Review Comment: ```suggestion @Parameter(name = ApiConstants.POD_ID, type = CommandType.UUID, entityType = PodResponse.class, description = "destination Pod ID to deploy the VM to - parameter available for root admin only", since = "4.21") ``` -- 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: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org