Repository navigation
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 #5871 / 75bba74Virtual 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 enhances the supervisor failure test suite by adding IPv6 support and improving the robustness of FIB validation. It introduces several utility functions to streamline gRIBI operations and telemetry verification, allowing for more granular testing of route persistence during controller switchover events. 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 gRIBI supervisor failure test to support dual-stack (IPv4 and IPv6) validation and adds a post-switchover Phase 2 FIB validation. It refactors prefix installation and verification, moves switchover helpers to a shared package, and adds batch-add and verification methods to the gRIBI client. The review feedback suggests improving test robustness by deferring traffic shutdown, optimizing telemetry verification to run concurrently rather than sequentially, and fixing a potential panic in telemetry validation when a path is not present.
71a7ca3 to
e7acb9d
Compare
e7acb9d to
6614912
Compare
c2edc44 to
07fefb3
Compare
07fefb3 to
a2b26f6
Compare
|
/Gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the supervisor failure test to support dual-stack (IPv4 and IPv6) traffic, introducing helper functions to generate IP prefixes, appending dual-stack flows, and verifying traffic loss during and after a supervisor switchover. The gRIBI client is also updated to support custom redundancy modes and bulk IP prefix additions. The review feedback highlights critical issues: first, using gnmi.Watch during a supervisor switchover can be unreliable due to gNMI server restarts, so a manual polling loop is recommended; second, the generateIPv4Prefixes and generateIPv6Prefixes helper functions lack nil checks on IP parsing and use unsafe byte incrementing, which can cause runtime panics or incorrect prefix generation.
|
/fptest virtual |
|
/fptest physical |
f5fa1b4 to
0a6d46a
Compare
|
/fptest virtual |
|
/fptest physical |
|
/fptest juniper-physical |
|
/fptest arista-physical |
15b4c67 to
f7605f7
Compare
|
/Gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the supervisor failure test to support dual-stack (IPv4 and IPv6) configurations and validates traffic flow across supervisor switchovers. It also introduces a new deviation to gracefully close the gRIBI client before switchover if required by the platform. Review feedback recommends checking for nil returns from net.ParseIP to avoid runtime panics, adding a sleep delay in the gRIBI reconnection retry loop to prevent a tight loop, and registering clientB cleanup operations using t.Cleanup() after a successful start instead of using defer statements before starting, in alignment with the repository style guide.
0f154db to
ee871eb
Compare
|
/Gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the supervisor failure test to support dual-stack (IPv4 and IPv6) traffic and validates both TE-8.2.1 and TE-8.2.2 test cases. It introduces a new deviation gribi_close_client_before_switchover to gracefully close the gRIBI client before a switchover if required by the platform, and adds helper methods to the gRIBI client for batch programming of IPv4 and IPv6 prefixes. The feedback recommends registering a deferred cleanup to stop ATE traffic when starting it, ensuring that traffic does not run indefinitely if the test fails or times out. Comment 1 was removed because it suggests using a manual polling loop with static sleeps, which is a banned anti-pattern under the repository's style guide.
|
/fptest physical |
1 similar comment
|
/fptest physical |
e178e2a to
5040c15
Compare
5040c15 to
aaa1f1b
Compare
|
/Gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the supervisor failure test to support dual-stack (IPv4 and IPv6) traffic flows, introduces the gribi_close_client_before_switchover deviation, and refactors the test logic to improve switchover and telemetry validation. The review feedback recommends avoiding gnmi.Watch with .Await when waiting for a device to boot up after a switchover to prevent connection errors, suggesting a manual polling loop with gnmi.Get instead. Additionally, the reviewer suggests using the modern net/netip package for safer IP address parsing, registering the ATE protocol cleanup immediately after configuration to guarantee execution, and simplifying the client closing logic.
|
/fptest physical |
6ea4180 to
725d241
Compare
…into P4RT_supervisor_failure_test # Conflicts: # internal/deviations/deviations.go # proto/metadata.proto # proto/metadata_go_proto/metadata.pb.go
a8e420e to
af9fb5f
Compare
|
/fptest physical |
|
/fptest physical |
…into P4RT_supervisor_failure_test
This branch has not been deployed
6 participants
This test verifies hitless failover and route persistence on dual-supervisor routers by injecting 100 gRIBI routes (50 IPv4 and 50 IPv6), initiating continuous data-plane traffic, and triggering a supervisor switchover via gNOI (SwitchControlProcessor). It ensures that forwarding hardware continues to pass packets with 0% packet loss throughout the outage, confirms that the new primary supervisor accurately updates its OpenConfig redundancy telemetry, and proves the control plane remains healthy by successfully reconnecting and programming 100 additional routes post-switchover without disrupting the existing forwarding table.