feat: Double GUE Decapsulation (PF-1.26)#5266
Conversation
Signed-off-by: Darren O’Connor <mellowd@google.com>
Pull Request Functional Test Report for #5266 / 12d1d42Virtual Devices
Hardware Devices
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the feature profiles by adding a comprehensive test for double GUEv1 decapsulation, a critical capability for advanced overlay probing in network devices. Concurrently, it refines the test registry by removing obsolete entries and updating details for existing ones, ensuring the test suite remains current and accurate. This effort improves both the functional coverage and the maintainability of the testing infrastructure. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new test case, PF-1.26, for double GUEv1 decapsulation, complete with its README documentation and metadata. The test registry has been updated by removing several outdated test entries, modifying descriptions and README links for existing ones, and repurposing an entry for the new PF-1.26 test. Additionally, a new test entry, RT-9, has been added to the registry. The review comments highlight several issues: the PF-1.26 test's metadata specifies an insufficient testbed for its described topology, the RT-9 test entry has an incorrect README link, the PF-1.26 README contains inconsistencies in its ATE configuration table and logical flaws in its negative test case verification steps, and the Canonical OC example in the README does not align with the described IPv6 test flows.
feature/policy_forwarding/otg_tests/double_gue_decap/metadata.textproto
Outdated
Show resolved
Hide resolved
| | **Flow Type #1 (IPv6/IPv6/IPv4)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6080 | 32 | 60 | | ||
| | | Inner IP | IPV6-SRC-HOST | IPV4-DST-HOST | N/A | 20 | 50 | | ||
| | **Flow Type #2 (IPv6/IPv6/IPv6)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6080 | 32 | 60 | | ||
| | | Inner IP | IPV6-SRC-HOST | IPV6-DST-HOST | N/A | 20 | 50 | | ||
| | **Flow Type #3 (IPv6/IPv6/IPv4)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6085 | 32 | 60 | | ||
| | | Inner IP | IPV6-SRC-HOST | IPV4-DST-HOST | N/A | 20 | 50 | | ||
| | **Flow Type #4 (IPv6/IPv6/IPv4)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6080 | 32 | 60 | | ||
| | | Inner IP | IPV6-SRC-HOST | IPV6-DST-HOST | N/A | 20 | 50 | | ||
| | **Flow Type #5 (IPv6/IPv6/IPv6)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6080 | 32 | 60 | | ||
| | | Inner IP | IPV6-SRC-HOST | IPV6-DST-HOST | N/A | 20 | 50 | |
There was a problem hiding this comment.
The ATE configuration table has several inconsistencies between the "Flow Type" column and the header details, which could cause confusion when implementing the test.
- Flow Type Add the ISIS feature profile #1 (IPv6/IPv6/IPv4): The inner IP has
Source IPasIPV6-SRC-HOST. For an IPv4 inner header, this should be an IPv4 source address. - Flow Type Add the initial ISIS base and adjacency feature profile #3 (IPv6/IPv6/IPv4): Similar to Flow Add the ISIS feature profile #1, the inner IP has
Source IPasIPV6-SRC-HOST. This should be an IPv4 source. - Flow Type Add System Profile #4 (IPv6/IPv6/IPv4): The table specifies an IPv6 inner header (
IPV6-SRC-HOSTtoIPV6-DST-HOST), which contradicts both the flow type name and the test casePF-1.26.4which states it uses an "IPv4 inner header".
Please correct the table to align with the test case descriptions.
- Updated testbed in metadata to TESTBED_DUT_ATE_4LINKS - Fixed ATE configuration table inconsistencies for IPv4 inner headers - Corrected verification steps for negative test cases (LPM lookup failure) - Updated Canonical OC JSON and paths to match on ipv6 destination address
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new test plan (PF-1.26) for Double GUEv1 Decapsulation, including a README, metadata, and a registry entry. The feedback identifies significant inconsistencies in the ATE configuration table, where the flow definitions do not align with the stated objectives for IPv4 and IPv6 middle headers, and contains redundant flow definitions that should be consolidated.
| | Flow Type | Header Layer | Source IP | Destination IP | UDP Port | DSCP | TTL | | ||
| | :--- | :--- | :--- | :--- | :--- | :--- | :--- | | ||
| | **Flow Type #1 (IPv6/IPv6/IPv4)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6080 | 32 | 60 | | ||
| | | Inner IP | IPV4-SRC-HOST | IPV4-DST-HOST | N/A | 20 | 50 | | ||
| | **Flow Type #2 (IPv6/IPv6/IPv6)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6080 | 32 | 60 | | ||
| | | Inner IP | IPV6-SRC-HOST | IPV6-DST-HOST | N/A | 20 | 50 | | ||
| | **Flow Type #3 (IPv6/IPv6/IPv4)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6085 | 32 | 60 | | ||
| | | Inner IP | IPV4-SRC-HOST | IPV4-DST-HOST | N/A | 20 | 50 | | ||
| | **Flow Type #4 (IPv6/IPv6/IPv4)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6080 | 32 | 60 | | ||
| | | Inner IP | IPV4-SRC-HOST | IPV4-DST-HOST | N/A | 20 | 50 | | ||
| | **Flow Type #5 (IPv6/IPv6/IPv6)** | Outer IP | ATE-P1-IP | DECAP-DST-OUTER | 6080 | 35 | 70 | | ||
| | | Middle IP | IPV6-MID-SRC | DECAP-DST-INNER | 6080 | 32 | 60 | | ||
| | | Inner IP | IPV6-SRC-HOST | IPV6-DST-HOST | N/A | 20 | 50 | |
There was a problem hiding this comment.
The ATE Configuration table and related sections contain several inconsistencies that make the test plan difficult to follow.
-
Contradictory Information:
- The Objective (line 10) and Traffic Generation section (lines 56-61) state that the test covers both IPv4 and IPv6 middle headers (H2).
- However, this table only defines flows with an IPv6 middle header. For example,
Flow Type #1is labeled(IPv6/IPv6/IPv4), and itsMiddle IPlayer isIPV6-MID-SRC. - The
Traffic Generationsection further confuses this by describingFlow #1as having anIPv4 H2.
-
Redundant Flow Definitions:
Flow Type #4is identical toFlow Type #1.Flow Type #5is identical toFlow Type #2.
To improve clarity and correctness, please revise the ATE Configuration table and surrounding text to be consistent. I recommend:
- Correcting the flow definitions in the table to match the stated objective of testing both IPv4 and IPv6 middle headers.
- Removing the redundant
Flow Type #4andFlow Type #5. The negative test cases (PF-1.26.4andPF-1.26.5) can refer to the primary flow types with a note about using an unreachable destination IP.
For example, you could simplify the table to define base flows and then describe the variations in the test case descriptions. This would make the overall test plan much easier to understand.
References
- Test plans should be structured with separate subtests for distinct logical test cases, avoiding redundant definitions and simplifying by defining base flows and describing variations.
- Update testbed to TESTBED_DUT_ATE_4LINKS - Fix readme link for RT-9 in test registry - Correct ATE Configuration table to accurately reflect IPv4/IPv6 headers - Fix verification steps for negative tests to specify LPM failure on inner header - Update Canonical OC example to match IPv6 outer header - Add IPv6 destination-address to OpenConfig Path coverage
Replacing PR 5081 with a clean, rebased version.