Repository navigation
TE-5.1: Expand gRIBI Get RPC Specification for Scale, Filtering, and Multi-VRF - #6244
jayakumarramisetty wants to merge 4 commits into
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 significantly expands the TE-5.1 gRIBI Get RPC test specification. The changes introduce rigorous validation for scale, filtering, and multi-VRF scoping, ensuring that the gRIBI implementation maintains performance and correctness under complex control-plane conditions. The updates provide a structured approach to testing dual-stack environments and verifying gRPC error handling, ultimately improving the robustness of the gRIBI control-plane validation suite. 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
|
Pull Request Functional Test Report for #6244 / 5ae000fVirtual 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 significantly expands the test plan documentation for the gRIBI Get RPC test in README.md, adding detailed procedures, environment setup steps, canonical OpenConfig JSON configurations, and five comprehensive test cases (TE-5.1.1 through TE-5.1.5) covering baseline Get RPC, dual-stack scale, AFT type filtering, multi-VRF scoping, and negative/edge-case validation. The required gNMI and gRIBI paths and RPCs are also updated. Feedback on the changes identifies an arithmetic error in the IPv4 address generation formula for the scale test case, which would result in an invalid IP address when i = 256.
- TE-5.1.1: non-leader gRIBI-B Modify operations now expect `AFTResult.status = FAILED` (`ProgrammingFailed`). Added a Flush from gRIBI-B with its own election ID that must fail with `FAILED_PRECONDITION` / `NOT_PRIMARY`; the leader's entries are confirmed unchanged by the Get steps that follow. - TE-5.1.1: TODOs are now requirements, checked with `Get(aft: ALL)`, because an `aft: IPV4` Get cannot return NH/NHG entries. Exact counts (5, then 8), `rib_status`/`fib_status = PROGRAMMED`, and a `<= 5 s` latency budget are now required; the "originally noted as" history notes are gone. - TE-5.1.1: the unresolvable next-hop case now follows gRIBI spec 4.1.3.2: `NH 1001` (`192.0.2.254`) -> `NHG 1042` -> `203.0.113.0/24` must return `FIB_PROGRAMMED`, and Get must return the entries PROGRAMMED/PROGRAMMED. A NOTE says that `get_rpc_test.go` (which programs `192.0.2.254/32` and asserts the entry is absent) must be updated. - TE-5.1.1: Get runs only after the static route is present in the AFT, so the static-route exclusion check is deterministic. - Added "Common validations": a Get result check (exact set, field-for-field equality with what was programmed, statuses, latency definition) and an AFT consistency check keyed on `programmed-id`/`programmed-index` with `origin-protocol = GRIBI` scoping. Leaves that some platforms do not report are excluded neutrally. - TE-5.1.2: replaced the invalid IPv4 generator with `100.64.<floor(i/256)>.<i mod 256>/32` (contiguous `100.64.0.0 + i`, same endpoints, checked in Python) and added explicit IPv6 generators. - TE-5.1.2: added a diverse entry set: a 1:3 weighted multi-member NHG with a decap-to-default backup NHG, an IP-in-IP encap NH, and `/24` + `/28` IPv4 entries. Decap `network_instance` is gated on platform support; entry counts are now 2,012 (DEFAULT) and 3,012 (all NIs). - Acknowledgements are awaited once, after the full NH -> NHG -> prefix sequence, with explicit timeouts. This handles platforms that defer `FIB_PROGRAMMED` for unreferenced NH/NHG. - Added a scale rationale: the counts are functional-scale parameters, and full-scale Get performance is left to a dedicated scale test. - TE-5.1.3: all specific `AFTType`s are covered (adds `MAC` and `POLICY_FORWARDING`). Unsupported AFT types expect `UNIMPLEMENTED`, chosen per platform (no either/or). Added a check that the filtered results together equal `ALL`. - TE-5.1.4: VRF-A Get checks now include NHG/NHG-NI fields and the AFT consistency check, and the counts are updated. - TE-5.1.5: added `aft = INVALID` -> `INVALID_ARGUMENT`. A non-existent NI now expects only `INVALID_ARGUMENT` (spec 4.1.7/4.3.1). Session stability is checked in a measurable way. - Cleanup: flush VRF-A, then DEFAULT, with the leader election ID; expect `result = OK` and an empty `Get(all)`. A fallback `Flush(override, all)` is registered in `t.Cleanup()`. - Setup: `DEFAULT` now means the DUT's platform-specific default NI name (`default_network_instance` deviation); added an optional explicit-interface step and the gRIBI-A-first leader confirmation. The Canonical OC is labeled as the end-state configuration. - Template alignment: added a `## Topology` section and renamed the platform heading to `## Required DUT platform`. The coverage block adds `programmed-id`, `programmed-index`, `origin-protocol` (v4/v6), `backup-next-hop-group`, encap/decap/ip-in-ip/NH `network-instance`, and ipv4 `next-hop-group-network-instance` (41 paths, all validated). - Title is unchanged, so `metadata.textproto` and `testregistry.textproto` are untouched. Validators: canonicalocspec=PASS, readme_spec=PASS Follow-up (V1-V3): - Election IDs: gRIBI-A connects first, learns the DUT's highest election ID (which persists across sessions, spec 4.1.2.1) and advertises +1 (`EID_A`). gRIBI-B connects with the strictly lower `(11, 0)` and must observe `EID_A` in its `ModifyResponse.election_id`. Every non-override Flush from gRIBI-A uses `EID_A`; the NOT_PRIMARY Flush check uses gRIBI-B's `(11, 0)`. - AFT consistency check: an AFT NHG "matches" a gRIBI NHG on `programmed-id` plus members/weights (via `programmed-index`), and each gRIBI NHG needs at least one match (was exactly one). On platforms that do not report backup next-hop-groups in AFT telemetry, backup-only NHG 301 / NH 31 need not appear in the AFT, but Get must still return them. - Encap NH 32 now sets `network_instance = DEFAULT`, matching existing gRIBI encap tests; the gating for the NH 31 decap `network_instance` stays vendor-neutral. Validators (after follow-up): canonicalocspec=PASS, readme_spec=PASS
… and accept NOT_FOUND in TE-5.1.5 - Gate NextHop 31 decap, NextHop 32 IP-in-IP encap, and NextHopGroup 300 backup_next_hop_group = 301 in TE-5.1.2/TE-5.1.3 on platform capability deviations (substituting plain IPv4 next-hops / omitting backup_next_hop_group when unsupported) so the 2,012-entry scale count remains invariant across existing test groups and KNE targets. - Accept canonical gRPC INVALID_ARGUMENT or NOT_FOUND for non-existent network instance Get in TE-5.1.5 per gRIBI spec sections 4.1.7 and 4.2.1.
e795c4b to
deea005
Compare
Summary
TE-5.1(feature/gribi/otg_tests/get_rpc_test)TE-5.1(gRIBI.Get) control-plane specification to validate dual-stack1,000-prefix IPv4 and1,000-prefix IPv6 scale completeness and response latency (<= 5 seconds), isolatedAFTTypefiltering (IPV4,IPV6,NEXTHOP_GROUP,NEXTHOP,MPLS), multi-VRFnetwork_instancescoping (namevs.allacrossDEFAULTandL3VRFinstances), and negative error handling (INVALID_ARGUMENT/NOT_FOUND).gRIBI,gNMI, OTG (TESTBED_DUT_ATE_2LINKS)TE-5.1.1: Baseline Get RPC, Non-Leader Rejection, and Static ExclusionTE-5.1.2: Dual-Stack Scale Get Accuracy, Completeness, and LatencyTE-5.1.3: GetRequest AFT Type Filtering (AFTTypeOptions)TE-5.1.4: Multi-VRF Network-Instance Scoping (namevsall)TE-5.1.5: Negative and Edge-Case Get ValidationRevision 2 (review feedback addressed)
AFTResult.status = FAILED(ProgrammingFailed). Added a Flush from gRIBI-B with its own election ID that must fail withFAILED_PRECONDITION/NOT_PRIMARY; the leader's entries are confirmed unchanged by the Get steps that follow.Get(aft: ALL), because anaft: IPV4Get cannot return NH/NHG entries. Exact counts (5, then 8),rib_status/fib_status = PROGRAMMED, and a<= 5 slatency budget are now required; the "originally noted as" history notes are gone.NH 1001(192.0.2.254) ->NHG 1042->203.0.113.0/24must returnFIB_PROGRAMMED, and Get must return the entries PROGRAMMED/PROGRAMMED. A NOTE says thatget_rpc_test.go(which programs192.0.2.254/32and asserts the entry is absent) must be updated.programmed-id/programmed-indexwithorigin-protocol = GRIBIscoping. Leaves that some platforms do not report are excluded neutrally.100.64.<floor(i/256)>.<i mod 256>/32(contiguous100.64.0.0 + i, same endpoints, checked in Python) and added explicit IPv6 generators./24+/28IPv4 entries. Decapnetwork_instanceis gated on platform support; entry counts are now 2,012 (DEFAULT) and 3,012 (all NIs).FIB_PROGRAMMEDfor unreferenced NH/NHG.AFTTypes are covered (addsMACandPOLICY_FORWARDING). Unsupported AFT types expectUNIMPLEMENTED, chosen per platform (no either/or). Added a check that the filtered results together equalALL.Both README validators used by CI (
validate_readme_canonicalocspec,validate_readme_spec) pass locally on this revision.