RT-7.5: Change extended community verification to wait for expected value#5332
RT-7.5: Change extended community verification to wait for expected value#5332andrewzhang624 wants to merge 1 commit intoopenconfig:mainfrom
Conversation
…alue Edited functions validateRouteCommunity(V4|V6)Prefix in link_bandwidth_test.go to move the matching of the link-bandwidth extended community into the predicate of the WatchAll call. Correspondingly changed the `t.Errorf` calls to `t.Logf` calls accompanied by `return false` to retry, and added `t.Errorf` after WatchAll timeout.
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 a race condition in the link bandwidth test suite where the test would intermittently fail because it attempted to validate route communities before the policy-driven updates were fully propagated. By moving the validation logic into the 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. Footnotes
|
|
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 #5332 / c14820eVirtual 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 refactors the BGP route community validation for IPv4 and IPv6 prefixes by moving the validation logic directly into the gnmi.WatchAll predicate. This change improves the efficiency of the tests and allows for more granular error reporting when a watch fails. The review feedback identifies a potential nil pointer dereference when accessing prefix values, a logic flaw where empty community lists might cause false positives in specific test cases, and suggests aligning error message formatting with Go style conventions by removing redundant prefixes and using lowercase.
| bgpPrefix, _ := v.Val() | ||
| if bgpPrefix.GetAddress() == v4Prefix { |
There was a problem hiding this comment.
The code ignores the present boolean returned by v.Val(). If the prefix is deleted during the watch, bgpPrefix will be nil, and calling bgpPrefix.GetAddress() will cause a panic. It is safer to check for presence before accessing the value.
| bgpPrefix, _ := v.Val() | |
| if bgpPrefix.GetAddress() == v4Prefix { | |
| bgpPrefix, present := v.Val() | |
| if present && bgpPrefix.GetAddress() == v4Prefix { |
| case "100:100": | ||
| for _, gotCommunity := range bgpPrefix.Community { | ||
| t.Logf("community AS:%d val: %d", gotCommunity.GetCustomAsNumber(), gotCommunity.GetCustomAsValue()) | ||
| if gotCommunity.GetCustomAsNumber() != 100 || gotCommunity.GetCustomAsValue() != 100 { | ||
| t.Errorf("ERROR: community is not 100:100 got AS number:%d AS value:%d", gotCommunity.GetCustomAsNumber(), gotCommunity.GetCustomAsValue()) | ||
| t.Logf("community is not 100:100 got AS number:%d AS value:%d", gotCommunity.GetCustomAsNumber(), gotCommunity.GetCustomAsValue()) | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
In the "100:100" case, if bgpPrefix.Community is empty, the loop is skipped and the function eventually returns true at line 519. This causes the watch to succeed even if the expected community has not arrived yet. You should add a check to ensure the community list is not empty.
| t.Errorf("ERROR: No Route found for prefix %v", v4Prefix) | ||
| } else { | ||
| t.Errorf("ERROR: For Prefix %v, did not get expected lb Bandwidth", v4Prefix) |
There was a problem hiding this comment.
The "ERROR: " prefix in t.Errorf is redundant as the testing framework already identifies the output as an error. Following Go conventions, error messages should be concise and typically start with a lowercase letter.
| t.Errorf("ERROR: No Route found for prefix %v", v4Prefix) | |
| } else { | |
| t.Errorf("ERROR: For Prefix %v, did not get expected lb Bandwidth", v4Prefix) | |
| t.Errorf("no route found for prefix %v", v4Prefix) | |
| } else { | |
| t.Errorf("for prefix %v, did not get expected lb bandwidth", v4Prefix) |
2 participants
Edited functions
validateRouteCommunity(V4|V6)Prefixin link_bandwidth_test.go to move the matching of the link-bandwidth extended community into the predicate of the WatchAll call.Correspondingly changed the
t.Errorfcalls tot.Logfcalls accompanied byreturn falsein order to retry, and addedt.Errorfafter WatchAll timeout.This fixes a test failure where an Arista DUT was failing with
ERROR: extended community is empty, expected %vdue to a race condition; currently, the function waits for any prefix to be present before grabbing a one-time snapshot withgnmi.GetAll. Because the prefix advertisements are continuously present throughout the test, this snapshot, if too soon, could fail to include the incoming update to implement the policy to add the community.Precedent for this waiting pattern is seen in
featureprofiles/feature/bgp/static_route_bgp_redistribution/otg_tests/static_route_bgp_redistribution_test/static_route_bgp_redistribution_test.go, in functionvalidateLearnedIPv4Prefix.