net: NAD reference live-update STD#4578
Open
azhivovk wants to merge 1 commit intoRedHatQE:mainfrom
Open
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 two new pytest modules that introduce four Polarion-annotated, documentation-only test stubs for live-update NAD reference scenarios (three for l2-bridge, one for localnet); all tests are disabled from collection by setting Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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-2
requested review from
Anatw,
EdDev,
nirdothan and
servolkov
![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: 5
🤖 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/nad_ref_change/test_nad_ref_change.py`:
- Around line 64-79: The test function
test_non_migratable_vm_nad_change_not_applied currently only asserts
connectivity before/after and must explicitly assert that the NAD update is
rejected or surfaces an error; change the Expected to a single assertion that
the VM update call fails with a validation/error response (or that the VM's
RestartRequired/error condition is set) and move the connectivity checks into
Preconditions so the test verifies ONE negative outcome (rejection/error) rather
than just unchanged traffic which could be a silent-success.
- Line 21: The test uses placeholder Polarion IDs in the pytest.mark.polarion
decorator (e.g., pytest.mark.polarion("CNV-00000")); replace these placeholders
with the actual, unique Polarion IDs for this test and update the other
occurrences of pytest.mark.polarion in the same module (the other decorators
currently using CNV-00001/CNV-00002) so each test has a correct, non-colliding
ID; ensure you only change the string argument(s) passed to pytest.mark.polarion
and keep the decorator usage intact.
- Line 18: The module-level "__test__ = False" is causing pytest to skip all
tests in this file; remove that module-level flag and instead mark each
placeholder test individually by adding per-function attributes: set
test_bridge_binding_vlan_change_on_running_vm.__test__ = False,
test_multiple_secondary_networks_independently_updated.__test__ = False, and
test_non_migratable_vm_nad_change_not_applied.__test__ = False so only those
placeholder tests are suppressed rather than the whole module.
In `@tests/network/localnet/nad_ref_change/test_nad_ref_change.py`:
- Line 18: The Polarion marker in the test decorator
pytest.mark.polarion("CNV-00000") is using a placeholder ID; replace "CNV-00000"
with the correct Polarion test case ID (or remove the marker if none applies) so
the test uses a unique, real traceability ID; update the string in the
pytest.mark.polarion(...) call in test_nad_ref_change.py accordingly.
- Line 15: Module-level __test__ = False is ineffective; attach the attribute to
the test function itself. Remove the module-level __test__ line and add __test__
= False immediately after the function definition for
test_localnet_secondary_network_vlan_change_on_running_vm (i.e., set
test_localnet_secondary_network_vlan_change_on_running_vm.__test__ = False or
place __test__ = False on the next line after the def) so pytest will skip
collecting that specific function.
🪄 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: c5027a12-939b-4f1d-b0c8-ced5dcdb6ba2
📒 Files selected for processing (4)
tests/network/l2_bridge/nad_ref_change/__init__.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/network/localnet/nad_ref_change/__init__.pytests/network/localnet/nad_ref_change/test_nad_ref_change.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:failed
azhivovk
force-pushed
the
std_nad_ref_change
branch
from
7d1b3b0 to
dfb543a
Compare
Contributor
Author
|
Change: do not collect the new tests |
azhivovk
force-pushed
the
std_nad_ref_change
branch
from
0bcb31e to
8d0562d
Compare
Contributor
Author
|
Change: edit commit message |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
servolkov
suggested changes
Apr 27, 2026
| """ | ||
| Live Update NetworkAttachmentDefinition Reference Tests — Linux Bridge | ||
|
|
||
| STP: https://github.com/RedHatQE/openshift-virtualization-tests-design-docs/pull/74 |
Contributor
There was a problem hiding this comment.
pls add link to the doc, not PR
| STP: https://github.com/RedHatQE/openshift-virtualization-tests-design-docs/pull/74 | ||
|
|
||
| Preconditions: | ||
| - Four Linux bridge CNI Network Attachment Definitions: NAD-A1, NAD-A2, NAD-B1, NAD-B2 |
Contributor
There was a problem hiding this comment.
How many VLANs do we have at the end? 2 or 4? I am a bit confused with the naming. To be honest, this did not allow me to better understand further scenarios.
Contributor
Author
There was a problem hiding this comment.
Yes, overall 4 different VLANs
Fixed
| - Four Linux bridge CNI Network Attachment Definitions: NAD-A1, NAD-A2, NAD-B1, NAD-B2 | ||
| (NAD-A1 and NAD-B1 on different VLANs; NAD-A2 and NAD-B2 on different VLANs) | ||
| - Running VLAN A1 reference VM connected to NAD-A1 | ||
| - Running VLAN A2 reference VM connected to NAD-A2 |
Contributor
There was a problem hiding this comment.
this VM is only used in test_multiple_secondary_networks_independently_updated() (as well as Running VLAN B2 reference VM). So they should live only as test precondition, not module.
|
|
||
| Preconditions: | ||
| - TCP connectivity established between the under-test VM and the VLAN A1 reference VM | ||
| - Guest secondary interface MAC address, name, and IP addresses recorded before the NAD reference change |
Contributor
There was a problem hiding this comment.
IMO it fits better into steps than precondition, having a fixture to store VM data sounds inappropriate to me.
| - Running VLAN A2 reference VM connected to NAD-A2 | ||
| - Running VLAN B1 reference VM connected to NAD-B1 | ||
| - Running VLAN B2 reference VM connected to NAD-B2 | ||
| - Running under-test VM with two bridge-bound secondary interfaces connected to NAD-A1 and NAD-A2 |
Contributor
There was a problem hiding this comment.
If this VM is supposed to be re-created for each test - this is not module precondition rather test level precondition.
azhivovk
force-pushed
the
std_nad_ref_change
branch
from
4e50ba9 to
6fd528a
Compare
Contributor
Author
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/nad_ref_change/test_nad_ref_change.py`:
- Around line 23-25: The test-level Preconditions docstrings must mirror the
shared Preconditions by explicitly listing the directly-used reference VM and
connectivity details: update each test's Preconditions to include a line like "-
Reference VM (the target reference VM used by the test) is running and
reachable" and also restate the relevant VLAN/TCP connectivity and the
under-test VM's bridge-bound secondary interface (e.g., reference the
"bridge-bound secondary interface", "reference VM", and "initial VLAN" terms
already used in the shared block) so each test-level docstring duplicates the
shared resource usage.
- Around line 46-69: The docstring's Step 1 and Expected are inconsistent with
the claim "independently updated": instead of only updating both secondary
networks simultaneously, modify the test steps in the docstring to (a) update
the first secondary NIC to its target NAD, verify loss of connectivity on its
initial VLAN and gain of TCP connectivity to its target VLAN, then (b) update
the second secondary NIC to its target NAD and verify loss/gain for its VLAN;
also update the Expected section to explicitly require that each interface loses
connectivity on its initial VLAN and gains connectivity on its respective target
VLAN and that the VM remains running after each change; change references in the
docstring to "first secondary interface" and "second secondary interface" to
match the existing names used in the test description so reviewers can locate
the relevant assertions.
In `@tests/network/localnet/nad_ref_change/test_nad_ref_change.py`:
- Around line 22-25: The test docstring's "Preconditions:" block currently lists
only the initial NAD and its reference VM; add the target VLAN reference VM to
that Preconditions list so it matches the later connectivity check (the
connectivity validation to the target VLAN reference VM referenced in the test).
Update the test-level Preconditions in the test_nad_ref_change docstring to
include the target VLAN reference VM alongside the existing initial NAD and
reference VM entries so shared vs. test-specific preconditions are consistent.
🪄 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: 20dd57c5-6b69-4884-96e9-29a4a4cab01c
📒 Files selected for processing (4)
tests/network/l2_bridge/nad_ref_change/__init__.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/network/localnet/nad_ref_change/__init__.pytests/network/localnet/nad_ref_change/test_nad_ref_change.py
Comment on lines
+46
to
+69
| Test that multiple secondary networks on the same running VM can each be | ||
| independently updated to different NADs. | ||
|
|
||
| Preconditions: | ||
| - Two additional Linux bridge Network Attachment Definitions on different VLANs: | ||
| one initial NAD and one target NAD for the second secondary interface of the under-test VM | ||
| - Running reference VM on the initial VLAN of the second secondary interface | ||
| - Running reference VM on the target VLAN of the second secondary interface | ||
| - Running under-test VM with two bridge-bound secondary interfaces, | ||
| first interface connected to the first initial NAD, second interface connected to the second initial NAD | ||
| - TCP connectivity established between the under-test VM and the reference VM on the initial VLAN | ||
| of the first secondary interface | ||
| - TCP connectivity established between the under-test VM and the reference VM on the initial VLAN | ||
| of the second secondary interface | ||
|
|
||
| Steps: | ||
| 1. Update both secondary networks of the under-test VM simultaneously, | ||
| each to its respective target NAD | ||
|
|
||
| Expected: | ||
| - Under-test VM remains running after both NAD reference changes | ||
| - Under-test VM has TCP connectivity to the reference VM on the target VLAN of the first secondary interface | ||
| - Under-test VM has TCP connectivity to the reference VM on the target VLAN of the second secondary interface | ||
| """ |
Contributor
There was a problem hiding this comment.
[HIGH] “Independently updated” is not verified by a simultaneous-only step.
The current step updates both interfaces at once, so the docstring does not actually prove independence. Also, Expected omits explicit loss of connectivity to the initial VLANs, which weakens NAD-switch validation.
Suggested scenario wording to match intent
Steps:
- 1. Update both secondary networks of the under-test VM simultaneously,
- each to its respective target NAD
+ 1. Update the first secondary network of the under-test VM to its target NAD
+ 2. Verify the second secondary network remains on its initial NAD behavior
+ 3. Update the second secondary network of the under-test VM to its target NAD
Expected:
- Under-test VM remains running after both NAD reference changes
- Under-test VM has TCP connectivity to the reference VM on the target VLAN of the first secondary interface
- Under-test VM has TCP connectivity to the reference VM on the target VLAN of the second secondary interface
+ - Under-test VM has no TCP connectivity to the reference VM on the initial VLAN of the first secondary interface
+ - Under-test VM has no TCP connectivity to the reference VM on the initial VLAN of the second secondary interface🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py` around lines
46 - 69, The docstring's Step 1 and Expected are inconsistent with the claim
"independently updated": instead of only updating both secondary networks
simultaneously, modify the test steps in the docstring to (a) update the first
secondary NIC to its target NAD, verify loss of connectivity on its initial VLAN
and gain of TCP connectivity to its target VLAN, then (b) update the second
secondary NIC to its target NAD and verify loss/gain for its VLAN; also update
the Expected section to explicitly require that each interface loses
connectivity on its initial VLAN and gains connectivity on its respective target
VLAN and that the VM remains running after each change; change references in the
docstring to "first secondary interface" and "second secondary interface" to
match the existing names used in the test description so reviewers can locate
the relevant assertions.
Comment on lines
+22
to
+25
| Preconditions: | ||
| - Running under-test VM with bridge binding connected to the initial NAD | ||
| - TCP connectivity established between the under-test VM and the reference VM on the initial VLAN | ||
|
|
Contributor
There was a problem hiding this comment.
[MEDIUM] Add the target VLAN reference VM to test-level Preconditions.
Line 31 validates connectivity to the target VLAN reference VM, but that resource is not listed in this test’s Preconditions: block.
Suggested docstring adjustment
Preconditions:
- Running under-test VM with bridge binding connected to the initial NAD
- TCP connectivity established between the under-test VM and the reference VM on the initial VLAN
+ - Running reference VM on the target VLANAs per coding guidelines: “STD docstrings: shared vs. test-specific preconditions … When a shared resource is directly used, it must appear in both shared and test-level preconditions”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/network/localnet/nad_ref_change/test_nad_ref_change.py` around lines 22
- 25, The test docstring's "Preconditions:" block currently lists only the
initial NAD and its reference VM; add the target VLAN reference VM to that
Preconditions list so it matches the later connectivity check (the connectivity
validation to the target VLAN reference VM referenced in the test). Update the
test-level Preconditions in the test_nad_ref_change docstring to include the
target VLAN reference VM alongside the existing initial NAD and reference VM
entries so shared vs. test-specific preconditions are consistent.
STD for the tests of the live NAD reference update feature, which allows VM owners to change the secondary network of a running VM without rebooting and without any change to the guest interface. The module-level preconditions capture the shared setup for that CNI type, including the bridge/localnet NADs and the reference VMs needed to verify connectivity. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Assisted-by: Claude Sonnet 4.6
azhivovk
force-pushed
the
std_nad_ref_change
branch
from
6fd528a to
fe0fb12
Compare
openshift-virtualization-qe-bot-4
removed
the
changes-requested-coderabbitai[bot]
label
Contributor
Author
|
Change: Add polarion IDs |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py (4)
23-25:⚠️ Potential issue | 🟡 MinorMEDIUM: Add the target reference VM to this test's Preconditions.
Line 33 checks connectivity to the target VLAN reference VM, but that resource is only described in the module-level docstring. Please repeat it here so the test-level Preconditions mirror the shared resource usage.
Suggested patch
Preconditions: - Running under-test VM with a bridge-bound secondary interface connected to the initial NAD - TCP connectivity established between the under-test VM and the reference VM on the initial VLAN + - Running reference VM on the target VLANAs per coding guidelines: when a shared resource is directly used, it must appear in both shared and test-level Preconditions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py` around lines 23 - 25, The test-level Preconditions in tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py currently list the under-test VM and initial VLAN reference VM but omit the target VLAN reference VM that the test actually queries (see the connectivity check around line 33); update the test's Preconditions paragraph to include the target reference VM (e.g., "the target VLAN reference VM reachable on the target VLAN") so the test-level Preconditions mirror the shared module docstring and reflect the resource used by the connectivity check.
49-59:⚠️ Potential issue | 🟡 MinorMEDIUM: Mirror the first interface's target reference VM in Preconditions.
Line 67 asserts target-VLAN connectivity for the first secondary interface, but the test-level Preconditions only name the second interface's target VLAN resources. Add the first interface's target reference VM here so the docstring matches the behavior under test.
Suggested patch
Preconditions: + - Running reference VM on the target VLAN of the first secondary interface - Two additional Linux bridge Network Attachment Definitions on different VLANs: one initial NAD and one target NAD for the second secondary interface of the under-test VM - Running reference VM on the initial VLAN of the second secondary interface - Running reference VM on the target VLAN of the second secondary interfaceAs per coding guidelines: shared vs. test-specific Preconditions must mirror any shared resource that is directly used by the test body/Expected section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py` around lines 49 - 59, The Preconditions docstring in test_nad_ref_change.py is missing the target reference VM for the first secondary interface; update the docstring block labeled "Preconditions" to include a line mirroring the second interface's target VM entry (e.g., "Running reference VM on the target VLAN of the first secondary interface") so the declared preconditions match the assertions in the test (see the test's target-VLAN connectivity assertion for the first secondary interface).
81-83:⚠️ Potential issue | 🟡 MinorMEDIUM: Add the target reference VM to the negative test Preconditions.
Line 90 checks that the VM has no connectivity to the target VLAN, so the target VLAN reference VM should be listed here as a directly-used shared resource.
Suggested patch
Preconditions: - Running non-migratable under-test VM with a bridge-bound secondary interface connected to the initial NAD - TCP connectivity established between the non-migratable under-test VM and the reference VM on the initial VLAN + - Running reference VM on the target VLANAs per coding guidelines: directly-used shared resources must be repeated in the test-level Preconditions block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py` around lines 81 - 83, The test Preconditions block in tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py must explicitly list the target reference VM as a directly-used shared resource; update the "Preconditions:" section (the block shown around the existing bridge-bound secondary interface and initial VLAN) to include the target VLAN reference VM so it matches the later assertion at line ~90 that the VM has no connectivity to the target VLAN. Ensure the added item names the same test fixture/VM identifier used elsewhere in the file (the target reference VM variable/name) so reviewers can see it is a directly-used shared resource.
61-68:⚠️ Potential issue | 🟠 MajorHIGH: The "independently updated" scenario is not actually independent.
Updating both secondary networks in one step does not verify independent update behavior; a combined change can hide ordering bugs. Split this into per-interface updates and assert each interface's initial VLAN loses connectivity before moving to the next one.
Suggested patch
Steps: - 1. Update both secondary networks of the under-test VM simultaneously, - each to its respective target NAD + 1. Update the first secondary network of the under-test VM to its target NAD + 2. Verify the second secondary network remains on its initial NAD behavior + 3. Update the second secondary network of the under-test VM to its target NAD Expected: - Under-test VM remains running after both NAD reference changes - Under-test VM has TCP connectivity to the reference VM on the target VLAN of the first secondary interface - Under-test VM has TCP connectivity to the reference VM on the target VLAN of the second secondary interface + - Under-test VM has no TCP connectivity to the reference VM on the initial VLAN of the first secondary interface + - Under-test VM has no TCP connectivity to the reference VM on the initial VLAN of the second secondary interface🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py` around lines 61 - 68, The test currently updates both secondary networks at once in test_nad_ref_change.py which masks ordering bugs; change the test to perform per-interface updates instead: first update the first secondary interface to its target NAD using the same helper(s) currently used for updates (e.g., the code that performs the simultaneous NAD update), then assert the under-test VM loses TCP connectivity on the original VLAN and gains connectivity on the target VLAN for that first interface before proceeding; next, perform the same steps for the second secondary interface (update its NAD, assert loss of connectivity on its initial VLAN, then assert connectivity on its target VLAN). Ensure you reuse the existing VM/network helpers in the test and add explicit connectivity assertions between each per-interface update rather than doing a combined update.tests/network/localnet/nad_ref_change/test_nad_ref_change.py (1)
22-25:⚠️ Potential issue | 🟡 MinorMEDIUM: Mirror the target reference VM in test-level Preconditions.
Line 31 depends on the target VLAN reference VM, but this test only names the initial VLAN resources in
Preconditions:. Please duplicate the target reference VM here so the STD traceability stays aligned with the shared/test-specific split.Suggested patch
Preconditions: - Running under-test VM with a bridge-bound secondary interface connected to the initial NAD - TCP connectivity established between the under-test VM and the reference VM on the initial VLAN + - Running reference VM on the target VLANAs per coding guidelines: STD docstrings must keep shared resources in the module docstring and repeat any directly-used shared resource in the test-level
Preconditions:.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/network/localnet/nad_ref_change/test_nad_ref_change.py` around lines 22 - 25, The test-level Preconditions in tests/network/localnet/nad_ref_change/test_nad_ref_change.py currently list only the initial VLAN resources (initial NAD and bridge) but the test body references the target VLAN reference VM (used around the assertion at the comment near line 31); update the test's Preconditions block to also name/describe the target reference VM (the VM on the target VLAN) so the docstring mirrors the actual resources used by the test (repeat the shared resource entry for the target VLAN reference VM alongside the existing initial VLAN entries).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py`:
- Around line 23-25: The test-level Preconditions in
tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py currently list the
under-test VM and initial VLAN reference VM but omit the target VLAN reference
VM that the test actually queries (see the connectivity check around line 33);
update the test's Preconditions paragraph to include the target reference VM
(e.g., "the target VLAN reference VM reachable on the target VLAN") so the
test-level Preconditions mirror the shared module docstring and reflect the
resource used by the connectivity check.
- Around line 49-59: The Preconditions docstring in test_nad_ref_change.py is
missing the target reference VM for the first secondary interface; update the
docstring block labeled "Preconditions" to include a line mirroring the second
interface's target VM entry (e.g., "Running reference VM on the target VLAN of
the first secondary interface") so the declared preconditions match the
assertions in the test (see the test's target-VLAN connectivity assertion for
the first secondary interface).
- Around line 81-83: The test Preconditions block in
tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py must explicitly
list the target reference VM as a directly-used shared resource; update the
"Preconditions:" section (the block shown around the existing bridge-bound
secondary interface and initial VLAN) to include the target VLAN reference VM so
it matches the later assertion at line ~90 that the VM has no connectivity to
the target VLAN. Ensure the added item names the same test fixture/VM identifier
used elsewhere in the file (the target reference VM variable/name) so reviewers
can see it is a directly-used shared resource.
- Around line 61-68: The test currently updates both secondary networks at once
in test_nad_ref_change.py which masks ordering bugs; change the test to perform
per-interface updates instead: first update the first secondary interface to its
target NAD using the same helper(s) currently used for updates (e.g., the code
that performs the simultaneous NAD update), then assert the under-test VM loses
TCP connectivity on the original VLAN and gains connectivity on the target VLAN
for that first interface before proceeding; next, perform the same steps for the
second secondary interface (update its NAD, assert loss of connectivity on its
initial VLAN, then assert connectivity on its target VLAN). Ensure you reuse the
existing VM/network helpers in the test and add explicit connectivity assertions
between each per-interface update rather than doing a combined update.
In `@tests/network/localnet/nad_ref_change/test_nad_ref_change.py`:
- Around line 22-25: The test-level Preconditions in
tests/network/localnet/nad_ref_change/test_nad_ref_change.py currently list only
the initial VLAN resources (initial NAD and bridge) but the test body references
the target VLAN reference VM (used around the assertion at the comment near line
31); update the test's Preconditions block to also name/describe the target
reference VM (the VM on the target VLAN) so the docstring mirrors the actual
resources used by the test (repeat the shared resource entry for the target VLAN
reference VM alongside the existing initial VLAN entries).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad85afba-6ae8-42c9-af89-b0084a7cff1d
📒 Files selected for processing (4)
tests/network/l2_bridge/nad_ref_change/__init__.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/network/localnet/nad_ref_change/__init__.pytests/network/localnet/nad_ref_change/test_nad_ref_change.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
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
8 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.
STD for the tests of the live NAD reference update feature, which allows VM owners to change the secondary network of a running VM without rebooting and without any change to the guest interface.
The module-level preconditions capture the shared setup for that CNI type, including the bridge/localnet NADs and the reference VMs needed to verify connectivity.
jira-ticket: https://redhat.atlassian.net/browse/CNV-78930
Summary by CodeRabbit