From 1f9cbd4454b3926da1dc5563336b409aaa22a457 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Mon, 4 May 2026 14:56:34 +0530 Subject: [PATCH] address review comments --- .../admin/backup/CreateImageTransferCmd.java | 3 ++- .../backup/FinalizeImageTransferCmd.java | 2 +- .../command/admin/backup/StartBackupCmd.java | 2 +- .../org/apache/cloudstack/backup/Backup.java | 2 +- .../cloudstack/backup/BackupManager.java | 2 +- .../converter/BackupVOToBackupConverter.java | 4 +-- .../backup/KVMBackupExportServiceImpl.java | 27 ++++++++----------- 7 files changed, 19 insertions(+), 23 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/CreateImageTransferCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/CreateImageTransferCmd.java index cc6992afd88..c98bfb85052 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/CreateImageTransferCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/CreateImageTransferCmd.java @@ -65,7 +65,8 @@ public class CreateImageTransferCmd extends BaseCmd implements AdminCmd { @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") + description = "Format for the image transfer: raw/cow. 'raw' will create an NBD backend. 'cow' will use the File backend." + + "For download, only the 'raw' format is supported. Default: raw") private String format; public Long getBackupId() { diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/FinalizeImageTransferCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/FinalizeImageTransferCmd.java index dbbe18ed280..dfc43e233bf 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/FinalizeImageTransferCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/FinalizeImageTransferCmd.java @@ -32,7 +32,7 @@ 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", + description = "Finalize an image transfer. This API is intended for testing only and is disabled by default.", responseObject = SuccessResponse.class, since = "4.23.0", authorized = {RoleType.Admin}) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/StartBackupCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/StartBackupCmd.java index a5c4773c0fc..1bf6d45db04 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/StartBackupCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/StartBackupCmd.java @@ -37,7 +37,7 @@ 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.", + description = "Start a VM backup session using pull mode backup-begin on the KVM host. This API is intended for testing only and is disabled by default.", responseObject = BackupResponse.class, since = "4.23.0", authorized = {RoleType.Admin}) diff --git a/api/src/main/java/org/apache/cloudstack/backup/Backup.java b/api/src/main/java/org/apache/cloudstack/backup/Backup.java index 42afc7f196c..2d68f18b953 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/Backup.java +++ b/api/src/main/java/org/apache/cloudstack/backup/Backup.java @@ -39,7 +39,7 @@ public interface Backup extends ControlledEntity, InternalIdentity, Identity { Long getHostId(); enum Status { - Allocated, Queued, BackingUp, ReadyForTransfer, FinalizingTransfer, BackedUp, Error, Failed, Restoring, Removed, Expunged + Allocated, Queued, BackingUp, ReadyForImageTransfer, FinalizingImageTransfer, BackedUp, Error, Failed, Restoring, Removed, Expunged } class Metric { diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java index 587b7c32105..0da11bbd651 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java @@ -58,7 +58,7 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer ConfigKey BackupProviderPlugin = new ValidatedConfigKey<>("Advanced", String.class, "backup.framework.provider.plugin", "dummy", - "The backup and recovery provider plugin. Valid plugin values: dummy, veeam, networker, nas", + "The backup and recovery provider plugin. Valid plugin values: dummy, veeam, networker and nas", true, ConfigKey.Scope.Zone, BackupFrameworkEnabled.key(), value -> validateBackupProviderConfig((String)value)); ConfigKey BackupSyncPollingInterval = new ConfigKey<>("Advanced", Long.class, diff --git a/plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/converter/BackupVOToBackupConverter.java b/plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/converter/BackupVOToBackupConverter.java index 2f2b40908e8..04bac31a3d6 100644 --- a/plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/converter/BackupVOToBackupConverter.java +++ b/plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/converter/BackupVOToBackupConverter.java @@ -86,9 +86,9 @@ public class BackupVOToBackupConverter { return "initializing"; case BackingUp: return "starting"; - case ReadyForTransfer: + case ReadyForImageTransfer: return "ready"; - case FinalizingTransfer: + case FinalizingImageTransfer: return "finalizing"; case Restoring: case BackedUp: diff --git a/server/src/main/java/org/apache/cloudstack/backup/KVMBackupExportServiceImpl.java b/server/src/main/java/org/apache/cloudstack/backup/KVMBackupExportServiceImpl.java index 57a09453144..1e54b9d8195 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/KVMBackupExportServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/KVMBackupExportServiceImpl.java @@ -138,8 +138,11 @@ public class KVMBackupExportServiceImpl extends ManagerBase implements KVMBackup VmWorkJobHandlerProxy jobHandlerProxy = new VmWorkJobHandlerProxy(this); - private boolean isKVMBackupExportServiceSupported(Long zoneId) { - return !BackupFrameworkEnabled.value() || StringUtils.equals("dummy", BackupProviderPlugin.valueIn(zoneId)); + private void verifyKVMBackupExportServiceSupported(Long zoneId) { + if (BackupFrameworkEnabled.value() && !StringUtils.equals("dummy", BackupProviderPlugin.valueIn(zoneId))) { + throw new CloudRuntimeException("Veeam-KVM integration can not be used along with the " + BackupProviderPlugin.valueIn(zoneId) + + " backup provider. Either set backup.framework.enabled to false or set the Zone level config backup.framework.provider.plugin to \"dummy\"."); + } } @Override @@ -151,10 +154,7 @@ public class KVMBackupExportServiceImpl extends ManagerBase implements KVMBackup 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\"."); - } + verifyKVMBackupExportServiceSupported(vm.getDataCenterId()); if (vm.getState() != State.Running && vm.getState() != State.Stopped) { throw new CloudRuntimeException("VM must be running or stopped to start backup"); @@ -281,7 +281,7 @@ public class KVMBackupExportServiceImpl extends ManagerBase implements KVMBackup // Update backup with checkpoint creation time backup.setCheckpointCreateTime(answer.getCheckpointCreateTime()); - updateBackupState(backup, Backup.Status.ReadyForTransfer); + updateBackupState(backup, Backup.Status.ReadyForImageTransfer); queueBackupFinalizeWaitWorkJob(vm, backup); return backup; } @@ -329,7 +329,7 @@ public class KVMBackupExportServiceImpl extends ManagerBase implements KVMBackup throw new CloudRuntimeException("VM not found: " + vmId); } - updateBackupState(backup, Backup.Status.FinalizingTransfer); + updateBackupState(backup, Backup.Status.FinalizingImageTransfer); List transfers = imageTransferDao.listByBackupId(backupId); for (ImageTransferVO transfer : transfers) { @@ -607,10 +607,7 @@ public class KVMBackupExportServiceImpl extends ManagerBase implements KVMBackup throw new CloudRuntimeException("Volume not found with the specified Id"); } - if (!isKVMBackupExportServiceSupported(volume.getDataCenterId())) { - throw new CloudRuntimeException("Veeam-KVM integration can not be used along with the " + BackupProviderPlugin.valueIn(volume.getDataCenterId()) + - " backup provider. Either set backup.framework.enabled to false or set the Zone level config backup.framework.provider.plugin to \"dummy\"."); - } + verifyKVMBackupExportServiceSupported(volume.getDataCenterId()); ImageTransferVO existingTransfer = imageTransferDao.findByVolume(volume.getId()); if (existingTransfer != null) { @@ -805,10 +802,8 @@ public class KVMBackupExportServiceImpl extends ManagerBase implements KVMBackup if (vm == null) { throw new CloudRuntimeException("VM not found: " + cmd.getVmId()); } - 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\"."); - } + + verifyKVMBackupExportServiceSupported(vm.getDataCenterId()); if (vm.getState() != State.Running && vm.getState() != State.Stopped) { throw new CloudRuntimeException("VM must be running or stopped to delete checkpoint");