STP for Live Update NetworkAttachmentDefinition Reference (VEP 140)#39
STP for Live Update NetworkAttachmentDefinition Reference (VEP 140)#39yossisegev wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
WalkthroughA new test plan document was added for the Live Update NetworkAttachmentDefinition Reference (Hotpluggable NAD Ref) feature, outlining QE strategy, detailed test scenarios, environment/tooling requirements, risk assessment, and requirements-to-test traceability for live-updating a VM's NAD reference. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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
💡 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: 2
🤖 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-stp.md`:
- Line 32: Fix the typos across the document by replacing the incorrect tokens
with the corrected spellings: change "NAd reference" to "NAD reference",
"dependecny" -> "dependency", "implmenetation" -> "implementation", "backgrond"
-> "background", "letncy cannot be measured,for example" -> "latency cannot be
measured, for example", "evetually" -> "eventually", "is mot affected" -> "is
not affected", "dependancy" -> "dependency", "migartion" -> "migration",
"refelcted" -> "reflected", "environemnt" -> "environment", and "migratin" ->
"migration"; ensure you update each occurrence of the quoted phrases (e.g., the
header/text containing "NAd reference", the paragraphs with
"dependecny"/"implmenetation", the sentence with "letncy...", etc.) so the
document reads correctly and consistently.
- Around line 156-162: Update the traceability table to add a Requirement row
for the "P1 feature-gate disabled behavior" mapping the in-scope item "Verify
behavior when LiveUpdateNADRef feature-gate is disabled (attempting to change
NAD reference produces failure/error)" to a Test Scenario that attempts a
networkName update with the feature-gate off and asserts failure/error, and add
another Requirement row for "MAC preservation" mapping the in-scope item
"Verifying that guest interface identity (e.g. MAC) is preserved where
applicable" to a Test Scenario that changes NAD reference and verifies the guest
MAC is unchanged; also update the existing VEP-140 (NFR) regression row to
explicitly mention "interface hot-plug regression" in the Requirement
Summary/Test Scenario to reflect the specific regression concern named elsewhere
in the document.
frenzyfriday
left a comment
frenzyfriday
left a comment
There was a problem hiding this comment.
Thanks for the STP! Lgtm in general, just a few questions/comments
| | Check | Done | Details/Notes | Comments | | ||
| |:---------------------------------------|:-----|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:---------| | ||
| | **Review Requirements** | [V] | VEP #140: Change NAD reference on running VM via migration; bridge binding; no RestartRequired when only networkName (i.e. the NAd reference) changes; migration triggered when networkName changes. | | | ||
| | **Understand Value** | [V] | VM owners can re-assign new networks (e.g. a different VLAN) to VMs without reboot, avoiding workload impact and preserving guest interface identity (e.g. MAC). | | |
There was a problem hiding this comment.
VM owners can re-assign new networks
new networks but of the same binding type (but maybe that is already clear)
There was a problem hiding this comment.
@frenzyfriday Your input is correct, but I think it's too detailed to be specified here. WDYT?
| |:---------------------------------|:-----|:------------------------------------------------------------------------------------------------------------------------------------------|:---------| | ||
| | **Developer Handoff/QE Kickoff** | [V] | Meeting where Dev introduced QE through NAD change in VM, migration condition handling, and DNC compatibility (Option 1 vs 2). | | | ||
| | **Technology Challenges** | [V] | Requires live-migratable VM and bridge binding; multus secondary networks. | | | ||
| | **Test Environment Needs** | [V] | Cluster nodes with secondary interfaces, bridge binding, live migration enabled, NMState operator; LiveUpdateNADRef feature-gate enabled. | | |
There was a problem hiding this comment.
Is it worth mentioning that it needs to be run on a cluster with >=2 nodes (because it will be migrating)? I mention this because we added a similar decorator in the T1 tests, not sure that is applicable here though
There was a problem hiding this comment.
Such decorator is applicable in pytest, but I consider it as an implementation detail that doesn't belong here; the mentioning of "live migration enabled" covers this requirement.
WDYT?
|
|
||
| **Testing Goals:** | ||
| - **[P0]** Change NAD reference on a running VM (bridge binding) and verify the VM is connected to the new network and has connectivity (E2E). | ||
| - **[P1]** Verify behavior when LiveUpdateNADRef feature-gate is disabled (e.g. attempting to change NAD reference produces failure/error). |
There was a problem hiding this comment.
With FG off it just adds a restartRequired condition to the VM I think
There was a problem hiding this comment.
Again - I think that this is an implementation detail, which is out-of-scope here.
And if we test it without the FG and without actively restarting the VM then the test will fail, which is what we want. It is explicitly defined in the user story in the VEP:
As a VM owner, I should be able to re-assign VMs to a new VLAN ID, without having to reboot my VMs.
WDYT?
There was a problem hiding this comment.
ah yes! I thought for some reason the VM would fail 🤦🏽♀️ . Yeah the test would fail correct.
| | Seamless network connectivity until action is completed | Not guaranteed by design | [ ] | | ||
| | Changing NAD reference on non-migratable VMs | Not supported | [ ] | | ||
| | Changing guest network configuration | Not in scope; guest may need separate reconfig | [ ] | | ||
| | In-place NAD swap with Dynamic Networks Controller | User's scenarios are oblivious to the way DNC is interacted | [ ] | |
There was a problem hiding this comment.
User's scenarios are oblivious to the way DNC is interacted
Yes, but as per the current implementation In-place NAD swap (without migration) will not happen both with/without DNC
There was a problem hiding this comment.
Actually I think that referring to the DNC method that is used is just confusing.
I am removing this line from the table, let me know what you think.
|
|
||
| | Check | Done | Details/Notes | Comments | | ||
| |:---------------------------------------|:-----|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:---------| | ||
| | **Review Requirements** | [V] | VEP #140: Change NAD reference on running VM via migration; bridge binding; no RestartRequired when only networkName (i.e. the NAd reference) changes; migration triggered when networkName changes. | | |
There was a problem hiding this comment.
In all relevant columns need to check [x] instead of [V]
There was a problem hiding this comment.
@azhivovk I am not sure that this is true (if there is an explicit requirement to mark these boxes only with "x" then please refer me to it), but anyway I changed it per your comment.
| | **Review Requirements** | [V] | VEP #140: Change NAD reference on running VM via migration; bridge binding; no RestartRequired when only networkName (i.e. the NAd reference) changes; migration triggered when networkName changes. | | | ||
| | **Understand Value** | [V] | VM owners can re-assign new networks (e.g. a different VLAN) to VMs without reboot, avoiding workload impact and preserving guest interface identity (e.g. MAC). | | | ||
| | **Customer Use Cases** | [V] | As a VM owner, I should be able to re-assign VMs to a new VLAN ID without having to reboot my VMs. | | | ||
| | **Testability** | [V] | Core flow is testable: Update networkName on VM → wait for completion (mostly for the migration in the background) → verify VM is connected to the new network and has connectivity. Non-goals (e.g., non-migratable VMs, CNI type change) are explicit. | | |
There was a problem hiding this comment.
Need to rephrase this because you already mention it in the first test case
There was a problem hiding this comment.
I am not sure I understand what you are asking for.
Anyway I changed the contents of the "Review Requirement" entry, please see if it make more sense to you now.
There was a problem hiding this comment.
The view here is confusing so my comment was not clear, sorry
I meant that in Testability you described what's already described in the first test case under ### III. Test Scenarios & Traceability
| - **[P0]** Change NAD reference on a running VM (bridge binding) and verify the VM is connected to the new network and has connectivity (E2E). | ||
| - **[P1]** Verify behavior when LiveUpdateNADRef feature-gate is disabled (e.g. attempting to change NAD reference produces failure/error). | ||
| - **[P1]** Regression: existing secondary network and migration flows unchanged when not using this feature, including interface hot-plug. | ||
|
|
There was a problem hiding this comment.
Can we consider two additional cases:
- Negative scenario - when hot-swapping a VM's network, verify that the old network has no connectivity in the VM (e.g., ping to a target on the old network fails).
- After NAD hot-swap, verify that the VM can be live-migrated successfully and that connectivity on the new network is preserved (same as for any VM on that network)
There was a problem hiding this comment.
- I consider this scenario as unexpected behavior, and it should not be tested.
- Isn't it what I specified in the P0 entry?
WDYT?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
stps/sig-network/hotpluggable-nad-ref-stp.md (1)
91-92:⚠️ Potential issue | 🟡 MinorFix remaining spelling errors in strategy table entries.
There are still typos in these rows (
exisiting,reqiurement,byt,hance). Please correct them to avoid ambiguity in a formal STP.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/hotpluggable-nad-ref-stp.md` around lines 91 - 92, Fix typos in the strategy table entries: in the row that begins with "Dependencies" replace "exisiting" with "existing" and "reqiurement" with "requirement" (the cell containing "KubeVirt virt-controller; Multus (just exisiting functionality, no new reqiurement); optional DNC"), and in the "Cross Integrations" row replace "byt" with "by" and "hance" with "hence" (the cell containing "Bridge binding in VMs is tested byt network SIG, hance changes in bridge binding are also under the network sig testing domain."); update only those words so the rest of the phrasing remains unchanged.
🧹 Nitpick comments (2)
stps/sig-network/hotpluggable-nad-ref-stp.md (2)
155-161: Optionally reduce Requirement ID repetition in traceability rows.For rows under the same epic, consider keeping the first Requirement ID populated and leaving subsequent Requirement ID cells empty to reduce redundancy.
Based on learnings: In the openshift-virtualization-tests-design-docs repository, when documenting test scenarios in the "Test Scenarios & Traceability" table where multiple test scenarios fall under the same epic, it's acceptable and preferred to leave the Requirement ID cells empty for subsequent rows after the first row which contains the epic ID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/hotpluggable-nad-ref-stp.md` around lines 155 - 161, The table repeats the same Requirement ID "VEP-140" across multiple rows; update the "Test Scenarios & Traceability" table so only the first row for the epic contains "VEP-140" in the Requirement ID column and remove (empty) the Requirement ID cells for the subsequent rows that belong to the same epic (rows containing the summaries "Change NAD reference on running VM...", "Migration triggered and completes...", "RestartRequired not set...", "No regression in secondary network...", and "Guest interface identity preserved...") to reduce redundancy while keeping the Test Scenario(s), Tier, and Priority columns unchanged.
102-102: Consider using version-range wording for long-lived STP clarity.Instead of a single fixed version (
4.22), consider phrasing as4.22 onward(and optionally note backportability if applicable) to keep this plan accurate across releases.Based on learnings: In the openshift-virtualization-tests-design-docs repository, when documenting "OCP & OpenShift Virtualization Version(s)" in STP test environment tables, features may specify "v4.X onward" while also noting "Can be backported to all versions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/hotpluggable-nad-ref-stp.md` at line 102, Update the version wording in the STP table entry for "OCP & OpenShift Virtualization Version(s)" where "4.22" is currently used for the LiveUpdateNADRef feature: replace the fixed "4.22" with a range-style phrase such as "4.22 onward" and optionally append a short parenthetical note like "(can be backported where applicable)" next to the LiveUpdateNADRef mention to indicate long-lived applicability and backportability.
🤖 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-stp.md`:
- Line 64: The P2 goal "Verify behavior when LiveUpdateNADRef feature-gate is
disabled (e.g. attempting to change NAD reference produces failure/error)" is
not present in the traceability table; either add a corresponding mapping row in
the traceability table that references this exact P2 goal and points to the
related test cases/requirements, or remove that P2 goal line entirely if it is
out of scope; update the traceability table entries that enumerate
goals-to-tests so the P2 feature-gate-disabled scenario is consistently
represented (or removed) throughout the document.
---
Duplicate comments:
In `@stps/sig-network/hotpluggable-nad-ref-stp.md`:
- Around line 91-92: Fix typos in the strategy table entries: in the row that
begins with "Dependencies" replace "exisiting" with "existing" and "reqiurement"
with "requirement" (the cell containing "KubeVirt virt-controller; Multus (just
exisiting functionality, no new reqiurement); optional DNC"), and in the "Cross
Integrations" row replace "byt" with "by" and "hance" with "hence" (the cell
containing "Bridge binding in VMs is tested byt network SIG, hance changes in
bridge binding are also under the network sig testing domain."); update only
those words so the rest of the phrasing remains unchanged.
---
Nitpick comments:
In `@stps/sig-network/hotpluggable-nad-ref-stp.md`:
- Around line 155-161: The table repeats the same Requirement ID "VEP-140"
across multiple rows; update the "Test Scenarios & Traceability" table so only
the first row for the epic contains "VEP-140" in the Requirement ID column and
remove (empty) the Requirement ID cells for the subsequent rows that belong to
the same epic (rows containing the summaries "Change NAD reference on running
VM...", "Migration triggered and completes...", "RestartRequired not set...",
"No regression in secondary network...", and "Guest interface identity
preserved...") to reduce redundancy while keeping the Test Scenario(s), Tier,
and Priority columns unchanged.
- Line 102: Update the version wording in the STP table entry for "OCP &
OpenShift Virtualization Version(s)" where "4.22" is currently used for the
LiveUpdateNADRef feature: replace the fixed "4.22" with a range-style phrase
such as "4.22 onward" and optionally append a short parenthetical note like
"(can be backported where applicable)" next to the LiveUpdateNADRef mention to
indicate long-lived applicability and backportability.
| **Testing Goals:** | ||
| - **[P0]** Change NAD reference on a running VM (bridge binding) and verify the VM is connected to the new network and has connectivity (E2E). | ||
| - **[P1]** Regression: existing secondary network and migration flows unchanged when not using this feature, including interface hot-plug. | ||
| - **[P2]** Verify behavior when LiveUpdateNADRef feature-gate is disabled (e.g. attempting to change NAD reference produces failure/error). |
There was a problem hiding this comment.
Align testing goals with traceability mapping.
Line 64 defines a P2 feature-gate-disabled goal, but the traceability table does not map that scenario. Please either add a corresponding row in Lines 155-162 or remove the goal if intentionally out of scope.
Also applies to: 155-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stps/sig-network/hotpluggable-nad-ref-stp.md` at line 64, The P2 goal "Verify
behavior when LiveUpdateNADRef feature-gate is disabled (e.g. attempting to
change NAD reference produces failure/error)" is not present in the traceability
table; either add a corresponding mapping row in the traceability table that
references this exact P2 goal and points to the related test cases/requirements,
or remove that P2 goal line entirely if it is out of scope; update the
traceability table entries that enumerate goals-to-tests so the P2
feature-gate-disabled scenario is consistently represented (or removed)
throughout the document.
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.
Special notes for your reviewer
Summary by CodeRabbit