Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new OpenShift Virtualization software test plan document for the "Live Update NetworkAttachmentDefinition Reference" initiative. The document defines metadata, terminology, feature behavior, QE checklists, prioritized test goals, environment prerequisites, entry criteria, risks/limitations, traceable test scenarios (VIRTSTRAT-560), and approvals. (301 added lines) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@stps/sig-network/hotpluggable-nad-ref.md`:
- Around line 14-17: Remove the standard term definition "VEP = KubeVirt
Enhancement Proposal" from the "Document Conventions" section and leave only
feature-specific abbreviations (keep "NAD = NetworkAttachmentDefinition" and
"DNC = Dynamic Networks Controller"); update the "Document Conventions"
paragraph to define only NAD and DNC and remove the VEP line so the section
contains feature-specific terms only.
- Around line 97-99: Update the "Topology Considerations" section to explicitly
state cluster requirements to match the Technology Challenges and STP I.3
guidance: replace the "No known limitations when considering different
topologies" line with a clear statement that the feature requires a multi-node
cluster with live-migration support (CRI/migration features enabled), explicitly
excludes single-node clusters, and describe the testing impact (tests must run
on clusters with live migration enabled, sufficient nodes/resources for live
migration, and any shared storage or networking requirements). Ensure the
phrasing references the same feature constraints described in "Technology
Challenges" so the document is consistent.
- Line 10: The QE Owner entry "**QE Owner(s):** Asia Khromov" is missing the
required GitHub handle; update this metadata line to include the QE's GitHub
handle (e.g., change the "**QE Owner(s):** Asia Khromov" entry to include the
handle in parentheses or adjacent text such as "**QE Owner(s):** Asia Khromov
(`@github_handle`)") so the QE Owner(s) metadata contains both name and contact
information for traceability.
- Around line 68-78: Move the three items currently labeled "Explicit non-goal"
out of the Known Limitations section (I.2) and add them to the Out of Scope
(Testing Scope Exclusions) section (II.1): "Migrating between CNI types",
"Changing network binding/plugin", and "No limitation of migration retries
because of missing Network Attachment Definition"; for each, remove the bullet
from I.2 and create corresponding bullets under II.1 with a short rationale
"Explicit non-goal of the feature per VEP design" and the sign-off "Ronen
Sde-Or, 04/2026" (use the exact phrasing of each title to locate the items).
- Around line 41-43: The Acceptance Criteria currently has a single checklist
item; split it into multiple independent, verifiable list items under the
"Acceptance Criteria" header so each is a specific pass/fail condition. Replace
the single line "- *List the acceptance criteria:* Change the secondary network
reference on a running VM and verify the VM is connected to the new network and
has connectivity." with separate bullets such as verifying the reference change
applied, confirming the VM is attached to the new network, validating network
connectivity from the VM to an external endpoint, confirming no disruption to
primary network, and documenting rollback steps; ensure each new item is phrased
as an observable outcome and can be tested independently (search for the
"Acceptance Criteria" section / the existing checklist entry to update).
🪄 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: 5354581e-a937-452c-985d-1031989697cc
📒 Files selected for processing (1)
stps/sig-network/hotpluggable-nad-ref.md
azhivovk
force-pushed
the
nad_change_stp
branch
from
9d41a3f to
db9fff3
Compare
|
Change: apply bot's suggestions |
|
@azhivovk it looks like it's worth adding the devs from your team to the OWNER list. Consider, please. |
| NAD = NetworkAttachmentDefinition, a Multus secondary network definition | ||
| DNC = Dynamic Networks Controller (Multus dynamic networks controller) |
There was a problem hiding this comment.
It looks like the 2 lines are supposed to be separated, but in preview it turns out
Document Conventions (if applicable): NAD = NetworkAttachmentDefinition, a Multus secondary network definition DNC = Dynamic Networks Controller (Multus dynamic networks controller)
Please search how to do that in markdown (
tag, double-space at the line-end, etc.).
Also note that this occurs in other places in the doc as well.
| ### **Feature Overview** | ||
|
|
||
| This feature enables VM owners to change the secondary network of a running VM without rebooting. When the user updates the network reference in the VM configuration, the VM is reconnected to the new network (e.g., a different VLAN) without requiring a restart, preserving guest interface identity. | ||
| The supported scope is bridge binding on live-migratable VMs. |
| - The VM remains running (not rebooted) after the NAD reference update | ||
| - The VM establishes connectivity on the new secondary network | ||
| - The VM no longer has connectivity on the previous secondary network | ||
| - The guest interface identity (e.g., MAC address) is preserved |
|
|
||
| ### **Feature Overview** | ||
|
|
||
| This feature enables VM owners to change the secondary network of a running VM without rebooting. When the user updates the network reference in the VM configuration, the VM is reconnected to the new network (e.g., a different VLAN) without requiring a restart, preserving guest interface identity. |
There was a problem hiding this comment.
This overview doesn't mention the required migration.
On one hand it makes sense, because the migration part is an implementation detail; but OTOH, you do mention the required migration in other places in the STP (in the "Known Limitations", for example), so maybe it makes sense to mention it here, for example
| This feature enables VM owners to change the secondary network of a running VM without rebooting. When the user updates the network reference in the VM configuration, the VM is reconnected to the new network (e.g., a different VLAN) without requiring a restart, preserving guest interface identity. | |
| ..., the VM is reconnected to the new network (e.g., a different VLAN) without requiring a restart (but rather migrating the VM), preserving guest interface identity. | |
| sdfd |
@frenzyfriday WDYT?
There was a problem hiding this comment.
yes, it makes sense to add the migration here. Though it is an implementation detail, the user right now has no other way to monitor the change than to check for the migration criteria
There was a problem hiding this comment.
You are right, it's important to mention it in the overview since we mentioned the migration limitations
|
|
||
| #### **3. Test Environment** | ||
|
|
||
| - **Cluster Topology:** 3-master/3-worker bare-metal (minimum 2 workers) |
There was a problem hiding this comment.
IIUC, it's the default cluster setup we mention in the STP
There was a problem hiding this comment.
@rnetser is this true?
And if so - why? We shouldn't be limited only to BM IMO.
|
|
||
| - **OCP & OpenShift Virtualization Version(s):** OCP 4.22 with OpenShift Virtualization version 4.22 | ||
|
|
||
| - **CPU Virtualization:** VT-x (Intel) or AMD-V enabled |
There was a problem hiding this comment.
So it won't be supported on ARM-based nodes?
There was a problem hiding this comment.
IIUC it's the equivalent of agnostic, as written in the stp template:
- **CPU Virtualization:** VT-x (Intel) or AMD-V enabled
<!-- Change only if specific CPU requirements exist -->
There was a problem hiding this comment.
I think that the template's default corresponds to the current RHEL nodes we have, which use AMD.
Now that multi-arch is about to be supported, I think that this section should also mention ARM.
|
|
||
| - **Required Operators:** NMState Operator (for bridge interface configuration) | ||
|
|
||
| - **Platform:** Bare metal |
| - *Details:* Not applicable; feature requires bare-metal cluster with live migration support. | ||
|
|
||
| #### **3. Test Environment** | ||
|
|
There was a problem hiding this comment.
If the feature is limited only to bridge-binding, as specified in the "Test Coverage" section below, then I think that multi-NIC nodes should also be mentioned here as a test environment requirement.
There was a problem hiding this comment.
I mentioned it in
- **Network:** OVN-Kubernetes, IPv4, IPv6, secondary bridge network
Do I need to mention it in more places? Cluster topology maybe?
There was a problem hiding this comment.
My opinion is that you should explicitly mention that multi NIC nodes are required, either in network or cluster topology.
However, maybe you think that this mention is clear and enough, and in that case - I won't insist.
|
|
||
| - [x] **Risk:** Seamless network connectivity during the NAD swap is not guaranteed by design and cannot be tested as a pass/fail condition. | ||
| - **Mitigation:** Tests will verify connectivity before and after the swap; transient disruption during swap is accepted. | ||
| - *Alternative validation approach:* Verify connectivity is restored on new network after swap completes. |
There was a problem hiding this comment.
Please be clear here, on whether the connectivity is expected to be seamlessly restored (meaning - a connection that was established before the NAD swap will disrupted during the change, and will eventually be restored without the user having to actively restore it), or rather the user will have to actively renew this connection.
If I understand the feature and its VEP correctly, then it's the second option.
There was a problem hiding this comment.
IIUC according to the VEP, the connectivity restoration is not seamlessly maintained and if the connectivity was not preserved after the NAD update, the user needs to manually renew the connections.
I added it here, thanks
| - Security: no new RBAC or authentication changes. | ||
| - Monitoring/Observability: no new metrics or alerts required. | ||
| - Scalability: no new scale requirements. | ||
| - UI: no UI changes introduced by this feature. |
There was a problem hiding this comment.
Yes, as of now. i will talk to the UI team to get their opinions.
| - *Test Scenario:* [Tier 2] Verify that a VM can be successfully live-migrated after completing a NAD reference update, and maintains connectivity on the new network. | ||
| - *Priority:* P2 | ||
|
|
||
| - **[VIRTSTRAT-560]** — As a VM owner, I want my VM to remain connected to its updated secondary network after a cluster upgrade. |
There was a problem hiding this comment.
The implementation did not add a breaking change. So is the cluster upgrade test necessary still?
There was a problem hiding this comment.
To me it sounds like a scenario we should test, but maybe I'm wrong
@EdDev WDYT?
|
|
||
| --- | ||
|
|
||
| ### **III. Test Scenarios & Traceability** |
There was a problem hiding this comment.
Should we add a Ipv6 scenario somewhere? And should we add a test for VLAN change explicitly?
There was a problem hiding this comment.
IPv6 is covered as a subtest within the existing scenarios, not a separate user story.
I plan to follow the way we create new tests: setup the VM networks according to existing cluster network stack - if we run on dual-stack cluster we test both IP families and if we run on single-stack cluster, we test accordingly.
Regarding the VLAN change - I edited the first test scenario
azhivovk
force-pushed
the
nad_change_stp
branch
from
db9fff3 to
4a38937
Compare
|
STP for covering updating network (NAD) reference on live VMs. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Co-Authored-By: Yossi Segev <ysegev@redhat.com>
azhivovk
force-pushed
the
nad_change_stp
branch
from
4a38937 to
9c490d0
Compare
openshift-virtualization-qe-bot
added
size/XL
and removed
lgtm-coderabbitai[bot]
size/L
labels
|
Change: add localnet scenario since bridge binding is used |
| - *Details:* Not applicable; feature requires bare-metal cluster with live migration support. | ||
|
|
||
| #### **3. Test Environment** | ||
|
|
There was a problem hiding this comment.
My opinion is that you should explicitly mention that multi NIC nodes are required, either in network or cluster topology.
However, maybe you think that this mention is clear and enough, and in that case - I won't insist.
|
|
||
| #### **3. Test Environment** | ||
|
|
||
| - **Cluster Topology:** 3-master/3-worker bare-metal (minimum 2 workers) |
There was a problem hiding this comment.
@rnetser is this true?
And if so - why? We shouldn't be limited only to BM IMO.
|
|
||
| - **OCP & OpenShift Virtualization Version(s):** OCP 4.22 with OpenShift Virtualization version 4.22 | ||
|
|
||
| - **CPU Virtualization:** VT-x (Intel) or AMD-V enabled |
There was a problem hiding this comment.
I think that the template's default corresponds to the current RHEL nodes we have, which use AMD.
Now that multi-arch is about to be supported, I think that this section should also mention ARM.
9 participants
STP Metadata
VEP issue: https://github.com/kubevirt/enhancements/blob/main/veps/sig-network/hotpluggable-nad-ref.md
What this PR does
STP for covering updating network (NAD) reference on live VMs.
This feature enables VM owners to change the secondary network of a running VM without rebooting. When the user updates the network reference in the VM configuration, the VM is reconnected to the new network (e.g., a different VLAN) without requiring a restart, preserving guest interface identity.
The supported scope is bridge binding on live-migratable VMs.
Summary by CodeRabbit