Fix LACP configuration setup for TestLACPMember and TestLagMacAddress gNMI-1.10#5034
Fix LACP configuration setup for TestLACPMember and TestLagMacAddress gNMI-1.10#5034youssefyamoutyy wants to merge 3 commits intoopenconfig:mainfrom
Conversation
Summary of ChangesHello @youssefyamoutyy, 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 significantly enhances the reliability and independence of LACP-related tests by introducing dynamic configuration management. It provides helper functions to programmatically set up and tear down LACP, LAG, and member port configurations on the DUT. This change eliminates the dependency on pre-loaded LACP configurations, preventing tests from being skipped and ensuring a consistent testing environment through atomic updates and complete cleanup. 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. 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
|
Pull Request Functional Test Report for #5034 / c57d2ecVirtual Devices
Hardware Devices
|
youssefyamoutyy
changed the title
![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 helper functions to dynamically set up and tear down LACP configurations for tests, which is a great improvement. My review focuses on improving the robustness of these new helpers. I've suggested replacing a time.Sleep with a more reliable gnmi.Await to wait for state changes, in accordance with the repository's style guide. I've also recommended making the teardown process atomic using a gnmi.SetBatch for consistency with the atomic setup.
| // Apply all configurations atomically | ||
| gnmi.Update(t, dut, gnmi.OC().Config(), d) | ||
| t.Logf("Configured LACP LAG interface %s with ports %s and %s", aggID, dp1.Name(), dp2.Name()) | ||
| time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
The use of time.Sleep is discouraged as it can lead to flaky tests. The repository style guide (lines 53-54) recommends using gnmi.Watch with .Await to wait for specific conditions. Instead of a fixed sleep, you should wait for the aggregate interface to become operationally UP.
| time.Sleep(2 * time.Second) | |
| gnmi.Await(t, dut, gnmi.OC().Interface(aggID).OperStatus().State(), 2*time.Minute, operStatusUp) |
References
- The style guide explicitly states to avoid using
time.Sleepand instead usegnmi.Watchwith.Awaitfor waiting on conditions to prevent flaky tests. (link)
| for _, port := range []*ondatra.Port{dp1, dp2} { | ||
| gnmi.Delete(t, dut, gnmi.OC().Interface(port.Name()).Ethernet().AggregateId().Config()) | ||
| } | ||
|
|
||
| // Remove LAG interface | ||
| gnmi.Delete(t, dut, gnmi.OC().Interface(aggID).Config()) | ||
|
|
||
| // Remove LACP interface configuration | ||
| gnmi.Delete(t, dut, gnmi.OC().Lacp().Interface(aggID).Config()) |
There was a problem hiding this comment.
The teardownLACPConfig function uses multiple gnmi.Delete calls, which are not atomic. While the setup is atomic, the teardown is not. For consistency and robustness, it's better to perform the teardown atomically as well. You can achieve this by using a gnmi.SetBatch to group all delete operations into a single gNMI Set request.
b := &gnmi.SetBatch{}
for _, port := range []*ondatra.Port{dp1, dp2} {
gnmi.BatchDelete(b, gnmi.OC().Interface(port.Name()).Ethernet().AggregateId().Config())
}
// Remove LAG interface
gnmi.BatchDelete(b, gnmi.OC().Interface(aggID).Config())
// Remove LACP interface configuration
gnmi.BatchDelete(b, gnmi.OC().Lacp().Interface(aggID).Config())
b.Set(t, dut)
Pull Request Test Coverage Report for Build 21530991490Details
💛 - Coveralls |
4 participants
Brief description and need for this PR
TestLagMacAddressandTestLACPMembertests were being skipped when base LACP configuration was not pre-loaded on the deviceProposed changes
Implemented dynamic LACP configuration setup/teardown helper functions that allows those two tests to run.
The change uses one
gNMI.Update()to apply all LACP, LAG, and member port configs simultaneously.The changes also ensure complete teardown of configurations after test completion and atomic updates prevent connection loss and intermediate bad states.