Skip to content

Added vddk support in vmware to kvm migrations#12970

Draft
harikrishna-patnala wants to merge 6 commits intoapache:4.22from
shapeblue:Introduce-vddk-in-vmware-to-kvm-migrations-422
Draft

Added vddk support in vmware to kvm migrations#12970
harikrishna-patnala wants to merge 6 commits intoapache:4.22from
shapeblue:Introduce-vddk-in-vmware-to-kvm-migrations-422

Conversation

@harikrishna-patnala
Copy link
Copy Markdown
Member

@harikrishna-patnala harikrishna-patnala commented Apr 7, 2026

Description

This PR introduces the VDDK implementation as part of VMware to KVM migration which reduces the migration time by half when compared to the existing implementation with exportOVF

Solution: Introduce VDDK configuration at both the host agent level and per-API-call level, wired through the migration execution pipeline on the KVM host.


Key Capabilities

Capability Detail
Host-level config Four new agent.properties keys configure VDDK defaults per KVM host
API-level overrides Callers can pass VDDK settings via importVm details, overriding host defaults
Secure password handling vCenter password written to a temp file, passed via -ip or --password-file based on virt-v2v version, and deleted immediately after use
Auto-thumbprint fetch If no thumbprint is configured, it is automatically retrieved from vCenter via openssl s_client

Configuration Keys

libguestfs.backend   # LIBGUESTFS backend (default: direct)
vddk.lib.dir         # Path to VDDK library directory
vddk.transports      # Transport preference, e.g. nbd:nbdssl
vddk.thumbprint      # vCenter SHA1 thumbprint (auto-fetched if unset)

Precedence

API details  >  agent.properties

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@harikrishna-patnala harikrishna-patnala force-pushed the Introduce-vddk-in-vmware-to-kvm-migrations-422 branch from b95cfb7 to 7dc5d50 Compare April 7, 2026 04:48
@harikrishna-patnala
Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@harikrishna-patnala harikrishna-patnala added this to the 4.22.1 milestone Apr 7, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 7, 2026

Codecov Report

❌ Patch coverage is 44.27245% with 180 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.62%. Comparing base (4708121) to head (7fb0b85).

Files with missing lines Patch % Lines
.../wrapper/LibvirtConvertInstanceCommandWrapper.java 46.82% 82 Missing and 10 partials ⚠️
...ervisor/kvm/resource/LibvirtComputingResource.java 9.80% 46 Missing ⚠️
...va/com/cloud/agent/api/ConvertInstanceCommand.java 50.00% 15 Missing ⚠️
.../apache/cloudstack/vm/UnmanagedVMsManagerImpl.java 69.76% 5 Missing and 8 partials ⚠️
.../java/com/cloud/agent/api/to/RemoteInstanceTO.java 53.84% 6 Missing ⚠️
...java/com/cloud/agent/manager/AgentManagerImpl.java 0.00% 5 Missing ⚠️
...e/cloudstack/api/command/admin/vm/ImportVmCmd.java 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               4.22   #12970      +/-   ##
============================================
+ Coverage     17.60%   17.62%   +0.01%     
- Complexity    15677    15698      +21     
============================================
  Files          5918     5918              
  Lines        531681   531982     +301     
  Branches      65005    65040      +35     
============================================
+ Hits          93623    93776     +153     
- Misses       427498   427631     +133     
- Partials      10560    10575      +15     
Flag Coverage Δ
uitests 3.70% <ø> (-0.01%) ⬇️
unittests 18.70% <44.27%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@harikrishna-patnala
Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@harikrishna-patnala
Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17380

@harikrishna-patnala
Copy link
Copy Markdown
Member Author

@blueorangutan package

# Optional vCenter SHA1 thumbprint for VMware to KVM conversion via VDDK, passed as
# -io vddk-thumbprint=<value>. If unset, CloudStack computes it on the KVM host via openssl.
#vddk.thumbprint=

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# Instance conversion VIRT_V2V_TMPDIR env var
#convert.instance.env.virtv2v.tmpdir=

# LIBGUESTFS backend to use for VMware to KVM conversion via VDDK (default: direct)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should keep other possible options in the comment (other than direct) ?

}

public boolean getUseVddk() {
return BooleanUtils.toBooleanDefaultIfNull(useVddk, false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return BooleanUtils.toBooleanDefaultIfNull(useVddk, false);
return BooleanUtils.toBooleanDefaultIfNull(useVddk, true);

defaulted to true

return exitValue == 0;
}


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

return StringUtils.defaultIfBlank(StringUtils.trimToNull(commandValue), StringUtils.trimToNull(agentValue));
}

protected boolean performInstanceConversionVddk(RemoteInstanceTO vmwareInstance, String originalVMName,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
protected boolean performInstanceConversionVddk(RemoteInstanceTO vmwareInstance, String originalVMName,
protected boolean performInstanceConversionUsingVddk(RemoteInstanceTO vmwareInstance, String originalVMName,

return false;
}

String passwordFilePath = String.format("/root/v2v.pass.cloud.%s",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use random file for each conversion as before, check below code ref. multiple conversions from the same vcenter would use the same hard-coded file, and earlier conversion can remove the file after conversion is finished.

private String createTemporaryPasswordFileAndRetrievePath(RemoteInstanceTO sourceInstance) {
String password = null;
if (sourceInstance.getHypervisorType() == Hypervisor.HypervisorType.VMware) {
password = sourceInstance.getVcenterPassword();
}
String passwordFile = String.format("/tmp/vmw-%s", UUID.randomUUID());
String msg = String.format("Creating a temporary password file for VMware instance %s conversion on: %s", sourceInstance.getInstanceName(), passwordFile);
s_logger.debug(msg);
Script.runSimpleBashScriptForExitValueAvoidLogging(String.format("echo \"%s\" > %s", password, passwordFile));
return passwordFile;
}

}

return new ConvertInstanceAnswer(cmd, temporaryConvertUuid);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

serviceOffering.getDiskOfferingId(), serviceOffering.getName()));
}
if (!volumeApiService.doesStoragePoolSupportDiskOffering(selectedStoragePool, rootDiskOffering)) {
throw new InvalidParameterValueException("Using VDDK, multiple storage pools cannot be used if a storage pool is selected for conversion");
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better rephrase the msg, to indicate that the selected compute offering picked different storage pool other than the selected pool for conversion.

(temporary) conversion storage in the earlier migration process is being considered as the primary storage allocated for the imported volumes while using VDDK and the offerings selected should honor that. Check if it is possible to list suitable compute/disk offerings with the selected pool in the UI? UI and Docs need to indicate this change for VDDK.

throw new InvalidParameterValueException(String.format("Cannot find disk offering with ID %s", diskOfferingId));
}
if (!volumeApiService.doesStoragePoolSupportDiskOffering(selectedStoragePool, diskOffering)) {
throw new InvalidParameterValueException("Using VDDK, multiple storage pools cannot be used if a storage pool is selected for conversion");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above for disk offerings.


setConvertInstanceEnv(convertEnvTmpDir, convertEnvVirtv2vTmpDir);

vddkLibDir = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.VDDK_LIB_DIR);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can auto detect vddk lib path with some standard path for it and set it in agent.properties when vddk is installed?

@sureshanaparti
Copy link
Copy Markdown
Contributor

sureshanaparti commented Apr 8, 2026

CheckConvertInstanceCommand should ensure vddk support on host when usevddk is enabled, in addition to virt-v2v, and skip checking ovf export (in LibvirtCheckConvertInstanceCommandWrapper)

private CheckConvertInstanceAnswer checkConversionSupportOnHost(HostVO convertHost, String sourceVM, boolean checkWindowsGuestConversionSupport) {

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a VDDK-based VMware→KVM conversion path (via virt-v2v) to speed up importVm migrations, with host-level defaults (agent.properties) and API-level overrides (importVm details), plus UI support and tests.

Changes:

  • Introduces usevddk API parameter and propagates VDDK settings through the VMware→KVM import pipeline into ConvertInstanceCommand.
  • Implements VDDK conversion execution on KVM hosts (password-file handling, vCenter thumbprint retrieval, virt-v2v option detection) and advertises host VDDK capability.
  • Updates UI to toggle VDDK mode and adjusts conversion options; adds i18n string and expands unit test coverage.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
ui/src/views/tools/ImportUnmanagedInstance.vue Adds “Use VDDK” toggle and alters conversion option interactions/params.
ui/public/locales/en.json Adds UI label for VDDK toggle.
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java Wires usevddk and details overrides into conversion flow; adds validation.
server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java Adds test cases for VDDK behavior and validations.
core/src/main/java/com/cloud/agent/api/ConvertInstanceCommand.java Adds VDDK-related fields to conversion command.
api/src/main/java/org/apache/cloudstack/api/command/admin/vm/ImportVmCmd.java Adds usevddk request parameter.
api/src/main/java/org/apache/cloudstack/api/ApiConstants.java Adds USE_VDDK constant.
api/src/main/java/com/cloud/host/Host.java Adds host detail key for VDDK support.
api/src/main/java/com/cloud/agent/api/to/RemoteInstanceTO.java Adds cluster/host fields needed for VPX URL construction.
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java Persists reported host VDDK support detail.
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java Loads VDDK agent.properties, detects virt-v2v password option, reports VDDK support.
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReadyCommandWrapper.java Reports host.vddk.support in Ready response.
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java Adds VDDK conversion execution path and thumbprint parsing.
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapperTest.java Adds unit tests around VDDK conversion behavior.
agent/src/main/java/com/cloud/agent/properties/AgentProperties.java Defines new agent properties for VDDK settings.
agent/conf/agent.properties Documents new VDDK-related configuration keys.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 191 to 201
<a-form-item name="convertstorageoption" ref="convertstorageoption">
<check-box-select-pair
:key="`convertstorageoption-${form.usevddk ? 'vddk' : 'default'}-${switches.forceConvertToPool ? 'pool' : 'tmp'}`"
layout="vertical"
v-if="cluster.hypervisortype === 'KVM' && selectedVmwareVcenter"
:resourceKey="cluster.id"
:selectOptions="storageOptionsForConversion"
:checkBoxLabel="switches.forceConvertToPool ? $t('message.select.destination.storage.instance.conversion') : $t('message.select.temporary.storage.instance.conversion')"
:defaultCheckBoxValue="false"
:defaultCheckBoxValue="form.usevddk"
:reversed="false"
@handle-checkselectpair-change="updateSelectedStorageOptionForConversion"
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:defaultCheckBoxValue="form.usevddk" will render the checkbox as checked when VDDK is enabled, but CheckBoxSelectPair only sets checked in created() and does not initialize selectedOption or emit handle-checkselectpair-change unless the user toggles the checkbox. This can leave selectedStorageOptionForConversion unset and prevent storage pools from being loaded while the UI appears selected. Consider explicitly calling updateSelectedStorageOptionForConversion(...) from onUseVddkChange, or updating CheckBoxSelectPair to initialize selectedOption + emit when defaultCheckBoxValue starts as true.

Copilot uses AI. Check for mistakes.
Comment on lines +314 to +355
String vcenterPassword = vmwareInstance.getVcenterPassword();
if (StringUtils.isBlank(vcenterPassword)) {
logger.error("({}) Could not determine vCenter password for {}", originalVMName, vmwareInstance.getVcenterHost());
return false;
}

String passwordFilePath = String.format("/root/v2v.pass.cloud.%s",
StringUtils.defaultIfBlank(vmwareInstance.getVcenterHost(), "unknown"));
try {
Files.writeString(Path.of(passwordFilePath), vcenterPassword);
logger.debug("({}) Written vCenter password to {}", originalVMName, passwordFilePath);
} catch (Exception e) {
logger.error("({}) Failed to write vCenter password file {}: {}", originalVMName, passwordFilePath, e.getMessage());
return false;
}

String vpxUrl = buildVpxUrl(vmwareInstance, originalVMName);

StringBuilder cmd = new StringBuilder();

String effectiveLibguestfsBackend = StringUtils.defaultIfBlank(libguestfsBackend, "direct");
cmd.append("export LIBGUESTFS_BACKEND=").append(effectiveLibguestfsBackend).append(" && ");

cmd.append("virt-v2v ");
cmd.append("--root first ");
cmd.append("-ic '").append(vpxUrl).append("' ");
if (StringUtils.isBlank(passwordOption)) {
logger.error("({}) Could not determine supported password file option for virt-v2v", originalVMName);
return false;
}

cmd.append(passwordOption).append(" ").append(passwordFilePath).append(" ");
cmd.append("-it vddk ");
cmd.append("-io vddk-libdir=").append(vddkLibDir).append(" ");
String vddkThumbprint = StringUtils.trimToNull(configuredVddkThumbprint);
if (StringUtils.isBlank(vddkThumbprint)) {
vddkThumbprint = getVcenterThumbprint(vmwareInstance.getVcenterHost(), timeout, originalVMName);
}
if (StringUtils.isBlank(vddkThumbprint)) {
logger.error("({}) Could not determine vCenter thumbprint for {}", originalVMName, vmwareInstance.getVcenterHost());
return false;
}
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performInstanceConversionVddk writes the vCenter password to a fixed path under /root derived from the vCenter host. This is vulnerable to races between concurrent conversions (overwriting/deleting each other’s password file) and may leave the password on disk on early returns (e.g., when passwordOption is blank or thumbprint lookup fails). Use a unique temp file (e.g., Files.createTempFile), set restrictive permissions (0600), and ensure deletion happens in a finally block that runs for all exit paths.

Copilot uses AI. Check for mistakes.
Comment on lines +332 to +377
StringBuilder cmd = new StringBuilder();

String effectiveLibguestfsBackend = StringUtils.defaultIfBlank(libguestfsBackend, "direct");
cmd.append("export LIBGUESTFS_BACKEND=").append(effectiveLibguestfsBackend).append(" && ");

cmd.append("virt-v2v ");
cmd.append("--root first ");
cmd.append("-ic '").append(vpxUrl).append("' ");
if (StringUtils.isBlank(passwordOption)) {
logger.error("({}) Could not determine supported password file option for virt-v2v", originalVMName);
return false;
}

cmd.append(passwordOption).append(" ").append(passwordFilePath).append(" ");
cmd.append("-it vddk ");
cmd.append("-io vddk-libdir=").append(vddkLibDir).append(" ");
String vddkThumbprint = StringUtils.trimToNull(configuredVddkThumbprint);
if (StringUtils.isBlank(vddkThumbprint)) {
vddkThumbprint = getVcenterThumbprint(vmwareInstance.getVcenterHost(), timeout, originalVMName);
}
if (StringUtils.isBlank(vddkThumbprint)) {
logger.error("({}) Could not determine vCenter thumbprint for {}", originalVMName, vmwareInstance.getVcenterHost());
return false;
}
cmd.append("-io vddk-thumbprint=").append(vddkThumbprint).append(" ");
if (StringUtils.isNotBlank(vddkTransports)) {
cmd.append("-io vddk-transports=").append(vddkTransports).append(" ");
}
cmd.append(originalVMName).append(" ");
cmd.append("-o local ");
cmd.append("-os ").append(temporaryConvertFolder).append(" ");
cmd.append("-of qcow2 ");
cmd.append("-on ").append(temporaryConvertUuid).append(" ");

if (verboseModeEnabled) {
cmd.append("-v ");
}

if (StringUtils.isNotBlank(extraParams)) {
cmd.append(extraParams).append(" ");
}

Script script = new Script("/bin/bash", timeout, logger);
script.add("-c");
script.add(cmd.toString());

Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VDDK path builds a single shell string and runs it via /bin/bash -c, concatenating values like VM name, paths, transports, and extraParams. Several of these are user/DB configurable and are not shell-escaped, which can break conversions for names with spaces/special chars and also introduces command-injection risk. Prefer constructing a Script("virt-v2v", ...) and passing arguments with script.add(...) (similar to performInstanceConversion), and set LIBGUESTFS_BACKEND via the env-map rather than export ... &&.

Copilot uses AI. Check for mistakes.
Comment on lines +455 to +490
/**
* Build vpx:// URL for virt-v2v
*
* Format:
* vpx://user@vcenter/DC/cluster/host?no_verify=1
*/
private String buildVpxUrl(RemoteInstanceTO vmwareInstance, String originalVMName) {

String vcenter = vmwareInstance.getVcenterHost();
String username = vmwareInstance.getVcenterUsername();
String datacenter = vmwareInstance.getDatacenterName();
String cluster = vmwareInstance.getClusterName();
String host = vmwareInstance.getHostName();

String encodedUsername = encodeUsername(username);

StringBuilder url = new StringBuilder();
url.append("vpx://")
.append(encodedUsername)
.append("@")
.append(vcenter)
.append("/")
.append(datacenter);

if (StringUtils.isNotBlank(cluster)) {
url.append("/").append(cluster);
}

if (StringUtils.isNotBlank(host)) {
url.append("/").append(host);
}

url.append("?no_verify=1");

logger.info("({}) Using VPX URL: {}", originalVMName, url);
return url.toString();
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildVpxUrl appends ?no_verify=1 and logs the full VPX URL (including the username) at INFO. Disabling verification undermines the thumbprint-based trust model, and logging credentials-related identifiers is risky. Consider removing no_verify=1 if not strictly required, URL-encoding datacenter/cluster/host path segments, and sanitizing logs (e.g., log only vCenter host + datacenter, or redact the username).

Copilot uses AI. Check for mistakes.
Comment on lines 1729 to +1746
@@ -1725,4 +1738,4 @@
serviceOffering, dataDiskOfferingMap, temporaryConvertLocation,
ovfTemplateOnConvertLocation, forceConvertToPool, extraParams);
} else {
// Uses KVM Host for OVF export to temporary conversion location, through ovftool
@@ -1730,7 +1743,7 @@
convertedInstance = convertVmwareInstanceToKVMAfterExportingOVFToConvertLocation(
sourceVMName, sourceVMwareInstance, convertHost, importHost,
convertStoragePools, serviceOffering, dataDiskOfferingMap,
temporaryConvertLocation, vcenter, username, password, datacenterName, forceConvertToPool, extraParams);
temporaryConvertLocation, vcenter, username, password, datacenterName, forceConvertToPool, extraParams, useVddk, details);
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment and log message in this branch say it always performs OVF export through ovftool, but this same path is now also used when useVddk is true (which does not export OVF). This is misleading for troubleshooting and future maintenance; update the comment/logs (or split into a dedicated VDDK method) so the selected conversion strategy is accurately reflected.

Copilot uses AI. Check for mistakes.
Comment on lines +1818 to +1832
if (!volumeApiService.doesStoragePoolSupportDiskOffering(selectedStoragePool, rootDiskOffering)) {
throw new InvalidParameterValueException("Using VDDK, multiple storage pools cannot be used if a storage pool is selected for conversion");
}
}

if (MapUtils.isNotEmpty(dataDiskOfferingMap)) {
for (Long diskOfferingId : dataDiskOfferingMap.values()) {
DiskOfferingVO diskOffering = diskOfferingDao.findById(diskOfferingId);
if (diskOffering == null) {
throw new InvalidParameterValueException(String.format("Cannot find disk offering with ID %s", diskOfferingId));
}
if (!volumeApiService.doesStoragePoolSupportDiskOffering(selectedStoragePool, diskOffering)) {
throw new InvalidParameterValueException("Using VDDK, multiple storage pools cannot be used if a storage pool is selected for conversion");
}
}
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateSelectedConversionStoragePoolForVddk throws "Using VDDK, multiple storage pools cannot be used if a storage pool is selected for conversion" when a disk offering is unsupported by the selected pool. The message doesn’t match the actual failure condition (disk offering incompatibility) and will be confusing to users. Consider a clearer message that names the selected pool and the disk offering that isn’t supported.

Copilot uses AI. Check for mistakes.
Comment on lines +5996 to +6014
try {
ProcessBuilder pb = new ProcessBuilder("virt-v2v", "--help");
Process process = pb.start();

String output = new String(process.getInputStream().readAllBytes());
process.waitFor();

if (output.contains("-ip <filename>")) {
return "-ip";
} else if (output.contains("--password-file")) {
return "--password-file";
} else {
LOGGER.error("virt-v2v does not support -ip or --password-file");
return null;
}
} catch (Exception e) {
LOGGER.error("Failed to detect virt-v2v password option: {}", e.getMessage());
return null;
}
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

detectPasswordFileOption() runs virt-v2v --help via ProcessBuilder without a timeout and without consuming stderr; if virt-v2v hangs or produces enough stderr output, agent startup could block indefinitely. Consider using the existing Script helper with a timeout (and redirected stderr), and check/handle the process exit code before trusting output.

Suggested change
try {
ProcessBuilder pb = new ProcessBuilder("virt-v2v", "--help");
Process process = pb.start();
String output = new String(process.getInputStream().readAllBytes());
process.waitFor();
if (output.contains("-ip <filename>")) {
return "-ip";
} else if (output.contains("--password-file")) {
return "--password-file";
} else {
LOGGER.error("virt-v2v does not support -ip or --password-file");
return null;
}
} catch (Exception e) {
LOGGER.error("Failed to detect virt-v2v password option: {}", e.getMessage());
return null;
}
final int timeout = 30;
final Script script = new Script("/bin/bash", timeout, LOGGER);
script.add("-c");
script.add("virt-v2v --help 2>&1");
final AllLinesParser parser = new AllLinesParser();
final String executionResult = script.execute(parser);
if (executionResult != null) {
LOGGER.error("Failed to detect virt-v2v password option: {}", executionResult);
return null;
}
final String output = parser.getLines();
if (StringUtils.isBlank(output)) {
LOGGER.error("Failed to detect virt-v2v password option: empty output from virt-v2v --help");
return null;
}
if (output.contains("-ip <filename>")) {
return "-ip";
} else if (output.contains("--password-file")) {
return "--password-file";
} else {
LOGGER.error("virt-v2v does not support -ip or --password-file");
return null;
}

Copilot uses AI. Check for mistakes.
throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
String.format("Parameters %s and %s are mutually exclusive",
ApiConstants.FORCE_MS_TO_IMPORT_VM_FILES, ApiConstants.USE_VDDK));
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can move this validation before checking for pool

serviceOffering, dataDiskOfferingMap, temporaryConvertLocation,
ovfTemplateOnConvertLocation, forceConvertToPool, extraParams);
} else {
// Uses KVM Host for OVF export to temporary conversion location, through ovftool
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Uses KVM Host for direct conversion using VDDK, or for OVF export to temporary conversion location through ovftool

@@ -1730,7 +1743,7 @@
convertedInstance = convertVmwareInstanceToKVMAfterExportingOVFToConvertLocation(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if we can have separate methods for conversion using vddk and export ovf on kvm host

otherwise, change method name

Suggested change
convertedInstance = convertVmwareInstanceToKVMAfterExportingOVFToConvertLocation(
convertedInstance = convertVmwareInstanceToKVMUsingVDDKOrAfterExportingOVFToConvertLocation(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants