net, stability, sriov: Split setup test_connectivity_of_hot_plugged_sriov_interface#4660
Open
azhivovk wants to merge 1 commit intoRedHatQE:mainfrom
Open
net, stability, sriov: Split setup test_connectivity_of_hot_plugged_sriov_interface#4660azhivovk wants to merge 1 commit intoRedHatQE:mainfrom
azhivovk wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRemoved a shared context-manager helper that created a VM and hot-plugged an SR-IOV interface; tests were refactored to create the VM and perform SR-IOV hot-plugging via separate pytest fixtures and updated network-attachment formatting. ChangesSR-IOV hot-plug test refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
|
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. |
openshift-virtualization-qe-bot-3
requested review from
EdDev,
frenzyfriday and
servolkov
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Author
|
/verified |
Contributor
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2078 🏁 Script executed: Length of output: 1396 🏁 Script executed: Length of output: 1737 🧠 Learnings used |
yossisegev
suggested changes
Apr 29, 2026
Contributor
yossisegev
left a comment
yossisegev
left a comment
There was a problem hiding this comment.
Thank you Asia, this should significantly contribute to our stability.
|
|
||
| update_hot_plug_config_in_vm(vm=vm, interfaces=interfaces, networks=networks) | ||
|
|
||
| wait_for_vm_interfaces(vmi=vm.vmi) |
Contributor
There was a problem hiding this comment.
Even though we need to consider the time of the "shadow" migration in this wait, 12 minutes (wait_for_vm_interfaces's default timeout value) seems quite a lot; if something went wrong, we better fail earlier.
@servolkov @Anatw @EdDev WDYT? Maybe I am not being realistic here?
Contributor
Author
There was a problem hiding this comment.
We could lower the timeout - according to the logs the time difference until the interface appeared was 30-60 seconds
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
azhivovk
changed the title
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-4660 published |
Collaborator
my point here is that while we test from user perspective, we do need to keep in mind that when something fails we need to have the proper data to review the failure and analyze. |
Contributor
Author
|
@rnetser Regarding the 1 minute - I put this PR on WIP so please ignore this timeout change. I wanted to run these changes in the pytest runner to see if the fixture change helped get must-gather logs collection at the correct place, since in the must-gather logs from the failure in CI, VM2 (where the hotplug failed) was torndown before must-gather took action and we couldn't analyze the logs properly. We agreed with @EdDev to merge the fixtures change if it really helps with the logs and then we could get logs regarding the failure of the sriov hotplug in the next run in CI. |
azhivovk
changed the title
Contributor
Author
|
Must-gather logs on the failed test in CI - VM1 is torndown early and logs are missing: Console: Must-gather: With the current fix we do collect the failed VM1 logs right after the error (with intentional failure in Must-gather: |
Contributor
Author
|
Change: Drop all changes and keep fixtures change only |
Contributor
Author
|
/wip cancel |
openshift-virtualization-qe-bot
changed the title
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
test_connectivity_of_hot_plugged_sriov_interface is failing: VMInterfaceStatusNotFoundError: Network interface named sriov-hot-plug-test-network was not found in VM sriov-hot-plug-vm1 And VM1 must-gather logs are missing due to early teardown, which makes it difficult to debug and understand the root cause of the failure. VM1 is torndown by exit of Resource class (which VirtualMachineForTests inherits from). Context managers unwind as the exception propagates through the stack, before any outer exception handler (pytest's hook) can see it. Currently the same fixture creates the VM (via with VirtualMachineForTests(...)) and performs the hot-plug. When hot-plug fails, Python's exception propagation triggers VirtualMachineForTests.__exit__ (inherited from Resource), which deletes the VM before the exception reaches pytest's pytest_exception_interact hook. As a result, must-gather runs after the VM is already deleted and cannot capture its state. The fix is to use separate fixtures for VM creation and SR-IOV interface hotplug - then must-gather runs right after setup error. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Change: edit commit message |
Contributor
|
Clean rebase detected — no code changes compared to previous head ( |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/network/l2_bridge/test_bridge_nic_hot_plug.py`:
- Around line 419-437: sriov_hot_plug_vm2 and
vm2_with_hot_plugged_sriov_interface duplicate the vm1 pair; refactor by
extracting a reusable factory/helper that creates the hot-plug VM fixture and a
corresponding helper to attach the SR-IOV interface so both vm1 and vm2 fixtures
call the same functions with different parameters (VM name suffix, consumed
network fixture and index_number seed); update the fixtures sriov_hot_plug_vm2
and vm2_with_hot_plugged_sriov_interface to delegate to that helper (referencing
sriov_hot_plug_vm2, vm1 equivalents, hot_plug_interface_and_set_address) so
duplication is removed while preserving behavior and fixture signatures.
🪄 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: 4916816e-bf70-44ef-8eaa-e8eb6c16ee3e
📒 Files selected for processing (2)
tests/network/l2_bridge/libl2bridge.pytests/network/l2_bridge/test_bridge_nic_hot_plug.py
💤 Files with no reviewable changes (1)
- tests/network/l2_bridge/libl2bridge.py
Comment on lines
+419
to
+437
| def sriov_hot_plug_vm2(namespace, unprivileged_client): | ||
| with create_vm_for_hot_plug( | ||
| namespace_name=namespace.name, | ||
| vm_name=f"{SRIOV}-{HOT_PLUG_STR}-vm2", | ||
| sriov_network_for_hot_plug=sriov_network_for_hot_plug, | ||
| ipv4_address=random_ipv4_address(net_seed=0, host_address=next(index_number)), | ||
| client=unprivileged_client, | ||
| ) as vm: | ||
| yield vm | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def vm2_with_hot_plugged_sriov_interface(sriov_hot_plug_vm2, sriov_network_for_hot_plug, namespace, index_number): | ||
| hot_plug_interface_and_set_address( | ||
| vm=sriov_hot_plug_vm2, | ||
| hot_plugged_interface_name=sriov_network_for_hot_plug.name, | ||
| net_attach_def_name=f"{namespace.name}/{sriov_network_for_hot_plug.name}", | ||
| ipv4_address=random_ipv4_address(net_seed=0, host_address=next(index_number)), | ||
| sriov=True, | ||
| ) | ||
| return sriov_hot_plug_vm2 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: sriov_hot_plug_vm2 / vm2_with_hot_plugged_sriov_interface are near-identical to the vm1 pair.
The only differences between the vm1 and vm2 pairs are the VM-name suffix and which fixture they consume. A shared factory helper could reduce duplication, e.g.:
💡 Optional DRY refactor
+def _make_sriov_hot_plug_vm(namespace, unprivileged_client, suffix):
+ with create_vm_for_hot_plug(
+ namespace_name=namespace.name,
+ vm_name=f"{SRIOV}-{HOT_PLUG_STR}-{suffix}",
+ client=unprivileged_client,
+ ) as vm:
+ yield vm
+
+
`@pytest.fixture`()
-def sriov_hot_plug_vm1(namespace, unprivileged_client):
- with create_vm_for_hot_plug(
- namespace_name=namespace.name,
- vm_name=f"{SRIOV}-{HOT_PLUG_STR}-vm1",
- client=unprivileged_client,
- ) as vm:
- yield vm
+def sriov_hot_plug_vm1(namespace, unprivileged_client):
+ yield from _make_sriov_hot_plug_vm(namespace, unprivileged_client, suffix="vm1")
`@pytest.fixture`()
-def sriov_hot_plug_vm2(namespace, unprivileged_client):
- with create_vm_for_hot_plug(
- namespace_name=namespace.name,
- vm_name=f"{SRIOV}-{HOT_PLUG_STR}-vm2",
- client=unprivileged_client,
- ) as vm:
- yield vm
+def sriov_hot_plug_vm2(namespace, unprivileged_client):
+ yield from _make_sriov_hot_plug_vm(namespace, unprivileged_client, suffix="vm2")The same pattern applies to vm1_with_hot_plugged_sriov_interface / vm2_with_hot_plugged_sriov_interface.
Given the PR's primary goal is explicit fixture separation for diagnostics, keeping them separate is also a defensible choice. Treat this as purely optional.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 428-428: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/network/l2_bridge/test_bridge_nic_hot_plug.py` around lines 419 - 437,
sriov_hot_plug_vm2 and vm2_with_hot_plugged_sriov_interface duplicate the vm1
pair; refactor by extracting a reusable factory/helper that creates the hot-plug
VM fixture and a corresponding helper to attach the SR-IOV interface so both vm1
and vm2 fixtures call the same functions with different parameters (VM name
suffix, consumed network fixture and index_number seed); update the fixtures
sriov_hot_plug_vm2 and vm2_with_hot_plugged_sriov_interface to delegate to that
helper (referencing sriov_hot_plug_vm2, vm1 equivalents,
hot_plug_interface_and_set_address) so duplication is removed while preserving
behavior and fixture signatures.
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
9 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.
test_connectivity_of_hot_plugged_sriov_interfaceis failing:VMInterfaceStatusNotFoundError: Network interface named sriov-hot-plug-test-network was not found in VM sriov-hot-plug-vm1And VM1 must-gather logs are missing due to early teardown, which makes it difficult to debug and understand the root cause of the failure.
VM1 is torndown by exit of
Resourceclass (which VirtualMachineForTests inherits from).Context managers unwind as the exception propagates through the stack, before any outer exception handler (pytest's hook) can see it.
Currently the same fixture creates the VM (via with VirtualMachineForTests(...)) and performs the hot-plug. When hot-plug fails, Python's exception propagation triggers
VirtualMachineForTests.__exit__(inherited from Resource), which deletes the VM before the exception reaches pytest's pytest_exception_interact hook. As a result, must-gather runs after the VM is already deleted and cannot capture its state.The fix is to use separate fixtures for VM creation and SR-IOV interface hotplug - then must-gather runs right after setup error.
Summary by CodeRabbit