Copilot commented on code in PR #12991:
URL: https://github.com/apache/cloudstack/pull/12991#discussion_r3063875262
##########
api/src/main/java/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java:
##########
@@ -153,6 +160,13 @@ private Long getProjectId() {
return projectId;
}
+ public Long getStorageId() {
+ if (snapshotId != null && storageId != null) {
+ throw new IllegalArgumentException("StorageId parameter cannot be
specified with the SnapshotId parameter.");
Review Comment:
Throwing `IllegalArgumentException` from a cmd accessor can result in
inconsistent API error handling (often surfacing as 500). Prefer raising a
`ServerApiException` with `ApiErrorCode.PARAM_ERROR` (or using built-in
parameter validation) so callers receive a clear, standard client error
response.
```suggestion
throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"StorageId parameter cannot be specified with the
SnapshotId parameter.");
```
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/converter/DataCenterJoinVOToDataCenterConverter.java:
##########
@@ -0,0 +1,78 @@
+// 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.veeam.api.converter;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.cloudstack.veeam.VeeamControlService;
+import org.apache.cloudstack.veeam.api.DataCentersRouteHandler;
+import org.apache.cloudstack.veeam.api.dto.DataCenter;
+import org.apache.cloudstack.veeam.api.dto.Link;
+import org.apache.cloudstack.veeam.api.dto.Ref;
+import org.apache.cloudstack.veeam.api.dto.SupportedVersions;
+import org.apache.cloudstack.veeam.api.dto.Version;
+
+import com.cloud.api.query.vo.DataCenterJoinVO;
+import com.cloud.org.Grouping;
+
+public class DataCenterJoinVOToDataCenterConverter {
+ public static DataCenter toDataCenter(final DataCenterJoinVO zone) {
+ final String id = zone.getUuid();
+ final String basePath = VeeamControlService.ContextPath.value();
+ final String href = basePath + DataCentersRouteHandler.BASE_ROUTE +
DataCentersRouteHandler.BASE_ROUTE + "/" + id;
Review Comment:
The `href` concatenation duplicates `BASE_ROUTE`, producing an incorrect
URL. Also, `Link.of(rel, href)` is being called with parameters reversed (rel
is being set to a URL and href to a literal like `clusters`). Build `href` as
`basePath + BASE_ROUTE + \"/\" + id`, and construct links as
`Link.of(\"clusters\", href + \"/clusters\")` (same for
networks/storagedomains).
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -577,7 +576,13 @@ public void allocate(final String vmInstanceName, final
VirtualMachineTemplate t
logger.debug("Allocating disks for {}", persistedVm);
- allocateRootVolume(persistedVm, template, rootDiskOfferingInfo,
owner, rootDiskSizeFinal, volume, snapshot);
+ if (_userVmMgr.isBlankInstance(template)) {
+ logger.debug("Template is a dummy template for hypervisor {},
skipping volume allocation", hyperType);
+ return;
Review Comment:
Returning from `allocate(...)` when the template is a blank/dummy template
likely aborts the rest of the VM allocation flow (beyond root volume
allocation). If the intent is only to skip root volume allocation, avoid
`return` here and instead conditionally skip `allocateRootVolume(...)` while
letting the method continue.
```suggestion
logger.debug("Template is a dummy template for hypervisor
{}, skipping root volume allocation", hyperType);
```
##########
core/src/main/java/org/apache/cloudstack/backup/CreateImageTransferCommand.java:
##########
@@ -0,0 +1,94 @@
+//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 com.cloud.agent.api.Command;
+
+public class CreateImageTransferCommand extends Command {
+ private String transferId;
+ private String exportName;
+ private String socket;
+ private String direction;
+ private String checkpointId;
+ private String file;
+ private ImageTransfer.Backend backend;
+ private int idleTimeoutSeconds;
+
+ public CreateImageTransferCommand() {
+ }
+
+ private CreateImageTransferCommand(String transferId, String direction,
String socket, int idleTimeoutSeconds) {
+ this.transferId = transferId;
+ this.direction = direction;
+ this.socket = socket;
+ this.idleTimeoutSeconds = idleTimeoutSeconds;
+ }
+
+ public CreateImageTransferCommand(String transferId, String direction,
String exportName, String socket, String checkpointId, int idleTimeoutSeconds) {
+ this(transferId, direction, socket, idleTimeoutSeconds);
+ this.backend = ImageTransfer.Backend.nbd;
+ this.exportName = exportName;
+ this.checkpointId = checkpointId;
+ }
+
+ public CreateImageTransferCommand(String transferId, String direction,
String socket, String file, int idleTimeoutSeconds) {
+ this(transferId, direction, socket, idleTimeoutSeconds);
+ if (direction == ImageTransfer.Direction.download.toString()) {
Review Comment:
String comparison uses `==`, so the download check can fail unexpectedly.
Use `.equals(...)` (or parse `direction` into `ImageTransfer.Direction` and
compare enums) to reliably reject the file backend for downloads.
```suggestion
if (ImageTransfer.Direction.download.toString().equals(direction)) {
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetImageTransferProgressCommandWrapper.java:
##########
@@ -0,0 +1,95 @@
+//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.List;
+import java.util.Map;
+
+import org.apache.cloudstack.backup.GetImageTransferProgressAnswer;
+import org.apache.cloudstack.backup.GetImageTransferProgressCommand;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+
+@ResourceWrapper(handles = GetImageTransferProgressCommand.class)
+public class LibvirtGetImageTransferProgressCommandWrapper extends
CommandWrapper<GetImageTransferProgressCommand, Answer,
LibvirtComputingResource> {
+ protected Logger logger = LogManager.getLogger(getClass());
+
+ @Override
+ public Answer execute(GetImageTransferProgressCommand cmd,
LibvirtComputingResource resource) {
+ try {
+ List<String> transferIds = cmd.getTransferIds();
+ Map<String, String> volumePaths = cmd.getVolumePaths();
+ Map<String, Long> volumeSizes = cmd.getVolumeSizes();
+ Map<String, Long> progressMap = new HashMap<>();
+
+ if (transferIds == null || transferIds.isEmpty()) {
+ return new GetImageTransferProgressAnswer(cmd, true, "No
transfers to check", progressMap);
+ }
+
+ for (String transferId : transferIds) {
+ String volumePath = volumePaths.get(transferId);
+ Long volumeSize = volumeSizes.get(transferId);
+
+ if (volumePath == null || volumeSize == null || volumeSize ==
0) {
+ logger.warn("Missing volume path or size for transferId:
{}", transferId);
+ progressMap.put(transferId, null);
+ continue;
+ }
+
+ try {
+ File file = new File(volumePath);
+ if (!file.exists()) {
+ logger.warn("Volume file does not exist: {}",
volumePath);
+ progressMap.put(transferId, null);
+ continue;
+ }
+
+ long currentSize = file.length();
+
+ if (volumePath.endsWith(".qcow2") ||
volumePath.endsWith(".qcow")) {
+ try {
+ currentSize =
KVMPhysicalDisk.getVirtualSizeFromFile(volumePath);
+ } catch (Exception e) {
+ logger.warn("Failed to get virtual size for qcow2
file: {}, using physical size", volumePath, e);
+ }
+ }
+ progressMap.put(transferId, currentSize);
+ logger.debug("Transfer {} progress, current: {})",
transferId, currentSize, volumeSize);
+
Review Comment:
`GetImageTransferProgressAnswer`/API field descriptions indicate progress is
a percentage (0-100), but this implementation stores a byte size
(physical/virtual size) in the progress map. Either compute and return a
percent based on `volumeSize` (and clamp to 0..100), or rename/retarget the
API/Answer field to clearly represent bytes transferred to avoid client
misinterpretation.
```suggestion
long progress = Math.max(0L, Math.min(100L, (currentSize
* 100L) / volumeSize));
progressMap.put(transferId, progress);
logger.debug("Transfer {} progress: {}% (current size:
{}, total size: {})", transferId, progress, currentSize, volumeSize);
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetImageTransferProgressCommandWrapper.java:
##########
@@ -0,0 +1,95 @@
+//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.List;
+import java.util.Map;
+
+import org.apache.cloudstack.backup.GetImageTransferProgressAnswer;
+import org.apache.cloudstack.backup.GetImageTransferProgressCommand;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+
+@ResourceWrapper(handles = GetImageTransferProgressCommand.class)
+public class LibvirtGetImageTransferProgressCommandWrapper extends
CommandWrapper<GetImageTransferProgressCommand, Answer,
LibvirtComputingResource> {
+ protected Logger logger = LogManager.getLogger(getClass());
+
+ @Override
+ public Answer execute(GetImageTransferProgressCommand cmd,
LibvirtComputingResource resource) {
+ try {
+ List<String> transferIds = cmd.getTransferIds();
+ Map<String, String> volumePaths = cmd.getVolumePaths();
+ Map<String, Long> volumeSizes = cmd.getVolumeSizes();
+ Map<String, Long> progressMap = new HashMap<>();
+
+ if (transferIds == null || transferIds.isEmpty()) {
+ return new GetImageTransferProgressAnswer(cmd, true, "No
transfers to check", progressMap);
+ }
+
+ for (String transferId : transferIds) {
+ String volumePath = volumePaths.get(transferId);
+ Long volumeSize = volumeSizes.get(transferId);
+
+ if (volumePath == null || volumeSize == null || volumeSize ==
0) {
+ logger.warn("Missing volume path or size for transferId:
{}", transferId);
+ progressMap.put(transferId, null);
+ continue;
+ }
+
+ try {
+ File file = new File(volumePath);
+ if (!file.exists()) {
+ logger.warn("Volume file does not exist: {}",
volumePath);
+ progressMap.put(transferId, null);
+ continue;
+ }
+
+ long currentSize = file.length();
+
+ if (volumePath.endsWith(".qcow2") ||
volumePath.endsWith(".qcow")) {
+ try {
+ currentSize =
KVMPhysicalDisk.getVirtualSizeFromFile(volumePath);
+ } catch (Exception e) {
+ logger.warn("Failed to get virtual size for qcow2
file: {}, using physical size", volumePath, e);
+ }
+ }
+ progressMap.put(transferId, currentSize);
+ logger.debug("Transfer {} progress, current: {})",
transferId, currentSize, volumeSize);
Review Comment:
The debug log format string has mismatched braces/parentheses and an
incorrect number of placeholders for the arguments provided, which will log
incorrectly and can confuse troubleshooting. Fix the format string to match the
provided values (e.g., include both current and total with matching `{}`
placeholders).
```suggestion
logger.debug("Transfer {} progress, current: {}, total:
{}", transferId, currentSize, volumeSize);
```
##########
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);
Review Comment:
Executing `virsh ...` via `bash -c` with unquoted/interpolated `vmName` and
`checkpointId` risks shell argument injection if those values contain
unexpected characters. Prefer invoking `virsh` with explicit arguments (no
shell), and consider using the existing `CHECKPOINT_DELETE_COMMAND` pattern
(including `--metadata`) to ensure consistent semantics.
```suggestion
Script script = new Script("virsh");
script.add("checkpoint-delete");
script.add(vmName);
script.add(checkpointId);
script.add("--metadata");
```
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/utils/JwtUtil.java:
##########
@@ -0,0 +1,57 @@
+// 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.veeam.utils;
+
+import java.nio.charset.StandardCharsets;
+import java.time.Instant;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+
+public class JwtUtil {
+ public static final String ALGORITHM = "HmacSHA256";
+ public static final String ISSUER = "veeam-control";
+
+ public static String issueHs256Jwt(String subject, String scope, long
ttlSeconds, String secret) throws Exception {
+ long now = Instant.now().getEpochSecond();
+ long exp = now + ttlSeconds;
+
+ String headerJson = "{\"alg\":\"HS256\",\"typ\":\"JWT\"}";
+ String payloadJson =
+ "{"
+ + "\"iss\":\"" + DataUtil.jsonEscape(ISSUER) + "\","
+ + "\"sub\":\"" + DataUtil.jsonEscape(subject) + "\","
+ + "\"scope\":\"" + DataUtil.jsonEscape(scope) + "\","
+ + "\"iat\":" + now + ","
+ + "\"exp\":" + exp
+ + "}";
+
Review Comment:
The JWT JSON is being constructed manually with a minimal escape function;
this can produce invalid JSON for control characters (e.g., newlines) and is
easy to get wrong over time. Prefer building the payload with Jackson (already
present in the module) or using a vetted JWT library to avoid subtle
encoding/escaping/security issues.
```suggestion
import java.util.LinkedHashMap;
import java.util.Map;
import javax.crypto.Mac;
import javax.crypto.spec.SecretKeySpec;
import com.fasterxml.jackson.databind.ObjectMapper;
public class JwtUtil {
public static final String ALGORITHM = "HmacSHA256";
public static final String ISSUER = "veeam-control";
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
public static String issueHs256Jwt(String subject, String scope, long
ttlSeconds, String secret) throws Exception {
long now = Instant.now().getEpochSecond();
long exp = now + ttlSeconds;
Map<String, Object> headerClaims = new LinkedHashMap<>();
headerClaims.put("alg", "HS256");
headerClaims.put("typ", "JWT");
Map<String, Object> payloadClaims = new LinkedHashMap<>();
payloadClaims.put("iss", ISSUER);
payloadClaims.put("sub", subject);
payloadClaims.put("scope", scope);
payloadClaims.put("iat", now);
payloadClaims.put("exp", exp);
String headerJson = OBJECT_MAPPER.writeValueAsString(headerClaims);
String payloadJson = OBJECT_MAPPER.writeValueAsString(payloadClaims);
```
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/converter/DataCenterJoinVOToDataCenterConverter.java:
##########
@@ -0,0 +1,78 @@
+// 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.veeam.api.converter;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.cloudstack.veeam.VeeamControlService;
+import org.apache.cloudstack.veeam.api.DataCentersRouteHandler;
+import org.apache.cloudstack.veeam.api.dto.DataCenter;
+import org.apache.cloudstack.veeam.api.dto.Link;
+import org.apache.cloudstack.veeam.api.dto.Ref;
+import org.apache.cloudstack.veeam.api.dto.SupportedVersions;
+import org.apache.cloudstack.veeam.api.dto.Version;
+
+import com.cloud.api.query.vo.DataCenterJoinVO;
+import com.cloud.org.Grouping;
+
+public class DataCenterJoinVOToDataCenterConverter {
+ public static DataCenter toDataCenter(final DataCenterJoinVO zone) {
+ final String id = zone.getUuid();
+ final String basePath = VeeamControlService.ContextPath.value();
+ final String href = basePath + DataCentersRouteHandler.BASE_ROUTE +
DataCentersRouteHandler.BASE_ROUTE + "/" + id;
+
+ final DataCenter dc = new DataCenter();
+
+ // ---- Identity ----
+ dc.setId(id);
+ dc.setHref(href);
+ dc.setName(zone.getName());
+ dc.setDescription(zone.getDescription());
+
+ // ---- State ----
+
dc.setStatus(Grouping.AllocationState.Enabled.equals(zone.getAllocationState())
? "up" : "down");
+ dc.setLocal("false");
+ dc.setQuotaMode("disabled");
+ dc.setStorageFormat("v5");
+
+ // ---- Versions ----
+ final Version ver = Version.fromPackageAndCSVersion(false);
+ dc.setVersion(ver);
+ dc.setSupportedVersions(new SupportedVersions(List.of(ver)));
+
+ // ---- mac_pool (static placeholder) ----
+ dc.setMacPool(Ref.of(basePath + "/macpools/default", "default"));
+
+ // ---- Related links ----
+ dc.link = Arrays.asList(
+ Link.of(href + "/clusters", "clusters"),
+ Link.of(href + "/networks", "networks"),
+ Link.of(href + "/storagedomains", "storagedomains")
+ );
Review Comment:
The `href` concatenation duplicates `BASE_ROUTE`, producing an incorrect
URL. Also, `Link.of(rel, href)` is being called with parameters reversed (rel
is being set to a URL and href to a literal like `clusters`). Build `href` as
`basePath + BASE_ROUTE + \"/\" + id`, and construct links as
`Link.of(\"clusters\", href + \"/clusters\")` (same for
networks/storagedomains).
##########
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##########
@@ -127,6 +127,18 @@ public class BackupResponse extends BaseResponse {
@Param(description = "Indicates whether the VM from which the backup was
taken is expunged or not", since = "4.22.0")
private Boolean isVmExpunged;
+ @SerializedName("from_checkpoint_id")
+ @Param(description = "Previous active checkpoint id for incremental
backups", since = "4.22.0")
+ private String fromCheckpointId;
+
+ @SerializedName("to_checkpoint_id")
Review Comment:
These new response field names use underscores (`from_checkpoint_id`,
`to_checkpoint_id`), which is inconsistent with typical CloudStack API response
naming (commonly lower-cased without underscores like `backupid`, `vmid`,
etc.). Consider aligning the serialized names with existing conventions to
minimize client surprises and maintain consistency.
```suggestion
@SerializedName("fromcheckpointid")
@Param(description = "Previous active checkpoint id for incremental
backups", since = "4.22.0")
private String fromCheckpointId;
@SerializedName("tocheckpointid")
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetImageTransferProgressCommandWrapper.java:
##########
@@ -0,0 +1,95 @@
+//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.List;
+import java.util.Map;
+
+import org.apache.cloudstack.backup.GetImageTransferProgressAnswer;
+import org.apache.cloudstack.backup.GetImageTransferProgressCommand;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+
+@ResourceWrapper(handles = GetImageTransferProgressCommand.class)
+public class LibvirtGetImageTransferProgressCommandWrapper extends
CommandWrapper<GetImageTransferProgressCommand, Answer,
LibvirtComputingResource> {
+ protected Logger logger = LogManager.getLogger(getClass());
+
+ @Override
+ public Answer execute(GetImageTransferProgressCommand cmd,
LibvirtComputingResource resource) {
+ try {
+ List<String> transferIds = cmd.getTransferIds();
+ Map<String, String> volumePaths = cmd.getVolumePaths();
+ Map<String, Long> volumeSizes = cmd.getVolumeSizes();
+ Map<String, Long> progressMap = new HashMap<>();
+
+ if (transferIds == null || transferIds.isEmpty()) {
+ return new GetImageTransferProgressAnswer(cmd, true, "No
transfers to check", progressMap);
+ }
+
+ for (String transferId : transferIds) {
+ String volumePath = volumePaths.get(transferId);
+ Long volumeSize = volumeSizes.get(transferId);
+
+ if (volumePath == null || volumeSize == null || volumeSize ==
0) {
+ logger.warn("Missing volume path or size for transferId:
{}", transferId);
+ progressMap.put(transferId, null);
+ continue;
+ }
+
+ try {
+ File file = new File(volumePath);
+ if (!file.exists()) {
+ logger.warn("Volume file does not exist: {}",
volumePath);
+ progressMap.put(transferId, null);
+ continue;
+ }
+
+ long currentSize = file.length();
+
+ if (volumePath.endsWith(".qcow2") ||
volumePath.endsWith(".qcow")) {
+ try {
+ currentSize =
KVMPhysicalDisk.getVirtualSizeFromFile(volumePath);
Review Comment:
`GetImageTransferProgressAnswer`/API field descriptions indicate progress is
a percentage (0-100), but this implementation stores a byte size
(physical/virtual size) in the progress map. Either compute and return a
percent based on `volumeSize` (and clamp to 0..100), or rename/retarget the
API/Answer field to clearly represent bytes transferred to avoid client
misinterpretation.
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/utils/PathUtil.java:
##########
@@ -0,0 +1,75 @@
+// 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.veeam.utils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+
+import com.cloud.utils.UuidUtils;
+
+public class PathUtil {
+ private static final boolean CONSIDER_ONLY_UUID_AS_ID = false;
+
+ public static List<String> extractIdAndSubPath(final String path, final
String baseRoute) {
+
+ if (StringUtils.isBlank(path)) {
+ return null;
+ }
+
+ // Remove base route (be tolerant of trailing slash in baseRoute)
+ String rest = path;
+ if (StringUtils.isNotBlank(baseRoute)) {
+ String normalizedBase = baseRoute.endsWith("/") &&
baseRoute.length() > 1
+ ? baseRoute.substring(0, baseRoute.length() - 1)
+ : baseRoute;
+ if (rest.startsWith(normalizedBase)) {
+ rest = rest.substring(normalizedBase.length());
+ }
+ }
+
+ // Expect "/{id}" or "/{id}/..." (no empty segments)
+ if (StringUtils.isBlank(rest) || !rest.startsWith("/")) {
+ return null; // /api/datacenters (no id) or invalid format
+ }
+
+ rest = rest.substring(1); // remove leading '/'
+
+ if (StringUtils.isBlank(rest)) {
+ return null;
+ }
+
+ final String[] parts = rest.split("/", -1);
+
+ // Collect non-blank segments
+ List<String> validParts = new ArrayList<>();
+ for (String part : parts) {
+ if (StringUtils.isNotBlank(part)) {
+ validParts.add(part);
+ }
+ }
+
+ // Validate first segment is a UUID
+ if (validParts.isEmpty() || (CONSIDER_ONLY_UUID_AS_ID &&
!UuidUtils.isUuid(validParts.get(0)))) {
+ return null;
+ }
Review Comment:
The indentation is inconsistent, and the UUID validation comment does not
match behavior when `CONSIDER_ONLY_UUID_AS_ID` is `false` (no UUID validation
occurs). Consider either enabling UUID validation (and enforcing it), or
updating the comment/logic to reflect the intended contract; also prefer
returning an empty list (or an Optional) instead of `null` to simplify callers.
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/utils/PathUtil.java:
##########
@@ -0,0 +1,75 @@
+// 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.veeam.utils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+
+import com.cloud.utils.UuidUtils;
+
+public class PathUtil {
+ private static final boolean CONSIDER_ONLY_UUID_AS_ID = false;
+
+ public static List<String> extractIdAndSubPath(final String path, final
String baseRoute) {
+
+ if (StringUtils.isBlank(path)) {
+ return null;
+ }
+
+ // Remove base route (be tolerant of trailing slash in baseRoute)
+ String rest = path;
+ if (StringUtils.isNotBlank(baseRoute)) {
+ String normalizedBase = baseRoute.endsWith("/") &&
baseRoute.length() > 1
+ ? baseRoute.substring(0, baseRoute.length() - 1)
+ : baseRoute;
+ if (rest.startsWith(normalizedBase)) {
+ rest = rest.substring(normalizedBase.length());
+ }
+ }
+
+ // Expect "/{id}" or "/{id}/..." (no empty segments)
+ if (StringUtils.isBlank(rest) || !rest.startsWith("/")) {
+ return null; // /api/datacenters (no id) or invalid format
+ }
+
+ rest = rest.substring(1); // remove leading '/'
+
+ if (StringUtils.isBlank(rest)) {
+ return null;
+ }
+
+ final String[] parts = rest.split("/", -1);
+
+ // Collect non-blank segments
+ List<String> validParts = new ArrayList<>();
+ for (String part : parts) {
+ if (StringUtils.isNotBlank(part)) {
+ validParts.add(part);
+ }
+ }
+
+ // Validate first segment is a UUID
+ if (validParts.isEmpty() || (CONSIDER_ONLY_UUID_AS_ID &&
!UuidUtils.isUuid(validParts.get(0)))) {
+ return null;
+ }
+
+ return validParts;
Review Comment:
The indentation is inconsistent, and the UUID validation comment does not
match behavior when `CONSIDER_ONLY_UUID_AS_ID` is `false` (no UUID validation
occurs). Consider either enabling UUID validation (and enforcing it), or
updating the comment/logic to reflect the intended contract; also prefer
returning an empty list (or an Optional) instead of `null` to simplify callers.
```suggestion
if (StringUtils.isBlank(path)) {
return new ArrayList<>();
}
// Remove base route (be tolerant of trailing slash in baseRoute)
String rest = path;
if (StringUtils.isNotBlank(baseRoute)) {
String normalizedBase = baseRoute.endsWith("/") &&
baseRoute.length() > 1
? baseRoute.substring(0, baseRoute.length() - 1)
: baseRoute;
if (rest.startsWith(normalizedBase)) {
rest = rest.substring(normalizedBase.length());
}
}
// Expect "/{id}" or "/{id}/..." (no empty segments)
if (StringUtils.isBlank(rest) || !rest.startsWith("/")) {
return new ArrayList<>(); // /api/datacenters (no id) or invalid
format
}
rest = rest.substring(1); // remove leading '/'
if (StringUtils.isBlank(rest)) {
return new ArrayList<>();
}
final String[] parts = rest.split("/", -1);
// Collect non-blank segments
List<String> validParts = new ArrayList<>();
for (String part : parts) {
if (StringUtils.isNotBlank(part)) {
validParts.add(part);
}
}
// Ensure the first segment exists; validate it as a UUID only when
configured to do so
if (validParts.isEmpty() || (CONSIDER_ONLY_UUID_AS_ID &&
!UuidUtils.isUuid(validParts.get(0)))) {
return new ArrayList<>();
}
return validParts;
```
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/utils/ResponseWriter.java:
##########
@@ -0,0 +1,83 @@
+// 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.veeam.utils;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+
+import javax.servlet.http.HttpServletResponse;
+
+import org.apache.cloudstack.veeam.api.dto.Fault;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+public final class ResponseWriter {
+ private static final Logger LOGGER =
LogManager.getLogger(ResponseWriter.class);
+
+ private final Mapper mapper;
+
+ public ResponseWriter(final Mapper mapper) {
+ this.mapper = mapper;
+ }
+
+ public void write(final HttpServletResponse resp, final int status, final
Object body, final Negotiation.OutFormat fmt)
+ throws IOException {
+
+ resp.setStatus(status);
+
+ if (body == null) {
+ resp.setContentLength(0);
+ return;
+ }
+
+ final String payload;
+ final String contentType;
+
+ try {
+ if (fmt == Negotiation.OutFormat.XML) {
+ contentType = "application/xml";
+ payload = mapper.toXml(body);
+ } else {
+ contentType = "application/json";
+ payload = mapper.toJson(body);
+ }
+ } catch (Exception e) {
+ // Last-resort fallback
+ resp.setStatus(500);
+ resp.setHeader("Content-Type", "text/plain");
+ resp.getWriter().write("Internal Server Error");
+ return;
+ }
+
+ LOGGER.info("Writing response: {}\n{}", status, payload);
Review Comment:
Logging full response payloads at INFO can leak sensitive data (tokens, IDs,
credentials) and can be very noisy/large in production. Consider switching to
DEBUG and/or logging only metadata (status, content-type, payload length) with
optional truncation.
```suggestion
LOGGER.debug("Writing response: status={}, contentType={},
payloadLength={}", status, contentType, payload.length());
```
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/dto/ReportedDevice.java:
##########
@@ -0,0 +1,84 @@
+// 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.veeam.api.dto;
+
+public class ReportedDevice extends BaseDto {
+ private String comment;
+ private String description;
+ private NamedList<Ip> ips;
+ private Mac Mac;
Review Comment:
Field name `Mac` is capitalized, which is inconsistent with Java conventions
and makes the code harder to read/maintain. Rename the field to `mac` and
update getters/setters accordingly (method names can remain `getMac/setMac`).
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/dto/ReportedDevice.java:
##########
@@ -0,0 +1,84 @@
+// 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.veeam.api.dto;
+
+public class ReportedDevice extends BaseDto {
+ private String comment;
+ private String description;
+ private NamedList<Ip> ips;
+ private Mac Mac;
+ private String name;
+ private String type;
+ private Vm vm;
+
+ public String getComment() {
+ return comment;
+ }
+
+ public void setComment(String comment) {
+ this.comment = comment;
+ }
+
+ public String getDescription() {
+ return description;
+ }
+
+ public void setDescription(String description) {
+ this.description = description;
+ }
+
+ public NamedList<Ip> getIps() {
+ return ips;
+ }
+
+ public void setIps(NamedList<Ip> ips) {
+ this.ips = ips;
+ }
+
+ public Mac getMac() {
+ return Mac;
+ }
+
+ public void setMac(Mac mac) {
+ Mac = mac;
+ }
Review Comment:
Field name `Mac` is capitalized, which is inconsistent with Java conventions
and makes the code harder to read/maintain. Rename the field to `mac` and
update getters/setters accordingly (method names can remain `getMac/setMac`).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStopNBDServerCommandWrapper.java:
##########
@@ -0,0 +1,72 @@
+//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 org.apache.cloudstack.backup.StopNBDServerCommand;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+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 = StopNBDServerCommand.class)
+public class LibvirtStopNBDServerCommandWrapper extends
CommandWrapper<StopNBDServerCommand, Answer, LibvirtComputingResource> {
+ protected Logger logger = LogManager.getLogger(getClass());
+
+ 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();
+ }
+
+ @Override
+ public Answer execute(StopNBDServerCommand cmd, LibvirtComputingResource
resource) {
+ try {
+ String unitName = "qemu-nbd-" + cmd.getTransferId().hashCode();
Review Comment:
Using `hashCode()` for a systemd unit name introduces a collision risk
(different transferIds can produce the same hash), which could stop/reset the
wrong service. Prefer deriving the unit name from the actual transfer ID
(sanitized) or using a collision-resistant encoding (e.g., base32/base64url of
the UUID) with a safe length limit.
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/filter/AllowedClientCidrsFilter.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
+// 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.veeam.filter;
+
+import java.io.IOException;
+import java.net.InetAddress;
+import java.util.List;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.apache.cloudstack.veeam.VeeamControlService;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import com.cloud.utils.net.NetUtils;
+
+public class AllowedClientCidrsFilter implements Filter {
+
+ private static final Logger LOGGER =
LogManager.getLogger(AllowedClientCidrsFilter.class);
+
+ private final VeeamControlService veeamControlService;
+
+ public AllowedClientCidrsFilter(VeeamControlService veeamControlService) {
+ this.veeamControlService = veeamControlService;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig) {
+ // no-op
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain)
+ throws IOException, ServletException {
+ if (!(request instanceof HttpServletRequest) || !(response instanceof
HttpServletResponse)) {
+ chain.doFilter(request, response);
+ return;
+ }
+
+ final HttpServletRequest req = (HttpServletRequest) request;
+ final HttpServletResponse resp = (HttpServletResponse) response;
+
+ if (veeamControlService == null) {
+ LOGGER.warn("Failed to inject VeeamControlService, allowing
request by default");
+ chain.doFilter(request, response);
Review Comment:
Failing open when `VeeamControlService` injection is missing makes the
allow-list enforcement ineffective and can unintentionally expose the endpoint.
Prefer failing closed (e.g., return 503/500) or at least deny with 403 when
CIDR enforcement cannot be evaluated.
```suggestion
LOGGER.error("Failed to inject VeeamControlService, rejecting
request because allowed client CIDRs cannot be evaluated");
resp.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE,
"Service Unavailable");
```
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/api/converter/AsyncJobJoinVOToJobConverter.java:
##########
@@ -0,0 +1,90 @@
+// 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.veeam.api.converter;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.cloudstack.jobs.JobInfo;
+import org.apache.cloudstack.veeam.VeeamControlService;
+import org.apache.cloudstack.veeam.api.JobsRouteHandler;
+import org.apache.cloudstack.veeam.api.dto.Job;
+import org.apache.cloudstack.veeam.api.dto.Ref;
+import org.apache.cloudstack.veeam.api.dto.ResourceAction;
+import org.apache.cloudstack.veeam.api.dto.VmAction;
+
+import com.cloud.api.query.vo.AsyncJobJoinVO;
+import com.cloud.api.query.vo.UserVmJoinVO;
+
+public class AsyncJobJoinVOToJobConverter {
+
+ public static Job toJob(AsyncJobJoinVO vo) {
+ Job job = new Job();
+ final String basePath = VeeamControlService.ContextPath.value();
+ job.setId(vo.getUuid());
+ job.setHref(basePath + JobsRouteHandler.BASE_ROUTE + "/" +
vo.getUuid());
+ job.setAutoCleared(Boolean.TRUE.toString());
+ job.setExternal(Boolean.TRUE.toString());
+ job.setLastUpdated(System.currentTimeMillis());
+ job.setStartTime(vo.getCreated().getTime());
+ JobInfo.Status status = JobInfo.Status.values()[vo.getStatus()];
+ Long endTime = System.currentTimeMillis();
+ if (status == JobInfo.Status.SUCCEEDED) {
+ job.setStatus("finished");
+ job.setEndTime(System.currentTimeMillis());
+ } else if (status == JobInfo.Status.FAILED) {
+ job.setStatus(status.name().toLowerCase());
+ } else if (status == JobInfo.Status.CANCELLED) {
+ job.setStatus("aborted");
+ } else {
+ job.setStatus("started");
+ endTime = null;
+ }
+ if (endTime != null) {
+ job.setEndTime(endTime);
+ }
+ job.setOwner(Ref.of(basePath + "/api/users/" + vo.getUserUuid(),
vo.getUserUuid()));
+ job.setDescription("Something");
+ job.setLink(Collections.emptyList());
+ return job;
+ }
+
+ public static List<Job> toJobList(List<AsyncJobJoinVO> vos) {
+ return
vos.stream().map(AsyncJobJoinVOToJobConverter::toJob).collect(Collectors.toList());
+ }
+
+ protected static void fillAction(final ResourceAction action, final
AsyncJobJoinVO vo) {
+ final String basePath = VeeamControlService.ContextPath.value();
+ action.setJob(Ref.of(basePath + JobsRouteHandler.BASE_ROUTE +
vo.getUuid(), vo.getUuid()));
Review Comment:
The job href is missing a `/` separator between `BASE_ROUTE` and the UUID,
producing malformed URLs. Use `basePath + JobsRouteHandler.BASE_ROUTE + \"/\" +
vo.getUuid()` (consistent with `toJob(...)`).
```suggestion
action.setJob(Ref.of(basePath + JobsRouteHandler.BASE_ROUTE + "/" +
vo.getUuid(), vo.getUuid()));
```
##########
scripts/vm/hypervisor/kvm/imageserver/__main__.py:
##########
@@ -0,0 +1,20 @@
+# 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.
+
+from .server import main
+
+main()
Review Comment:
Calling `main()` unconditionally at import-time can trigger side effects if
`imageserver.__main__` is imported indirectly (tests/tools). Prefer guarding
with `if __name__ == \"__main__\": main()` so it only starts when executed as a
script/module.
```suggestion
if __name__ == "__main__":
main()
```
--
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]
