feat: add NIC name preservation check for vSphere migrations#524
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 36 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds guest NIC name detection for source vSphere Linux guests via VMware Guest Operations, records destination interface names, and validates post-migration that guest NIC names were preserved by matching MAC addresses and comparing names. ChangesGuest NIC Name Preservation Across vSphere Migration
sequenceDiagram
participant PreparedPlan as prepared_plan
participant Detect as detect_guest_nic_names
participant GuestOps as run_command_in_vmware_guest
participant VMDetails as vm_details.network_interfaces
participant CheckVMs as check_vms
participant CheckNIC as check_nic_name_preservation
PreparedPlan->>Detect: invoke for VSPHERE non-Windows VM
Detect->>GuestOps: run "ip -j link show"
GuestOps-->>Detect: JSON interface list
Detect->>VMDetails: set guest_nic_name by matching MACs
CheckVMs->>CheckNIC: call with source_vm_data and destination VM
CheckNIC->>VMDetails: compare source.guest_nic_name to dest.guest_interface_name
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
CRITICAL: Verify guest Linux credentials and vCenter connectivity checks exist and are not bypassed. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
/verified |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conftest.py`:
- Around line 1153-1162: The try/except around detect_guest_nic_names is too
broad and hides real errors; narrow it to only catch the expected Guest Ops
runtime errors (e.g., the specific guest operations exception class your
provider raises) while allowing all other exceptions to propagate. Replace the
bare except in the block calling detect_guest_nic_names (the call using
source_provider, provider_vm_api, fixture_store["source_provider_data"],
vm_details) so it catches the provider/guest-ops specific exception type(s) and
logs via LOGGER.warning(f"Failed to detect guest NIC names for VM {vm['name']}:
{e}"), but re-raise any other Exception so regressions surface. Ensure you
import or reference the actual exception class used by the guest ops code and
only handle that here.
In `@libs/providers/openshift.py`:
- Line 254: The value assigned to "guest_interface_name" can be None when
interface["interfaceName"] exists but is null; change the expression that sets
guest_interface_name (the dict entry with key "guest_interface_name" in
libs/providers/openshift.py) to normalize falsy/None to an empty string by using
interface.get("interfaceName") or "" so downstream comparisons always see a str.
In `@utilities/post_migration.py`:
- Around line 735-746: The loop that compares source_mac to dest_nics can
silently succeed when there is no matching destination NIC; normalize MAC
formats by stripping non-hex delimiters and lowercasing both values (e.g.,
derive a normalized_source_mac from source_nic and normalize each dest_mac in
the loop) and set a found flag when a match occurs; after the loop, if not
found, raise/assert/log an error (use LOGGER.error and an AssertionError or
raise RuntimeError) indicating missing destination NIC for source_nic_name and
include normalized_source_mac so the failure is explicit; keep the existing
assertion that dest_nic_name == source_nic_name when a match is found.
- Around line 1435-1443: The NIC-name preservation check is currently nested
inside the SSH/power-state gating and thus skipped when vm_ssh_connections is
not provided; move the try/except block that calls check_nic_name_preservation
(with source_vm_data and destination_vm) out of the SSH/power-state section so
it always runs when source_vm_data is present and source_provider.type ==
Provider.ProviderType.VSPHERE, preserving the existing exception handling that
appends to res[vm_name] (using vm_name) on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a267d2c8-c6ea-44fe-b595-9c5a36a5bc36
📒 Files selected for processing (4)
conftest.pylibs/providers/openshift.pyutilities/post_migration.pyutilities/vmware_guest_operations.py
|
/verified |
MiriSafra
changed the title
Automatically verifies that guest OS NIC names (e.g., ens192, eth0) are preserved after migration by comparing source names (collected via VMware Guest Operations) against destination names (from KubeVirt guest agent).
Addresses CodeRabbit review: interface.get("interfaceName", "") can
return None when key exists with null value. Using `or ""` handles both
missing key and null value cases.
MiriSafra
force-pushed
the
feat/MTV-5007-dhcp-nic-name-preservation
branch
from
252457e to
3097320
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utilities/post_migration.py`:
- Around line 1435-1443: The NIC-name preservation check is being run even when
destination guest-agent metadata may not be available; update the logic around
check_nic_name_preservation in check_vms()/the block with source_vm_data and
destination_vm so it only runs when guest-agent data is present or forced:
either call/create destination_vm with wait_for_guest_agent=True when
source_provider.type == Provider.ProviderType.VSPHERE and source_vm_data
contains guest_interface_name, or skip the call to check_nic_name_preservation
when the destination_vm has no confirmed guest-agent metadata (e.g., empty guest
interfaces or empty guest_interface_name) or when vm["guest_agent"] is not
enabled; reference check_nic_name_preservation, destination_vm, source_vm_data
and vm["guest_agent"] to locate and implement the conditional gating or forced
wait.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e33627c-7af0-4fd7-94db-0732f16311be
📒 Files selected for processing (4)
conftest.pylibs/providers/openshift.pyutilities/post_migration.pyutilities/vmware_guest_operations.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conftest.py`:
- Around line 1153-1158: Wrap the call to detect_guest_nic_names in a try/except
block mirroring the pattern used by detect_vmware_ip_origins_via_guest_ops: call
detect_guest_nic_names(...) inside try, re-raise ValueError
(configuration/credentials errors), and catch all other exceptions to log a
warning (preserving the exception message/context) and continue so NIC detection
is non-blocking; use the same logger used elsewhere in this file and keep
behavior consistent with the IP origin detection logic.
In `@utilities/post_migration.py`:
- Around line 731-734: The check that raises ValueError when dest_nics is empty
creates an implicit dependency on guest-agent data (guest_interface_name) and
can produce false negatives in post_migration NIC-name verification; update the
logic in the function that inspects source_nics/dest_nics so it either enforces
guest-agent availability (e.g., verify any nic.get("guest_interface_name")
exists on dest_nics and raise a clear ValueError referencing
guest-agent/wait_for_guest_agent) or skip the verification gracefully when
guest-agent data is missing (log/warn and return) — locate the code around the
dest_nics/source_nics checks and the use of guest_interface_name to implement
one of these two fixes consistently.
In `@utilities/vmware_guest_operations.py`:
- Line 306: The call to json.loads(output) (assigning to interfaces) can raise
JSONDecodeError on non-JSON or corrupted guest output; either document
JSONDecodeError in the function's Raises: section if fail-fast is intended, or
wrap the json.loads(output) call in a try/except JSONDecodeError and re-raise a
clearer exception (e.g., ValueError or GuestCommandError) with context including
the original error and the raw output to aid debugging; update the function
docstring accordingly to reflect the chosen behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 340319a0-f0ee-4dfd-a0e2-f6338a3293e1
📒 Files selected for processing (3)
conftest.pyutilities/post_migration.pyutilities/vmware_guest_operations.py
- Skip NIC name preservation check when guest agent is not active to avoid false failures from empty guest_interface_name - Document json.JSONDecodeError in detect_guest_nic_names docstring
|
/verified |
|
/lgtm |
|
Test Oracle server is not responding, skipping test analysis |
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
|
Regression / flaky setup after merge — Seeing repeated setup failures on Error: Stack: What happens:
Asymmetry with existing code:
Suggested fix (any of):
Repro: |
|
@prabinovRedhat Thanks, fix in #536 , since the udev rules only run under that config |
|
Verified, #536 resolves offload test failure introduced by this PR. |
8 participants
Summary
Adds automatic NIC name preservation verification for all vSphere Linux VM migrations (MTV-5007, MTV-4897).
detect_guest_nic_names()inutilities/vmware_guest_operations.py— runsip link showinside VMware guest via Guest Operations API, stores NIC name per MAC inguest_nic_namefielddetect_guest_nic_names()inconftest.pyprepared_planfor vSphere Linux VMs (after IP origins detection)guest_interface_namefield (from KubeVirt VMIinterfaceName) to network_interfaces inlibs/providers/openshift.pycheck_nic_name_preservation()inutilities/post_migration.py— matches by MAC address, asserts source NIC name == destination NIC name; hooked intocheck_vms()for vSphere migrationsSummary by CodeRabbit