net: Nad live-update non-migratable VM test#5212
Open
azhivovk wants to merge 2 commits into
Open
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? |
Contributor
📝 WalkthroughWalkthroughAdds ChangesNon-migratable VM NAD reference change test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 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
openshift-virtualization-qe-bot-6
requested review from
nirdothan,
rnetser and
servolkov
azhivovk
changed the title
openshift-virtualization-qe-bot-6
requested review from
RoniKishner,
frenzyfriday,
orelmisan and
yossisegev
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![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: 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 `@libs/vm/factory.py`:
- Around line 44-47: The condition on line 44 incorrectly suppresses the default
containerdisk insertion whenever any DataVolume is present, breaking the ability
to use DataVolumes as additional disks alongside a boot disk. Replace the logic
that checks for any DataVolume with a check that determines whether a boot disk
or containerdisk already exists. The containerdisk should be inserted by default
unless a boot disk is already present, allowing DataVolumes to coexist as
additional disks. Modify the condition in the containerdisk_storage function
call to verify the absence of an actual boot disk rather than the absence of any
DataVolume.
In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py`:
- Around line 244-248: The wait_for_vmi_condition_status call needs to be
anchored to the pre-patch resource state to ensure it validates the
RestartRequired condition is set as a result of the current patch operation, not
from a stale previous state. Capture the resourceVersion of
non_migratable_under_test_vm before calling patch_nad_references, then pass this
resourceVersion to the wait_for_vmi_condition_status call so it waits for a
condition transition that occurs after the patch, not a pre-existing condition
state.
In `@tests/network/libs/nad_ref.py`:
- Around line 27-40: The patch_nad_references function contains duplicate
network mutation logic that is already present in update_nad_references,
specifically the block that iterates through networks and updates the
multus.networkName based on the nad_name_by_net mapping. Extract this common
logic (the deepcopy, the for loop iterating through networks, and the
conditional update of network.multus.networkName) into a separate shared helper
function, then refactor both patch_nad_references and update_nad_references to
call this new helper function instead of duplicating the code.
🪄 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: 70873300-8c28-46c1-a952-0a736d2105fb
📒 Files selected for processing (6)
libs/vm/factory.pylibs/vm/spec.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/network/libs/nad_ref.py
Add DataVolumeRef dataclass and dataVolume field to Volume so VM factories can reference DataVolume-backed disks. Add dataVolumeTemplates to VMSpec so a VM can inline DataVolume templates at spec level, enabling ReadWriteOnce-backed VMs that are not live-migratable. Guard containerdisk insertion in _fill_vm_spec_defaults so it is skipped when the spec already carries a DataVolume volume, preventing a duplicate boot disk. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
azhivovk
force-pushed
the
nad-ref-non-migratable
branch
from
524eef8 to
3ab53f3
Compare
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
Contributor
Author
|
Change: apply bot's suggestions |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/nad_ref_change/lib_helpers.py`:
- Around line 88-89: The data_volume_template parameter currently accepts raw
dict objects, which bypasses typed resource composition and is brittle for
schema drift. Replace the dict[str, Any] type annotation and contract to use a
structured, typed builder or ocp-resources class instead. Identify or create an
appropriate typed class from the ocp-resources library that represents the data
volume template structure, update the parameter type accordingly, and ensure
callers pass structured instances instead of raw YAML-like dictionaries. This
centralizes serialization logic and maintains type safety across the API
contract.
- Around line 135-136: Replace the truthiness checks with explicit `is not None`
checks to ensure fail-fast behavior on malformed input. At line 135, change the
condition that checks `data_volume_template` from a truthiness check to
`data_volume_template is not None`. Apply the same change at line 144 for the
analogous variable (likely `vm_template` based on the context), replacing its
truthiness check with an explicit `is not None` comparison. This prevents empty
dictionaries from being silently skipped and ensures that malformed input fails
loudly as intended by the coding guidelines.
🪄 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: cec88a75-1765-44d8-b547-12f714e844a2
📒 Files selected for processing (6)
libs/vm/factory.pylibs/vm/spec.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/network/libs/nad_ref.py
jpeimer
reviewed
Jun 14, 2026
Implement test_non_migratable_vm_nad_change_not_applied (CNV-15947): verifies that updating the NAD reference on a non-migratable VM does not apply live — the VM reports a RestartRequired condition and retains connectivity to the original VLAN-A. A blank 20Gi RWO DataVolume is attached via two_secondary_bridge_vm's new data_volume_template parameter, making the VM non-live-migratable. Connectivity to VLAN-A is verified as a fixture precondition. Quarantined via CNV-87878: the RestartRequired condition is not yet wired up in KubeVirt — the wait_for_condition call will be unblocked when the product fix lands. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
azhivovk
force-pushed
the
nad-ref-non-migratable
branch
from
3ab53f3 to
1cf99bd
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Contributor
Author
|
Change: apply requested DV changes after CR |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
Contributor
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 `@tests/network/l2_bridge/nad_ref_change/conftest.py`:
- Around line 191-219: The non_migratable_baseline_connectivity fixture
duplicates connectivity verification logic from another fixture, with only the
client VM parameter differing. Extract the duplicated logic (the two
filter_link_local_addresses and poll_tcp_connectivity loops that verify baseline
connectivity) into a shared helper function called assert_baseline_connectivity
in lib_helpers.py that accepts client_vm and ref_vm as parameters. Then refactor
the non_migratable_baseline_connectivity fixture to call this helper function
instead of containing the inline logic. Also refactor the original baseline
connectivity fixture at lines 122-149 to use the same extracted helper function
to eliminate duplication and ensure a single maintenance path.
🪄 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: 95eb4872-74f4-4318-8741-5b24f85a610a
📒 Files selected for processing (4)
tests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/network/libs/nad_ref.py
Contributor
|
/lgtm |
frenzyfriday
approved these changes
Jun 17, 2026
frenzyfriday
left a comment
frenzyfriday
left a comment
Contributor
There was a problem hiding this comment.
/lgtm thanks!
The comments that I have are nits
| ) | ||
| blank_dv.to_dict() | ||
| dv_template = blank_dv.res | ||
| with two_secondary_bridge_vm( |
Contributor
There was a problem hiding this comment.
[nit]: Is there a fixture for creating secondary bridge vm with one secondary nw? The name two_secondary_bridge_vm confused me initially a bit because this VM has only one secondary iface. maybe it would be better to use a different fixture that creates a VM with 1 secondary net or rename the two_secondary_bridge_vm to something like one_or_more_secondary_bridge_vm?
Contributor
Author
There was a problem hiding this comment.
I can change it to use secondary_network_vm under tests/network/l2_bridge/libl2bridge.py which has 1 secondary Linux bridge network
I implemented it like this since two_secondary_bridge_vm already has runcmd which is required for the arp ignore commands
| ip_addresses=lookup_iface_status(vm=ref_vm, iface_name=LINUX_BRIDGE_IFACE_NAME_1).ipAddresses | ||
| ): | ||
| with subtests.test(msg=f"IPv{server_ip.version} on {bridge_nad_a.name}"): | ||
| assert_connectivity( |
Contributor
There was a problem hiding this comment.
[question]: Why does the non_migratable_baseline_connectivity use poll_tcp_connectivity but the test use assert_connectivity ?
Contributor
Author
There was a problem hiding this comment.
assert is for usage in test only and we use this helper to align with the existing tests
This was requested when the existing NAD live update tests merged, for clearance about the test flow
I used the polling helper since it's part of the setup
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4810. Overlapping fileslibs/vm/factory.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
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.
What this PR does / why we need it:
Adds
test_non_migratable_vm_nad_change_not_applied: verifies that updating the NAD reference on a non-migratable VM does not apply live and the VM reports aRestartRequiredcondition and retains connectivity to the original VLAN-A.To support this, the PR also:
MigrationRequiredWhich issue(s) this PR fixes: -
Special notes for reviewer:
The test is quarantined via @pytest.mark.jira("CNV-87878", run=False): the
RestartRequiredcondition is not yet wired up in KubeVirt.jira-ticket: https://redhat.atlassian.net/browse/CNV-89745
Summary by CodeRabbit
Release Notes
New Features
Tests