Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Pull Request Functional Test Report for #5209 / afc221bVirtual Devices
Hardware Devices
|
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 resolves critical issues preventing the TUN-1.4 interface-based IPv6 GRE encapsulation test from running successfully on Cisco devices. It integrates Cisco-specific tunnel configuration logic, standardizes network instance setup, and significantly improves the reliability of ECMP load balancing validation, ensuring the test accurately reflects forwarding behavior rather than failing due to platform variations. Highlights
Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 introduces support for Cisco devices in the TUN-1.4 test case by adding necessary deviations and a vendor-specific CLI configuration function for GRE tunnels. A key improvement is the complete rewrite of the ECMP load-balancing validation logic, making it significantly more robust by basing calculations on observed traffic and adding better input validation and undercount checks. The changes are well-aligned with the description and substantially improve the test's reliability. I have one minor suggestion to improve code consistency, and the use of t.Fatalf in the suggested code is consistent with our guidelines for critical test failures.
| switch dut.Vendor() { | ||
| case ondatra.JUNIPER: | ||
| config = configureTunnelEndPoints(intf, unit, tunnelSrc, tunnelDst, tunnelIpv6address, Ipv6Mask) | ||
| case ondatra.CISCO: | ||
| config = configureTunnelEndPointsCisco(intf, unit, tunnelDst, tunnelIpv6address, Ipv6Mask) | ||
| t.Logf("Push the CLI config:\n%s", config) | ||
|
|
||
| default: | ||
| t.Errorf("Invalid Tunnel endpoint configuration") | ||
| t.Fatalf("Tunnel endpoint configuration is not defined for %s", dut.Vendor()) | ||
| } |
There was a problem hiding this comment.
For better consistency and to avoid duplicating the logging statement if more vendors are added, consider moving the t.Logf call to after the switch statement. This ensures that the generated config is logged for any vendor that provides one, without needing to add the log line in each case.
switch dut.Vendor() {
case ondatra.JUNIPER:
config = configureTunnelEndPoints(intf, unit, tunnelSrc, tunnelDst, tunnelIpv6address, Ipv6Mask)
case ondatra.CISCO:
config = configureTunnelEndPointsCisco(intf, unit, tunnelDst, tunnelIpv6address, Ipv6Mask)
default:
t.Fatalf("Tunnel endpoint configuration is not defined for %s", dut.Vendor())
}
t.Logf("Push the CLI config for %s:\n%s", dut.Vendor(), config)References
- Using
t.Fatalfin the default case for an unsupported vendor configuration is appropriate as it represents a critical setup failure, aligning with rules that allowt.Fatalfor critical components or processes not found (Rules 2 and 3).
2 participants
Added Cisco deviation for TUN-1.4 in
tunnel_state_path_unsupported = true
tunnel_config_path_unsupported = true
This enables Cisco to use deviation-aware paths in this test.
Updated Cisco tunnel endpoint programming path in
Added explicit Cisco branch in configureTunnelInterface.
Added hard failure if generated CLI config is empty.
Added Cisco CLI generator in
Uses interface tunnel-ipN
Configures GRE IPv6 encapsulation mode, loopback source, destination, and tunnel IPv6 address.
This fixed prior CLI syntax/semantic issues seen on Cisco runs.
Network-instance setup was normalized via helper in
Use fptest.ConfigureDefaultNetworkInstance to avoid per-platform NI inconsistencies.
ECMP validation logic was made robust in
Keeps formula-based expected total for observability/logging.
Uses observed egress deltas as authoritative total for per-link balance checks.
Adds zero-observed guard and undercount guard using minObservedTotalRatioPct.
Retains tolerance-window validation per link.
Added new threshold constant in
minObservedTotalRatioPct = 80
Prevents silent pass when DUT egress is unexpectedly low versus formula expectation