net: Introduce dual-stream linux bridge tests#4772
Open
azhivovk wants to merge 2 commits intoRedHatQE:mainfrom
Open
net: Introduce dual-stream linux bridge tests#4772azhivovk wants to merge 2 commits intoRedHatQE:mainfrom
azhivovk wants to merge 2 commits intoRedHatQE:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds infrastructure for testing VM networking connectivity across clusters with both RHCOS 9 and RHCOS 10 worker nodes. It introduces nodeSelector support to VM specs, utilities to discover and pin VMs to specific RHCOS versions, fixtures to set up dual-interface test VMs with secondary-interface cloud-init, and integration tests validating TCP connectivity survives VM migration between RHCOS versions. ChangesMixed RHCOS Cluster Connectivity Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: The PR spans 8 files with heterogeneous roles (spec schema, node/network utilities, test fixtures, test logic, and pytest config). Core logic density is moderate—the node selection and VM pinning utilities are straightforward iterators and patchers; bridge VM construction chains together interfaces, cloud-init, and nodeSelector assignment. Bridge fixtures orchestrate multiple dependencies and validate interface state. Migration tests refactor from parameterless to fixture-driven, introducing subtests and IP-version-specific assertions. The breadth across utilities, fixtures, and test methods demands separate reasoning for each area, but repetition patterns (two mirror-image migration tests, two similar VM fixtures) reduce net complexity. Schema and config changes are trivial. 🚥 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. |
openshift-virtualization-qe-bot-5
requested review from
Anatw,
EdDev,
RoniKishner,
dshchedr,
frenzyfriday,
nirdothan,
rnetser,
vsibirsk and
yossisegev
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pytest.ini (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore required PR template sections in the PR description
The PR description in the provided context is missing the required template headings (
What this PR does / why we need it,Which issue(s) this PR fixes,Special notes for reviewer,jira-ticket). Please restore them before merge.As per coding guidelines: “Required sections (must be present, even if empty)… If any required section is absent… flag it as HIGH severity.”
🤖 Prompt for 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. In `@pytest.ini` at line 1, Restore the required PR description headings in the PR (add the headings verbatim): "What this PR does / why we need it", "Which issue(s) this PR fixes", "Special notes for reviewer", and "jira-ticket"; update the PR description to include those sections (they can be empty but must be present) so the template requirements are satisfied before merging, and confirm the PR body now contains each exact heading.tests/network/libs/vm_factory.py (1)
26-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMEDIUM: Avoid silently dropping
template_labelswhennodeis providedLine 28 introduces an
elifpath that suppresses label application whenevernodeis set. Keep labels independent, and only gate anti-affinity on node pinning if needed.Suggested diff
- if node is not None: - spec.template.spec.nodeSelector = {HOSTNAME_LABEL: node.hostname} - elif template_labels: + if template_labels: spec.template.metadata.labels = spec.template.metadata.labels or {} # type: ignore spec.template.metadata.labels.update(template_labels) # type: ignore - # Use the first label key and first value as the anti-affinity label to use: - label, *_ = template_labels.items() - spec.template.spec.affinity = new_pod_anti_affinity(label=label, namespaces=anti_affinity_namespaces) + if node is not None: + spec.template.spec.nodeSelector = {HOSTNAME_LABEL: node.hostname} + elif template_labels: + # Use the first label key and first value as the anti-affinity label to use: + label, *_ = template_labels.items() + spec.template.spec.affinity = new_pod_anti_affinity(label=label, namespaces=anti_affinity_namespaces)🤖 Prompt for 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. In `@tests/network/libs/vm_factory.py` around lines 26 - 33, The current branch only applies template_labels when node is None, dropping labels whenever node pinning occurs; refactor so template_labels are always merged into spec.template.metadata.labels (update spec.template.metadata.labels = ... and spec.template.metadata.labels.update(template_labels) when template_labels is truthy), then separately handle node pinning by setting spec.template.spec.nodeSelector when node is not None, and only create/specify spec.template.spec.affinity via new_pod_anti_affinity when node is None (or when you explicitly want to gate anti-affinity), using the first key/value from template_labels and anti_affinity_namespaces as before.
🤖 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 `@libs/vm/spec.py`:
- Line 34: The model field currently declared as nodeSelector with a "# noqa:
N815" should be renamed to snake_case (node_selector) and the linter suppression
removed; then add an explicit serialization alias so external Kubernetes
JSON/YAML still emits "nodeSelector" (e.g., use your model system's alias/Field
parameter like Field(alias="nodeSelector") or serializer mapping and ensure
dict/JSON export uses aliases). Update any places that reference nodeSelector to
use node_selector or to serialize via the alias, and remove the "# noqa: N815"
comment.
In `@tests/network/l2_bridge/rhel9_rhel10_cluster/conftest.py`:
- Line 76: The assertion in the test fixture currently just checks
iface_cloud_init.addresses without context; update the assertion to include a
descriptive failure message so failures show intent — change the assert on
iface_cloud_init.addresses in the conftest fixture to include a message (e.g.,
referencing iface_cloud_init or its name) that explains that no addresses were
assigned to the interface.
In `@tests/network/l2_bridge/rhel9_rhel10_cluster/lib_helpers.py`:
- Around line 16-46: The function bridge_vm lacks a required Google-style
docstring describing its parameters, return value, and side effects; add a
Google-format docstring above bridge_vm that documents each parameter
(namespace, name, client: DynamicClient, bridge_network_name, cloud_init_iface:
EthernetDevice, node: Node), the returned BaseVirtualMachine, and side effects
(node pinning via spec.template.spec.nodeSelector using HOSTNAME_LABEL, added
interfaces/networks using Devices/Interface/Network with
LINUX_BRIDGE_IFACE_NAME, cloud-init userdata built with cloudinit.UserData and
cloudinit.asyaml, disk/volume creation via cloudinitdisk_storage, and that the
final VMI is created by fedora_vm), and mention any important expectations or
invariants (e.g., cloud_init_iface used as eth1); keep it in Google docstring
format.
In `@tests/network/localnet/rhel9_rhel10_cluster/conftest.py`:
- Line 33: The assertion "assert iface_cloud_init.addresses" (and the similar
assertion at line 69) lacks a failure message; update both assertions in the
conftest.py fixture so they include explicit descriptive messages (e.g.,
indicating "unsupported cluster IP-family state: no addresses on
iface_cloud_init") to make test failures diagnosable; locate the assertions
referencing iface_cloud_init (and the second similar assert) and change them to
use the assert condition, "descriptive message" pattern.
---
Outside diff comments:
In `@pytest.ini`:
- Line 1: Restore the required PR description headings in the PR (add the
headings verbatim): "What this PR does / why we need it", "Which issue(s) this
PR fixes", "Special notes for reviewer", and "jira-ticket"; update the PR
description to include those sections (they can be empty but must be present) so
the template requirements are satisfied before merging, and confirm the PR body
now contains each exact heading.
In `@tests/network/libs/vm_factory.py`:
- Around line 26-33: The current branch only applies template_labels when node
is None, dropping labels whenever node pinning occurs; refactor so
template_labels are always merged into spec.template.metadata.labels (update
spec.template.metadata.labels = ... and
spec.template.metadata.labels.update(template_labels) when template_labels is
truthy), then separately handle node pinning by setting
spec.template.spec.nodeSelector when node is not None, and only create/specify
spec.template.spec.affinity via new_pod_anti_affinity when node is None (or when
you explicitly want to gate anti-affinity), using the first key/value from
template_labels and anti_affinity_namespaces as before.
🪄 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: fd3b3507-394a-4715-8e06-28d1b71547a1
📒 Files selected for processing (12)
libs/vm/spec.pypytest.initests/network/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/lib_helpers.pytests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.pytests/network/libs/cloudinit.pytests/network/libs/nodes.pytests/network/libs/vm_factory.pytests/network/localnet/liblocalnet.pytests/network/localnet/rhel9_rhel10_cluster/conftest.pytests/network/localnet/rhel9_rhel10_cluster/test_connectivity.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
azhivovk
force-pushed
the
new_rhcos9-10_tests
branch
from
b12e932 to
03b1c98
Compare
Contributor
Author
|
Change: rebase |
Contributor
Author
|
Change: order commits, edit PR description |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
azhivovk
force-pushed
the
new_rhcos9-10_tests
branch
from
039af50 to
c58887e
Compare
openshift-virtualization-qe-bot
added
size/L
and removed
commented-azhivovk
lgtm-coderabbitai[bot]
commented-coderabbitai[bot]
size/XL
labels
Contributor
Author
|
Change: Drop udn changes |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
- Add mixed_os_nodes marker to pytest.ini to select tests only on compatible clusters - Add rhcos9_node/rhcos10_node fixtures to network conftest - Add update_vm_node_selector helper and HOSTNAME_LABEL to nodes lib Signed-off-by: Asia Khromov <azhivovk@redhat.com> Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Live migration between RHCOS 9 and RHCOS 10 nodes can expose kernel bridge driver incompatibilities that break active TCP connections. These tests verify that secondary Linux bridge network connectivity is preserved during cross-OS migration in both directions. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
azhivovk
force-pushed
the
new_rhcos9-10_tests
branch
from
c58887e to
00747ed
Compare
Contributor
Author
|
Change: edit commits |
Contributor
|
Clean rebase detected — no code changes compared to previous head ( |
Contributor
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 `@tests/network/l2_bridge/rhel9_rhel10_cluster/conftest.py`:
- Around line 40-91: bridge_server_vm and bridge_client_vm are duplicate
fixtures; extract their shared setup into a contextmanager bridge_test_vm in
lib_helpers.py and have both fixtures call it; implement bridge_test_vm to
accept namespace, name, client, bridge_network_name, host_address, node, use
cidr_addresses_by_family, bridge_vm, vm.start, vm.wait_for_agent_connected and
wait_for_ifaces_status with LINUX_BRIDGE_IFACE_NAME as in the diff, then update
bridge_server_vm and bridge_client_vm in conftest.py to invoke bridge_test_vm
with the appropriate name ("server-vm"/"client-vm") and host_address
(_SERVER_HOST_ADDRESS/_CLIENT_HOST_ADDRESS) and yield the returned vm.
- Line 26: Update each pytest fixture return annotation that currently uses a
single-parameter collections.abc.Generator to the three-parameter form
Generator[T, None, None]; e.g., change Generator[NetworkAttachmentDefinition] to
Generator[NetworkAttachmentDefinition, None, None] (and do the same for the
other three fixture annotations at the same locations), and ensure the Generator
import from collections.abc remains present or is added if missing.
In `@tests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.py`:
- Around line 20-22: Add the pytest tier marker to classify this complex
platform-specific test: above the existing decorators on class TestConnectivity
(which currently has `@pytest.mark.mixed_os_nodes` and `@pytest.mark.incremental`),
add `@pytest.mark.tier3` so the test is both environment-filtered and correctly
labeled as a tier3 test.
🪄 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: 8497d612-93c5-4748-9e43-1bd6b2816ae5
📒 Files selected for processing (8)
libs/net/ip.pylibs/vm/spec.pypytest.initests/network/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/lib_helpers.pytests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.pytests/network/libs/nodes.py
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
7 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.
Short description:
More details: STD: #4569
What this PR does / why we need it:
Added automation for new dual-stream tests
Which issue(s) this PR fixes: -
Special notes for reviewer:
Follow-up implementation of new tests of linux bridge
jira-ticket: https://redhat.atlassian.net/browse/CNV-86194
Summary by CodeRabbit
New Features
Tests