[WIP] Giro/pf 1.22 cisco patch#5506
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 necessary infrastructure and configuration updates to enable PF-1.22 testing on Cisco devices. It includes vendor-specific handling for aggregate interfaces, GUE decapsulation via CLI, and improvements to the robustness of protocol startup and traffic verification in the existing test 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
|
![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 GUE decapsulation and hashing test to support IPv6 outer headers and adds Cisco-specific CLI configurations for decapsulation. Key feedback includes avoiding direct use of dut.Vendor() in favor of the deviations package, removing unnecessary time.Sleep calls, restoring commented-out flow indices to maintain test coverage, cleaning up unused variables and commented-out code, and ensuring all new deviations are tracked with valid issue tracker URLs.
| // TODO: remove this | ||
| if dut.Vendor() == ondatra.ARISTA { | ||
| cfgplugins.ConfigureLoadbalance(t, dut) | ||
| } |
There was a problem hiding this comment.
Avoid using dut.Vendor() for vendor-specific logic or configurations in tests. Instead, use the deviations package to maintain test abstraction and portability across different vendors. Since load_balance_policy_oc_unsupported is defined as a deviation, you should use deviations.LoadBalancePolicyOCUnsupported(dut) here.
if deviations.LoadBalancePolicyOCUnsupported(dut) {
cfgplugins.ConfigureLoadbalance(t, dut)
}References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| ate.OTG().PushConfig(t, top) | ||
| time.Sleep(pushStartWaitTime) | ||
| t.Log("Starting protocols...") |
There was a problem hiding this comment.
Avoid using time.Sleep to wait between pushing config and starting protocols. The style guide recommends using gnmi.Watch with .Await for waiting on conditions, which is already handled by waitForOTGProtocolsUpWithRetry right after starting protocols. This unnecessary sleep adds a 20-second delay to every test run.
| ate.OTG().PushConfig(t, top) | |
| time.Sleep(pushStartWaitTime) | |
| t.Log("Starting protocols...") | |
| ate.OTG().PushConfig(t, top) | |
| t.Log("Starting protocols...") |
References
- Avoid time.Sleep: Use gnmi.Watch with .Await for waiting on conditions. (link)
| activeFlowIndices := []int{ | ||
| 2, // First ECMP case: keep only this forwarding check enabled for now. | ||
| // 1, | ||
| // 3, | ||
| // 4, | ||
| // 5, | ||
| // 6, | ||
| // 7, | ||
| // 8, | ||
| // 9, | ||
| // 10, | ||
| } |
There was a problem hiding this comment.
Commenting out almost all flow indices severely reduces test coverage. Please ensure that all test cases/flows are restored and executed before merging this pull request.
| } | ||
| } | ||
| t.Logf("Load balancing has been verified on the LAG interfaces.") | ||
| _ = excludeLag |
There was a problem hiding this comment.
The excludeLag variable is completely unused now because testLoadBalance no longer accepts it as a parameter. Instead of using the blank identifier _ = excludeLag to suppress the compiler error, please remove the variable and its assignments entirely.
| aggIDsList = append(aggIDsList, AggregateInterfaceIDs[dut.Vendor()][(aggID1)]) | ||
| //cfgplugins.DeleteAggregate(t, dut, AggregateInterfaceIDs[dut.Vendor()][(aggID1)], dutAggPorts1) |
There was a problem hiding this comment.
Commented-out code should be removed to keep the codebase clean and maintainable. Also, please avoid using dut.Vendor() directly in tests to maintain vendor abstraction.
| aggIDsList = append(aggIDsList, AggregateInterfaceIDs[dut.Vendor()][(aggID1)]) | |
| //cfgplugins.DeleteAggregate(t, dut, AggregateInterfaceIDs[dut.Vendor()][(aggID1)], dutAggPorts1) | |
| aggIDsList = append(aggIDsList, AggregateInterfaceIDs[dut.Vendor()][(aggID1)]) |
References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| aggIDsList = append(aggIDsList, AggregateInterfaceIDs[dut.Vendor()][(aggID2)]) | ||
| //cfgplugins.DeleteAggregate(t, dut, AggregateInterfaceIDs[dut.Vendor()][(aggID2)], dutAggPorts2) |
There was a problem hiding this comment.
Commented-out code should be removed to keep the codebase clean and maintainable. Also, please avoid using dut.Vendor() directly in tests to maintain vendor abstraction.
| aggIDsList = append(aggIDsList, AggregateInterfaceIDs[dut.Vendor()][(aggID2)]) | |
| //cfgplugins.DeleteAggregate(t, dut, AggregateInterfaceIDs[dut.Vendor()][(aggID2)], dutAggPorts2) | |
| aggIDsList = append(aggIDsList, AggregateInterfaceIDs[dut.Vendor()][(aggID2)]) |
References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| bool prefix_limit_exceeded_telemetry_unsupported = 139; | ||
| // Skip setting allow-multiple-as while configuring eBGP | ||
| // Arista: partnerissuetracker.corp.google.com/issues/317422300 | ||
| // TODO CISCO: Add bug here |
There was a problem hiding this comment.
Deviations must be tracked with an issue tracker URL. Please replace this placeholder TODO with a valid issue tracker link (e.g., https://issuetracker.google.com/issues/xxxxx or https://github.com/openconfig/featureprofiles/issues/xxxxx).
References
- Add a comment to the accessor function containing a URL link to an issue tracker which tracks removal of the deviation. The format should be https://issuetracker.google.com/issues/xxxxx. (link)
| bool macsec_unsupported = 279; | ||
|
|
||
| // Arista b/390506900 | ||
| // TODO: Add Cisco bug here |
There was a problem hiding this comment.
Deviations must be tracked with an issue tracker URL. Please replace this placeholder TODO with a valid issue tracker link (e.g., https://issuetracker.google.com/issues/xxxxx or https://github.com/openconfig/featureprofiles/issues/xxxxx).
References
- Add a comment to the accessor function containing a URL link to an issue tracker which tracks removal of the deviation. The format should be https://issuetracker.google.com/issues/xxxxx. (link)
2 participants
Intermediate fix to test PF-1.22 with IXIA for Cisco DUT