Conversation
|
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:
WalkthroughIntroduces a new Software Test Plan (STP) for VM integration testing of BGP and EVPN with UDN in OpenShift/Kubernetes OVN-K, covering metadata, requirements, design review, test strategy, environments, scenarios, acceptance criteria, risks, limitations, and governance. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. |
openshift-virtualization-qe-bot-3
requested review from
azhivovk and
yossisegev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
stps/sig-network/EVPN.md (1)
17-33: Consider hyphenating compound adjectives for consistency.Multiple instances of compound adjectives could be hyphenated for better readability:
- Line 18: "User Defined Network" → "User-Defined Network"
- Line 18: "OVN-K based network type" → "OVN-K-based network type"
- Line 30: "User Defined Networks" → "User-Defined Networks"
While these are stylistic improvements, they enhance readability when the compound adjective modifies a noun.
📝 Proposed hyphenation improvements
-**Document Conventions:** -- UDN: User Defined Network (OVN-K based network type). +**Document Conventions:** +- UDN: User-Defined Network (OVN-K-based network type).-From the OCP-V perspective this feature enables OpenShift Virtualization VMs connected to primary OVN-Kubernetes -User Defined Networks (UDNs) to participate in an EVPN fabric. +From the OCP-V perspective this feature enables OpenShift Virtualization VMs connected to primary OVN-Kubernetes +User-Defined Networks (UDNs) to participate in an EVPN fabric.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/EVPN.md` around lines 17 - 33, Hyphenate the compound adjectives in EVPN.md for consistency: change "User Defined Network" to "User-Defined Network" (both the singular instance and the plural "User Defined Networks"), and change "OVN-K based network type" to "OVN-K-based network type"; update the occurrences in the "Document Conventions" section and the "Feature Overview" paragraph where these phrases appear (search for the exact strings "User Defined Network", "User Defined Networks", and "OVN-K based network type" to locate and replace).
🤖 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/EVPN.md`:
- Around line 141-142: Replace the typos in the Risks table entries: change
"collobaration" to "collaboration" in the Timeline/Schedule row and change
"single-stuck" to "single-stack" in the Test Coverage row so terminology matches
the rest of the document (see the "Timeline/Schedule" and "Test Coverage" table
cells in the shown diff).
- Line 97: Fix the typo in the table cell for the "Cloud Testing" row: change
"Bare Metal environment is requried." to "Bare Metal environment is required." —
update the EVPN.md markdown content where the Cloud Testing table row text
contains "requried".
- Line 114: Update the typo in EVPN.md by replacing "infrustructure" with the
correct spelling "infrastructure" in the table row containing "**Special
Configurations** | Source Provider Virtualization Platform | Should be
configured next to CNV QE bare metal infrustructure"; ensure the table cell text
is updated to "Should be configured next to CNV QE bare metal infrastructure".
- Line 74: The table row under the "IPv6 support" entry contains a typo
"single-stuck" — update that text to "single-stack" so it matches the usage
elsewhere (e.g., the other "IPv6 single-stack" occurrence). Edit the Markdown
line that currently reads "| IPv6 support | IPv6
single-stuck support is not ready due to
https://github.com/FRRouting/frr/issues/5885.
| [x] phoracek@redhat.com (02/2026) |" and replace "single-stuck" with
"single-stack".
---
Nitpick comments:
In `@stps/sig-network/EVPN.md`:
- Around line 17-33: Hyphenate the compound adjectives in EVPN.md for
consistency: change "User Defined Network" to "User-Defined Network" (both the
singular instance and the plural "User Defined Networks"), and change "OVN-K
based network type" to "OVN-K-based network type"; update the occurrences in the
"Document Conventions" section and the "Feature Overview" paragraph where these
phrases appear (search for the exact strings "User Defined Network", "User
Defined Networks", and "OVN-K based network type" to locate and replace).
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@stps/sig-network/EVPN.md`:
- Line 114: Fix the misspelling in the markdown table cell: update the string
"Should be configured next to CNV QE bare metal infrustructure" to "Should be
configured next to CNV QE bare metal infrastructure" (look for the table row
containing the "Special Configurations" column or that exact sentence in
EVPN.md).
- Line 74: Fix the typo in the EVPN.md table row where "IPv6 support" currently
reads "IPv6 single-stuck support"; change "single-stuck" to "single-stack" so
the entry matches the correct terminology used elsewhere (see the "IPv6 support"
table row and the phrase "single-stuck" to locate and update the text).
- Line 97: The markdown table row for "Cloud Testing" contains a typo: change
the word "requried" to "required" in the cell that currently reads "Bare Metal
environment is requried." Locate the "Cloud Testing" row in EVPN.md and update
that cell text to "Bare Metal environment is required." Ensure only the spelling
is corrected and formatting of the table remains unchanged.
- Around line 141-142: In the EVPN.md table rows under the "Timeline/Schedule"
and "Test Coverage" entries, fix the typos: change "collobaration" to
"collaboration" and change "single-stuck" to "single-stack" (to match the usage
elsewhere, e.g., line referencing "single-stack"); update those exact strings in
the text so the table reads correctly.
|
Change: fix typos |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
stps/sig-network/EVPN.md (2)
116-122: Verify if Testing Tools section should remain empty.All tool categories are currently empty. If specific tools, frameworks, or CI/CD pipelines will be used for testing, consider documenting them here for completeness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/EVPN.md` around lines 116 - 122, The "3.1. Testing Tools & Frameworks" table is empty; either populate it with the actual tools or mark it explicitly as TBD/None. Update the "3.1. Testing Tools & Frameworks" section to list the chosen Test Framework(s) (e.g., pytest, Robot Framework), CI/CD pipeline(s) (e.g., GitHub Actions, Jenkins), and any Other Tools (e.g., tox, Molecule, linter, traffic generators), or replace the "-" entries with "TBD" or "None" and a short note explaining that tooling will be decided later so readers aren’t left with blank fields.
164-164: Consider using consistent terminology.Line 44 uses "network configuration" while this line uses "network data". Consider using "network configuration" consistently for clarity.
♻️ Proposed refinement
-| | As an admin, I want to migrate a VM from the Source Provider to OCP keeping its network data | Verify migration from Source Provider to OCP over EVPN tunnel with preserving connectivity and VM network data | Tier 2 | P0 | +| | As an admin, I want to migrate a VM from the Source Provider to OCP keeping its network configuration | Verify migration from Source Provider to OCP over EVPN tunnel with preserving connectivity and VM network configuration | Tier 2 | P0 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/EVPN.md` at line 164, The term "network data" in the EVPN.md table row (the cell reading "Verify migration from Source Provider to OCP over EVPN tunnel with preserving connectivity and VM network data") should be changed to "network configuration" to match the terminology used earlier ("network configuration"); update that table cell text so it reads "preserving connectivity and VM network configuration" to maintain consistent wording across the document.
🤖 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/EVPN.md`:
- Line 56: The notation "CUDN\UDN" in the EVPN.md API Extensions table uses a
backslash; update it to use a forward slash ("CUDN/UDN") to match the Document
Conventions and the earlier "CUDN/UDN resource" wording. Locate the table entry
containing "CUDN\UDN" (the API Extensions row) and replace the backslash with a
forward slash so the term is consistently "CUDN/UDN" throughout the document.
- Line 44: Update the grammar in the EVPN.md table row that currently reads
"User migrate a VM from Source Provider to OpenShift over the stretched L2 EVPN
network preserving source VM network configuration." — change it to "A user
migrates a VM from the Source Provider to OpenShift over the stretched L2 EVPN
network, preserving the source VM's network configuration." Ensure you update
the phrase in the table cell text (the line containing "User migrate a VM...")
and adjust articles/possessive ("the Source Provider", "source VM's") and add
the comma before "preserving".
---
Nitpick comments:
In `@stps/sig-network/EVPN.md`:
- Around line 116-122: The "3.1. Testing Tools & Frameworks" table is empty;
either populate it with the actual tools or mark it explicitly as TBD/None.
Update the "3.1. Testing Tools & Frameworks" section to list the chosen Test
Framework(s) (e.g., pytest, Robot Framework), CI/CD pipeline(s) (e.g., GitHub
Actions, Jenkins), and any Other Tools (e.g., tox, Molecule, linter, traffic
generators), or replace the "-" entries with "TBD" or "None" and a short note
explaining that tooling will be decided later so readers aren’t left with blank
fields.
- Line 164: The term "network data" in the EVPN.md table row (the cell reading
"Verify migration from Source Provider to OCP over EVPN tunnel with preserving
connectivity and VM network data") should be changed to "network configuration"
to match the terminology used earlier ("network configuration"); update that
table cell text so it reads "preserving connectivity and VM network
configuration" to maintain consistent wording across the document.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
stps/sig-network/EVPN.md (2)
18-19: Standardize compound-adjective hyphenation for readability.Several compound adjectives appear without hyphens (e.g., “OVN-K based,” “pod level,” “Bare Metal”). Consider normalizing to “OVN-K-based,” “pod-level,” “bare-metal,” etc., for consistency in technical documentation.
Also applies to: 30-32, 55-56, 76-77, 97-97, 105-106, 111-111, 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/EVPN.md` around lines 18 - 19, The document uses unhyphenated compound adjectives; update instances such as "OVN-K based" to "OVN-K-based" and normalize other compound adjectives (e.g., "pod level" → "pod-level", "Bare Metal" → "bare-metal", "cluster level" → "cluster-level", "project level" → "project-level") throughout the EVPN.md content (notably the lines around the UDN and CUDN/UDN resource descriptions and the other indicated ranges) so all compound modifiers are consistently hyphenated.
46-47: Align acceptance criteria with the stated P0 goals.Testing goals include “Source Provider Migration,” but acceptance criteria only list two items. Either add a third criterion that explicitly marks migration as deferred until the Source Provider environment exists, or note the deferral in the acceptance criteria list for clarity.
✏️ Possible adjustment
- | **Acceptance Criteria** | [x] | 1. Connectivity: UDN VM can ping Source Provider endpoint on same subnet.<br/>2. Mobility: Connection survives UDN VM Live Migration. | | + | **Acceptance Criteria** | [x] | 1. Connectivity: UDN VM can ping Source Provider endpoint on same subnet.<br/>2. Mobility: Connection survives UDN VM Live Migration.<br/>3. Migration: Source Provider → OCP migration validated once Source Provider environment is available (deferred until GA). | |Also applies to: 65-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/EVPN.md` around lines 46 - 47, Update the "Acceptance Criteria" table in EVPN.md to align with the P0 goals by either adding a third, explicit migration criterion (e.g., "3. Source Provider Migration: Connection survives Source Provider Live Migration" but conditioned on the Source Provider environment being available) or by adding a clear note alongside the existing two criteria that migration testing is deferred until the Source Provider environment exists; make the same change for the second Acceptance Criteria block referenced (the similar block at lines ~65-68) so both instances consistently state the migration requirement or its deferral.
🤖 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/EVPN.md`:
- Line 152: Change the grammar of the known-limitation sentence in EVPN.md from
"Single-Stack IPv6 are not supported" to the singular form "Single-Stack IPv6 is
not supported," leaving the rest of the sentence intact ("due to upstream Linux
Kernel limitations regarding IPv6-based VXLAN tunnels, the EVPN VTEP (Underlay)
requires IPv4") so the statement reads correctly.
---
Nitpick comments:
In `@stps/sig-network/EVPN.md`:
- Around line 18-19: The document uses unhyphenated compound adjectives; update
instances such as "OVN-K based" to "OVN-K-based" and normalize other compound
adjectives (e.g., "pod level" → "pod-level", "Bare Metal" → "bare-metal",
"cluster level" → "cluster-level", "project level" → "project-level") throughout
the EVPN.md content (notably the lines around the UDN and CUDN/UDN resource
descriptions and the other indicated ranges) so all compound modifiers are
consistently hyphenated.
- Around line 46-47: Update the "Acceptance Criteria" table in EVPN.md to align
with the P0 goals by either adding a third, explicit migration criterion (e.g.,
"3. Source Provider Migration: Connection survives Source Provider Live
Migration" but conditioned on the Source Provider environment being available)
or by adding a clear note alongside the existing two criteria that migration
testing is deferred until the Source Provider environment exists; make the same
change for the second Acceptance Criteria block referenced (the similar block at
lines ~65-68) so both instances consistently state the migration requirement or
its deferral.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
stps/sig-network/EVPN.md (1)
46-46: Consider aligning Acceptance Criteria with Testing Goals.The Acceptance Criteria lists only 2 items (Connectivity and Mobility), while Testing Goals (lines 65-67) include a third P0 goal: "Source Provider Migration". Although line 67 notes this scenario is not currently testable, consider either adding it to the Acceptance Criteria with a note about testability constraints, or explicitly stating in the Acceptance Criteria that Source Provider Migration is excluded due to environment limitations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-network/EVPN.md` at line 46, The Acceptance Criteria table currently lists only "Connectivity" and "Mobility" but Testing Goals includes a separate P0 "Source Provider Migration" scenario; update the Acceptance Criteria (the table under "Acceptance Criteria") to either add "Source Provider Migration" as an item with a note about current testability constraints, or add a clarifying sentence to the Acceptance Criteria stating that "Source Provider Migration" is excluded due to environment limitations referenced in the "Testing Goals" section (see "Testing Goals" and the "Source Provider Migration" mention) so both sections remain aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@stps/sig-network/EVPN.md`:
- Line 44: Replace the grammatically incorrect table cell text ("A user migrates
a VM from Source Provider to OpenShift over the stretched L2 EVPN network
preserving source VM network configuration.") so it uses correct subject-verb
agreement and includes the article as suggested; update the row under the
"**Customer Use Cases**" column to read exactly "A user migrates a VM from
Source Provider to OpenShift over the stretched L2 EVPN network, preserving the
source VM's network configuration." to fix agreement and clarity.
---
Nitpick comments:
In `@stps/sig-network/EVPN.md`:
- Line 46: The Acceptance Criteria table currently lists only "Connectivity" and
"Mobility" but Testing Goals includes a separate P0 "Source Provider Migration"
scenario; update the Acceptance Criteria (the table under "Acceptance Criteria")
to either add "Source Provider Migration" as an item with a note about current
testability constraints, or add a clarifying sentence to the Acceptance Criteria
stating that "Source Provider Migration" is excluded due to environment
limitations referenced in the "Testing Goals" section (see "Testing Goals" and
the "Source Provider Migration" mention) so both sections remain aligned.
|
/assign-reviewer @maiqueb @anuragthehatter |
|
not adding reviewer maiqueb @anuragthehatter by user comment, maiqueb @anuragthehatter is not part of contributers |
|
@maiqueb, @anuragthehatter I can't add you as reviewers, pls, review. |
stps/sig-network/EVPN.md
Outdated
| | **Special Hardware** | N/A | Agnostic. | | ||
| | **Storage** | N/A | Agnostic. | | ||
| | **Network** | Primary UDN, OVN-K | Single stack IPv4 underlays. Local Gateway Mode is mandatory. | | ||
| | **Required Operators** | OVN-K (default), MTV | | |
There was a problem hiding this comment.
In this context, it represents an OCP deployed component. AFAIK, OVN-K is deployed by CNO by default.
Perhaps the name "Operators" should be generalized, but the intention is that.
maiqueb
left a comment
maiqueb
left a comment
There was a problem hiding this comment.
@servolkov one question: I miss the user story (in the test scenarios section) to migrate a VM from an external virt platform onto openshift, guaranteeing the connections are not broken.
I assume you didn't write it down since we won't have an environment to test that out.
Am I correct ?
@maiqueb there is a basic scenario in the list: "Verify migration from Source Provider to OCP over VXLAN tunnel with preserving connectivity and VM network data." |
that's enough for me. Sorry for not having "seen" it. |
|
Change: addressed new portion of comments (clarifications and polishing). |
EdDev
left a comment
EdDev
left a comment
There was a problem hiding this comment.
Thank you, looks very good.
I am unsure about the plans for the SP part and if it is crucial here.
I am also not clear if this is planned to GA in 4.22 or not. The SP was mentioned as planned for GA and not "now". So it is confusing.
stps/sig-network/EVPN.md
Outdated
| - **[P0]** Basic L2 Connectivity (East-West): Verify two UDN VMs connected to the same L2 EVPN network can communicate with each other. | ||
| - **[P0]** Stretched L2 Connectivity (East-West): Verify a UDN VM can communicate with the Source Provider on the same subnet via the EVPN fabric. | ||
| - **[P0]** UDN VM Live Migration (Internal): Verify that a UDN VM connected via EVPN L2 fabric can live-migrate between OCP nodes without losing connectivity to the Source Provider\another UDN VM. | ||
| - **[P0]** Source Provider Migration: Verify migration from Source Provider to OCP UDN namespace over EVPN tunnel. Note: Since we lack a physical Source Provider environment, the scenario is not testable at this stage. |
There was a problem hiding this comment.
If it is not testable, why do you mention it as P0?
You could place it as P2 or P3 and mention it as a risk. There you can mention that the risk is mitigated by having alternative converge that mimics the original.
Also relevant to any other SP mentioning below.
There was a problem hiding this comment.
it is a deadlock. If it is a P0, it means it is a must and you cannot overcome this with an emulation.
Because you have no alternative, you are blocked.
Lest see what @rnetser says, I have no strong opinion on this one.
There was a problem hiding this comment.
The STP was updated to avoid confusion, the note has been removed as well.
| - **[P0]** Source Provider Migration: Verify migration from Source Provider to OCP UDN namespace over EVPN tunnel. Note: Since we lack a physical Source Provider environment, the scenario is not testable at this stage. | ||
| - **[P1]** L3 Routed Connectivity (North-South): Verify a UDN VM can communicate with the Source Provider on a different subnet via the EVPN L3 fabric (IP-VRF). | ||
| - **[P1]** L3 UDN VM Live Migration (Internal): Verify that a VM connected via L3 EVPN can live-migrate between OCP nodes without losing routed connectivity to the Source Provider\another UDN VM. | ||
| - **[P2]** UDN VM cold reboot: Verify that a UDN VM connected via EVPN retains connectivity to the Source Provider after a cold reboot. |
There was a problem hiding this comment.
Is there a specific reason cold reboot is relevant here?
There was a problem hiding this comment.
During cold reboot VMI + virt-launcher are deleted and created again - we want to ensure that EVPN fabric works ok with such kind of user operation and VM reachability is not affected. This operation is different from just VM creation.
There was a problem hiding this comment.
Ensuring is nice, but why would this be any different from a simple migration or a creation of a new VM?
If there is no special reason, I see no point in having it.
In other words, if the feature does not differentiate this from a new VM or from a migration, there is no apparent reason to have this check.
There was a problem hiding this comment.
for my knowledge; can you please explain how these are different?
There was a problem hiding this comment.
If there is no special reason, I see no point in having it.
The reason is not special, behavior is just different from my perspective. A general assumption\definition that it behaves similar is very brave. Cold reboot is needed to ensure EVPN correctly cleans up the network routes when a VM is completely turned off, and successfully restores routing when it starts back up. Creating new VM does not involve EVPN cleanup mechanism (I call it "cleanup", but more proficient to call it "withdrawal"), migration scenario is also considered different by me: the IP address never completely drops off the network. So, it behaves differently, at least from my current understanding.
I would say opposite: I prefer to be convinced that this scenario is really unnecessary.
There was a problem hiding this comment.
If verification of cleanup is needed, a different test/assert is required. Once it is cleaned up, we are back to a new VM scenario AFAIU.
We need to have a balance here, otherwise you will add a large amount of tests with very little value. Tests in T2 are expensive.
Bottom line, I think this is one step to far.
There was a problem hiding this comment.
was there anything specific done in this arear wrt vms?
- how is it different from cleanup when deleting a pod?
- successfully restores routing when it starts back up - how is it different from a vm started for the first time?
btw, what happens when you restart a vm and the previous resources are not cleaned up yet? anything that can lead to conflicts/other?
There was a problem hiding this comment.
@rnetser I am really sorry, but I am not in favor to have so much text in the thread answering all your questions. If you need so many details, please, don't hesitate to ask offline\read enhancement deeper\ask SDN devs\ask AI.
We need to have a balance here, otherwise you will add a large amount of tests with very little value.
@EdDev totally agree with this statement. At the same time, regarding the cleanup verification: while the SDN team is testing the EVPN withdrawal mechanism, as the OCP-V owner, my goal is to ensure that when our virtualization lifecycle triggers this mechanism (like during a VM cold reboot), the integration works perfectly.
Anyway, if you feel strongly that this scenario should be removed, just give me a short reply and we can move forward.
There was a problem hiding this comment.
@servolkov my questions are related to this discussion to understand why cold reboot is needed
stps/sig-network/EVPN.md
Outdated
| | Automation Testing | Ensures test cases are automated for continuous integration and regression coverage | Y | | | ||
| | Performance Testing | Validates feature performance meets requirements (latency, throughput, resource usage) | N | Not OCP-V scope. Tracker: https://issues.redhat.com/browse/PERFSCALE-4393 | | ||
| | Security Testing | Verifies security requirements, RBAC, authentication, authorization, and vulnerability scanning | N/A | There are no known security risks. If there are any, the core functionality is expected to cover them. | | ||
| | Usability Testing | Validates user experience, UI/UX consistency, and accessibility requirements. Does the feature require UI? If so, ensure the UI aligns with the requirements | N | API only, no UI. | |
There was a problem hiding this comment.
From a quality perspective, do you think this feature can be used without a UI?
In the end, it is about OCP-V providing functionality to customer that can use it. If it is hard to use, then the usability is a concern. But these are only questions, I do not have the answers myself.
In this context, I do not think it is limited to UI. The usability needs to be evaluated at some level.
There was a problem hiding this comment.
From a quality perspective, do you think this feature can be used without a UI?
Honestly, yes. Since EVPN is configured (without UI), next interaction may involve UI for migration, MTV resources, but it sounds regular for me - migration is EVPN independent in terms of UI.
There was a problem hiding this comment.
I do not understand your answer here.
Please be specific and answer this question: "Can a cluster operator, when working with OCP-V, can easily operate (enable, configure, etc) a network to support EVPN?"
The fact reality is one way or the other is not relevant. In the end, this feature is to be used by our customers, so if they have hard time to do so, it is our interest to raise it as a problem and raise flags.
If you are good with the current status, then you are welcome to resolve this.
There was a problem hiding this comment.
+1 on @EdDev note that usability is not only in terms of ui; i.e - does the feature require any functionality to facilitate its usage by the user
as the console has a networking section, should any new optinos be added there?
for vms - under their network configuration - should anythign new be added?
There was a problem hiding this comment.
I got now the point about UI. I have updated the section to be more transparent. Please, take a look. But short answer here to all your questions: I don't see any usability-related issues regarding specifically OCP-V.
stps/sig-network/EVPN.md
Outdated
| | Upgrade Testing | Validates upgrade paths from previous versions, data migration, and configuration preservation | Y | | | ||
| | Backward Compatibility Testing | Ensures feature maintains compatibility with previous API versions and configurations | N | There is no known impact on existing functionality. | | ||
| | Dependencies | Dependent on deliverables from other components/products? Identify what is tested by which team. | Y | Core functionality of the feature is dependent on OCP Network coverage. This is a hard assumption. | | ||
| | Cross Integrations | Does the feature affect other features/require testing by other components? Identify what is tested by which team. | Y | **OCP CORENET**: core functionality of EVPN feature, corner cases; **OCP-V network**: migration from Source Provider to OCP, connectivity checks; **MTV**: migration requires MTV, however, changes for the operator are unknown; migration is considered EVPN independent. | |
There was a problem hiding this comment.
I am unsure why we need involve MTV here. Sharing a network between the SP and the cluster is not planned at this stage. Right? The only think you can do is to mimic it.
There was a problem hiding this comment.
It relates to Source Provider migration. The scenrio is included to STP even it is not testable now. While we can't test it -> we don't need MTV, but once infra is available -> we need MTV. Based on what I was told - we have to reflect general plan in STP, even if some goals are not achievable. In common case - MTV is required, but not right now.
There was a problem hiding this comment.
With the same reasoning I could claim that the whole feature is not testable so we do not need to write a single test.
The section is discussing the test strategy which is not feasible at this stage.
I fail to understand how this makes any sense, but I am fine if @rnetser acks it explicitly.
Please resolve it once @rnetser acks.
There was a problem hiding this comment.
In common case - MTV is required, but not right now.
this should apear under limitations and be approved; i.e i fthere is a flow that is expected to be supposed and we cannot test it - it should be acked
@EdDev to your point - the stp should raise anything that cannot be tested and this must be approved
There was a problem hiding this comment.
I forgot to update this section - MTV dependency has been removed from the STP.
There was a problem hiding this comment.
@rnetser , I guess there is a gap here.
It is not ok to pass "approve" requests to others, it is up to the Dev & QE members to decide if a scenario is to be checked directly or to mimic it. The risk is all there, a PM or other stakeholder should not be bothered by this, unless we have no alternatives.
From a different angle, if I was a PM, I would not care about the engineering team inability to perform the task... I would expect them to find the way to cover the needed.
This STP is passing things on others to approve, too much, and that is not reasonable IMO.
stps/sig-network/EVPN.md
Outdated
| | Dependencies | Dependent on deliverables from other components/products? Identify what is tested by which team. | Y | Core functionality of the feature is dependent on OCP Network coverage. This is a hard assumption. | | ||
| | Cross Integrations | Does the feature affect other features/require testing by other components? Identify what is tested by which team. | Y | **OCP CORENET**: core functionality of EVPN feature, corner cases; **OCP-V network**: migration from Source Provider to OCP, connectivity checks; **MTV**: migration requires MTV, however, changes for the operator are unknown; migration is considered EVPN independent. | | ||
| | Monitoring | Does the feature require metrics and/or alerts? | N/A | No major need has been identified. | | ||
| | Cloud Testing | Does the feature require multi-cloud platform testing? Consider cloud-specific features. | N | Bare Metal environment is required. | |
There was a problem hiding this comment.
This is suppose to be an out-of-scope item, right?
There was a problem hiding this comment.
I am tired of repeating same info in many places. Isn't clear that "Bare Metal environment is required" reading the STP? If this info is clear, I think we are ok. If I add it to out-of-scope, what will be changed in terms of STP understanding?
There was a problem hiding this comment.
If you put it in out-of-scope, then you can mention here "see out-of-scope section" or something similar.
But I understand your point... IMO it is more about making sure this one is considered, something that is not assured in the out-of-scope.
Calling for @rnetser , although you could have just added it there and get this over with.
There was a problem hiding this comment.
@EdDev yes, it is here to make sure it is not missed during planning
i find i more holistic to have topics like out-of-scope inclulding everything and, where needed, reference the section
for this particular item - this falls under limitations rather than out of scope
and as a side note i think it will make more sense to have the risks and limitations right after the out of scope section, wdyt?
There was a problem hiding this comment.
and as a side note i think it will make more sense to have the risks and limitations right after the out of scope section, wdyt?
I don't have opinion, honestly. Probably, I need to work with more number of STPs.
There was a problem hiding this comment.
for this particular item - this falls under limitations rather than out of scope
And what is the reasoning?
out-of-scope for me means: "Not part of the feature, not planned in the future, etc".
limitations for me means: "While it may be expected, currently it is not possible. I.e. it makes sense for it to work, but we are unable to validate it or currently the feature is no covering it, but probably should".
We need to outline rules which are clear when one need to appear in one place and when it should not.
I am not sure now what are these rules.
| | | As an admin, I want to migrate a VM from the Source Provider to OCP keeping its network data. | Verify migration from Source Provider to OCP over VXLAN tunnel with preserving connectivity and VM network data. | Tier 2 | P0 | | ||
| | | As a user, I want my VM to reach external networks on different subnets over the L3 VPN. | Verify a UDN VM can communicate with an external provider on a different subnet via IP-VRF. | Tier 2 | P1 | | ||
| | | As an admin, I want to live-migrate an L3 EVPN-connected VM between OCP nodes without dropping connections. | Verify a UDN VM connected via L3 EVPN retains connectivity to a different external subnet/another UDN VM during live migration. | Tier 2 | P1 | | ||
| | | As a user, I can stop and start an EVPN-connected UDN VM and immediately establish new connections. | Verify that a UDN VM connected via EVPN retains connectivity to the Source Provider after a cold VM reboot (stop\start). | Tier 2 | P2 | |
There was a problem hiding this comment.
I am not sure if this is needed. What reasoning is there to check this reboot?
Is something happening (or not happening) as part of the reboot?
There was a problem hiding this comment.
answered above (it was the same question). Lets continue there.
|
Change: polishing, less duplication and more generalization. |
| - **[P0]** Source Provider Migration: Verify migration from Source Provider to OCP UDN namespace over EVPN tunnel. Note: Since we lack a physical Source Provider environment, the scenario is not testable at this stage. | ||
| - **[P1]** L3 Routed Connectivity (North-South): Verify a UDN VM can communicate with the Source Provider on a different subnet via the EVPN L3 fabric (IP-VRF). | ||
| - **[P1]** L3 UDN VM Live Migration (Internal): Verify that a VM connected via L3 EVPN can live-migrate between OCP nodes without losing routed connectivity to the Source Provider\another UDN VM. | ||
| - **[P2]** UDN VM cold reboot: Verify that a UDN VM connected via EVPN retains connectivity to the Source Provider after a cold reboot. |
There was a problem hiding this comment.
for my knowledge; can you please explain how these are different?
stps/sig-network/EVPN.md
Outdated
| | Automation Testing | Ensures test cases are automated for continuous integration and regression coverage | Y | | | ||
| | Performance Testing | Validates feature performance meets requirements (latency, throughput, resource usage) | N | Not OCP-V scope. Tracker: https://issues.redhat.com/browse/PERFSCALE-4393 | | ||
| | Security Testing | Verifies security requirements, RBAC, authentication, authorization, and vulnerability scanning | N/A | There are no known security risks. If there are any, the core functionality is expected to cover them. | | ||
| | Usability Testing | Validates user experience, UI/UX consistency, and accessibility requirements. Does the feature require UI? If so, ensure the UI aligns with the requirements | N | API only, no UI. | |
There was a problem hiding this comment.
+1 on @EdDev note that usability is not only in terms of ui; i.e - does the feature require any functionality to facilitate its usage by the user
as the console has a networking section, should any new optinos be added there?
for vms - under their network configuration - should anythign new be added?
stps/sig-network/EVPN.md
Outdated
| | Upgrade Testing | Validates upgrade paths from previous versions, data migration, and configuration preservation | Y | | | ||
| | Backward Compatibility Testing | Ensures feature maintains compatibility with previous API versions and configurations | N | There is no known impact on existing functionality. | | ||
| | Dependencies | Dependent on deliverables from other components/products? Identify what is tested by which team. | Y | Core functionality of the feature is dependent on OCP Network coverage. This is a hard assumption. | | ||
| | Cross Integrations | Does the feature affect other features/require testing by other components? Identify what is tested by which team. | Y | **OCP CORENET**: core functionality of EVPN feature, corner cases; **OCP-V network**: migration from Source Provider to OCP, connectivity checks; **MTV**: migration requires MTV, however, changes for the operator are unknown; migration is considered EVPN independent. | |
There was a problem hiding this comment.
In common case - MTV is required, but not right now.
this should apear under limitations and be approved; i.e i fthere is a flow that is expected to be supposed and we cannot test it - it should be acked
@EdDev to your point - the stp should raise anything that cannot be tested and this must be approved
| - **[P1]** L3 UDN VM Live Migration (Internal): Verify that a VM connected via L3 EVPN can live-migrate between OCP nodes without losing routed connectivity to the Source Provider/another UDN VM. | ||
| - **[P2]** UDN VM cold reboot: Verify that a UDN VM connected via EVPN retains connectivity to the Source Provider after a cold reboot. | ||
|
|
||
| **Out of Scope (Testing Scope Exclusions)** |
There was a problem hiding this comment.
maybe i missed it but the requirement mentions EVPN multihoming as acceptance criteria and i did not see any ref to this here
There was a problem hiding this comment.
The CORENET test plan explicitly handles this, I believe it relates to out of scope Core OCP network functionality section.
There was a problem hiding this comment.
as multihoming is explicity mentioned as a requirement; i sugget adding it under oout of scope (+as covered by corenet)
There was a problem hiding this comment.
okay, if you think it deserves special attention... Added
|
|
||
| ### **III. Test Scenarios & Traceability** | ||
|
|
||
| | Requirement ID | Requirement Summary | Test Scenario(s) | Tier | Priority | |
There was a problem hiding this comment.
what about negative use cases? recovery flows that affects VMs and not part of ocp testing?
There was a problem hiding this comment.
could you please provide with examples.
There was a problem hiding this comment.
in general there are no failed migration tests (btw, not all should be tier2), for example what happens to a vm that is migrated to a node with VTEP in error state - is there any error, is the migration blocked?
what happens if migration fails mid-transaction - is it possible taht both nodes withh advertise the ip/mac?
another use case, related to migration (not a negative one) -should you test node drain, which triggers eviction - make sure nothing blocks the migration?
There was a problem hiding this comment.
is it possible taht both nodes withh advertise the ip/mac
No, the protocol prevents that.
There was a problem hiding this comment.
My point of view:
- Target VTEP in error state: OCP-V does not control the VTEP/UDN CR lifecycle. A VTEP error inherently puts the UDN into an error state, which simply results in a standard fallback scenario unrelated to EVPN. The SDN team is about to test negative scenarios for CR and correct state movements.
- Migration fails mid-transaction: @maiqueb answered
- Node drain/eviction: I don't see how it relates to EVPN.
There was a problem hiding this comment.
Node drain/eviction
triggers live migration; i am not sure if it uses the exact same path as user-triggered migration on not and if there may be something to consider here. if it is the same, then nothing is needed.
as for othe rnegative scenarios - i am not familiar enough with the feature to propose any that are related only to VMs. I want to make sure this aspect was thought of
stps/sig-network/EVPN.md
Outdated
| | | As a user, I want my VM to reach external networks on different subnets over the L3 VPN. | Verify a UDN VM can communicate with an external provider on a different subnet via IP-VRF. | Tier 2 | P1 | | ||
| | | As an admin, I want to live-migrate an L3 EVPN-connected VM between OCP nodes without dropping connections. | Verify a UDN VM connected via L3 EVPN retains connectivity to a different external subnet/another UDN VM during live migration. | Tier 2 | P1 | | ||
| | | As a user, I can stop and start an EVPN-connected UDN VM and immediately establish new connections. | Verify that a UDN VM connected via EVPN retains connectivity to the Source Provider after a cold VM reboot (stop\start). | Tier 2 | P2 | | ||
| | | As an admin, I want to upgrade the cluster without losing EVPN settings or VM connectivity. | Verify an existing connection is preserved, a new connection can be established after upgrade. | Tier 2 | P2 | |
There was a problem hiding this comment.
upgrade appears under testing goals, yet there is ref to an explicit scneario that should be covered
what happens with existing CUDNs? with my very limited understanding - they will have to recreate the network and re-migrate the VMs?
There was a problem hiding this comment.
Sorry, I am not following you. You have left your comment against explicit upgrade scenario: "existing connection is preserved, a new connection can be established after upgrade". It is about existing UDN and connected VMs. Nothing about migration. In STD it will be exposed.
There was a problem hiding this comment.
my comment is about upgrades
- i do not understand what is the test here as above, upgrade in testing goals mentions
Validates upgrade paths from previous versionsbut iiuc the flow you are reffering to here is from this version to the next one, where EVPN is available. so to be explicit , please update the test goal - which leads to my question on what happens to exising VMS with CUDN that would like to use EVPN
a new connection can be established after upgrade- shouldn't the connectivity prevail an upgrade?
There was a problem hiding this comment.
1-2. Updated, I hope I follow you.
3. but the full scenario is "Verify an existing connection is preserved, a new connection can be established after upgrade.". Probably it will be 2 tests in STD, I don't know, but I am declaring clearly what we want to check.
| | **Customer Use Cases** | [x] | 1. A user migrates a VM from Source Provider to OpenShift over the stretched L2 EVPN network preserving source VM network configuration and maintains ongoing, continuous communication with the rest of the application that remains in the Source Provider.<br>2. UDN VM requires routed ingress and egress over an L3 VPN to communicate with external networks (different subnets) in the Source Provider. | | | ||
| | **Testability** | [x] | Testable via a software-based external VTEP emulation. | | | ||
| | **Acceptance Criteria** | [x] | 1. Connectivity: UDN VMs can ping each other over EVPN and Source Provider endpoint on same subnet (stretched L2).<br/>2. Mobility: Connection survives UDN VM Live Migration within the same cluster/Source Provider. <br/>3. VMs can communicate over the L3 VPN (different subnets), and these established L3 connections survive UDN VM internal Live Migration. | | | ||
| | **Non-Functional Requirements (NFRs)** | [x] | N/A | | |
There was a problem hiding this comment.
therea re some, please add ref to the test strategy table
There was a problem hiding this comment.
in OCP-V context - what do you mean?
There was a problem hiding this comment.
reading this again - upgrade appears (nfd) appears as part of test strategy; what is the requirement in terms of upgrade.
(you can ignore the original comment in this thread)
| | **Review Requirements** | [x] | Enable BGP EVPN for UDNs to support L2 and L3 extension. | | | ||
| | **Understand Value** | [x] | Customers can migrate VMs from source virtualization platform to OpenShift UDN namespace while preserving their IP addresses and L2 connectivity (Stretched L2). VMs maintain Layer 3 routed access to different external subnets over the VPN, ensuring seamless communication with resources remaining in the source cluster. | | | ||
| | **Customer Use Cases** | [x] | 1. A user migrates a VM from Source Provider to OpenShift over the stretched L2 EVPN network preserving source VM network configuration and maintains ongoing, continuous communication with the rest of the application that remains in the Source Provider.<br>2. UDN VM requires routed ingress and egress over an L3 VPN to communicate with external networks (different subnets) in the Source Provider. | | | ||
| | **Testability** | [x] | Testable via a software-based external VTEP emulation. | | |
There was a problem hiding this comment.
any risks with sw-based emulation? any limitations ? if none, please mention, if ys - please add
There was a problem hiding this comment.
There was a problem hiding this comment.
Your link refers to 2. Technology and Design Review, but initially you asked about risks, in the Risks section I am declaring that Lack of real environment = risk, mitigation strategy is emulation. Does not sound ok? As for me it is quite clear, but let me know.
There was a problem hiding this comment.
Are the any risks or limitations when using the emulation? if yes - please add, if not - please add a note here that no expected risks using emulation
|
Change: much more polishing |
| | Performance Testing | Validates feature performance meets requirements (latency, throughput, resource usage) | N | Not OCP-V scope. Tracker: https://issues.redhat.com/browse/PERFSCALE-4393 | | ||
| | Security Testing | Verifies security requirements, RBAC, authentication, authorization, and vulnerability scanning | N/A | There are no known security risks. If there are any, the core functionality is expected to cover them. | | ||
| | Usability Testing | Validates user experience, UI/UX consistency, and accessibility requirements. Does the feature require UI? If so, ensure the UI aligns with the requirements | N | General EVPN configuration is out of scope for OCP-V. While there are new API fields to define an EVPN-backed UDN, VMs simply consume these networks using standard, existing network attachments. This does not introduce any complex usability hurdles for VM users. | | ||
| | Compatibility Testing | Ensures feature works across supported platforms, versions, and configurations | Y | | |
There was a problem hiding this comment.
Shouldn't this be marked as N/A? Since under Known Limitations it says:
Bare Metal Only: this feature is supported on neither public cloud platforms nor virtual environments.
There was a problem hiding this comment.
this comment is against empty line, but I guess you are taking about Cloud Testing section.
I don't mind (however, I don't see a big difference). fixed
|
It seems like the additional commit is not really needed, can you please squash them into a single commit? |
Signed-off-by: Sergei Volkov <sevolkov@redhat.com>
13 participants
STP Metadata
VEP issue: https://github.com/openshift/enhancements/blob/master/enhancements/network/ovn-kubernetes-evpn.md
What this PR does
This PR introduce EVPN STP.
Summary by CodeRabbit