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 enhances the BGP Monitoring Protocol (BMP) test suite by introducing vendor-specific configurations, particularly for Juniper devices. It refactors existing BGP configuration logic to support platform-specific CLI commands for features like maximum routes and multipath, and ensures consistent default network instance settings in tests. The changes aim to improve the robustness and compatibility of BGP and BMP configurations across different network device vendors. 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
|
Pull Request Functional Test Report for #5202 / e6876c6Virtual Devices
Hardware Devices
|
![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 vendor-specific configurations for BMP tests, particularly for Juniper, and refactors the default network instance configuration. The changes are generally good, but I've found a critical bug in the new Juniper BMP configuration logic within internal/cfgplugins/bgp.go. The code incorrectly appends to a string builder without resetting it, which will generate invalid CLI commands and cause test failures. I've also noted a deviation from the repository's style guide for cfgplugins. I've provided detailed comments with suggested fixes for these issues.
| case ondatra.JUNIPER: | ||
| fmt.Fprintf(bmpConfig, ` | ||
| routing-options { | ||
| autonomous-system %d; | ||
| bmp { | ||
| statistics-timeout 30; | ||
| station r-bmp { | ||
| connection-mode active; | ||
| route-monitoring { | ||
| loc-rib; | ||
| pre-policy; | ||
| post-policy; | ||
| rib-out pre-policy post-policy; | ||
| } | ||
| station-address %s; | ||
| station-port %d; | ||
| } | ||
| } | ||
| }`, cfgParams.DutAS, cfgParams.StationAddr, cfgParams.StationPort) | ||
| helpers.GnmiCLIConfig(t, dut, bmpConfig.String()) | ||
|
|
||
| if cfgParams.PrePolicy { | ||
| t.Log("Configured BMP station with pre-policy export") | ||
| fmt.Fprintf(bmpConfig, ` | ||
| routing-options { | ||
| bmp { | ||
| station r-bmp { | ||
| route-monitoring { | ||
| pre-policy; | ||
| rib-out pre-policy; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `) | ||
| helpers.GnmiCLIConfig(t, dut, bmpConfig.String()) | ||
| } | ||
|
|
||
| if cfgParams.PostPolicy { | ||
| t.Log("Configured BMP station with post-policy export") | ||
| fmt.Fprintf(bmpConfig, ` | ||
| routing-options { | ||
| bmp { | ||
| station r-bmp { | ||
| route-monitoring { | ||
| post-policy; | ||
| rib-out post-policy; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `) | ||
| helpers.GnmiCLIConfig(t, dut, bmpConfig.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a bug in how the Juniper BMP configuration is constructed. The strings.Builder (bmpConfig) is continuously appended to within the if cfgParams.PrePolicy and if cfgParams.PostPolicy blocks without being reset. This will lead to an invalid CLI configuration string if more than one of these blocks is executed, as it will concatenate multiple routing-options structures.
To fix this, you should call bmpConfig.Reset() before generating the configuration within each conditional block.
Additionally, the configuration logic appears redundant. The initial block configures pre-policy, post-policy, and rib-out pre-policy post-policy. The conditional blocks then re-apply a subset of this configuration. Consider refactoring to build a single configuration block based on the provided flags to avoid sending multiple, potentially overlapping, configuration updates.
case ondatra.JUNIPER:
fmt.Fprintf(bmpConfig, `
routing-options {
autonomous-system %d;
bmp {
statistics-timeout 30;
station r-bmp {
connection-mode active;
route-monitoring {
loc-rib;
pre-policy;
post-policy;
rib-out pre-policy post-policy;
}
station-address %s;
station-port %d;
}
}
}`, cfgParams.DutAS, cfgParams.StationAddr, cfgParams.StationPort)
helpers.GnmiCLIConfig(t, dut, bmpConfig.String())
if cfgParams.PrePolicy {
bmpConfig.Reset()
t.Log("Configured BMP station with pre-policy export")
fmt.Fprintf(bmpConfig, `
routing-options {
bmp {
station r-bmp {
route-monitoring {
pre-policy;
rib-out pre-policy;
}
}
}
}
`)
helpers.GnmiCLIConfig(t, dut, bmpConfig.String())
}
if cfgParams.PostPolicy {
bmpConfig.Reset()
t.Log("Configured BMP station with post-policy export")
fmt.Fprintf(bmpConfig, `
routing-options {
bmp {
station r-bmp {
route-monitoring {
post-policy;
rib-out post-policy;
}
}
}
}
`)
helpers.GnmiCLIConfig(t, dut, bmpConfig.String())
}
}| case ondatra.JUNIPER: | ||
| fmt.Fprintf(bmpConfig, ` | ||
| routing-options { | ||
| autonomous-system %d; | ||
| bmp { | ||
| statistics-timeout 30; | ||
| station r-bmp { | ||
| connection-mode active; | ||
| route-monitoring { | ||
| loc-rib; | ||
| pre-policy; | ||
| post-policy; | ||
| rib-out pre-policy post-policy; | ||
| } | ||
| station-address %s; | ||
| station-port %d; | ||
| } | ||
| } | ||
| }`, cfgParams.DutAS, cfgParams.StationAddr, cfgParams.StationPort) | ||
| helpers.GnmiCLIConfig(t, dut, bmpConfig.String()) | ||
|
|
||
| if cfgParams.PrePolicy { | ||
| t.Log("Configured BMP station with pre-policy export") | ||
| fmt.Fprintf(bmpConfig, ` | ||
| routing-options { | ||
| bmp { | ||
| station r-bmp { | ||
| route-monitoring { | ||
| pre-policy; | ||
| rib-out pre-policy; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `) | ||
| helpers.GnmiCLIConfig(t, dut, bmpConfig.String()) | ||
| } | ||
|
|
||
| if cfgParams.PostPolicy { | ||
| t.Log("Configured BMP station with post-policy export") | ||
| fmt.Fprintf(bmpConfig, ` | ||
| routing-options { | ||
| bmp { | ||
| station r-bmp { | ||
| route-monitoring { | ||
| post-policy; | ||
| rib-out post-policy; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `) | ||
| helpers.GnmiCLIConfig(t, dut, bmpConfig.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation for Juniper in this case block violates the cfgplugins guidelines from CONTRIBUTING.md (summarized in internal/cfgplugins/README.md). The plugin directly applies configuration using helpers.GnmiCLIConfig instead of appending it to the gnmi.SetBatch.
According to the style guide, cfgplugins should not apply configuration themselves. They should add to the batch, and the calling test should execute batch.Set(). This allows for composing multiple configurations before applying them.
This function should be refactored to create a gnmi.Update with origin: "cli" and add it to the provided batch.
References
- cfgplugins functions should append configuration to a
gnmi.SetBatchobject and return it. The calling test is responsible for applying the configuration. This function directly applies configuration usinghelpers.GnmiCLIConfig, violating this pattern. (link)
3 participants
Hi,