Auto v2v cases from customer bugs #6519
Conversation
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
from
bdfcef4 to
eb94a52
Compare
|
@xiaodwan please review, thanks! avocado run --vt-type v2v function_test_esx.positive_test.verify_custom_path_cert.esx_80.libvirt.it_vddk.vpx_uriJOB ID : 2b7d54049b75a96b00f30ad412c6691480f606b8 avocado run --vt-type v2v specific_kvm..ubuntu_usr_partition.esx.esx_80.output_mode.libvirt.it_vddkJOB ID : 5847566bba9be3ed23849c9b531f9818423ba83d avocado run --vt-type v2v specific_kvm.positive_test.linux.check_pnp_service.esx.esx_80.output_mode.libvirt.it_vddkJOB ID : e2a63e850ee5a613a3eb6c741977054af518db73 |
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
from
eb94a52 to
65c2711
Compare
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
from
65c2711 to
1e3d20e
Compare
|
@xiaodwan updated, pls review again, thanks! avocado run --vt-type v2v specific_kvm.positive_test..check_pnp_service.esx.esx_80.output_mode.libvirt.it_vddkJOB ID : b64841dca063e0824d5ebd3beab902a1b28b9fbc |
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
from
1e3d20e to
ffd54dc
Compare
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
2 times, most recently
from
042091a to
5bcb2e2
Compare
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
2 times, most recently
from
dfe63a5 to
729ce04
Compare
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
from
729ce04 to
fb66d66
Compare
WalkthroughAdds ESX and KVM V2V test variants and checkpoints: a new ESX certificate-check path that builds a custom cacert URL and rewrites the virt-v2v command, plus a KVM Windows PnP log-based verification flow. Changes are limited to tests/configs; no public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TR as TestRunner
participant ESX as function_test_esx.py
participant FS as HostTmp
participant V2V as virt-v2v
participant VC as vCenter (FQDN)
TR->>ESX: run checkpoint=verify_custom_path_cert
ESX->>FS: create tmp cert dir (…/vmwarecert)
ESX->>ESX: verify_certificate(certs_src_dir -> dest_tmp)
note right of ESX: on success, rebuild virt-v2v command\nadd /?cacert=<dest>/faea32cd.0 and target=VC FQDN
ESX->>V2V: execute rebuilt command
V2V-->>ESX: result
ESX-->>TR: report result
sequenceDiagram
autonumber
participant TR as TestRunner
participant KVM as specific_kvm.py
participant HV as Hypervisor
participant G as Guest (Windows)
participant FS as HostTmp
TR->>KVM: check_result(checkpoint=check_pnp_service)
KVM->>HV: start VM
KVM-->>TR: perform standard output-mode checks
rect rgba(230,245,255,0.4)
note right of KVM: Retry loop (up to ~5 min active retries, includes 25min reboot wait)
KVM->>G: wait/reboot
KVM->>HV: copy "/Program Files/Guestfs/Firstboot" -> HostTmp
KVM->>FS: read Firstboot/log.txt
alt uninstall message found
KVM-->>KVM: stop retries
else
KVM->>HV: destroy/start VM and retry
end
end
alt no log produced
KVM-->>TR: record failure
else
KVM-->>TR: fail if "Plug and Play service is not available" >= 2; else pass
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2v/tests/src/function_test_esx.py (1)
523-533: Critical: verify_certificate overwrites /etc/hosts and misuses pipe; make idempotent and safe.
- open('/etc/hosts', "w") clobbers the entire file; use append-if-missing.
- 'yum install ... | update-ca-trust' uses a pipe instead of sequencing; use '&&'.
- Prefer Python copy over scp for local paths; ensure dest exists.
- def verify_certificate(certs_src_dir, certs_dest_dir, vcenter_fdqn, vcenter_ip): - process.run('yum install ca-certificates -y | update-ca-trust', shell=True) - mount_cert_dir = utils_v2v.v2v_mount(certs_src_dir, 'certs_src_dir') - if not os.path.exists(certs_dest_dir): - os.makedirs(certs_dest_dir) - process.run('scp -r %s/* %s' % (mount_cert_dir, certs_dest_dir), shell=True) - process.run('umount %s' % mount_cert_dir, shell=True) - process.run('update-ca-trust extract', shell=True) - with open('/etc/hosts', "w") as f: - f.write('%s %s' % (vcenter_ip, vcenter_fdqn)) + def verify_certificate(certs_src_dir, certs_dest_dir, vcenter_fdqn, vcenter_ip): + process.run('yum -y install ca-certificates && update-ca-trust', shell=True) + mount_cert_dir = utils_v2v.v2v_mount(certs_src_dir, 'certs_src_dir') + os.makedirs(certs_dest_dir, exist_ok=True) + try: + # local copy is enough; avoid shell quoting pitfalls with scp + shutil.copytree(mount_cert_dir, certs_dest_dir, dirs_exist_ok=True) + finally: + process.run('umount %s' % mount_cert_dir, shell=True) + process.run('update-ca-trust extract', shell=True) + # append mapping only if absent; do not clobber /etc/hosts + try: + with open('/etc/hosts', 'r+') as f: + content = f.read() + entry = f'{vcenter_ip} {vcenter_fdqn}' + if entry not in content: + if not content.endswith('\n'): + f.write('\n') + f.write(entry + '\n') + except Exception as e: + LOG.warning('Failed to update /etc/hosts safely: %s', e)
🧹 Nitpick comments (3)
v2v/tests/src/specific_kvm.py (1)
589-614: Prefer virsh APIs and avoid shell=True where possible.S604 warnings are legitimate here. Use virsh.start/destroy (you already import virsh) and Python I/O instead of invoking cat/virsh via shell. If shell is unavoidable for virt-copy-out, keep strict quoting and pass ignore_status=True until the log appears.
v2v/tests/cfg/specific_kvm.cfg (2)
318-320: Ubuntu /usr-partition case added without a checkpoint.If you intend extra validation on /usr, consider adding a dedicated checkpoint hook; otherwise this remains a plain conversion run.
312-314: Polish skip_reason text.Minor wording for clarity.
Apply this diff:
- skip_reason = "it's a customer guest and we don't know login info" + skip_reason = "Customer-provided guest; login credentials are unknown"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2v/tests/cfg/function_test_esx.cfg(1 hunks)v2v/tests/cfg/specific_kvm.cfg(2 hunks)v2v/tests/src/function_test_esx.py(2 hunks)v2v/tests/src/specific_kvm.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2v/tests/src/specific_kvm.py (1)
v2v/tests/src/function_test_esx.py (1)
log_fail(108-113)
🪛 Ruff (0.12.2)
v2v/tests/src/specific_kvm.py
591-591: Function call with shell=True parameter identified, security issue
(S604)
600-600: Function call with shell=True parameter identified, security issue
(S604)
602-602: Function call with shell=True parameter identified, security issue
(S604)
603-603: Function call with shell=True parameter identified, security issue
(S604)
605-605: 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.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
🔇 Additional comments (6)
v2v/tests/cfg/function_test_esx.cfg (2)
906-913: New verify_custom_path_cert variant looks good.The parameters align with the new custom cert-path flow (no dest path in cfg; code creates a temp dest). Version gate also scopes the change appropriately.
906-913: Confirm naming and version gate.
- The key vcenter_fdqn is consistently used in this file, but the common acronym is FQDN. Verify the code reads vcenter_fdqn (not vcenter_fqdn) to avoid a silent mismatch.
- Double-check the minimum libvirt "[10.10.0-7.6,)" gate matches the first build that supports the cacert URL param you rely on.
v2v/tests/cfg/specific_kvm.cfg (2)
309-317: check_pnp_service variant wiring looks correct.Gated to esx_80 with a sensible version floor and vm-check disabled due to unknown creds. Good.
333-333: Validate the narrowed positive_test matrix.
- linux: excluding check_pnp_service is fine.
- windows: default now runs only check_pnp_service. Confirm this intentional reduction in coverage; if not, move check_pnp_service to its own windows sub-variant without using only.
Also applies to: 354-356
v2v/tests/src/function_test_esx.py (2)
945-952: LGTM: checkpoint wiring includes verify_custom_path_cert where cmd_only/auto_clean apply.
1003-1012: LGTM: ensure new_cmd flows execute for verify_custom_path_cert.
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
from
fb66d66 to
979ac31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2v/tests/src/specific_kvm.py (1)
589-616: Harden the PnP log loop: ignore copy-out failures until file exists, avoid shell where possible, and gate to libvirt output.Without
ignore_status=True,virt-copy-outcan fail before the log exists and break the loop;catruns before the existence check; repeatedvirshvia shell is unnecessary. Also, run this branch only foroutput_mode == 'libvirt'.Apply:
- if checkpoint == 'check_pnp_service': - log_dir = data_dir.get_tmp_dir() - dest_log = os.path.join(log_dir, 'Firstboot', 'log.txt') - process.run("virsh start %s" % vm_name, shell=True, ignore_status=True) - LOG.info("Waiting 25 minutes to wait for guest finishing reboot") + if checkpoint == 'check_pnp_service' and output_mode == 'libvirt': + log_dir = data_dir.get_tmp_dir() + dest_log = os.path.join(log_dir, 'Firstboot', 'log.txt') + virsh.start(vm_name, debug=True, ignore_status=True) + LOG.info("Waiting 25 minutes for the guest to finish reboot cycles") time.sleep(25 * 60) # Now check for the log file in the next 5 minutes - LOG.info("Checking for log file in 5 minutes") + LOG.info("Checking for Firstboot/log.txt for 5 minutes") end_time = time.time() + 5 * 60 - log_file = '' + log_content = '' while time.time() < end_time: - process.run("virsh start %s" % vm_name, shell=True, ignore_status=True) + virsh.start(vm_name, debug=True, ignore_status=True) time.sleep(60) # check every 1 minute - process.run("virsh destroy %s" % vm_name, shell=True, ignore_status=True) + virsh.destroy(vm_name, debug=True, ignore_status=True) process.run("virt-copy-out -d %s '/Program Files/Guestfs/Firstboot' %s" % (vm_name, log_dir), - shell=True) - log_file = process.run('cat %s' % dest_log, shell=True, ignore_status=True).stdout_text + shell=True, ignore_status=True) if not os.path.exists(dest_log): continue + with open(dest_log, 'r', encoding='utf-8', errors='ignore') as f: + log_content = f.read() # guest will finish all reboots if info uninstalling firstboot service shows in the log - if re.search(r'uninstalling firstboot service.*', log_file): + if re.search(r'uninstalling firstboot service', log_content, re.IGNORECASE): break if not os.path.exists(dest_log): log_fail("firstboot log is not generated") - if len(re.findall(r'The Plug and Play service is not available.*', log_file)) >= 2: + if len(re.findall(r'The Plug and Play service is not available.*', log_content)) >= 2: log_fail('Found drivers are not installed due to pnp services are not available')
🧹 Nitpick comments (3)
v2v/tests/src/specific_kvm.py (1)
589-616: Optional: double-check stability after the “uninstalling firstboot service” marker.To avoid edge cases where the log appends shortly after the break, consider a final short re-copy/read (e.g., +30s) before counting PnP errors.
v2v/tests/cfg/specific_kvm.cfg (2)
309-317: Constrain the new case to libvirt output to match the code path.The PnP check runs only in the libvirt flow; running it with other outputs is wasteful or may no-op. Add:
- check_pnp_service: only esx_80 + only output_mode.libvirt version_required = "[virt-v2v-2.7.1-10,)" skip_vm_check = yes skip_reason = "it's a customer guest and we don't know login info" boottype = 2 main_vm = VM_NAME_WIN_GUEST_WITH_GPO_SPECIFIC_SERVICES_V2V_EXAMPLE checkpoint = check_pnp_service
354-356: Also pin Windows default to libvirt output to avoid scheduling no-op combos.This keeps the matrix tight and consistent with the implementation.
- default: - only check_pnp_service + only check_pnp_service + only output_mode.libvirt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2v/tests/cfg/function_test_esx.cfg(1 hunks)v2v/tests/cfg/specific_kvm.cfg(2 hunks)v2v/tests/src/function_test_esx.py(2 hunks)v2v/tests/src/specific_kvm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2v/tests/cfg/function_test_esx.cfg
- v2v/tests/src/function_test_esx.py
🧰 Additional context used
🧬 Code graph analysis (1)
v2v/tests/src/specific_kvm.py (1)
v2v/tests/src/function_test_esx.py (1)
log_fail(108-113)
🪛 Ruff (0.12.2)
v2v/tests/src/specific_kvm.py
592-592: Function call with shell=True parameter identified, security issue
(S604)
601-601: Function call with shell=True parameter identified, security issue
(S604)
603-603: Function call with shell=True parameter identified, security issue
(S604)
604-604: Function call with shell=True parameter identified, security issue
(S604)
606-606: 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.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
🔇 Additional comments (1)
v2v/tests/cfg/specific_kvm.cfg (1)
333-333: LGTM: Linux path correctly excludes the Windows-only checkpoint.No action needed.
v2v/tests/src/specific_kvm.py
Outdated
| # Now check for the log file in the next 5 minutes | ||
| LOG.info("Checking for log file in 5 minutes") | ||
| end_time = time.time() + 5 * 60 | ||
| log_file = '' |
There was a problem hiding this comment.
Suggest to change the name "log_file" to "log_content" because it saves the content of the file.
There was a problem hiding this comment.
updated, please review, thanks!
1. Add case about ubuntu guest with usr partition 2. Add case about windows guest with specific services 3. Add case about connect vmware source with custom cert path Signed-off-by: Ming Xie <[email protected]>
mxie91
force-pushed
the
add-v2v-cases-for-customer-bugs
branch
from
979ac31 to
c23549c
Compare
Summary by CodeRabbit