Adding test for TE-14.4#5366
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 introduces new automated tests for gRIBI scaling and gNSI Certz trust bundle rotation. The changes include a comprehensive test suite for the TE-14.4 scaling target, utilizing shared configuration helpers to manage complex DUT and ATE setups, as well as a robust test for verifying trust bundle rotation under various conditions. 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
|
Pull Request Functional Test Report for #5366 / 977842bVirtual 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 introduces a trust bundle rotation test for GNSI Certz and a full-scale gRIBI scaling test (T2) supported by a new shared configuration plugin. The review identifies several areas for improvement, specifically recommending the removal of global state in table-driven tests to ensure test independence and the elimination of commented-out code. Additionally, the feedback stresses adherence to style guides regarding time.Sleep, the need for robust error handling during entry creation, and refinements to packet validation logic to ensure more reliable and clear testing of network scenarios.
| prevClientCertFile = "" | ||
| prevClientKeyFile = "" | ||
| prevTrustBundleFile = "" |
There was a problem hiding this comment.
Using global variables to maintain state between test cases in a table-driven loop makes the tests order-dependent and fragile. If a previous test case fails or is skipped, it may leave these variables in an inconsistent state for subsequent tests. Consider passing the state explicitly between functions or managing it within a context object passed through the test cases.
| if deviations.InterfaceConfigVRFBeforeAddress(dut) { | ||
| t.Log("Configure/update Network Instance type") | ||
| dutConfNIPath := d.NetworkInstance(deviations.DefaultNetworkInstance(dut)) | ||
| gnmi.Replace(t, dut, dutConfNIPath.Type().Config(), oc.NetworkInstanceTypes_NETWORK_INSTANCE_TYPE_DEFAULT_INSTANCE) |
There was a problem hiding this comment.
For consistency and performance, consider adding this configuration to the vrfBatch instead of using gnmi.Replace directly. This ensures all initial configurations are applied in a single transaction where possible.
| gnmi.Replace(t, dut, dutConfNIPath.Type().Config(), oc.NetworkInstanceTypes_NETWORK_INSTANCE_TYPE_DEFAULT_INSTANCE) | |
| gnmi.BatchReplace(vrfBatch, dutConfNIPath.Type().Config(), oc.NetworkInstanceTypes_NETWORK_INSTANCE_TYPE_DEFAULT_INSTANCE) |
| // ConfigureVRFSelectionPolicy(t, dut) | ||
| } |
There was a problem hiding this comment.
Avoid checking in commented-out code that appears to be essential for the test logic. If the VRF selection policy is required but currently causing issues, it should be enabled and the failure tracked via the linked issue, or marked as an expected failure/skipped using appropriate testing primitives.
| // if err := gSession.AwaitTimeout(context.Background(), t, 20*time.Second); err != nil { | ||
| // t.Fatalf("gRIBI batch programming failed: %v", err) | ||
| // } |
There was a problem hiding this comment.
Commented-out code should be removed to maintain code cleanliness. The accompanying TODO and the workaround already document the issue.
| // } | ||
| } | ||
| // TODO: A time.Sleep is used as a temporary workaround. This will be fixed once the underlying issue is resolved. | ||
| time.Sleep(wTime) |
There was a problem hiding this comment.
The style guide recommends avoiding time.Sleep and using gnmi.Watch with .Await for waiting on conditions. While gRIBI programming is not directly observable via gNMI Watch, using AwaitTimeout on the gRIBI session is the preferred synchronization mechanism once the underlying bug mentioned in the comments is resolved.
References
- Avoid time.Sleep: Use gnmi.Watch with .Await for waiting on conditions. (link)
| // BuildStaticGroups generates entries for the two static NHGs (S1 → REPAIR_VRF, S2 → decap DEFAULT). | ||
| func BuildStaticGroups(t *testing.T, dut *ondatra.DUTDevice, ctx context.Context, defaultVRF string) (uint64, uint64) { | ||
| t.Helper() | ||
| s1NHG, s2NHG := StaticS1NHG, StaticS2NHG |
There was a problem hiding this comment.
Errors from gribi.NHEntry and gribi.NHGEntry should be checked to ensure the test fails correctly if entry creation fails, rather than silently ignoring them with _.
| } | ||
|
|
||
| for vi, vrf := range encapVRFs { | ||
| v4Prefixes, _ := iputil.GenerateIPsWithStep(fmt.Sprintf("200.%d.0.1", vi), NumEncapIPv4PerVRF, CommonPrefixStep) |
There was a problem hiding this comment.
Errors from iputil.GenerateIPsWithStep should be checked to prevent the test from proceeding with invalid or empty prefix lists.
| // For Decap (ExpectedOuterSrc == "") this field is left empty and | ||
| // the DstIP check inside validateIPv4Header will be skipped because | ||
| // the captured decapped packet's dst is the inner-packet's original dst. | ||
| DstIP: exp.ExpectedOuterSrc, |
There was a problem hiding this comment.
Repurposing the DstIP field to carry the expected outer-source IP is confusing and potentially incorrect if the underlying validateIPv4Header helper checks this field against the packet's destination IP. It is better to use a field that matches the semantic meaning or extend the helper to support source IP validation.
| // the captured decapped packet's dst is the inner-packet's original dst. | ||
| DstIP: exp.ExpectedOuterSrc, | ||
| Tos: tosByte, | ||
| }, |
There was a problem hiding this comment.
Spot-checking only the first DSCP value when OTG is configured to cycle through multiple values (SetValues) can lead to flaky test results. If a captured packet happens to have the second DSCP value, the TOS validation will fail. Consider validating against the full set of expected DSCP values.
| // and skip DstIP (the decapped dst is the original inner-packet dst, not | ||
| // a DUT-stamped tunnel address). | ||
| pv.IPv4Layer.SkipProtocolCheck = true | ||
| pv.IPv4Layer.DstIP = "" // do not constrain dst for decap |
There was a problem hiding this comment.
The destination IP of the decapped packet is known (DecapIPv4InnerDst). It should be validated here to ensure the packet was correctly decapped and forwarded to the expected inner destination, rather than leaving the constraint empty.
| pv.IPv4Layer.DstIP = "" // do not constrain dst for decap | |
| pv.IPv4Layer.DstIP = DecapIPv4InnerDst |
Steubert
force-pushed
the
FPT_OTG_TE-14.4
branch
from
8fd660a to
08b25ea
Compare
2 participants
README Location: https://github.com/open-traffic-generator/featureprofiles/blob/main/feature/gribi/otg_tests/gribi_full_scale_t2/README.md
Raised an issue for the failure https://partnerissuetracker.corp.google.com/u/1/issues/500317744
Logs Attached.
https://partnerissuetracker.corp.google.com/u/1/issues/504579186