Conversation
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 updates the gRIBI Get RPC test documentation to provide a more comprehensive validation suite. It introduces structured test cases for verifying gRIBI Get operations under various conditions, including scale, multi-client scenarios, and specific network instance configurations, ensuring robust behavior of the gRIBI implementation. 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
|
ghost
changed the title
Pull Request Functional Test Report for #5986 / 86c1629Virtual Devices
Hardware Devices
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the test plan for the gRIBI Get RPC test in README.md by expanding the procedure into detailed test cases (TestID-5.1.1 through TestID-5.1.6) covering scale, non-leader clients, filtering by AFT type, specific network instances, unresolved next-hops, and negative scenarios. It also updates the OpenConfig path coverage and specifies FFF as the required DUT platform. The reviewer suggested resolving an ambiguity in the unresolved next-hop test case (TestID-5.1.5) to ensure deterministic testing, recommending a single expected behavior and using a deviation if implementations differ.
| * Send the generated AFT entries via the gRIBI `Modify` RPC for `VRF-A`. | ||
| * **Step 3 - Validation with gNMI and Traffic:** | ||
| * Validate entries are installed through gNMI AFT telemetry at `/network-instances/network-instance[name=VRF-A]/afts/ipv6-unicast/ipv6-entry/state/prefix` before proceeding. | ||
| * Send traffic validating the programmed routes in `VRF-A` (e.g., encapsulated/tagged if topology supports). |
There was a problem hiding this comment.
I don't think we can support this with the requested testbed topology (2 links). Also I believe that this test was designed to be a pure gRIBI/control plane test and not intended to validate data traffic behavior.
| * Ensure only the 1,000 IPv4 entries are returned, with no IPv6, NH, or NHG entries. | ||
| * **Step 2 - Validate NextHopGroup Filter:** | ||
| * Issue a `Get` RPC from gRIBI-A specifying the `DEFAULT` network instance and filtering for `NEXTHOP_GROUP` AFT entries. | ||
| * Ensure only the configured NextHopGroup(s) are returned. |
There was a problem hiding this comment.
can we also perform the same validation for IPv6 and NEXTHOP?
| * Issue a `Get` RPC from gRIBI-A specifying the `DEFAULT` network instance and requesting all AFT entries (AFT parameter set to ALL). | ||
| * Ensure that exactly 1,000 IPv4 entries, 1,000 IPv6 entries, and their associated NextHops and NextHopGroups are returned. | ||
| * Ensure all entries are returned with `fib_status` = `PROGRAMMED`. | ||
| * Measure the latency of the `Get` RPC response. Ensure it completes in a reasonable time. |
There was a problem hiding this comment.
I don't have a specific number to apply to this, but we should include an expectation for time to complete and ensure we do not exceed that value.
| * With the configuration from TestID-5.1.1 still active, issue a `Get` RPC from gRIBI-B (the non-leader client) for all AFT entries in the `DEFAULT` network instance. | ||
| * Ensure that exactly 1,000 IPv4 entries, 1,000 IPv6 entries, and their associated NH/NHGs are returned. | ||
| * Ensure all entries are returned with `fib_status` = `PROGRAMMED`. | ||
| * Measure the latency of the `Get` RPC response. |
There was a problem hiding this comment.
Same general comment as 5.1.1 - we should measure and also validate against a specific time to complete value.
| * Wait for gNMI AFT telemetry to reflect the state of this entry (should not be present or not programmed in the FIB). | ||
| * **Step 3 - Validate Get RPC:** | ||
| * Issue a `Get` RPC from gRIBI-A for the `DEFAULT` network instance. | ||
| * Ensure that the `IPEntry` for `203.0.113.0/24` is returned with `fib_status` = `NOT_PROGRAMMED` and `rib_status` = `PROGRAMMED`. |
There was a problem hiding this comment.
Just a general comment, this is a change in behavior (but I believe the proper expected responses) versus the existing code. The logic needs to be modified to make sure we check fib and rib status properly.
ghost
closed this
2 participants
System Test Plan see for more info: b/540866941
Objective: Validate that gRIBI
Getrequests accurately, completely, and efficiently return the current state of the gRIBI AFT, testing differentGetrequest parameters.Interfaces Used: gRIBI, gNMI
Expected Outcome Summary:
Getrequest for all network instances returns exactly those entries and their full parameters, matching what was programmed.Getrequest for a specific network instance returns only the entries within that instance.Getrequest for a non-existent network instance returns an empty result or appropriate indication, without error.Getoperations are consistently aligned with the state reported via gNMI AFT streaming telemetry.GetAPI supports any form of filtering (e.g., by key, type), test that these filters work correctly.Getoperations on a large AFT should complete within a reasonable time.