Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. #4620

judyjoseph · 2020-05-19T20:01:21Z

- Why I did it

The config bgp shutdown and startup commands were failing to toggle the state for the external BGP neighbor sessions. This was root-caused to the following error when adding an external peer with vtysh command line in bgpcfgd daemon.

% Peer with AS 65200 cannot be in this peer-group, members must be all internal or all external
line 16: Failure to communicate[13] to bgpd, line: neighbor 10.0.0.1 peer-group PEER_V4

In multi-npu platforms we have both internal bgp sessions between the npu's and external bgp sessions with external neighbors. The template was trying to add both the IBGP and EBP neighbors into the same bgp peer group which failed.

This resulted in the bgpcfgd's add_peer() functionality being partially successful, the neighbor was not put into any BGP PEER group.

- How I did it

Add new BGP peer group's PEER_V4_INT and PEER_V6_INT and add the internal BGP sessions ( IBGP ) neighbors into this group. The external BGP sessions ( EBGP ) neighbors will continue to be in the existing group PEER_V4 and PEER_V6 respectively.

- How to verify it
The config bgp shutdown and startup commands works fine and the BGP neighbor state is updated correctly.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

…BGP sessions will be added to this peer group while the external BGP sessions will be added to the exising PEER_V4 and PEER_V6 peer group.

lguohan · 2020-05-20T02:07:50Z

dockers/docker-fpm-frr/frr/bgpd/templates/general/instance.conf.j2

 {%   if CONFIG_DB__DEVICE_METADATA['localhost']['sub_role'] == 'BackEnd' %}
    neighbor {{ neighbor_addr }} route-map FROM_BGP_PEER_V4_INT in
 {%   endif %}
 {% elif neighbor_addr | ipv6 %}
  address-family ipv6
+{% if bgp_session["asn"] == bgp_asn %}


why is this not consistent with line 45?

Sure, I have updated here too to check "ASIC" keyword to identify internal sessions. thanks.

…ghbors.

pavel-shirshov · 2020-05-20T19:01:53Z

I'd suggest to solve this by following way:

Introduce new key for CONFIG_DB. Currently we have BGP_NEIGHBOR key for the bgp peer. We could use BGP_CHASSIS_NEIGHBOR for bgp neighbors for other asics on one chassis.
Then we can add a new object for BGP_CHASSIS_NEIGHBOR inside of bgpcfgd.
Then we can develop peer.conf, instance.conf and policy.conf for the BGP_CHASSIS_NEIGHBOR.

pavel-shirshov

I'd suggest to solve this by following way:

Introduce new key for CONFIG_DB. Currently we have BGP_NEIGHBOR key for the bgp peer. We could use BGP_CHASSIS_NEIGHBOR for bgp neighbors for other asics on one chassis.
Then we can add a new object for BGP_CHASSIS_NEIGHBOR inside of bgpcfgd.
Then we can develop peer.conf, instance.conf and policy.conf for the BGP_CHASSIS_NEIGHBOR.

judyjoseph · 2020-05-20T20:46:04Z

I'd suggest to solve this by following way:

Introduce new key for CONFIG_DB. Currently we have BGP_NEIGHBOR key for the bgp peer. We could use BGP_CHASSIS_NEIGHBOR for bgp neighbors for other asics on one chassis.
Then we can add a new object for BGP_CHASSIS_NEIGHBOR inside of bgpcfgd.
Then we can develop peer.conf, instance.conf and policy.conf for the BGP_CHASSIS_NEIGHBOR.

Thanks for the comment. So this suggestion involves creating a new table BGP_CHASSIS_NEIGHBOR with the same key as NEIGHBOR_IP, but will have all the internal BGP sessions per ASIC ? This could be a bigger change which will involve changes from the minigraph parsing to how to populate the configDB and then changes in bgpcfgd. Do we foresee any specific attributes for BGP_CHASSIS_NEIGHBOR entries, Is that the reason why going for a different table ?

Another idea which I have is if we can add a "new field" in the existing BGP_NEIGHBOR table to tell if it is "INTERNAL" or "EXTERNAL" bgp neighbor ? The changes needed would be a bit more minimal with this and we decide on internal/external neighbor while parsing minigraph and use that "field" everywhere.

@pavel-shirshov, do let me know suggestions.

pavel-shirshov · 2020-05-20T21:06:54Z

We already have 2 other BGP tables: BGP DYNAMIC PEERS and BGP MONITORS. I'd suggest to create another table, which makes all other things cleaner (you don't need to filter internal peers everytime we have iterating through BGP NEIGHBORS, we don't need to create if then else logic for every bgp template and so on )

judyjoseph · 2020-05-20T21:56:26Z

Okay Pavel, I will look at creating this new table BGP_CHASSIS_NEIGHBOR. There should not be any changes in DB migrator as these are new tables.

pavel-shirshov

Approve for now, but please use new config_db entry for the internal bgp sessions

judyjoseph · 2020-05-21T01:42:21Z

Thanks Pavel, will take up the the activity for creating BGP_CHASSIS_NEIGHBOR and raise a new PR.

judyjoseph · 2020-05-21T01:42:29Z

retest broadcom please

venkatmahalingam · 2020-05-21T01:58:58Z

Okay Pavel, I will look at creating this new table BGP_CHASSIS_NEIGHBOR. There should not be any changes in DB migrator as these are new tables.

Hello Team,

Introducing new tables like this will create confusion when we map these config DB tables to SONIC YANG for user configurations, as much as possible, having the generic BGP tables (e.g BGP_NEIGHBOR, BGP_PEER_GROUP..etc) would be better.

Please refer the below pull request for new tables Dell team has introduced to provide extensive BGP configurations support thru mgmt-framework.

https://github.com/Azure/SONiC/pull/544/files

pavel-shirshov · 2020-05-21T02:16:16Z

@venkat24 My opinion is that the internals of the multiasic sonic is orthogonal for bgp yang model of the whole device. I don't see a reason to expose the internal bgp sessions outside.
Another table will effectively hide implementation details from the yang model.

venkatmahalingam · 2020-05-21T02:21:04Z

@venkat24 My opinion is that the internals of the multiasic sonic is orthogonal for bgp yang model of the whole device. I don't see a reason to expose the internal bgp sessions outside.
Another table will effectively hide implementation details from the yang model.

Okay, is there any way to have "INTERNAL or any other better name" part of the table name to indicate that the table is meant for internal use only, so that user will not be confused with BGP tables that are exposed to the user for configuration via config_db.json or mgmt-framework.

pavel-shirshov · 2020-05-21T02:42:09Z

@venkat24 We'll discuss this as soon as we start working on it. I'll work with Guohan on this

judyjoseph · 2020-05-21T03:51:14Z

@venkat24 We'll discuss this as soon as we start working on it. I'll work with Guohan on this

Closing this PR now, will get the right approach after discussions.

* Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. The internal BGP sessions will be added to this peer group while the external BGP sessions will be added to the exising PEER_V4 and PEER_V6 peer group. * Check for "ASIC" keyword in the hostname to identify the internal neighbors.

* [201911][devices] skip_fancontrol for wedge 100 barefoot platforms (sonic-net#4528) * [device] DellEMC s5232f 50G hwsku support (sonic-net#4525) * [device] DellEmc S5232 support for new hwsku C8D48 8 100G ports and 48 50G ports * 10G ports update for S5232 hwsku-C8D48 Signed-off-by: Srideep Devireddy <srideep_devireddy@dell.com> * DellEMC S6000 updated sensors.conf (sonic-net#4568) Change PSU MAX temperature to 80 degree Change tmp75 sensors default temperature value from 25/50 to 70/80 degree. * [sonic-slave-stretch]: install same version for docker-ce and docker-ce-cli difference versions can cause compatibility issue between the server and client Signed-off-by: Guohan Lu <lguohan@gmail.com> * [baseimage]: install same version for docker-ce and docker-ce-cli Signed-off-by: Guohan Lu <lguohan@gmail.com> * [FRR]: Update frr to latest 7.2.1-s3 (sonic-net#4294) - Updated to latest frr 7.2.1 from the master. - Updated patches accordingly * [sonic-buildimage] updated minigraph for ACL Table data and ACL Interface Binding for Multi-NPU platforms (sonic-net#4491) * [sonic-buildimage] updated minigraph for ACL Table data and ACL Interface binding update for multu-npu platform based on subrole as "Frontend" or "Backend". For backend npu no ACL table is associated. For frontend npu only front-panel interface are associated. Updated with test case and fix typo in sample-mingraph for npu Address Review comments Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Fixed the logic as per preview comment. Interface Filter logic only applies to Everflow/Mirror tables. * Address Review Comments. * Changes for LLDP docker to support multi-npu platforms (sonic-net#4530) * Changes for LLDP for Multi NPU Platoforms:- a) Enable LLDP for Host namespace for Management Port b) Make sure Management IP is avaliable in per asic namespace needed for LLDP Chassis configuration c) Make sure chassis mac-address is correct in per asic namespace d) Do not run lldp on eth0 of per asic namespace and avoid chassis configuration for same e) Use Linux hostname instead from Device Metadata for lldp chassis configuration since in multi-npu platforms device metadata hostname will be differnt Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Address Review Comment with following changes: a) Use Device Metadata hostname even in per namespace conatiner. updated minigraph parsing for same to have hostname as system hostname and add new key for asic name b) Minigraph changes to have MGMT_INTERFACE Key in per asic/namespace config also as needed for LLDP for setting chassis management IP. Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Address Review Comments * Moved utility functions for multi-npu platforms from sonic-utilities to sonic_device_util.py (sonic-net#4559) * Moved utility functions for multi-npu platforms from sonic-utilities config/main.py to here so that they can be used any module * Fix the issue with test run during compilation with acl-uploader PR#908 of sonic-utilities. * Fix get_num_npu as it was retuning string and not int * Address Review Comments * Address Review Comments * Fix for issue where image is compile with flag ENABLE_DHCP_GRAPH_SERVICE (sonic-net#4573) and then we load image and reboot even if there was existing config_db.json we will look for DHCP Service. we should disbale update_graph in such cases. This behaviour is silimar to what we have in 201811 image. * Change to enable redistribute connected on Frontend asics instead of backend asics (sonic-net#4588) Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com> * [DellEMC] S6000 Disable Low power mode by default (sonic-net#4592) * [BFN] Updated Barefoot SDK to 2020-05-07 (sonic-net#4566) Signed-off-by: Andriy Kokhan <akokhan@barefootnetworks.com> * [minigraph] Add tags for egress mirror tables (sonic-net#4526) Signed-off-by: Danny Allen <daall@microsoft.com> * [Submodule update] sonic-utlities with PR's [201911][show] Fix abbreviations for 'show ip bgp ...' commands (sonic-net#909) Changes to support acl-loader and mirror-session config commands for multi-npu platforms. (sonic-net#908) Changes to commands config reload/load-minigraph (sonic-net#919) Stop/Start restapi server upon config reload (sonic-net#911) [config] Add 'interface transceiver' subgroup with 'lpmode' and 'reset' subcommands (sonic-net#904) * [minigraph] Support FECDisabled in minigraph parser (sonic-net#4556) (sonic-net#4624) Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com> * [ntp] enable/disable NTP long jump according to reboot type (sonic-net#4577) * [ntp] enable/disable NTP long jump according to reboot type - Enable NTP long jump after cold reboot. - Disable NTP long jump after warrm/fast reboot. Signed-off-by: Ying Xie <ying.xie@microsoft.com> * fix typo * further refactoring * use sonic-db-cli instead * [arista]: remove the soc property disabling sram scan (sonic-net#4623) * Changes to support config-setup service for multi-npu (sonic-net#4609) * Changes to support config-setup service for multi-npu platforms. For Multi-npu we are not supporting as of now config initializtion and ZTP. It will support creating config db from minigraph or using config db from previous file system Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Address Review Comments. * Address Review comments * Address Review Comments of using pyhton based config load_minigraph/ config save/config reload from shell scripts so that we don't duplicate code. Also while running from shell we will skip stop/start services done by those commands. * Updated to use python command so no code duplication. * [config]: Fix the device type and internal bgp session status for multi NPU platforms (sonic-net#4600) * The following changes for multi-npu platforms are done - Set the type in device_metadata for asic configuration to be same as host - Set the admin-status of internal bgp sessions as up Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com> * Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. (sonic-net#4620) * Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. The internal BGP sessions will be added to this peer group while the external BGP sessions will be added to the exising PEER_V4 and PEER_V6 peer group. * Check for "ASIC" keyword in the hostname to identify the internal neighbors. * [submodule update] sonic-swss with PR [vnet] Fix IP2ME route creation logic for BITMAP VNET interface (sonic-net#1284) * [submodule update] sonic-util Revert "[config] Add 'interface transceiver' subgroup with 'lpmode' and 'reset' subcommands (sonic-net#904)" Multi-asic changes for config bgp commands and utilities. (sonic-net#910) * [submodule update] sonic-rest API's PR#39 Setup module versioning Add support for get all Vlans (#37) * Update golang version for 1.11.5 to 1.14.2 (sonic-net#4520) Co-authored-by: Myron Sosyak <49795530+msosyak@users.noreply.github.com> Co-authored-by: Srideep <srideep_devireddy@dell.com> Co-authored-by: paavaanan <paavaanan_t_n@dell.com> Co-authored-by: Guohan Lu <lguohan@gmail.com> Co-authored-by: pavel-shirshov <pavelsh@microsoft.com> Co-authored-by: abdosi <58047199+abdosi@users.noreply.github.com> Co-authored-by: arlakshm <55814491+arlakshm@users.noreply.github.com> Co-authored-by: Santhosh Kumar T <53558409+santhosh-kt@users.noreply.github.com> Co-authored-by: Andriy Kokhan <43479230+akokhan@users.noreply.github.com> Co-authored-by: Danny Allen <daall@microsoft.com> Co-authored-by: Abhishek Dosi <abdosi@microsoft.com> Co-authored-by: Qi Luo <qiluo-msft@users.noreply.github.com> Co-authored-by: Ying Xie <yxieca@users.noreply.github.com> Co-authored-by: Samuel Angebault <staphylo@arista.com> Co-authored-by: judyjoseph <53951155+judyjoseph@users.noreply.github.com>

Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. The internal …

53cd8d4

…BGP sessions will be added to this peer group while the external BGP sessions will be added to the exising PEER_V4 and PEER_V6 peer group.

judyjoseph requested review from pavel-shirshov and arlakshm May 19, 2020 22:35

lguohan reviewed May 20, 2020

View reviewed changes

Check for "ASIC" keyword in the hostname to identify the internal nei…

2616f36

…ghbors.

judyjoseph added the Request for 201911 Branch label May 20, 2020

pavel-shirshov suggested changes May 20, 2020

View reviewed changes

pavel-shirshov approved these changes May 21, 2020

View reviewed changes

judyjoseph merged commit 4ba2f60 into sonic-net:master May 21, 2020

judyjoseph deleted the bgp_peer_group branch May 21, 2020 03:52

abdosi added Multi-NPU 🔢 Included in 201911 Branch labels May 21, 2020

judyjoseph mentioned this pull request Oct 1, 2020

[multi-ASIC] BGP internal neighbor table support #5520

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. #4620

Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. #4620

judyjoseph commented May 19, 2020

lguohan May 20, 2020

judyjoseph May 20, 2020

pavel-shirshov commented May 20, 2020

pavel-shirshov left a comment

judyjoseph commented May 20, 2020

pavel-shirshov commented May 20, 2020

judyjoseph commented May 20, 2020

pavel-shirshov left a comment

judyjoseph commented May 21, 2020

judyjoseph commented May 21, 2020

venkatmahalingam commented May 21, 2020

pavel-shirshov commented May 21, 2020

venkatmahalingam commented May 21, 2020

pavel-shirshov commented May 21, 2020

judyjoseph commented May 21, 2020

Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. #4620

Adding new BGP peer groups PEER_V4_INT and PEER_V6_INT. #4620

Conversation

judyjoseph commented May 19, 2020

lguohan May 20, 2020

Choose a reason for hiding this comment

judyjoseph May 20, 2020

Choose a reason for hiding this comment

pavel-shirshov commented May 20, 2020

pavel-shirshov left a comment

Choose a reason for hiding this comment

judyjoseph commented May 20, 2020

pavel-shirshov commented May 20, 2020

judyjoseph commented May 20, 2020

pavel-shirshov left a comment

Choose a reason for hiding this comment

judyjoseph commented May 21, 2020

judyjoseph commented May 21, 2020

venkatmahalingam commented May 21, 2020

pavel-shirshov commented May 21, 2020

venkatmahalingam commented May 21, 2020

pavel-shirshov commented May 21, 2020

judyjoseph commented May 21, 2020