net: Introduce interfaces stability test#3930
Merged
rnetser merged 1 commit intoRedHatQE:mainfrom Mar 9, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional IPv6 DHCP field to cloud-init EthernetDevice, and introduces fixtures, helper utilities, and tests to validate IP persistence for VMs attached to a Linux bridge, including verification before and after migration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/vmi/conftest.py`:
- Line 1: Replace the deprecated typing.Generator import with the modern one
from collections.abc: update the import that currently references
typing.Generator to import Generator from collections.abc, and ensure any type
annotations that use typing.Generator (e.g., in fixtures or functions in this
module) now refer to collections.abc.Generator so the code is compatible with
Python 3.12+.
- Around line 17-19: The fixture ip_persistence_bridge_device_name incorrectly
uses yield without teardown; change it to return the string instead of yielding
and update its type annotation from Generator[str, None, None] to just str
(i.e., def ip_persistence_bridge_device_name(index_number: Generator[int, None,
None]) -> str: return f"br{next(index_number)}test"). Also remove any now-unused
typing Generator import or adjust the index_number parameter type if you prefer,
ensuring no teardown is implied.
- Around line 66-74: Remove the unnecessary defensive check around
spec.template.spec.domain.devices added after calling base_vmspec();
base_vmspec() always initializes devices=Devices(), so delete the if statement
and assign spec.template.spec.domain.devices.interfaces unconditionally to the
list of Interface(...) values, then set spec.template.spec.networks to the
Network(...) list (using Multus(networkName=bridge_network.name)) — update the
block that references base_vmspec(),
spec.template.spec.domain.devices.interfaces, Interface, Network, Multus, and
bridge_network accordingly.
In `@tests/network/vmi/libippersistence.py`:
- Around line 14-17: The function vm_cloud_init_data currently accepts two
boolean positional parameters (ipv4_supported_cluster, ipv6_supported_cluster);
make them keyword-only by adding a bare * in the parameter list (i.e., change
the signature of vm_cloud_init_data to enforce keyword-only args) so callers
must use named arguments and avoid ambiguous positional calls like
vm_cloud_init_data(True, False).
- Line 46: The log message hardcodes "5-minute" and can drift from the single
source of truth TIMEOUT_SECONDS; change the message in the LOGGER.info call to
compute the human-readable duration from TIMEOUT_SECONDS (e.g., derive minutes
using integer division or rounding) and interpolate that computed value instead
of the literal "5-minute" while keeping vmi_name and context_msg unchanged.
- Around line 41-43: The parameter name on monitor_vmi_interfaces is misleading:
rename the parameter from vmi to vm to match its BaseVirtualMachine type, update
all internal references (replace vmi.vmi.name and vmi.namespace with vm.vmi.name
and vm.namespace), and adjust any local references in monitor_vmi_interfaces
accordingly; also update call sites that pass the argument by position or name
(e.g., test_ip_persistence.py where callers use vmi=...) to use the new
parameter name or continue using the named arg as appropriate.
- Around line 51-70: The watch iterator created via watcher = watch.Watch() and
started by vmi_resource.watch(...) can leak the underlying HTTP stream if you
break early; wrap the for loop in a try/finally and call watcher.stop() in the
finally block to ensure the watch is stopped deterministically when the timeout
branch (time.time() - start_time >= TIMEOUT_SECONDS) triggers or any other early
exit occurs, leaving the rest of the loop logic (event_type, vmi_obj, iteration
increment and logging) unchanged.
In `@tests/network/vmi/test_ip_persistence.py`:
- Around line 11-21: Add the tier3 pytest marker to both tests by decorating
test_ip_address_persistence and test_ip_address_persistence_after_migration with
`@pytest.mark.tier3` (in addition to the existing `@pytest.mark.polarion`
decorators) so the functions monitor_vmi_interfaces and migrate_vm_and_verify
runs are classified as tier3; ensure the decorator appears immediately above
each def to match existing style.
|
D/S test |
azhivovk
force-pushed
the
test_ip_flapping
branch
from
5a90b5b to
71b60a6
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:failed
Contributor
Author
Contributor
/approve |
Mulitple users have encountered events in which vmi interfaces or ipAddress field disappeared. On setup we wait for all VM interfaces to stabilize before verifying interfaces stability. During tests, using watch kuberenetes API, which is used by wrapper's watcher, we monitor for such events and verify all interfaces are stable. Signed-off-by: Asia Khromov <azhivovk@redhat.com>
Contributor
Author
|
Change: Rename new lib to |
Contributor
Author
|
/verified Verified on BM with cnv-4.22: |
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: 1978 🏁 Script executed: Length of output: 1735 🧠 Learnings used |
EdDev
suggested changes
Mar 9, 2026
Contributor
EdDev
left a comment
EdDev
left a comment
There was a problem hiding this comment.
Change: Rename new lib to lib_helpers.py as discussed offline
As discussed in [1] and in many passed discussions from when the libs concept has been introduced, I will not accept these names.
Said that, I am aware of the root approver opinion and therefore overriding this vote is legit from my side.
All other changes here looks good to me.
[1] #3930 (comment)
Anatw
approved these changes
Mar 9, 2026
Collaborator
|
/approve |
rnetser
approved these changes
Mar 9, 2026
Contributor
Author
|
/check-can-merge |
Collaborator
following this thread; as discussed, I am approving and merging this PR |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
Contributor
Author
|
/cherry-pick cnv-4.21 |
|
Cherry-picked PR net: Introduce interfaces stability test into cnv-4.21 |
Contributor
Author
|
/cherry-pick cnv-4.20 cnv-4.19 cnv-4.18 |
|
Cherry-picked PR net: Introduce interfaces stability test into cnv-4.20 |
|
Cherry-picked PR net: Introduce interfaces stability test into cnv-4.18 |
|
Cherry-picked PR net: Introduce interfaces stability test into cnv-4.19 |
This was referenced Mar 10, 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
11 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.
Mulitple users have encountered events in which
vmi interfaces or ipAddress field disappeared.
VMI interface status is unstable during boot - interfaces may
temporarily disappear from the status until all are stable.
Asserting before interfaces stabilize risks a false negative - during setup
we wait for all VM interfaces to stabilize before verifying interfaces stability.
During tests, using watch kuberenetes API, which
is used by wrapper's watcher, we monitor for such
events and verify all interfaces are stable.
jira-ticket: https://issues.redhat.com/browse/CNV-79267
Summary by CodeRabbit