T7364: Fixing Route reflector client check not working for peer-group #4452
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👍 |
|
Update please the Use git commit --amend + force-push |
My bad, just fixed it |
|
Smoketest does not like your changes :) |
|
Just looked into it quickly, seems to be because the fix i did is only working when using the route reflector client on the neighbor directly, not in a peer group, i'll check if i can add a condition to fix it |
|
Should be fixed, when changing the condition, there was no check if we were in a peer group, so it tried to find the peer group of a peer group to find the ASN, failing to do so and exiting the if. elif neighbor == 'peer_group':
peer_group_as = peer_config.get('remote_as') |
|
CI integration ❌ failed! Details
|
|
The failing in the tests seems unrelated : def test_ethtool_evpn_uplink_tracking(self):
for interface in self._interfaces:
self.cli_set(self._base_path + [interface, 'evpn', 'uplink'])
self.cli_commit()
for interface in self._interfaces:
frrconfig = self.getFRRconfig(f'interface {interface}', endsection='^exit')
self.assertIn(' evpn mh uplink', frrconfig)Maybe a change in current that affected the tests ? |
It's a false positive caused by a timing error when reading back the |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the route reflector client validation when using a peer-group in BGP configurations. The changes update variable names and the conditional logic to correctly validate the remote AS values from both the peer and its peer-group.
- Rename variable from peer_group_as to peer_as for clarity.
- Adjust conditional checks to verify remote AS values correctly.
- Add a fallback check into the peer group if the direct check does not pass.
Comments suppressed due to low confidence (1)
src/conf_mode/protocols_bgp.py:422
- The variable 'peer_group' is not defined in this context. Consider using 'peer_config["peer_group"]' to retrieve the peer group name before passing it to dict_search.
peer_group_as = dict_search(f'peer_group.{peer_group}.remote_as', bgp)
|
@Hanarion, can you add an example of the set commands to reproduce when it failed? (which exac issue solves this PR) |
Sure, here is an example (Also added in the original PR content) : |
sever-sever
approved these changes
May 1, 2025
dmbaturin
approved these changes
May 1, 2025
github-actions
bot
added
the
mirror-initiated
vyosbot
added
mirror-completed
and removed
mirror-initiated
|
@Hanarion there is a bug after this commit https://vyos.dev/T7497 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change summary
Fixing the route reflector client check in order for it to work when using a peer-group
Types of changes
Related Task(s)
https://vyos.dev/T7364
Related PR(s)
None
How to test / Smoketest result
Create a peer group, add a neighbour using this peer group and try to add the route-reflector-client attribute.
Checklist: