Copilot commented on code in PR #12970:
URL: https://github.com/apache/cloudstack/pull/12970#discussion_r3049389804
##########
ui/src/views/tools/ImportUnmanagedInstance.vue:
##########
@@ -184,12 +190,13 @@
</a-form-item>
<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"
Review Comment:
`: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.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java:
##########
@@ -275,4 +299,194 @@ protected void addExtraParamsToScript(String extraParams,
Script script) {
protected String encodeUsername(String username) {
return URLEncoder.encode(username, Charset.defaultCharset());
}
+
+ private String resolveVddkSetting(String commandValue, String agentValue) {
+ return
StringUtils.defaultIfBlank(StringUtils.trimToNull(commandValue),
StringUtils.trimToNull(agentValue));
+ }
+
+ protected boolean performInstanceConversionVddk(RemoteInstanceTO
vmwareInstance, String originalVMName,
+ String
temporaryConvertFolder, String vddkLibDir,
+ String libguestfsBackend,
String vddkTransports,
+ String
configuredVddkThumbprint,
+ long timeout, boolean
verboseModeEnabled, String extraParams,
+ String
temporaryConvertUuid, String passwordOption) {
+
+ 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;
+ }
+ 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());
+
+ String logPrefix = String.format("(%s) virt-v2v vddk import",
originalVMName);
+ OutputInterpreter.LineByLineOutputLogger outputLogger =
+ new OutputInterpreter.LineByLineOutputLogger(logger,
logPrefix);
+
+ logger.info("({}) Starting virt-v2v VDDK conversion", originalVMName);
+ script.execute(outputLogger);
+
+ int exitValue = script.getExitValue();
+ if (exitValue != 0) {
+ logger.error("({}) virt-v2v failed with exit code {}",
originalVMName, exitValue);
+ }
+ try {
+ Files.deleteIfExists(Path.of(passwordFilePath));
+ logger.debug("({}) Deleted password file {}", originalVMName,
passwordFilePath);
+ } catch (Exception e) {
+ logger.warn("({}) Failed to delete password file {}: {}",
originalVMName, passwordFilePath, e.getMessage());
+ }
+
+ return exitValue == 0;
+ }
+
+
+ protected String getVcenterThumbprint(String vcenterHost, long timeout,
String originalVMName) {
+ if (StringUtils.isBlank(vcenterHost)) {
+ return null;
+ }
+
+ String endpoint = String.format("%s:443", vcenterHost);
+ String command = String.format("openssl s_client -connect '%s'
</dev/null 2>/dev/null | " +
+ "openssl x509 -fingerprint -sha1 -noout", endpoint);
+
+ Script script = new Script("/bin/bash", timeout, logger);
+ script.add("-c");
+ script.add(command);
+
+ OutputInterpreter.AllLinesParser parser = new
OutputInterpreter.AllLinesParser();
+ script.execute(parser);
+
+ String output = parser.getLines();
+ if (script.getExitValue() != 0) {
+ logger.error("({}) Failed to fetch vCenter thumbprint for {}",
originalVMName, vcenterHost);
+ return null;
+ }
+
+ String thumbprint = extractSha1Fingerprint(output);
+ if (StringUtils.isBlank(thumbprint)) {
+ logger.error("({}) Failed to parse vCenter thumbprint from output
for {}", originalVMName, vcenterHost);
+ return null;
+ }
+ return thumbprint;
+ }
+
+ private String extractSha1Fingerprint(String output) {
+ String parsedOutput = StringUtils.trimToEmpty(output);
+ if (StringUtils.isBlank(parsedOutput)) {
+ return null;
+ }
+
+ for (String line : parsedOutput.split("\\R")) {
+ String trimmedLine = StringUtils.trimToEmpty(line);
+ if (StringUtils.isBlank(trimmedLine)) {
+ continue;
+ }
+
+ Matcher matcher = SHA1_FINGERPRINT_PATTERN.matcher(trimmedLine);
+ if (matcher.find()) {
+ return matcher.group(1).toUpperCase(Locale.ROOT);
+ }
+
+ // Fallback for raw fingerprint-only output.
+ if (trimmedLine.matches("(?i)[0-9a-f]{2}(:[0-9a-f]{2})+")) {
+ return trimmedLine.toUpperCase(Locale.ROOT);
+ }
+ }
+ return null;
+ }
+
+ /**
+ * 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();
Review Comment:
`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).
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1730,7 +1743,7 @@ protected UserVm
importUnmanagedInstanceFromVmwareToKvm(DataCenter zone, Cluster
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);
Review Comment:
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.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -5946,6 +5988,40 @@ public boolean hostSupportsOvfExport() {
return exitValue == 0;
}
+ /**
+ * Detect which password option virt-v2v supports by examining its --help
output
+ * @return "-ip" if supported (virt-v2v >= 2.8.1), "--password-file" if
older version, or null if detection fails
+ */
+ protected String detectPasswordFileOption() {
+ 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;
+ }
Review Comment:
`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.
```suggestion
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;
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java:
##########
@@ -275,4 +299,194 @@ protected void addExtraParamsToScript(String extraParams,
Script script) {
protected String encodeUsername(String username) {
return URLEncoder.encode(username, Charset.defaultCharset());
}
+
+ private String resolveVddkSetting(String commandValue, String agentValue) {
+ return
StringUtils.defaultIfBlank(StringUtils.trimToNull(commandValue),
StringUtils.trimToNull(agentValue));
+ }
+
+ protected boolean performInstanceConversionVddk(RemoteInstanceTO
vmwareInstance, String originalVMName,
+ String
temporaryConvertFolder, String vddkLibDir,
+ String libguestfsBackend,
String vddkTransports,
+ String
configuredVddkThumbprint,
+ long timeout, boolean
verboseModeEnabled, String extraParams,
+ String
temporaryConvertUuid, String passwordOption) {
+
+ 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;
+ }
+ 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());
+
Review Comment:
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 ... &&`.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java:
##########
@@ -275,4 +299,194 @@ protected void addExtraParamsToScript(String extraParams,
Script script) {
protected String encodeUsername(String username) {
return URLEncoder.encode(username, Charset.defaultCharset());
}
+
+ private String resolveVddkSetting(String commandValue, String agentValue) {
+ return
StringUtils.defaultIfBlank(StringUtils.trimToNull(commandValue),
StringUtils.trimToNull(agentValue));
+ }
+
+ protected boolean performInstanceConversionVddk(RemoteInstanceTO
vmwareInstance, String originalVMName,
+ String
temporaryConvertFolder, String vddkLibDir,
+ String libguestfsBackend,
String vddkTransports,
+ String
configuredVddkThumbprint,
+ long timeout, boolean
verboseModeEnabled, String extraParams,
+ String
temporaryConvertUuid, String passwordOption) {
+
+ 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;
+ }
Review Comment:
`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.
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1785,6 +1798,41 @@ protected void checkConversionStoragePool(Long
convertStoragePoolId, boolean for
}
}
+ protected void validateSelectedConversionStoragePoolForVddk(boolean
useVddk, Long convertStoragePoolId,
+
ServiceOfferingVO serviceOffering, Map<String, Long> dataDiskOfferingMap) {
+ if (!useVddk || convertStoragePoolId == null) {
+ return;
+ }
+
+ StoragePoolVO selectedStoragePool =
primaryDataStoreDao.findById(convertStoragePoolId);
+ if (selectedStoragePool == null) {
+ return;
+ }
+
+ if (serviceOffering.getDiskOfferingId() != null) {
+ DiskOfferingVO rootDiskOffering =
diskOfferingDao.findById(serviceOffering.getDiskOfferingId());
+ if (rootDiskOffering == null) {
+ throw new InvalidParameterValueException(String.format("Cannot
find disk offering with ID %s that belongs to the service offering %s",
+ 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");
+ }
+ }
+
+ 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");
+ }
+ }
Review Comment:
`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.
--
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]
