MTV-4454 | Separate extra args for virt-v2v conversion and inspection#4509
Merged
mnecas merged 1 commit intokubev2v:mainfrom Feb 3, 2026
Merged
MTV-4454 | Separate extra args for virt-v2v conversion and inspection#4509mnecas merged 1 commit intokubev2v:mainfrom
mnecas merged 1 commit intokubev2v:mainfrom
Conversation
yaacov
requested review from
Hazanel,
mnecas,
mrnold and
solenoci
as code owners
Previously, VIRT_V2V_EXTRA_ARGS was applied to all virt-v2v commands including virt-v2v-inspector. This caused migrations to fail when using conversion-specific args like --parallel that are not recognized by virt-v2v-inspector. This change introduces a new VIRT_V2V_INSPECTOR_EXTRA_ARGS setting and separates the argument handling: - VIRT_V2V_EXTRA_ARGS now applies only to virt-v2v and virt-v2v-in-place - VIRT_V2V_INSPECTOR_EXTRA_ARGS applies only to virt-v2v-inspector - Shared args (--root, --mac, --key) remain in addCommonArgs() for all Breaking change: Users who relied on extra args being passed to both conversion and inspection will need to set both env vars. Ref: https://issues.redhat.com/browse/MTV-4454 Resolves: MTV-4454 Signed-off-by: yaacov <yzamir@redhat.com>
yaacov
force-pushed
the
separate-virt-v2v-exra-args
branch
from
510384b to
6bf3f33
Compare
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4509 +/- ##
=========================================
- Coverage 15.45% 8.91% -6.54%
=========================================
Files 112 468 +356
Lines 23377 53401 +30024
=========================================
+ Hits 3613 4763 +1150
- Misses 19479 48233 +28754
- Partials 285 405 +120
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai hi, can you review ? |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request adds virt-v2v-inspector extra arguments configuration throughout the Forklift codebase, introduces HyperV-related resource limit and request fields to ForkliftController CRD, expands RBAC permissions for CSI drivers, adds a suggestion field to status conditions in multiple CRDs, and refactors conversion argument handling to distinguish between conversion and inspection paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/virt-v2v/config/variables.go (1)
15-172:⚠️ Potential issue | 🟠 MajorFail fast on invalid
V2V_inspector_extra_argsJSON (don’t silently ignore).
Right now a malformed value is only printed and then ignored, which can hide misconfiguration. Return the parse error fromLoad()so startup fails with a clear message, and add tests for valid/invalid JSON inputs.💡 Proposed fix (propagate parse error)
- s.InspectorExtraArgs = s.getInspectorExtraArgs() + if s.InspectorExtraArgs, err = s.getInspectorExtraArgs(); err != nil { + return err + } -func (s *AppConfig) getInspectorExtraArgs() []string { +func (s *AppConfig) getInspectorExtraArgs() ([]string, error) { var extraArgs []string if envExtraArgs, found := os.LookupEnv(EnvInspectorExtraArgsName); found && envExtraArgs != "" { if err := json.Unmarshal([]byte(envExtraArgs), &extraArgs); err != nil { - fmt.Printf("Failed to parse %s: %v\n", EnvInspectorExtraArgsName, err) - return nil + return nil, fmt.Errorf("parse %s: %w", EnvInspectorExtraArgsName, err) } } - return extraArgs + return extraArgs, nil }As per coding guidelines: Error Handling: No silent failures. Errors must be checked, wrapped with context, and propagated; coverage: Make sure that the code has unit tests.
operator/.downstream_manifests (1)
7636-7641:⚠️ Potential issue | 🟡 MinorAlign CSV description with CRD wording.
Line 7636 mentions only OVA, while the CRD description (Line 203‑204) includes HyperV. Suggest mirroring that wording for consistency.
Proposed fix
- - description: Enable OVA appliance management endpoints (default false) + - description: Enable OVF-based appliance management endpoints (OVA, HyperV) (default false)
🤖 Fix all issues with AI agents
In `@operator/.downstream_manifests`:
- Around line 6733-6735: Update the RBAC rule that currently groups
"storageclasses" and "csidrivers" under the same verbs block: split the rule so
"storageclasses" retains "list" and "watch" (for the informer) while
"csidrivers" is limited to "get" only (since the controller only calls r.Get()
to verify the SMB CSI driver). Locate the rule containing the "storageclasses"
and "csidrivers" resources and adjust the verbs for "csidrivers" to only include
"get" (or create a separate rule for csidrivers with verbs: ["get"]).
In `@operator/.upstream_manifests`:
- Around line 249-268: This change mixes HyperV-specific config keys
(hyperv_container_limits_cpu, hyperv_container_limits_memory,
hyperv_container_requests_cpu, hyperv_container_requests_memory,
hyperv_provider_server_fqin) into a PR meant for virt-v2v args separation;
remove these hyperv_* entries from this PR's diff and extract them into a
separate focused PR that only adds the HyperV resource/image fields, leaving the
virt-v2v args changes isolated (search for the keys hyperv_container_limits_cpu,
hyperv_container_limits_memory, hyperv_container_requests_cpu,
hyperv_container_requests_memory, hyperv_provider_server_fqin to locate and
remove/extract).
- Line 7636: The CSV description string "Enable OVA appliance management
endpoints (default false)" is inconsistent with the CRD description that
references OVF-based endpoints (OVA, HyperV); update the CSV description (the
descriptor containing "Enable OVA appliance management endpoints") to match the
CRD wording — e.g., change it to "Enable OVF‑based appliance management
endpoints (OVA, HyperV) (default false)" or the exact phrase used in the CRD so
both documents use identical terminology.
In `@pkg/controller/plan/adapter/vsphere/builder.go`:
- Around line 299-305: Add a unit test that configures
settings.Settings.Migration.VirtV2vInspectorExtraArgs and asserts the Pod
environment builder includes an env var with Name "V2V_inspector_extra_args" and
Value equal to that setting; update the existing PodEnvironment tests (the test
that exercises the Pod environment builder / function that produces core.EnvVar
entries) to set the Migration.VirtV2vInspectorExtraArgs value, call the same
builder used in builder.go, and add an assertion verifying the presence and
value of the "V2V_inspector_extra_args" env var so the new lines are covered.
In `@pkg/settings/migration.go`:
- Around line 224-231: Add unit tests for the VirtV2vInspectorExtraArgs behavior
in pkg/settings/migration.go: write tests that set/clear the environment
variable name VirtV2vInspectorExtraArgs and assert that the function that
populates r.VirtV2vInspectorExtraArgs returns the JSON "[]" default when the env
var is unset, returns a JSON array encoding of strings (use strings.Fields
semantics) when the env var contains space-separated args, and treats an
explicitly empty env var value as the default/empty-array case; locate the code
paths by referencing the symbol VirtV2vInspectorExtraArgs and the assignment to
r.VirtV2vInspectorExtraArgs to ensure tests exercise the json.Marshal branch and
the default branch.
In `@pkg/virt-v2v/conversion/conversion.go`:
- Around line 94-106: Add unit tests for Conversion.addConversionExtraArgs,
Conversion.addInspectorExtraArgs and addVirtV2vVsphereArgsForInspection: create
a test table with cases where the respective
ExtraArgs/InspectorExtraArgs/VSphere args are nil (assert AddExtraArgs is not
called) and non-nil (assert AddExtraArgs is called with the exact slice), and
add a case to verify conversion vs inspector separation (e.g., set both
ExtraArgs and InspectorExtraArgs and assert only the target helper calls
AddExtraArgs). Use a small fake/mocking implementation of utils.CommandBuilder
that records AddExtraArgs invocations and compare recorded args to expected
values to validate behavior. Ensure tests live alongside conversion.go tests and
cover both nil and non-nil paths.
- Around line 222-247: The two functions addVirtV2vVsphereArgsForInspection and
addVirtV2vVsphereArgs share the same libvirt/common/VDDK argument assembly;
extract that shared logic into a helper (e.g., buildVsphereArgsCommon or
addVsphereBaseArgs) that accepts flags to control the two differences (whether
to call addConversionExtraArgs and whether VDDK config respects extra-args
override), then have addVirtV2vVsphereArgsForInspection call that helper with
parameters to skip addConversionExtraArgs and to always apply VddkConfFile,
while addVirtV2vVsphereArgs calls it with the original behavior; reference
existing symbols addCommonArgs, addConversionExtraArgs, VddkLibDir,
VddkConfFile, Fingerprint, LibvirtUrl, SecretKey, HostName, and VmName when
moving logic so behavior remains identical.
Comment on lines
+222
to
+247
| // addVirtV2vVsphereArgsForInspection adds vSphere-specific args WITHOUT conversion extra args | ||
| // This is used for remote inspection where we want inspector-specific args instead | ||
| func (c *Conversion) addVirtV2vVsphereArgsForInspection(cmd utils.CommandBuilder) (err error) { | ||
| cmd.AddArg("-i", "libvirt"). | ||
| AddArg("-ic", c.LibvirtUrl). | ||
| AddArg("-ip", c.SecretKey). | ||
| AddArg("--hostname", c.HostName) | ||
|
|
||
| err = c.addCommonArgs(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Note: NO addConversionExtraArgs here - this is for inspection | ||
| if info, err := os.Stat(c.VddkLibDir); err == nil && info.IsDir() { | ||
| cmd.AddArg("-it", "vddk") | ||
| cmd.AddArg("-io", fmt.Sprintf("vddk-libdir=%s", c.VddkLibDir)) | ||
| cmd.AddArg("-io", fmt.Sprintf("vddk-thumbprint=%s", c.Fingerprint)) | ||
| // Always use vddk-config for inspection if it exists (no extra args override) | ||
| if _, err := os.Stat(c.VddkConfFile); !errors.Is(err, os.ErrNotExist) { | ||
| cmd.AddArg("-io", fmt.Sprintf("vddk-config=%s", c.VddkConfFile)) | ||
| } | ||
| } | ||
| cmd.AddPositional("--") | ||
| cmd.AddPositional(c.VmName) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared vSphere argument building logic.
addVirtV2vVsphereArgsForInspection duplicates most of addVirtV2vVsphereArgs with intentional differences in:
- No
addConversionExtraArgscall - VDDK config always applied without extra args override check
While the separation is correct for the PR's objectives, the shared logic (libvirt args, common args, VDDK setup) could be extracted to a helper to reduce duplication. This can be addressed in a follow-up if desired.
🤖 Prompt for AI Agents
In `@pkg/virt-v2v/conversion/conversion.go` around lines 222 - 247, The two
functions addVirtV2vVsphereArgsForInspection and addVirtV2vVsphereArgs share the
same libvirt/common/VDDK argument assembly; extract that shared logic into a
helper (e.g., buildVsphereArgsCommon or addVsphereBaseArgs) that accepts flags
to control the two differences (whether to call addConversionExtraArgs and
whether VDDK config respects extra-args override), then have
addVirtV2vVsphereArgsForInspection call that helper with parameters to skip
addConversionExtraArgs and to always apply VddkConfFile, while
addVirtV2vVsphereArgs calls it with the original behavior; reference existing
symbols addCommonArgs, addConversionExtraArgs, VddkLibDir, VddkConfFile,
Fingerprint, LibvirtUrl, SecretKey, HostName, and VmName when moving logic so
behavior remains identical.
This was referenced Feb 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, VIRT_V2V_EXTRA_ARGS was applied to all virt-v2v commands including virt-v2v-inspector. This caused migrations to fail when using conversion-specific args like --parallel that are not recognized by virt-v2v-inspector.
This change introduces a new VIRT_V2V_INSPECTOR_EXTRA_ARGS setting and separates the argument handling:
Breaking change: Users who relied on extra args being passed to both conversion and inspection will need to set both env vars.
How to test:
a. set updated controller and virt-v2v images
b. update plan CRD
c. set virt_v2v_extra_args that only works with virt-v2v for example --parallel
the migration should pass
Ref: https://issues.redhat.com/browse/MTV-4454
Resolves: MTV-4454