Skip to content
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

Updated BGP AF module for PIP IP feature #62

Merged
merged 7 commits into from
Feb 11, 2022
Merged

Updated BGP AF module for PIP IP feature #62

merged 7 commits into from
Feb 11, 2022

Conversation

stalabi1
Copy link
Collaborator

@stalabi1 stalabi1 commented Feb 10, 2022

SUMMARY

BGP AF module needed to be updated with advertise-pip, advertise-pip-ip, advertise-pip-peer-ip, and advertise-svi-ip attributes for PIP IP feature

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

sonic_bgp_af

ADDITIONAL INFORMATION

bgp_af_regression_report.pdf
show_running_configuration_bgp_output.txt
show_running_configuration_bgp_output2.txt

@kerry-meyer
Copy link
Collaborator

Please verify that execution of the playbook operations to add (merge) each of these new attributes causes the expected corresponding additions/changes to the CLI "show running-configuration" output and add an enclosure to this PR containing the "show running-configuration" output.

@stalabi1
Copy link
Collaborator Author

@kerry-meyer Added 'show running-configuration bgp' output.

@kerry-meyer
Copy link
Collaborator

Thanks for posting the "show running-configuration bgp' output.

There is one additional case that should be verified:

If the bool "advertise-pip" value is set to "true" with no local or peer IP address configured, the corresponding CLI output should reflect this:

For example:

sonic(config-router-bgp-af)# do show running-configuration bgp
!
router bgp 30
log-neighbor-changes
timers 60 180
!
address-family l2vpn evpn
advertise-pip <<<<<<<<<

If either of the IP addresses is configured (local or peer), only that configuration will be displayed, and the value of the "advertise-pip" bool configuration is not displayed.

Please execute and post the "show running-configuration" result for an additional test to verify that the "advertise-pip" configuration displays as expected after executing the change via an Ansible playbook to set this bool without also setting a value for either of the IP addresses.

@kerry-meyer
Copy link
Collaborator

kerry-meyer commented Feb 11, 2022

Please also consider the comment that I have posted inline for the "plugins/module_utils/network/sonic/config/bgp_af/bgp_af.py" file to reduce repeated code for the "get_delete__request" functions.

It looks like all of the functions in the group from " get_delete_advertise_pip_request" through "get_delete_advertise_all_vni_request", with the exception of "get_delete_dampening_request", could be eliminated through the use of one new common function (e.g. "get_delete_evpn_attribute_request").

Copy link
Collaborator

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional changes and UT verification look good. Thank you for providing these.

Approved.

@kerry-meyer kerry-meyer merged commit e25fb8a into ansible-collections:main Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants