-
Notifications
You must be signed in to change notification settings - Fork 197
Update `ECMP hashing for GUE flows with IPv4|UDP outer header on deca… #4960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…psulation node` README.md for v6 transport Change v4 transport with v6 transport.
Summary of ChangesHello @sachendras, 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 updates the documentation and configuration examples for GUEv1 decapsulation and ECMP hashing tests. The primary goal is to transition the test scenarios from using IPv4 as the outer transport header to IPv6, ensuring that the test cases and associated configurations accurately reflect IPv6-based GUE deployments and their expected behavior on decapsulating nodes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the README.md to change the GUE decapsulation test from using an IPv4 outer header to an IPv6 outer header. While the documentation changes are mostly correct, there are a couple of inconsistencies in the updated tables and paths that need to be addressed. More importantly, the Go test code in this directory has not been updated and still implements the test with an IPv4 outer header. This creates a discrepancy between the test plan (README.md) and the implementation, which violates the repository's contribution guidelines stating that 'Test code must follow the steps documented in the test README.md'. To resolve this, please either update the Go test code to match the new README in this PR, or revert the README changes and create a new PR that updates both the documentation and the code together.
feature/policy_forwarding/decapsulation/otg_tests/ipv4_guev1_decap_and_hashing_test/README.md
Outdated
Show resolved
Hide resolved
feature/policy_forwarding/decapsulation/otg_tests/ipv4_guev1_decap_and_hashing_test/README.md
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 20484278921Details
💛 - Coveralls |
…ecap_and_hashing_test/README.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to update the GUE decapsulation test to use an IPv6 outer header instead of IPv4. However, there is a critical issue. While the documentation in README.md has been updated, the corresponding test implementation in ipv4_guev1_decap_and_hashing_test.go appears to be unchanged and still implements the test using IPv4. This is a violation of the repository's contribution guidelines, which state that test code must follow the test's README.md (see CONTRIBUTING.md, line 49). The test code must be updated to match the new documentation for this PR to be valid. I've also left some comments on the README.md to fix a few inconsistencies. For better long-term maintainability, please also consider renaming the test directory and file from ipv4_... to ipv6_... to accurately reflect its new purpose.
feature/policy_forwarding/decapsulation/otg_tests/ipv4_guev1_decap_and_hashing_test/README.md
Outdated
Show resolved
Hide resolved
| | | Outer | IPv4\|UDP(GUE v1) | ATE1-port IPv4 addr | DUT-DECAP-Address | 5996 (randomizable) | 6080 | | Src Port: Any unreserved UDP port; GUE v1 encapsulation | | ||
| | | Outer | IPv4\|UDP(GUE v1) | ATE1-port IPv6 addr | DUT-DECAP-Address-V6 | 5996 (randomizable) | 6080 | | Src Port: Any unreserved UDP port; GUE v1 encapsulation | | ||
| | **11**| **Overall** | **Payload o IPv6\|TCP o IPv4\|UDP(GUE v1)** | | | | | | | | ||
| | | Inner | IPv6\|TCP | H1v4 address | H4v4 address | 14 | 15 | | Src Port: Any unassigned TCP port; Dst Port: Any App/unassigned TCP port | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feature/policy_forwarding/decapsulation/otg_tests/ipv4_guev1_decap_and_hashing_test/README.md
Show resolved
Hide resolved
updated IPv4\|UDP(GUE v1) to IPv6\|UDP(GUE v1)
correction in flow11
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is incomplete. While the README.md has been updated to change the GUE decapsulation test from using an IPv4 outer header to an IPv6 outer header, the actual test implementation in ipv4_guev1_decap_and_hashing_test.go has not been modified. The Go code still configures and sends GUE packets with an IPv4 outer header, creating a critical discrepancy between the test plan and the code. The helper functions in internal/cfgplugins/policyforwarding.go also appear to be missing the necessary updates for IPv6. Please update the Go files to fully implement the IPv6 logic as described in the README.md. Additionally, for better maintainability, consider renaming the test directory to reflect that the test now focuses on IPv6.
feature/policy_forwarding/decapsulation/otg_tests/ipv4_guev1_decap_and_hashing_test/README.md
Outdated
Show resolved
Hide resolved
…ecap_and_hashing_test/README.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Change v4 transport with v6 transport.