[VMware to KVM Migration] Fix unused convert env vars by nvazquez · Pull Request #11947 · apache/cloudstack

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 42.85714% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.59%. Comparing base (b8ed34e) to head (1a64f64).
⚠️ Report is 60 commits behind head on 4.22.

Files with missing lines	Patch %	Lines
...ervisor/kvm/resource/LibvirtComputingResource.java	14.28%	6 Missing ⚠️
...s/src/main/java/com/cloud/utils/script/Script.java	60.00%	2 Missing and 2 partials ⚠️
.../wrapper/LibvirtConvertInstanceCommandWrapper.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.22   #11947   +/-   ##
=========================================
  Coverage     17.59%   17.59%           
- Complexity    15600    15602    +2     
=========================================
  Files          5910     5910           
  Lines        529755   529767   +12     
  Branches      64724    64726    +2     
=========================================
+ Hits          93228    93235    +7     
- Misses       426035   426038    +3     
- Partials      10492    10494    +2

Flag	Coverage Δ
uitests	`3.60% <ø> (ø)`
unittests	`18.66% <42.85%> (+<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.

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

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 @DaanHoogland I would prefer keeping it like this as script.execute(outputLogger); has 73 usages which we'll need passing null as the second parameter. I think keeping it like this it cleaner

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

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

@shwstppr @DaanHoogland I would prefer keeping it like this as script.execute(outputLogger); has 73 usages which we'll need passing null as the second parameter. I think keeping it like this it cleaner

I don’t see a null @nvazquez , what do you mean?

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

Damans227 · 2025-12-02T13:44:08Z

@blueorangutan package

blueorangutan · 2025-12-02T13:46:04Z

@Damans227 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-12-02T14:55:18Z

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

rajujith · 2026-01-12T07:18:15Z

@nvazquez Since this is for the 4.22.1 release, could you retarget the PR to the 4.22 branch?

nvazquez · 2026-01-14T16:55:37Z

Thanks @rajujith retargeted to branch 4.22. @shwstppr @DaanHoogland I'll address your comments, thanks

nvazquez · 2026-01-14T17:50:45Z

@blueorangutan package

nvazquez · 2026-01-15T02:34:08Z

@blueorangutan package

blueorangutan · 2026-01-15T02:36:34Z

@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 · 2026-01-15T03:29:27Z

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

nvazquez · 2026-01-15T04:13:12Z

@blueorangutan test

blueorangutan · 2026-01-15T04:14:33Z

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

blueorangutan · 2026-01-15T04:17:07Z

[SF] Trillian Build Failed (tid-15186)

nvazquez · 2026-01-15T10:40:45Z

@blueorangutan test

blueorangutan · 2026-01-15T10:42:37Z

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

blueorangutan · 2026-01-15T10:46:06Z

[SF] Trillian Build Failed (tid-15189)

rajujith

@nvazquez I see the convert.instance.env.tmpdir getting used as a virt-v2v tmp directory but I do not see the other one being used although I set separate directories for both.
cc @NuxRo

NuxRo · 2026-01-23T08:51:07Z

@nvazquez @rajujith I do not remember at the moment under which circumstances TMPDIR was needed. I'd argue that it's good to have it in there even if in your test so far you have not seen it applied.

rajujith · 2026-01-23T12:51:02Z

@nvazquez, the usage of the configuration seems to be interchanged; otherwise its fine. Just wondering if we need a doc PR for this.

github-actions · 2026-01-26T07:21:00Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

shwstppr · 2026-01-28T05:00:29Z

@nvazquez can you please address the conflicts and comments from Jithin?

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

utils/src/main/java/com/cloud/utils/script/Script.java Outdated Show resolved Hide resolved

DaanHoogland reviewed Nov 3, 2025

View reviewed changes

rajujith added this to Apache CloudStack 4.22.1 Jan 5, 2026

rajujith moved this to In Progress in Apache CloudStack 4.22.1 Jan 7, 2026

Fix unused convert env vars

f893d3d

nvazquez force-pushed the 422-vmware-import-vars-unused branch from f2b6474 to f893d3d Compare January 14, 2026 16:53

boring-cyborg bot added the component:console-proxy label Jan 14, 2026

nvazquez changed the base branch from main to 4.22 January 14, 2026 16:53

Address review comments

1a64f64

nvazquez requested review from DaanHoogland and shwstppr January 14, 2026 17:51

nvazquez requested a review from rajujith January 15, 2026 10:40

rajujith reviewed Jan 22, 2026

View reviewed changes

github-actions bot added the status:has-conflicts label Jan 26, 2026

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!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

nvazquez Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaanHoogland Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

nvazquez Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Damans227 commented Dec 2, 2025

Uh oh!

blueorangutan commented Dec 2, 2025

Uh oh!

blueorangutan commented Dec 2, 2025

Uh oh!

rajujith commented Jan 12, 2026

Uh oh!

nvazquez commented Jan 14, 2026

Uh oh!

nvazquez commented Jan 14, 2026

Uh oh!

nvazquez commented Jan 15, 2026

Uh oh!

blueorangutan commented Jan 15, 2026

Uh oh!

blueorangutan commented Jan 15, 2026

Uh oh!

nvazquez commented Jan 15, 2026

Uh oh!

blueorangutan commented Jan 15, 2026

codecov bot commented Oct 30, 2025 •

edited

Loading

nvazquez Jan 14, 2026 •

edited

Loading

rajujith left a comment •

edited

Loading