Auto five v2v cases #6553
Auto five v2v cases #6553
Conversation
WalkthroughRemoved VM-name validation and adjusted disk-count path in kubevirt VM checks; added define_disk_path workflow to create/validate disk images during ESX v2v conversions; introduced an in_place conversion mode and routing to virt-v2v-in-place; various config changes to test variants and password/URI handling for ESX sources. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TestRunner
participant Options
participant VirtV2V
participant TempDir
participant VMCheck
Note over TestRunner,Options: Start v2v conversion (checkpoint may include define_disk_path or in_place)
TestRunner->>Options: Build v2v command
alt checkpoint == define_disk_path
Options->>TempDir: get_tmp_dir() -> dir
Options->>VirtV2V: add "-oo create=true" and "-oo disk=<dir>/disk1.img ... diskN.img"
else checkpoint == in_place
Options->>VirtV2V: substitute binary -> /usr/libexec/virt-v2v-in-place
Options->>VirtV2V: add "-i disk <disk_path> -O <xml_path>"
else
Options->>VirtV2V: standard virt-v2v args
end
VirtV2V->>TestRunner: run conversion (uses consolidated source_pwd)
TestRunner->>VMCheck: perform post-checks
alt define_disk_path
VMCheck->>TempDir: list disk*.img
VMCheck-->>TestRunner: validate count == vm_disk_count
else kubevirt
VMCheck-->>TestRunner: validate disks via `spec.template.spec.domain.devices.disks`
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
@xiaodwan please review and merge, thanks! (1/1) type_specific.io-github-autotest-libvirt.function_test_esx.positive_test.usr_partition.esx_70.libvirt.it_vddk.vpx_uri: PASS (235.46 s) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
provider/v2v_vmcheck_helper.py (1)
1123-1127: Harden KubeVirt disks path lookup and improve diagnosticsDirect indexing can KeyError on schema drift. Add safe access, fallback to legacy location, and log actual vs expected.
Apply:
- if disk_count != len(data['spec']['template']['spec']['domain']['devices']['disks']): - LOG.debug('expect disk count: %s' % disk_count) - result = False + # Prefer template spec, fall back to legacy location if needed + devices = (data.get('spec', {}) + .get('template', {}) + .get('spec', {}) + .get('domain', {}) + .get('devices', {})) or {} + disks = devices.get('disks') + if disks is None: + disks = (data.get('spec', {}) + .get('domain', {}) + .get('devices', {}) + .get('disks', [])) + actual = len(disks or []) + if disk_count != actual: + LOG.debug('expect disk count: %s, actual: %s', disk_count, actual) + result = False
🧹 Nitpick comments (6)
v2v/tests/cfg/v2v_options.cfg (1)
356-362: In-place variant: add preconditions and parameter sanity checks
- Ensure DEFAULT_TEST_IMAGE is defined and exists to avoid false failures.
- Gate on virt-v2v-in-place availability (path/version) to keep the suite portable; otherwise skip/cancel.
Can you confirm DEFAULT_TEST_IMAGE is always set in your CI inventory and that /usr/libexec/virt-v2v-in-place (or equivalent) is present on all runners?
provider/v2v_vmcheck_helper.py (1)
1118-1122: Guard against missing YAML outputglob()[0] will raise if no files exist. Add a short-circuit.
Use:
- with open(glob.glob("%s/*.yaml" % os_directory)[0])as fd: + yamls = glob.glob("%s/*.yaml" % os_directory) + if not yamls: + LOG.error('No KubeVirt YAML found in %s', os_directory) + return False + with open(yamls[0]) as fd:v2v/tests/src/v2v_options.py (2)
731-732: Verbosity control for in-place: consider a toggleExcluding “-v -x” can hide useful diagnostics. Consider a param (e.g., in_place_verbose=yes) to enable verbose when needed.
596-599: Tighten XML existence check and cleanupUse isfile for accuracy and remove the temp XML in finally to avoid leaks.
Apply:
- if checkpoint == 'in_place': - if not os.path.exists(xml_path): - test.fail('Not found guest xml file') + if checkpoint == 'in_place': + if not os.path.isfile(xml_path): + test.fail('Guest XML not created: %s' % xml_path)Outside this hunk, in the finally block, remove the temp file:
# after other cleanups if checkpoint == 'in_place' and 'xml_path' in locals() and os.path.isfile(xml_path): os.remove(xml_path)v2v/tests/src/function_test_esx.py (1)
744-750: Keep vpx_dc consistent between remote_uri and v2v_paramsYou nullify vpx_dc for remote_uri but v2v_params still holds the old value; this can lead to inconsistent behavior downstream.
if src_uri_type == 'esx': vpx_dc = None + v2v_params['vpx_dc'] = vpx_dc remote_uri = v2v_uri.get_uri(remote_host, vpx_dc, esx_ip)Would you confirm whether v2v_options uses v2v_params['vpx_dc'] later (and thus needs this alignment)?
v2v/tests/cfg/function_test_esx.cfg (1)
969-983: Typos in skip_reason (cosmetic, but easy win)“encryped” → “encrypted”; “boots up” → “boot”. Improves clarity.
- skip_reason = 'bug RHEL-93583 is not fixed and cannot boot into OS without password automatically' + skip_reason = 'bug RHEL-93583 is not fixed and cannot boot into OS without a password automatically' ... - skip_reason = 'luks encryped, cannot boots up without password' + skip_reason = 'LUKS-encrypted, cannot boot without a password'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
provider/v2v_vmcheck_helper.py(1 hunks)v2v/tests/cfg/function_test_esx.cfg(4 hunks)v2v/tests/cfg/v2v_options.cfg(1 hunks)v2v/tests/src/function_test_esx.py(3 hunks)v2v/tests/src/v2v_options.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
v2v/tests/src/function_test_esx.py
584-584: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
🔇 Additional comments (6)
provider/v2v_vmcheck_helper.py (1)
1113-1116: Confirm removal of name check is intentionalDropping metadata.name verification changes coverage. If KubeVirt now uses generateName or templated names, consider optionally validating when name exists.
v2v/tests/src/function_test_esx.py (1)
937-940: LGTM: unified ESX credentials flow into virsh_dargsUsing source_pwd for remote_pwd consolidates ESX/vCenter cases cleanly.
v2v/tests/cfg/function_test_esx.cfg (4)
455-458: LGTM: gating usr_partition with version and OGAC checkpointThe added version_required and checkpoint look correct and align with the test intent.
984-990: LGTM: print_blkhash gateVersion floor and expected message pattern look consistent with the feature.
765-777: Win BitLocker variants: looks good; confirm version gate for vTPM pathThe split into with/without TPM is clear. If virt-v2v/vDDK behavior differs for Win11+vTPM/BitLocker on certain streams, consider adding an explicit version_required bound here too.
539-545: Verified define_disk_path usage
Thedefine_disk_pathblock appears solely underesx_80anddest_kubevirtwithvm_disk_count = 4as intended.
mxie91
force-pushed
the
auto-v2v-cases-about-kubevirt-in-place-etc
branch
from
ddcff33 to
1f91337
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
v2v/tests/src/v2v_options.py (2)
851-854: Validate/quote disk_path and prepend a space when appending optionsWithout a leading space, flags can concatenate; paths aren’t quoted; disk_path isn’t validated. Quote both paths and fail fast on bad input.
Apply:
- if checkpoint == 'in_place': - disk_path = params.get('disk_path') - xml_path = os.path.join(data_dir.get_tmp_dir(), 'output.xml') - v2v_options += '-i disk %s -O %s' % (disk_path, xml_path) + if checkpoint == 'in_place': + disk_path = params.get('disk_path') + if not disk_path or not os.path.exists(disk_path): + test.cancel('Invalid disk_path: %s' % disk_path) + xml_path = os.path.join(data_dir.get_tmp_dir(), 'output.xml') + v2v_options += ' -i disk %s -O %s' % (shlex.quote(disk_path), shlex.quote(xml_path))Also add:
# at file top import shlex
883-884: Make virt-v2v → virt-v2v-in-place replacement robust across distrosCurrent regex misses bare “virt-v2v” and doesn’t verify in-place binary. Resolve path and replace token-safely.
Apply:
- if checkpoint == 'in_place': - cmd = re.sub(r".*/bin/virt-v2v", '/usr/libexec/virt-v2v-in-place', cmd) + if checkpoint == 'in_place': + v2v_in_place = '/usr/libexec/virt-v2v-in-place' + if not os.path.exists(v2v_in_place): + alt = shutil.which('virt-v2v-in-place') + if alt: + v2v_in_place = alt + # Replace '/usr/bin/virt-v2v', '/bin/virt-v2v', or bare 'virt-v2v' (not '-in-place') + cmd = re.sub(r'(?<=^|[;\s])(?:/usr/bin/|/bin/)?virt-v2v(?=\s)', v2v_in_place, cmd)v2v/tests/src/function_test_esx.py (3)
583-586: Avoid shell+regex ls; use glob and compare intsCurrent code is unsafe (shell=True) and can miscount; vm_disk_count type mismatch.
Apply:
- if 'define_disk_path' in checkpoint: - disk_num = len(re.findall(r'disk*.img', process.run('ls %s/disk*.img' % os_directory, shell=True).stdout_text)) - if disk_num != vm_disk_count: - test.fail('disk num is incorrect') + if 'define_disk_path' in checkpoint: + from glob import glob + expected = int(params_get(params, 'vm_disk_count')) + disk_num = len(glob(os.path.join(os_directory, 'disk*.img'))) + if disk_num != expected: + test.fail(f'disk num is incorrect: expected {expected}, got {disk_num}')
753-756: Secure password-file permissions and prevent duplicate -ipSet 0600 on the password file and avoid injecting -ip twice.
Apply:
source_pwd = vpx_passwd if src_uri_type != 'esx' else esxi_password with open(vpx_passwd_file, 'w') as f: f.write(source_pwd) - v2v_params['v2v_opts'] += " -ip %s" % vpx_passwd_file + os.chmod(vpx_passwd_file, 0o600) + # Guard to avoid duplicate auth flags + if '-ip ' not in v2v_params['v2v_opts'] and '--password-file' not in v2v_params['v2v_opts']: + v2v_params['v2v_opts'] += " -ip %s" % vpx_passwd_fileRun to confirm other places already add -ip/--password-file:
#!/bin/bash rg -nC2 -e "output_option\s*\+=\s*['\"]\s*-ip\b|--password-file" v2v/tests/src/v2v_options.py rg -nC2 -e "\-ip\s+%s" v2v/tests/src/function_test_esx.py
930-936: Honor vm_disk_count and isolate files in a unique temp dirHardcoding 4 and using a shared tmp dir risks collisions and false checks; also ensure cleanup via params.
Apply:
- if 'define_disk_path' in checkpoint: - os_directory = data_dir.get_tmp_dir() - vm_disk_count = params.get('vm_disk_count') - v2v_params['v2v_opts'] += ' -oo create=true' - for i in range(1, 5): - v2v_params['v2v_opts'] += ' -oo disk=%s/disk%s.img' % (os_directory, i) + if 'define_disk_path' in checkpoint: + os_directory = tempfile.mkdtemp(prefix='v2v_disk_', dir=data_dir.get_tmp_dir()) + params['os_directory'] = os_directory + vm_disk_count = int(params_get(params, 'vm_disk_count')) + v2v_params['v2v_opts'] += ' -oo create=true' + for i in range(1, vm_disk_count + 1): + v2v_params['v2v_opts'] += ' -oo disk=%s/disk%s.img' % (os_directory, i)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
provider/v2v_vmcheck_helper.py(1 hunks)v2v/tests/cfg/function_test_esx.cfg(4 hunks)v2v/tests/cfg/v2v_options.cfg(1 hunks)v2v/tests/src/function_test_esx.py(3 hunks)v2v/tests/src/v2v_options.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- provider/v2v_vmcheck_helper.py
- v2v/tests/cfg/v2v_options.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
v2v/tests/src/function_test_esx.py (3)
provider/v2v_vmcheck_helper.py (1)
run(211-227)v2v/tests/src/v2v_options.py (1)
run(32-958)v2v/tests/src/specific_kvm.py (1)
run(35-928)
🪛 Ruff (0.12.2)
v2v/tests/src/function_test_esx.py
584-584: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
🔇 Additional comments (8)
v2v/tests/src/v2v_options.py (2)
596-598: LGTM: post-run XML existence checkThe check is scoped under the same checkpoint and uses late-binding closure; safe as written.
731-731: LGTM: suppress verbose output for in_placeExcluding ‘in_place’ from “-v -x” aligns with its distinct output expectations.
v2v/tests/cfg/function_test_esx.cfg (4)
455-458: LGTM: usr_partition gating tightenedVersion gate and checkpoint update look correct.
763-777: LGTM: Win11 BitLocker vTPM split into variantsClear separation with appropriate gating and keys.
968-983: LGTM: SLES encrypted btrfs variantsConstraints and skip rationale look consistent.
984-990: LGTM: print_blkhash variant addedVersion gating and checkpoint name align with expectations.
v2v/tests/src/function_test_esx.py (2)
746-748: LGTM: esx src_uri_type clears vpx_dcSetting vpx_dc=None for direct ESXi makes URI construction correct.
937-939: LGTM: remote virsh uses resolved ESX passwordremote_pwd=source_pwd matches the unified credential handling.
mxie91
force-pushed
the
auto-v2v-cases-about-kubevirt-in-place-etc
branch
from
1f91337 to
6968790
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
v2v/tests/src/v2v_options.py (2)
852-855: Quote paths, validate disk_path, and prepend a space before optionsCurrent string may concatenate flags, and unquoted paths can break with spaces.
- if checkpoint == 'in_place': - disk_path = params.get('disk_path') - xml_path = os.path.join(data_dir.get_tmp_dir(), 'output.xml') - v2v_options += '-i disk %s -O %s' % (disk_path, xml_path) + if checkpoint == 'in_place': + import shlex + disk_path = params.get('disk_path') + if not disk_path or not os.path.exists(disk_path): + test.cancel('Invalid disk_path: %s' % disk_path) + xml_path = os.path.join(data_dir.get_tmp_dir(), 'output.xml') + v2v_options += ' -i disk %s -O %s' % (shlex.quote(disk_path), shlex.quote(xml_path))Add at top if missing:
import shlex
884-885: Make virt-v2v-in-place substitution robust (handles bare and alt paths, with fallback)Covers “virt-v2v” with/without absolute path and finds in-place binary via which.
- if checkpoint == 'in_place': - cmd = re.sub(r".*/bin/virt-v2v", '/usr/libexec/virt-v2v-in-place', cmd) + if checkpoint == 'in_place': + v2v_in_place = '/usr/libexec/virt-v2v-in-place' + if not os.path.exists(v2v_in_place): + alt = shutil.which('virt-v2v-in-place') + if alt: + v2v_in_place = alt + # replace standalone 'virt-v2v' (optionally prefixed with /usr/bin/ or /bin/) but not 'virt-v2v-in-place' + cmd = re.sub(r'(?<=\s|;)(?:/usr/bin/|/bin/)?virt-v2v(?=\s)', v2v_in_place, cmd)v2v/tests/src/function_test_esx.py (2)
752-757: Restrict password-file permissionsSet 0600 on the credential file.
vpx_passwd_file = params.get("vpx_passwd_file") source_pwd = vpx_passwd if src_uri_type != 'esx' else esxi_password with open(vpx_passwd_file, 'w') as f: f.write(source_pwd) + os.chmod(vpx_passwd_file, 0o600)
931-937: Type bug and resource hygiene: cast vm_disk_count, isolate files, wire cleanup
- range(1, vm_disk_count + 1) will raise if vm_disk_count is str.
- Use a dedicated temp dir and store it for cleanup.
- if 'define_disk_path' in checkpoint: - os_directory = data_dir.get_tmp_dir() - vm_disk_count = params.get('vm_disk_count') - v2v_params['v2v_opts'] += ' -oo create=true' - for i in range(1, vm_disk_count + 1): - v2v_params['v2v_opts'] += ' -oo disk=%s/disk%s.img' % (os_directory, i) + if 'define_disk_path' in checkpoint: + os_directory = tempfile.mkdtemp(prefix='v2v_disk_', dir=data_dir.get_tmp_dir()) + params['os_directory'] = os_directory # enable existing cleanup path + vm_disk_count = int(params_get(params, 'vm_disk_count')) + v2v_params['v2v_opts'] += ' -oo create=true' + for i in range(1, vm_disk_count + 1): + v2v_params['v2v_opts'] += ' -oo disk=%s/disk%s.img' % (os_directory, i)
🧹 Nitpick comments (3)
provider/v2v_vmcheck_helper.py (1)
1123-1127: Avoid KeyError on nested YAML and log actual vs expected disk countDeep indexing can raise KeyError when fields are missing. Compute safely and include actual value in logs.
- if disk_count != len(data['spec']['template']['spec']['domain']['devices']['disks']): - LOG.debug('expect disk count: %s' % disk_count) - result = False + try: + actual = len( + data.get('spec', {}) + .get('template', {}) + .get('spec', {}) + .get('domain', {}) + .get('devices', {}) + .get('disks', []) or [] + ) + except Exception as e: + LOG.error('Failed to parse disks from kubevirt YAML: %s', e) + return False + if disk_count != actual: + LOG.debug('Disk count mismatch: expected=%s, actual=%s', disk_count, actual) + result = Falsev2v/tests/src/v2v_options.py (1)
596-599: Clean up generated XML after assertionRemove the generated XML to avoid tmp accumulation.
- if checkpoint == 'in_place': - if not os.path.exists(xml_path): - test.fail('Not found guest xml file') + if checkpoint == 'in_place': + if not os.path.exists(xml_path): + test.fail('Not found guest xml file') + else: + try: + os.remove(xml_path) + except OSError: + LOG.debug('Failed to remove %s', xml_path)v2v/tests/src/function_test_esx.py (1)
583-587: Compare int-to-int and show mismatchAvoid implicit str/int comparison; report expected vs actual.
- if 'define_disk_path' in checkpoint: - from glob import glob - disk_num = len(glob(os.path.join(os_directory, 'disk*.img'))) - if disk_num != vm_disk_count: - test.fail('disk num is incorrect') + if 'define_disk_path' in checkpoint: + from glob import glob + expected = int(params_get(params, 'vm_disk_count')) + disk_num = len(glob(os.path.join(os_directory, 'disk*.img'))) + if disk_num != expected: + test.fail(f'disk num is incorrect: expected {expected}, got {disk_num}')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
provider/v2v_vmcheck_helper.py(1 hunks)v2v/tests/cfg/function_test_esx.cfg(4 hunks)v2v/tests/cfg/v2v_options.cfg(1 hunks)v2v/tests/src/function_test_esx.py(3 hunks)v2v/tests/src/v2v_options.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2v/tests/cfg/v2v_options.cfg
🔇 Additional comments (7)
v2v/tests/src/v2v_options.py (1)
731-731: Skip verbose flags for in_place: OKExcluding “-v -x” for in-place looks right to keep logs concise.
v2v/tests/src/function_test_esx.py (2)
745-751: Set vpx_dc=None for ESX URIs: verify URI builder handles NoneLooks sensible for direct ESXi, but please confirm utils_v2v.Uri('esx').get_uri(...) treats dc=None correctly across versions.
938-940: Unified ESX credential usage: OKUsing source_pwd consistently in virsh_dargs is clearer.
v2v/tests/cfg/function_test_esx.cfg (4)
455-458: usr_partition gating and checkpoint: OKVersion gate and OGAC checkpoint align with the new RHEL8 qemu-ga test.
539-545: define_disk_path variant: OK; ensure code casts vm_disk_countConfig looks good. Coupled code should cast vm_disk_count to int (see proposed fix in function_test_esx.py).
968-983: SLES encrypted btrfs variants: OKAppropriate gating and skips for password-required boots.
984-990: print_blkhash gate: OKVersion requirement added; message pattern matches expected nbdcopy flag emission.
v2v/tests/src/function_test_esx.py
Outdated
|
|
||
| if 'define_disk_path' in checkpoint: | ||
| os_directory = data_dir.get_tmp_dir() | ||
| vm_disk_count = params.get('vm_disk_count') |
There was a problem hiding this comment.
should be:
vm_disk_count` = int(params_get(params, 'vm_disk_count'))
There was a problem hiding this comment.
updated, please review, thanks!
mxie91
force-pushed
the
auto-v2v-cases-about-kubevirt-in-place-etc
branch
from
6968790 to
6cade94
Compare
|
@mxie91 please resolve the conflict issue. |
1. Check qemu-ga for rhel8 guest with usr partition 2. Convert sles15 guest with encrypted btrfs fs 3. Check output for virt-v2v-in-place 4. Convert win11 guest with vtpm and bitlocker 5. Check disks in specific path for kubevirt output Signed-off-by: Ming Xie <[email protected]>
mxie91
force-pushed
the
auto-v2v-cases-about-kubevirt-in-place-etc
branch
from
6cade94 to
a8d4a62
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Tests