Do not merge alex gribi scale#5424
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 the infrastructure and test implementation for the TE-14.3 gRIBI full-scale scaling test. By modularizing the configuration and validation logic into a shared helper package, the changes enable consistent testing across different scale targets (T1 and T2) while ensuring that the test suite remains maintainable and extensible for future scaling requirements. 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
|
|
/fptest virtual |
Pull Request Functional Test Report for #5424 / 638b094Virtual 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 new gRIBI full-scale scaling test (TE-14.3) for target T1, supported by a new shared cfgplugins package that handles topology setup, gRIBI entry programming, and traffic validation. Review feedback suggests increasing test coverage by verifying ARP resolution for all interfaces instead of a limited subset. Furthermore, several configuration functions in the cfgplugins package require refactoring to align with the repository's style guide, specifically by utilizing gnmi.SetBatch for batched configuration updates and adopting the required function signatures.
| // Limiting it to 100 since checking ARP for 1024 interfaces takes long time | ||
| ifs := interfaceNamesList | ||
| if len(ifs) >= 100 { | ||
| ifs = ifs[:100] | ||
| } | ||
| cfgplugins.IsIPv4InterfaceARPresolved(t, ate, cfgplugins.AddressFamilyParams{InterfaceNames: ifs}) | ||
| cfgplugins.IsIPv6InterfaceARPresolved(t, ate, cfgplugins.AddressFamilyParams{InterfaceNames: ifs}) |
There was a problem hiding this comment.
Limiting the ARP resolution check to the first 100 interfaces significantly reduces the test coverage, especially for a scale test intended to validate 640+ VLANs. This could allow configuration or connectivity issues on the remaining interfaces to go undetected. Verify ARP for all configured interfaces to ensure full test coverage.
| // Limiting it to 100 since checking ARP for 1024 interfaces takes long time | |
| ifs := interfaceNamesList | |
| if len(ifs) >= 100 { | |
| ifs = ifs[:100] | |
| } | |
| cfgplugins.IsIPv4InterfaceARPresolved(t, ate, cfgplugins.AddressFamilyParams{InterfaceNames: ifs}) | |
| cfgplugins.IsIPv6InterfaceARPresolved(t, ate, cfgplugins.AddressFamilyParams{InterfaceNames: ifs}) | |
| cfgplugins.IsIPv4InterfaceARPresolved(t, ate, cfgplugins.AddressFamilyParams{InterfaceNames: interfaceNamesList}) | |
| cfgplugins.IsIPv6InterfaceARPresolved(t, ate, cfgplugins.AddressFamilyParams{InterfaceNames: interfaceNamesList}) |
| func ConfigureDUT(t *testing.T, dut *ondatra.DUTDevice) { | ||
| t.Helper() | ||
| dp1 := dut.Port(t, "port1") | ||
| dp2 := dut.Port(t, "port2") | ||
| d := gnmi.OC() | ||
| vrfBatch := new(gnmi.SetBatch) | ||
|
|
||
| ConfigureHardwareInit(t, dut) | ||
| CreateGRIBIScaleVRFs(t, dut, vrfBatch) | ||
| portList := []*ondatra.Port{dp1, dp2} | ||
| dutPortAttrs := []attrs.Attributes{dutPort1Attr, dutPort2Attr} | ||
|
|
||
| for idx, a := range dutPortAttrs { | ||
| p := portList[idx] | ||
| intf := a.NewOCInterface(p.Name(), dut) | ||
| gnmi.BatchUpdate(vrfBatch, d.Interface(p.Name()).Config(), intf) | ||
| t.Logf("Configured DUT port %s (%s)", p.Name(), a.Desc) | ||
| } | ||
| fptest.ConfigureDefaultNetworkInstance(t, dut) | ||
|
|
||
| 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) | ||
| } | ||
| // Configure sub-interfaces on port1 (1 VLAN) and port2 (640 VLANs). | ||
| ConfigureDUTSubinterfaces(t, vrfBatch, new(oc.Root), dut, dp1, DUTPort1IPv4Start, DUTPort1IPv6Start, StartVLANPort1, NumPort1VLANs) | ||
| ConfigureDUTSubinterfaces(t, vrfBatch, new(oc.Root), dut, dp2, DUTPort2IPv4Start, DUTPort2IPv6Start, StartVLANPort2, NumPort2VLANs) | ||
| vrfBatch.Set(t, dut) | ||
| // TODO: VRF selection policy must be configured (Fix: 500317744 defect). | ||
| ConfigureVRFSelectionPolicy(t, dut) | ||
| } |
There was a problem hiding this comment.
The ConfigureDUT function does not adhere to the cfgplugins guidelines. According to the repository style guide (lines 101-102 and 105-106), configuration plugins should append to a provided *gnmi.SetBatch and the calling test code should be responsible for executing .Set(). Additionally, the function signature should include a configuration struct.
References
- Functions in
cfgpluginsmust follow a specific signature:(t *testing.T, dut *ondatra.DUTDevice, sb *gnmi.SetBatch, cfg MyConfigStruct) *gnmi.SetBatch. The plugin should append to the batch, and the caller is responsible for calling.Set(). (link)
| 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.
This gnmi.Replace call should be refactored to use the *gnmi.SetBatch object. Configuration plugins should avoid direct gNMI operations and instead accumulate changes in a batch to be applied by the caller.
References
- Configuration plugins should append to a
*gnmi.SetBatchobject rather than applying the configuration directly. (link)
| if deviations.InterfaceRefConfigUnsupported(dut) { | ||
| intf.InterfaceRef = nil | ||
| } | ||
| gnmi.Replace(t, dut, gnmi.OC().NetworkInstance(defaultVRF).PolicyForwarding().Config(), pf) |
There was a problem hiding this comment.
The ConfigureVRFSelectionPolicy function uses gnmi.Replace directly instead of appending to a *gnmi.SetBatch. This violates the cfgplugins guidelines which require batching configuration updates to allow callers to compose and apply configurations efficiently.
References
- Configuration plugins should append to a
*gnmi.SetBatchobject rather than applying the configuration directly. (link)
|
/fptest virtual |
|
/fptest virtual |
|
/fptest virtual |
|
/fptest virtual |
3 participants
No description provided.