Repository navigation
P4RT-5.3 : Add settle delays around StartProtocols so BGP reconverges before traffic - #6171
ArpitJoshi18 wants to merge 1 commit into
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 addresses intermittent PacketIn egress port failures in the P4RT-5.3 test by introducing explicit settle delays. By allowing sufficient time for BGP to reconverge following configuration and protocol initialization, the test stability is improved. 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 introduces static time.Sleep delays in packetin_test.go before and after starting OTG protocols to allow for BGP convergence. The review feedback correctly identifies this as a violation of the style guide, which prohibits static sleeps. The reviewer recommends replacing these with dynamic waiting mechanisms, such as gnmi.Watch or otgutils.WaitForARP, to prevent unnecessary delays in the test execution pipeline.
| time.Sleep(30 * time.Second) | ||
| ate.OTG().StartProtocols(t) | ||
| // Wait for the StartProtocols BGP flap to reconverge before sending traffic. | ||
| time.Sleep(60 * time.Second) |
There was a problem hiding this comment.
Avoid using static time.Sleep calls after pushing configurations or starting protocols in OTG tests, as they act as a fixed tax on the test execution pipeline. Instead, rely on dynamic waiting mechanisms. For example, use otgutils.WaitForARP to wait for ARP resolution, or use dynamic checks (like gnmi.Watch with .Await) to wait for BGP neighbor session states to reach ESTABLISHED before proceeding.
References
- Eliminating Polling Loops and Static Sleeps: Relying on static
time.Sleepcalls is banned. The use ofgnmi.Watchis mandatory to detect desired states in real-time. Wait times should not be hardcoded, as static sleeps become a fixed tax on the pipeline. (link) - Avoid using static sleeps (such as
time.Sleep) after pushing configurations or starting protocols in OTG tests. Instead, rely on dynamic waiting mechanisms likeWaitForARPafter starting protocols to ensure that the configuration push and protocol startup were successful.
Pull Request Functional Test Report for #6171 / 094e003Virtual Devices
Hardware Devices
|
This branch has not been deployed
2 participants
Mirror the TE-17.1 vrf_policy_driven_te as baseline setup to add settle pattern in the P4RT-5.3 test:
30s after PushConfig and 60s after StartProtocols.
This lets BGP reconverge
after the StartProtocols flap before traffic, fixing intermittent PacketIn
egress port 0 failures.