gribi/hashing_test: add dataplane hashing test with physical/software… - #5884
gribi/hashing_test: add dataplane hashing test with physical/software…#5884singhh07 wants to merge 1 commit into
Conversation
Pull Request Functional Test Report for #5884 / 271f4fdVirtual 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 introduces a new dataplane hashing test suite designed to verify traffic distribution across multiple paths using physical and software loopbacks. The changes include the core test logic, necessary platform-specific configuration deviations, and the required updates to the test registry to support automated test execution. 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 the 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 counterproductive. 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 dataplane hashing test (hashing_test.go) that utilizes physical and software loopbacks to verify traffic distribution across multiple stages using gRIBI and OTG. It also updates the test registry and metadata with platform-specific deviations. Feedback on the implementation highlights critical style guide violations and performance optimizations: first, a static sleep should be replaced with otgutils.WaitForARP before starting traffic to ensure proper neighbor resolution; second, the helper functions getPhysicalPortCounters and getEgressPacketsPhys should be refactored to use gnmi.OCBatch() to eliminate the 'N+1' query anti-pattern caused by calling gnmi.Lookup inside loops.
e8a1c06 to
4a8c82c
Compare
|
/gemini review |
|
@alokmtri-g to take it to green as this is a critical test for us. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new dataplane hashing test (hashing_test.go) using physical and software loopbacks across multiple VRFs, along with corresponding metadata and registry updates. The code review identified several important issues: a lifecycle ordering bug where the gRIBI client is closed before the cleanup flush runs, a performance anti-pattern of serial gNMI operations during cleanup that should be batched, an unnecessary static sleep after starting traffic, and a potential nil pointer dereference when checking subinterfaces.
8819872 to
271f4fd
Compare
|
/gemini review |
|
Please add passing logs for all vendors for taking this to green, please mention the run time of this test. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new test for dataplane hashing with physical and software loopbacks in the feature/gribi/otg_tests/hashing_test directory. It includes the implementation of the test logic, updates to the metadata for platform-specific deviations, and registration of the new test in the test registry. I have no feedback to provide as there were no review comments.
Summary
Adds automated dataplane hashing test (
Hashing) verifying traffic distribution across multiple Network Instances (Default, Transit, Self-Site, Egress) using a mix of physical loopback pairs and dynamically discovered software loopback interfaces with Link Aggregation Groups (LAGs).Changes
feature/gribi/otg_tests/hashing_test/hashing_test.gofeature/gribi/otg_tests/hashing_test/metadata.textprototestregistry.textprotoValidation
gofmt&goimportscleanrevivelint cleanRFC 5737/RFC 2544)