weizhouapache commented on code in PR #12991: URL: https://github.com/apache/cloudstack/pull/12991#discussion_r3166323692
########## agent/conf/agent.properties: ########## @@ -78,6 +78,14 @@ zone=default # Generated with "uuidgen". local.storage.uuid= +# Enable TLS for image server transfers. The keys are read from: +# cert file = /etc/cloudstack/agent/cloud.crt +# key file = /etc/cloudstack/agent/cloud.key +image.server.tls.enabled=true Review Comment: @shwstppr is `/etc/cloudstack/agent/cloud.ca.crt` used ? ########## api/src/main/java/org/apache/cloudstack/api/command/admin/backup/CreateImageTransferCmd.java: ########## @@ -0,0 +1,99 @@ +//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 +//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.backup; + +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.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.command.admin.AdminCmd; +import org.apache.cloudstack.api.response.BackupResponse; +import org.apache.cloudstack.api.response.ImageTransferResponse; +import org.apache.cloudstack.api.response.VolumeResponse; +import org.apache.cloudstack.backup.ImageTransfer; +import org.apache.cloudstack.backup.KVMBackupExportService; +import org.apache.cloudstack.context.CallContext; + +import com.cloud.utils.EnumUtils; + +@APICommand(name = "createImageTransfer", + description = "Create image transfer for a disk in backup. This API is intended for testing only and is disabled by default.", Review Comment: `This API is intended for testing only and is disabled by default.` this sentence exists in all APIs in this folder. is this correct ? @abh1sar ########## api/src/main/java/org/apache/cloudstack/api/command/admin/backup/FinalizeImageTransferCmd.java: ########## @@ -0,0 +1,69 @@ +//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 +//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.backup; + +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.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.command.admin.AdminCmd; +import org.apache.cloudstack.api.response.ImageTransferResponse; +import org.apache.cloudstack.api.response.SuccessResponse; +import org.apache.cloudstack.backup.ImageTransfer; +import org.apache.cloudstack.backup.KVMBackupExportService; +import org.apache.cloudstack.context.CallContext; + +@APICommand(name = "finalizeImageTransfer", + description = "Finalize an image transfe. This API is intended for testing only and is disabled by default.r", Review Comment: transfe -> transfer ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVmCheckpointCommandWrapper.java: ########## @@ -0,0 +1,80 @@ +//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 +//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.hypervisor.kvm.resource.wrapper; + +import java.util.Map; + +import org.apache.cloudstack.backup.DeleteVmCheckpointCommand; + +import com.cloud.agent.api.Answer; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.utils.script.Script; + +@ResourceWrapper(handles = DeleteVmCheckpointCommand.class) +public class LibvirtDeleteVmCheckpointCommandWrapper extends CommandWrapper<DeleteVmCheckpointCommand, Answer, LibvirtComputingResource> { + + @Override + public Answer execute(DeleteVmCheckpointCommand cmd, LibvirtComputingResource resource) { + if (cmd.isStoppedVM()) { + return deleteBitmapsOnDisks(cmd); + } + return deleteDomainCheckpoint(cmd); + } + + private Answer deleteDomainCheckpoint(DeleteVmCheckpointCommand cmd) { + String vmName = cmd.getVmName(); + String checkpointId = cmd.getCheckpointId(); + String virshCmd = String.format("virsh checkpoint-delete %s %s", vmName, checkpointId); + Script script = new Script("/bin/bash"); + script.add("-c"); + script.add(virshCmd); + String result = script.execute(); + if (result != null) { + return new Answer(cmd, false, "Failed to delete checkpoint: " + result); + } + return new Answer(cmd, true, "Checkpoint deleted"); + } + + /** + * Stopped VM: persistent bitmaps on disk images ({@code qemu-img bitmap --remove}), matching {@link LibvirtStartBackupCommandWrapper} bitmap --add. + */ + private Answer deleteBitmapsOnDisks(DeleteVmCheckpointCommand cmd) { + String checkpointId = cmd.getCheckpointId(); + Map<String, String> diskPathUuidMap = cmd.getDiskPathUuidMap(); + if (diskPathUuidMap == null || diskPathUuidMap.isEmpty()) { + return new Answer(cmd, false, "No disks provided for bitmap removal"); + } + for (Map.Entry<String, String> entry : diskPathUuidMap.entrySet()) { + String diskPath = entry.getKey(); + Script script = new Script("sudo"); Review Comment: `sudo` is not an issue for root user for users who add the kvm host using a regular user with sudo privilege, need to double-check. some script commands with `virsh` or `iptables` do not use `sudo` ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateImageTransferCommandWrapper.java: ########## @@ -0,0 +1,178 @@ +//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 +//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.hypervisor.kvm.resource.wrapper; + +import java.io.File; +import java.util.HashMap; +import java.util.Map; + +import org.apache.cloudstack.backup.CreateImageTransferAnswer; +import org.apache.cloudstack.backup.CreateImageTransferCommand; +import org.apache.cloudstack.backup.ImageTransfer; +import org.apache.cloudstack.storage.resource.IpTablesHelper; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import com.cloud.agent.api.Answer; +import com.cloud.hypervisor.kvm.resource.ImageServerControlSocket; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.utils.StringUtils; +import com.cloud.utils.script.Script; + +@ResourceWrapper(handles = CreateImageTransferCommand.class) +public class LibvirtCreateImageTransferCommandWrapper extends CommandWrapper<CreateImageTransferCommand, Answer, LibvirtComputingResource> { + protected Logger logger = LogManager.getLogger(getClass()); + + private static final String IMAGE_SERVER_TLS_CERT_FILE = "/etc/cloudstack/agent/cloud.crt"; + private static final String IMAGE_SERVER_TLS_KEY_FILE = "/etc/cloudstack/agent/cloud.key"; + + private void resetService(String unitName) { + Script resetScript = new Script("/bin/bash", logger); + resetScript.add("-c"); + resetScript.add(String.format("systemctl reset-failed %s || true", unitName)); + resetScript.execute(); + } + + private static String shellQuote(String value) { + return "'" + value.replace("'", "'\\''") + "'"; + } + + private boolean startImageServerIfNotRunning(int imageServerPort, String listenAddress, LibvirtComputingResource resource) { + final String imageServerPackageDir = resource.getImageServerPath(); + final String imageServerParentDir = new File(imageServerPackageDir).getParent(); + final String imageServerModuleName = new File(imageServerPackageDir).getName(); + final boolean tlsEnabled = resource.isImageServerTlsEnabled(); + String unitName = resource.IMAGE_SERVER_SYSTEMD_UNIT_NAME; + + Script checkScript = new Script("/bin/bash", logger); + checkScript.add("-c"); + checkScript.add(String.format("systemctl is-active --quiet %s", unitName)); + String checkResult = checkScript.execute(); + if (checkResult == null && ImageServerControlSocket.isReady()) { + return true; + } + + resetService(unitName); + if (checkResult != null) { + StringBuilder systemdRunCmd = new StringBuilder(String.format( + "systemd-run --unit=%s --property=Restart=no --property=WorkingDirectory=%s /usr/bin/python3 -m %s --listen %s --port %d", + unitName, shellQuote(imageServerParentDir), imageServerModuleName, shellQuote(listenAddress), imageServerPort)); + + if (tlsEnabled) { + systemdRunCmd.append(" --tls-enabled"); + systemdRunCmd.append(" --tls-cert-file ").append(IMAGE_SERVER_TLS_CERT_FILE); + systemdRunCmd.append(" --tls-key-file ").append(IMAGE_SERVER_TLS_KEY_FILE); + } + + Script startScript = new Script("/bin/bash", logger); + startScript.add("-c"); + startScript.add(systemdRunCmd.toString()); + String startResult = startScript.execute(); + + if (startResult != null) { + logger.error(String.format("Failed to start the Image server: %s", startResult)); + return false; + } + } + + int maxWaitSeconds = 10; + int pollIntervalMs = 1000; + int maxAttempts = (maxWaitSeconds * 1000) / pollIntervalMs; + boolean serverReady = false; + + for (int attempt = 0; attempt < maxAttempts; attempt++) { + if (ImageServerControlSocket.isReady()) { + serverReady = true; + logger.info(String.format("Image server control socket is ready (attempt %d)", attempt + 1)); + break; + } + try { + Thread.sleep(pollIntervalMs); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return false; + } + } + + if (!serverReady) { + logger.error(String.format("Image server control socket not ready within %d seconds", maxWaitSeconds)); + return false; + } + + String rule = String.format("-p tcp -m state --state NEW -m tcp --dport %d -j ACCEPT", imageServerPort); + IpTablesHelper.addConditionally(IpTablesHelper.INPUT_CHAIN, true, rule, Review Comment: it would be better to add the source cidr, we can implement in next stage. ########## tools/apidoc/gen_toc.py: ########## @@ -223,6 +223,15 @@ 'Management': 'Management', 'Backup' : 'Backup and Recovery', 'Restore' : 'Backup and Recovery', + 'startBackup' : 'Backup and Recovery', + 'finalizeBackup' : 'Backup and Recovery', + 'createImageTransfer' : 'Backup and Recovery', + 'finalizeImageTransfer' : 'Backup and Recovery', + 'listImageTransfers' : 'Backup and Recovery', + 'listVmCheckpoints' : 'Backup and Recovery', + 'deleteVmCheckpoint' : 'Backup and Recovery', + 'ImageTransfer' : 'Backup and Recovery', + 'VmCheckpoint' : 'Backup and Recovery', 'UnmanagedInstance': 'Virtual Machine', Review Comment: maybe only the last two are needed ``` 'ImageTransfer' : 'Backup and Recovery', 'VmCheckpoint' : 'Backup and Recovery', ``` ########## api/src/main/java/org/apache/cloudstack/backup/Backup.java: ########## @@ -30,8 +30,16 @@ public interface Backup extends ControlledEntity, InternalIdentity, Identity { + String getFromCheckpointId(); + + String getToCheckpointId(); + + Long getCheckpointCreateTime(); + + Long getHostId(); + enum Status { - Allocated, Queued, BackingUp, BackedUp, Error, Failed, Restoring, Removed, Expunged + Allocated, Queued, BackingUp, ReadyForTransfer, FinalizingTransfer, BackedUp, Error, Failed, Restoring, Removed, Expunged Review Comment: would ReadyForImageTransfer and FinalizingImageTransfer be better ? ########## api/src/main/java/org/apache/cloudstack/api/command/admin/backup/CreateImageTransferCmd.java: ########## @@ -0,0 +1,99 @@ +//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 +//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.backup; + +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.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.command.admin.AdminCmd; +import org.apache.cloudstack.api.response.BackupResponse; +import org.apache.cloudstack.api.response.ImageTransferResponse; +import org.apache.cloudstack.api.response.VolumeResponse; +import org.apache.cloudstack.backup.ImageTransfer; +import org.apache.cloudstack.backup.KVMBackupExportService; +import org.apache.cloudstack.context.CallContext; + +import com.cloud.utils.EnumUtils; + +@APICommand(name = "createImageTransfer", + description = "Create image transfer for a disk in backup. This API is intended for testing only and is disabled by default.", + responseObject = ImageTransferResponse.class, + since = "4.23.0", + authorized = {RoleType.Admin}) +public class CreateImageTransferCmd extends BaseCmd implements AdminCmd { + + @Inject + private KVMBackupExportService kvmBackupExportService; + + @Parameter(name = ApiConstants.BACKUP_ID, + type = CommandType.UUID, + entityType = BackupResponse.class, + description = "ID of the backup") + private Long backupId; + + @Parameter(name = ApiConstants.VOLUME_ID, + type = CommandType.UUID, + entityType = VolumeResponse.class, + required = true, + description = "ID of the disk/volume") + private Long volumeId; + + @Parameter(name = ApiConstants.DIRECTION, + type = CommandType.STRING, + required = true, + description = "Direction of the transfer: upload, download") + private String direction; + + @Parameter(name = ApiConstants.FORMAT, + type = CommandType.STRING, + description = "Format of the image: cow/raw. Currently only raw is supported for download. Defaults to raw if not provided") Review Comment: `Currently only raw is supported for download` I think the image is qcow2/cow format, right ? ########## api/src/main/java/org/apache/cloudstack/api/command/admin/backup/StartBackupCmd.java: ########## @@ -0,0 +1,120 @@ +//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 +//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.backup; + +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.BaseAsyncCreateCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.command.admin.AdminCmd; +import org.apache.cloudstack.api.response.BackupResponse; +import org.apache.cloudstack.api.response.UserVmResponse; +import org.apache.cloudstack.backup.Backup; +import org.apache.cloudstack.backup.BackupManager; +import org.apache.cloudstack.backup.KVMBackupExportService; +import org.apache.cloudstack.context.CallContext; + +import com.cloud.event.EventTypes; + +@APICommand(name = "startBackup", + description = "Start a VM backup session. This API is intended for testing only and is disabled by default.", Review Comment: `startBackup` is a common word. it would be better to explain what the intention is, and what hypervisors are supported, etc. ########## api/src/main/java/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java: ########## @@ -114,7 +114,8 @@ public class CreateVolumeCmd extends BaseAsyncCreateCustomIdCmd implements UserC type = CommandType.UUID, entityType = StoragePoolResponse.class, description = "Storage pool ID to create the volume in. Cannot be used with the snapshotid parameter.", - authorized = {RoleType.Admin}) + authorized = {RoleType.Admin}, + since = "4.23.0") Review Comment: good, this was missing in #12966 ########## api/src/main/java/org/apache/cloudstack/api/command/admin/vm/AssignVMCmd.java: ########## @@ -85,6 +85,8 @@ public class AssignVMCmd extends BaseCmd { "In case no security groups are provided the Instance is part of the default security group.") private List<Long> securityGroupIdList; + private boolean skipNetwork = false; Review Comment: is this for internal use ? can we add a comment ? ########## api/src/main/java/org/apache/cloudstack/backup/BackupManager.java: ########## @@ -58,7 +58,7 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer ConfigKey<String> BackupProviderPlugin = new ValidatedConfigKey<>("Advanced", String.class, "backup.framework.provider.plugin", "dummy", - "The backup and recovery provider plugin. Valid plugin values: dummy, veeam, networker and nas", + "The backup and recovery provider plugin. Valid plugin values: dummy, veeam, networker, nas", Review Comment: this may be not needed ########## api/src/main/java/org/apache/cloudstack/backup/KVMBackupExportService.java: ########## @@ -0,0 +1,100 @@ +//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 +//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 java.util.List; + +import org.apache.cloudstack.api.command.admin.backup.CreateImageTransferCmd; +import org.apache.cloudstack.api.command.admin.backup.DeleteVmCheckpointCmd; +import org.apache.cloudstack.api.command.admin.backup.FinalizeBackupCmd; +import org.apache.cloudstack.api.command.admin.backup.FinalizeImageTransferCmd; +import org.apache.cloudstack.api.command.admin.backup.ListImageTransfersCmd; +import org.apache.cloudstack.api.command.admin.backup.ListVmCheckpointsCmd; +import org.apache.cloudstack.api.command.admin.backup.StartBackupCmd; +import org.apache.cloudstack.api.response.CheckpointResponse; +import org.apache.cloudstack.api.response.ImageTransferResponse; +import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.framework.config.Configurable; + +import com.cloud.utils.component.PluggableService; + +/** + * Service for Creating Backups and ImageTransfer sessions which will be consumed by an external orchestrator. + */ +public interface KVMBackupExportService extends Configurable, PluggableService { Review Comment: I wonder if we could change to `BackupExportService`, although only KVM is supported as of now. it is good all new APIs do not have 'KVM' in the API name. ########## api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java: ########## @@ -64,6 +66,10 @@ public class DeployVMCmd extends BaseDeployVMCmd { @Parameter(name = ApiConstants.SNAPSHOT_ID, type = CommandType.UUID, entityType = SnapshotResponse.class, since = "4.21") private Long snapshotId; + @Parameter(name = "blank", type = CommandType.BOOLEAN, since = "4.23.0") + private Boolean blankInstance; Review Comment: @shwstppr can we add a description to this new parameter ? is it only available for admin ? if yes, may add a `authorized = {RoleType.Admin}` ########## server/src/main/java/com/cloud/vm/UserVmManagerImpl.java: ########## @@ -10075,4 +10113,33 @@ private void setVncPasswordForKvmIfAvailable(Map<String, String> customParameter vm.setVncPassword(customParameters.get(VmDetailConstants.KVM_VNC_PASSWORD)); } } + + protected boolean isBlankInstanceDefaultTemplate(VirtualMachineTemplate template) { + return KVM_VM_DUMMY_TEMPLATE_NAME.equals(template.getUniqueName()); Review Comment: maybe rename to `KVM_BLANK_VM_TEMPLATE_NAME` or so ########## server/src/main/java/com/cloud/projects/ProjectManagerImpl.java: ########## @@ -479,7 +479,7 @@ public ProjectAccount assignAccountToProject(Project project, long accountId, Pr return _projectAccountDao.persist(projectAccountVO); } - public ProjectAccount assignUserToProject(Project project, long userId, long accountId, Role userRole, Long projectRoleId) { + public ProjectAccount assignUserToProject(Project project, long userId, long accountId, Role userRole, Long projectRoleId) { Review Comment: seems not needed ########## server/src/main/java/com/cloud/api/ApiServer.java: ########## @@ -16,6 +16,10 @@ // under the License. package com.cloud.api; +import static com.cloud.user.AccountManagerImpl.apiKeyAccess; +import static org.apache.cloudstack.api.ApiConstants.PASSWORD_CHANGE_REQUIRED; +import static org.apache.cloudstack.user.UserPasswordResetManager.UserPasswordResetEnabled; Review Comment: are these from another PR ? ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapper.java: ########## @@ -0,0 +1,221 @@ +//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 +//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.hypervisor.kvm.resource.wrapper; + +import java.io.File; +import java.io.FileWriter; +import java.util.Map; + +import org.apache.cloudstack.backup.StartBackupAnswer; +import org.apache.cloudstack.backup.StartBackupCommand; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.LogManager; +import com.cloud.agent.api.Answer; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.utils.StringUtils; +import com.cloud.utils.script.Script; + +@ResourceWrapper(handles = StartBackupCommand.class) +public class LibvirtStartBackupCommandWrapper extends CommandWrapper<StartBackupCommand, Answer, LibvirtComputingResource> { + protected Logger logger = LogManager.getLogger(getClass()); + + @Override + public Answer execute(StartBackupCommand cmd, LibvirtComputingResource resource) { + if (cmd.isStoppedVM()) { + return handleStoppedVmBackup(cmd, cmd.getToCheckpointId()); + } + return handleRunningVmBackup(cmd, resource); + } + + public Answer handleRunningVmBackup(StartBackupCommand cmd, LibvirtComputingResource resource) { + String vmName = cmd.getVmName(); + String toCheckpointId = cmd.getToCheckpointId(); + String fromCheckpointId = cmd.getFromCheckpointId(); + Long fromCheckpointCreateTime = cmd.getFromCheckpointCreateTime(); + String socket = cmd.getSocket(); + + try { + if (StringUtils.isNotBlank(fromCheckpointId)) { + Answer redefineAnswer = ensureFromCheckpointExists(cmd, fromCheckpointId, fromCheckpointCreateTime); + if (redefineAnswer != null) { + return redefineAnswer; + } + } + + File dir = new File("/tmp/imagetransfer"); + if (!dir.exists()) { + dir.mkdirs(); + } + + // Create backup XML + String backupXml = createBackupXml(cmd, fromCheckpointId, socket, resource); + String checkpointXml = createCheckpointXml(toCheckpointId); + + // Write XMLs to temp files + File backupXmlFile = File.createTempFile("backup-", ".xml"); + File checkpointXmlFile = File.createTempFile("checkpoint-", ".xml"); + + try (FileWriter writer = new FileWriter(backupXmlFile)) { + writer.write(backupXml); + } + try (FileWriter writer = new FileWriter(checkpointXmlFile)) { + writer.write(checkpointXml); + } + + // Execute virsh backup-begin + String backupCmd = String.format("virsh backup-begin %s %s --checkpointxml %s", + vmName, backupXmlFile.getAbsolutePath(), checkpointXmlFile.getAbsolutePath()); + + Script script = new Script("/bin/bash"); + script.add("-c"); + script.add(backupCmd); + String result = script.execute(); + + backupXmlFile.delete(); + checkpointXmlFile.delete(); + + if (result != null) { + return new StartBackupAnswer(cmd, false, "Backup begin failed: " + result); + } + + long checkpointCreateTime = getCheckpointCreateTime(); + return new StartBackupAnswer(cmd, true, "Backup started successfully", checkpointCreateTime); + + } catch (Exception e) { + return new StartBackupAnswer(cmd, false, "Error starting backup: " + e.getMessage()); + } + } + + private Answer ensureFromCheckpointExists(StartBackupCommand cmd, String fromCheckpointId, Long fromCheckpointCreateTime) { + String vmName = cmd.getVmName(); + Script dumpScript = new Script("/bin/bash"); + dumpScript.add("-c"); + dumpScript.add(String.format("virsh checkpoint-dumpxml --domain %s --checkpointname %s --no-domain", + vmName, fromCheckpointId)); + if (dumpScript.execute() == null) { + return null; + } + if (fromCheckpointCreateTime == null) { + return new StartBackupAnswer(cmd, false, "From checkpoint create time is null for checkpoint " + fromCheckpointId); + } + + String redefineXml = createCheckpointXmlForRedefine(fromCheckpointId, fromCheckpointCreateTime); + File redefineFile; + try { + redefineFile = File.createTempFile("checkpoint-redefine-", ".xml"); + } catch (Exception e) { + return new StartBackupAnswer(cmd, false, "Failed to create temp file for checkpoint redefine: " + e.getMessage()); + } + try (FileWriter writer = new FileWriter(redefineFile)) { + writer.write(redefineXml); + } catch (Exception e) { + redefineFile.delete(); + return new StartBackupAnswer(cmd, false, "Failed to write checkpoint redefine XML: " + e.getMessage()); + } + String createCmd = String.format(LibvirtComputingResource.CHECKPOINT_CREATE_COMMAND, vmName, redefineFile.getAbsolutePath()); + Script createScript = new Script("/bin/bash"); + createScript.add("-c"); + createScript.add(createCmd); + String result = createScript.execute(); + redefineFile.delete(); + if (result != null) { + return new StartBackupAnswer(cmd, false, "Failed to redefine from-checkpoint " + fromCheckpointId + ": " + result); + } + return null; + } + + private String createCheckpointXmlForRedefine(String checkpointName, Long createTime) { + StringBuilder xml = new StringBuilder(); + xml.append("<domaincheckpoint>\n"); + xml.append(" <name>").append(checkpointName).append("</name>\n"); + xml.append(" <creationTime>").append(createTime).append("</creationTime>\n"); + xml.append("</domaincheckpoint>"); + return xml.toString(); + } + + private String createBackupXml(StartBackupCommand cmd, String fromCheckpointId, String socket, LibvirtComputingResource resource) { + StringBuilder xml = new StringBuilder(); + xml.append("<domainbackup mode=\"pull\">\n"); + + if (StringUtils.isNotBlank(fromCheckpointId)) { + xml.append(" <incremental>").append(fromCheckpointId).append("</incremental>\n"); + } + + xml.append(String.format(" <server transport=\"unix\" socket=\"/tmp/imagetransfer/%s.sock\"/>\n", socket)); + + xml.append(" <disks>\n"); + + Map<String, String> diskPathUuidMap = cmd.getDiskPathUuidMap(); + Map<String, String> diskPathLabelMap = resource.getDiskPathLabelMap(cmd.getVmName()); + + for (Map.Entry<String, String> entry : diskPathLabelMap.entrySet()) { + if (!diskPathUuidMap.containsKey(entry.getKey())) { + continue; + } + String diskName = entry.getValue(); + String export = diskPathUuidMap.get(entry.getKey()); + String scratchFile = "/var/tmp/scratch-" + export + ".qcow2"; Review Comment: it is nice to use `/var/tmp` . in some recent Linux releases, `/tmp` is a `tmpfs` filesystem with limited size. normally how big this scratch file is ? what if kvm host do not have sufficient space ? ########## server/src/main/java/org/apache/cloudstack/backup/KVMBackupExportServiceImpl.java: ########## @@ -0,0 +1,963 @@ +//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 +//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 static org.apache.cloudstack.backup.BackupManager.BackupFrameworkEnabled; +import static org.apache.cloudstack.backup.BackupManager.BackupProviderPlugin; + +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.stream.Collectors; + +import javax.inject.Inject; + +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.command.admin.backup.CreateImageTransferCmd; +import org.apache.cloudstack.api.command.admin.backup.DeleteVmCheckpointCmd; +import org.apache.cloudstack.api.command.admin.backup.FinalizeBackupCmd; +import org.apache.cloudstack.api.command.admin.backup.FinalizeImageTransferCmd; +import org.apache.cloudstack.api.command.admin.backup.ListImageTransfersCmd; +import org.apache.cloudstack.api.command.admin.backup.ListVmCheckpointsCmd; +import org.apache.cloudstack.api.command.admin.backup.StartBackupCmd; +import org.apache.cloudstack.api.response.CheckpointResponse; +import org.apache.cloudstack.api.response.ImageTransferResponse; +import org.apache.cloudstack.backup.dao.BackupDao; +import org.apache.cloudstack.backup.dao.ImageTransferDao; +import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.framework.jobs.AsyncJobExecutionContext; +import org.apache.cloudstack.framework.jobs.AsyncJobManager; +import org.apache.cloudstack.framework.jobs.impl.VmWorkJobVO; +import org.apache.cloudstack.jobs.JobInfo; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.commons.lang3.StringUtils; +import org.joda.time.DateTime; +import org.springframework.stereotype.Component; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.api.ApiDBUtils; +import com.cloud.exception.AgentUnavailableException; +import com.cloud.exception.OperationTimedoutException; +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; +import com.cloud.storage.ScopeType; +import com.cloud.storage.Storage; +import com.cloud.storage.StoragePoolHostVO; +import com.cloud.storage.Volume; +import com.cloud.storage.VolumeDetailVO; +import com.cloud.storage.VolumeStats; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.StoragePoolHostDao; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.storage.dao.VolumeDetailsDao; +import com.cloud.user.Account; +import com.cloud.user.AccountService; +import com.cloud.user.User; +import com.cloud.utils.NumbersUtil; +import com.cloud.utils.Pair; +import com.cloud.utils.ReflectionUse; +import com.cloud.utils.component.ManagerBase; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.VMInstanceDetailVO; +import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.VirtualMachine; +import com.cloud.vm.VirtualMachine.State; +import com.cloud.vm.VmDetailConstants; +import com.cloud.vm.VmWork; +import com.cloud.vm.VmWorkConstants; +import com.cloud.vm.VmWorkJobHandler; +import com.cloud.vm.VmWorkJobHandlerProxy; +import com.cloud.vm.VmWorkSerializer; +import com.cloud.vm.dao.VMInstanceDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; + +@Component +public class KVMBackupExportServiceImpl extends ManagerBase implements KVMBackupExportService, VmWorkJobHandler { + public static final String VM_WORK_JOB_HANDLER = KVMBackupExportServiceImpl.class.getSimpleName(); + private static final long BACKUP_FINALIZE_WAIT_CHECK_INTERVAL = 15 * 1000L; + + @Inject + private VMInstanceDao vmInstanceDao; + + @Inject + private VMInstanceDetailsDao vmInstanceDetailsDao; + + @Inject + private BackupDao backupDao; + + @Inject + private ImageTransferDao imageTransferDao; + + @Inject + private VolumeDao volumeDao; + + @Inject + private VolumeDetailsDao volumeDetailsDao; + + @Inject + private AgentManager agentManager; + + @Inject + private HostDao hostDao; + + @Inject + private PrimaryDataStoreDao primaryDataStoreDao; + + @Inject + private StoragePoolHostDao storagePoolHostDao; + + @Inject + AccountService accountService; + + @Inject + AsyncJobManager asyncJobManager; + + VmWorkJobHandlerProxy jobHandlerProxy = new VmWorkJobHandlerProxy(this); + + private boolean isKVMBackupExportServiceSupported(Long zoneId) { + return !BackupFrameworkEnabled.value() || StringUtils.equals("dummy", BackupProviderPlugin.valueIn(zoneId)); + } + + @Override + public Backup createBackup(StartBackupCmd cmd) { + Long vmId = cmd.getVmId(); + + VMInstanceVO vm = vmInstanceDao.findById(vmId); + if (vm == null) { + throw new CloudRuntimeException("VM not found: " + vmId); + } + + if (!isKVMBackupExportServiceSupported(vm.getDataCenterId())) { + throw new CloudRuntimeException("Veeam-KVM integration can not be used along with the " + BackupProviderPlugin.valueIn(vm.getDataCenterId()) + + " backup provider. Either set backup.framework.enabled to false or set the Zone level config backup.framework.provider.plugin to \"dummy\"."); + } Review Comment: this check is used in multiple locations, it would be better to extract to a new method ########## ui/src/components/view/SettingsTab.vue: ########## @@ -87,6 +87,7 @@ export default { } }, created () { + console.log('---------------', this.$route.meta.name) Review Comment: remove it ? -- 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]
