-
Notifications
You must be signed in to change notification settings - Fork 197
BMP-2.8: BMP Post Policy Test #4942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @ANISH-GOTTAPU, 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 introduces a comprehensive test for BGP Monitoring Protocol (BMP) post-policy route monitoring, ensuring that devices correctly report routes after policy application. It includes significant updates to BGP configuration plugins to support advanced routing policies and introduces a new BMP configuration plugin that intelligently adapts to platform capabilities, falling back to CLI commands when OpenConfig-based BMP is not fully supported. These changes enhance the testing framework's robustness and compatibility across different network devices. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 #4942 / c9b7556Virtual Devices
Hardware Devices
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new OTG test for BMP post-policy monitoring. It also includes necessary extensions to the BGP configuration plugin and adds a new BMP configuration plugin to support this test, particularly for platforms requiring CLI-based configuration.
Overall, the changes are good, but there are several issues in the new test file that need to be addressed. The pull request title and description are misleading as they refer to a 'Pre Policy Test', while the implementation is for a post-policy test. The test file itself contains incorrect constant values for IPv4 route counts, as well as misleading names and log messages that should be corrected for clarity and correctness.
feature/bgp/bmp/otg_tests/bmp_post_policy_test/bmp_post_policy_test.go
Outdated
Show resolved
Hide resolved
feature/bgp/bmp/otg_tests/bmp_post_policy_test/bmp_post_policy_test.go
Outdated
Show resolved
Hide resolved
feature/bgp/bmp/otg_tests/bmp_post_policy_test/bmp_post_policy_test.go
Outdated
Show resolved
Hide resolved
feature/bgp/bmp/otg_tests/bmp_post_policy_test/bmp_post_policy_test.go
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 20808398124Details
💛 - Coveralls |
|
@ANISH-GOTTAPU - I validated this PR in my setup and i see the BMP session does not get established. Similar to PR4912 where i saw the BMP session is not getting established. |
As discussed over the chat, I ran the test with latest image and shared the config for BMP. |
| } | ||
| } | ||
|
|
||
| // ConfigureBMP applies BMP station configuration on DUT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ANISH-GOTTAPU - Can you please take a relook at this code. PR4912 is now merged and hence this either is not needed or you will just need to rebase your branch and rewrite this part of code
| // Arista: b/335739231 | ||
| bool match_community_set_match_set_options_all_unsupported = 361; | ||
|
|
||
| // Arista https://partnerissuetracker.corp.google.com/issues/452484903 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ANISH-GOTTAPU - Since PR4912 is now merged.. Can you please remove this deviation configurations.. Only metadata.textproto needs to be configured.
| return lookupDUTDeviations(dut).GetMatchCommunitySetMatchSetOptionsAllUnsupported() | ||
| } | ||
|
|
||
| // BMPOCUnsupported returns true if BMP configuration is not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ANISH-GOTTAPU - This deviation changes are not required in this PR as PR4912 is now merged. Feel free to remove this now.
BMP-2.8: BMP Post Policy Test with OTG
pass log : https://partnerissuetracker.corp.google.com/issues/415458482#comment148