[VMware to KVM Migration] Fix unused convert env vars #11947

nvazquez · 2025-10-30T18:19:46Z

Description

This PR fixes unused convert env variables from PR #11594 on the KVM agents, when agent.properties set:

# Instance conversion TMPDIR env var
#convert.instance.env.tmpdir=

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

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

Screenshots (if appropriate):

How Has This Been Tested?

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

nvazquez · 2025-10-30T18:20:00Z

@blueorangutan package

blueorangutan · 2025-10-30T18:22:04Z

@nvazquez 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.

codecov · 2025-10-30T18:28:17Z

Codecov Report

❌ Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.56%. Comparing base (3d6cafe) to head (f2b6474).

Files with missing lines	Patch %	Lines
...s/src/main/java/com/cloud/utils/script/Script.java	64.70%	4 Missing and 2 partials ⚠️
.../wrapper/LibvirtConvertInstanceCommandWrapper.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #11947   +/-   ##
=========================================
  Coverage     17.56%   17.56%           
- Complexity    15543    15545    +2     
=========================================
  Files          5909     5909           
  Lines        529056   529070   +14     
  Branches      64617    64620    +3     
=========================================
+ Hits          92941    92946    +5     
- Misses       425661   425667    +6     
- Partials      10454    10457    +3

Flag	Coverage Δ
uitests	`3.58% <ø> (ø)`
unittests	`18.63% <61.90%> (+<0.01%)`	⬆️

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.

blueorangutan · 2025-10-30T19:33:45Z

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

nvazquez · 2025-10-30T20:36:48Z

@blueorangutan package

blueorangutan · 2025-10-30T20:38:05Z

@nvazquez 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 · 2025-10-30T21:54:34Z

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

nvazquez · 2025-10-30T22:28:22Z

@blueorangutan test

blueorangutan · 2025-10-30T22:30:04Z

@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2025-10-31T16:32:00Z

[SF] Trillian test result (tid-14752)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 61259 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11947-t14752-kvm-ol8.zip
Smoke tests completed. 148 look OK, 0 have errors, 1 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
all_test_human_readable_logs	`Skipped`	---	test_human_readable_logs.py

Copilot

Pull Request Overview

This PR adds support for passing custom environment variables when executing scripts via the Script class, specifically to support virt-v2v instance conversions. The changes enable passing environment variables like TMPDIR and VIRT_V2V_TMPDIR to the virt-v2v command, which is necessary for indirect tool invocations (e.g., virt-v2v calling qemu-img).

Key changes:

Extended the Script.execute() method to accept an optional environment variables array
Modified the script execution logic to use Runtime.exec() with environment variables when provided, falling back to ProcessBuilder when no environment is specified
Updated LibvirtConvertInstanceCommandWrapper to pass environment variables retrieved from the resource configuration

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
utils/src/main/java/com/cloud/utils/script/Script.java	Added new overloaded `execute()` methods to accept environment variables and modified execution logic to handle both ProcessBuilder and Runtime.exec() approaches
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java	Updated to retrieve and conditionally pass environment variables to the script execution

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

Copilot · 2025-11-03T11:04:40Z

utils/src/main/java/com/cloud/utils/script/Script.java

+        return executeInternal(interpreter, environment);
+    }
+
+    public String executeInternal(OutputInterpreter interpreter, String[] environment) {


The executeInternal method is public but appears to be an internal implementation detail. Consider making this method private or package-private to prevent direct external usage, as callers should use the execute() methods instead.

Suggested change

public String executeInternal(OutputInterpreter interpreter, String[] environment) {

private String executeInternal(OutputInterpreter interpreter, String[] environment) {

Copilot · 2025-11-03T11:04:40Z

utils/src/main/java/com/cloud/utils/script/Script.java

+                // Since Runtime.exec() does not support redirecting the error stream, then append 2>&1 to the command
+                String[] commands = new String[] {"sh", "-c", String.format("%s 2>&1", commandLine)};
+                // The PATH variable must be added for indirect calls within the running command
+                // Example: virt-v2v invokes qemu-img, which cannot be found if PATH is not set
+                String[] env = ArrayUtils.add(environment, String.format("PATH=%s", System.getenv("PATH")));
+                _process = Runtime.getRuntime().exec(commands, env, _workDir != null ? new File(_workDir) : null);


Wrapping the command in a shell (sh -c) could introduce command injection vulnerabilities if commandLine contains untrusted input. Ensure that all components of _command are properly validated or sanitized before being passed to this method, or consider using ProcessBuilder with proper environment variable support instead.

Suggested change

// Since Runtime.exec() does not support redirecting the error stream, then append 2>&1 to the command

String[] commands = new String[] {"sh", "-c", String.format("%s 2>&1", commandLine)};

// The PATH variable must be added for indirect calls within the running command

// Example: virt-v2v invokes qemu-img, which cannot be found if PATH is not set

String[] env = ArrayUtils.add(environment, String.format("PATH=%s", System.getenv("PATH")));

_process = Runtime.getRuntime().exec(commands, env, _workDir != null ? new File(_workDir) : null);

// Use ProcessBuilder to set environment variables and redirect error stream safely

ProcessBuilder pb = new ProcessBuilder(command);

pb.redirectErrorStream(true);

if (_workDir != null)

pb.directory(new File(_workDir));

// Set environment variables

for (String envVar : environment) {

int idx = envVar.indexOf('=');

if (idx > 0) {

String key = envVar.substring(0, idx);

String value = envVar.substring(idx + 1);

pb.environment().put(key, value);

}

}

// Ensure PATH is set

pb.environment().put("PATH", System.getenv("PATH"));

_process = pb.start();

Copilot · 2025-11-03T11:04:41Z

utils/src/main/java/com/cloud/utils/script/Script.java

+                String[] commands = new String[] {"sh", "-c", String.format("%s 2>&1", commandLine)};
+                // The PATH variable must be added for indirect calls within the running command
+                // Example: virt-v2v invokes qemu-img, which cannot be found if PATH is not set
+                String[] env = ArrayUtils.add(environment, String.format("PATH=%s", System.getenv("PATH")));


If System.getenv(\"PATH\") returns null (which can happen in some environments), this will add "PATH=null" to the environment variables, potentially breaking command execution. Add a null check and handle the case where PATH is not defined.

Suggested change

String[] env = ArrayUtils.add(environment, String.format("PATH=%s", System.getenv("PATH")));

String pathEnv = System.getenv("PATH");

String[] env = environment;

if (pathEnv != null) {

env = ArrayUtils.add(environment, String.format("PATH=%s", pathEnv));

} else {

_logger.warn("System environment variable PATH is not set; indirect command calls may fail.");

}

Copilot · 2025-11-03T11:04:41Z

...ain/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java

+        if (ArrayUtils.isEmpty(convertInstanceEnv)) {
+            script.execute(outputLogger);
+        } else {
+            script.execute(outputLogger, convertInstanceEnv);
+        }


The conditional check and branching can be simplified. The execute(outputLogger) method already calls execute(outputLogger, null), and the implementation handles null/empty arrays. You can directly call script.execute(outputLogger, convertInstanceEnv) without the conditional check, simplifying the code.

Suggested change

if (ArrayUtils.isEmpty(convertInstanceEnv)) {

script.execute(outputLogger);

} else {

script.execute(outputLogger, convertInstanceEnv);

}

script.execute(outputLogger, convertInstanceEnv);

shwstppr · 2025-11-03T11:05:38Z

utils/src/main/java/com/cloud/utils/script/Script.java

+                _process = pb.start();
+            } else {
+                // Since Runtime.exec() does not support redirecting the error stream, then append 2>&1 to the command
+                String[] commands = new String[] {"sh", "-c", String.format("%s 2>&1", commandLine)};


Using shell invocation may be comparatively unsafe versus ProcessBuilder. Not sure if possible to avoid this?
No log is added for this else block, may help

Javadoc for new execute overloaded method may help

agree, co-pilots comments are giving some hints.

DaanHoogland · 2025-11-03T14:19:33Z

utils/src/main/java/com/cloud/utils/script/Script.java

+    public String execute(OutputInterpreter interpreter, String[] environment) {
+        return executeInternal(interpreter, environment);
+    }
+
+    public String executeInternal(OutputInterpreter interpreter, String[] environment) {


Suggested change

public String execute(OutputInterpreter interpreter, String[] environment) {

return executeInternal(interpreter, environment);

}

public String executeInternal(OutputInterpreter interpreter, String[] environment) {

public String execute(OutputInterpreter interpreter, String[] environment) {

no need for the extra call on the stack, that I can see

DaanHoogland · 2025-11-03T14:23:54Z

utils/src/main/java/com/cloud/utils/script/Script.java

+                _process = pb.start();
+            } else {
+                // Since Runtime.exec() does not support redirecting the error stream, then append 2>&1 to the command
+                String[] commands = new String[] {"sh", "-c", String.format("%s 2>&1", commandLine)};


agree, co-pilots comments are giving some hints.

Fix unused convert env vars

f2b6474

nvazquez requested review from DaanHoogland, NuxRo, Pearl1594, shwstppr, vladimirpetrov and weizhouapache October 30, 2025 18:19

boring-cyborg bot added the component:kvm label Oct 30, 2025

DaanHoogland added the component:conversion label Oct 31, 2025

DaanHoogland added this to the 4.22.1 milestone Oct 31, 2025

shwstppr requested a review from Copilot November 3, 2025 10:54

Copilot AI reviewed Nov 3, 2025

View reviewed changes

shwstppr reviewed Nov 3, 2025

View reviewed changes

DaanHoogland reviewed Nov 3, 2025

View reviewed changes

	public String executeInternal(OutputInterpreter interpreter, String[] environment) {
	private String executeInternal(OutputInterpreter interpreter, String[] environment) {

-                // Since Runtime.exec() does not support redirecting the error stream, then append 2>&1 to the command
-                String[] commands = new String[] {"sh", "-c", String.format("%s 2>&1", commandLine)};
-                // The PATH variable must be added for indirect calls within the running command
-                // Example: virt-v2v invokes qemu-img, which cannot be found if PATH is not set
-                String[] env = ArrayUtils.add(environment, String.format("PATH=%s", System.getenv("PATH")));
-                _process = Runtime.getRuntime().exec(commands, env, _workDir != null ? new File(_workDir) : null);
+                // Use ProcessBuilder to set environment variables and redirect error stream safely
+                ProcessBuilder pb = new ProcessBuilder(command);
+                pb.redirectErrorStream(true);
+                if (_workDir != null)
+                    pb.directory(new File(_workDir));
+                // Set environment variables
+                for (String envVar : environment) {
+                    int idx = envVar.indexOf('=');
+                    if (idx > 0) {
+                        String key = envVar.substring(0, idx);
+                        String value = envVar.substring(idx + 1);
+                        pb.environment().put(key, value);
+                    }
+                }
+                // Ensure PATH is set
+                pb.environment().put("PATH", System.getenv("PATH"));
+                _process = pb.start();

-                String[] env = ArrayUtils.add(environment, String.format("PATH=%s", System.getenv("PATH")));
+                String pathEnv = System.getenv("PATH");
+                String[] env = environment;
+                if (pathEnv != null) {
+                    env = ArrayUtils.add(environment, String.format("PATH=%s", pathEnv));
+                } else {
+                    _logger.warn("System environment variable PATH is not set; indirect command calls may fail.");
+                }

[VMware to KVM Migration] Fix unused convert env vars #11947

Are you sure you want to change the base?

[VMware to KVM Migration] Fix unused convert env vars #11947

Uh oh!

Conversation

nvazquez commented Oct 30, 2025

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

nvazquez commented Oct 30, 2025

Uh oh!

blueorangutan commented Oct 30, 2025

Uh oh!

codecov bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Oct 30, 2025

Uh oh!

nvazquez commented Oct 30, 2025

Uh oh!

blueorangutan commented Oct 30, 2025

Uh oh!

blueorangutan commented Oct 30, 2025

Uh oh!

nvazquez commented Oct 30, 2025

Uh oh!

blueorangutan commented Oct 30, 2025

Uh oh!

blueorangutan commented Oct 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

shwstppr Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 30, 2025 •

edited

Loading