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

Add templates and code to support VoQ chassis iBGP peers #5622

Merged
merged 6 commits into from
Apr 16, 2021

Conversation

jmmikkel
Copy link
Contributor

@jmmikkel jmmikkel commented Oct 13, 2020

  • Add support to convert a new VoQChassisInternal element in the
    BGPSession element of the minigraph to a new BGP_VOQ_CHASSIS_NEIGHBOR
    table in CONFIG_DB.
  • Add a new set of "voq_chassis" templates to docker-fpm-frr
  • Add a new BGP peer manager to bgpcfgd to add neighbors from the
    BGP_VOQ_CHASSIS_NEIGHBOR table using the voq_chassis templates.
  • Add a test case for minigraph.py, making sure the VoQChassisInternal
    element creates a BGP_VOQ_CHASSIS_NEIGHBOR entry, but not if its
    value is "false".
  • Add a set of test cases for the new voq_chassis templates in
    sonic-bgpcfgd tests.

Note that the templates expect the new
"bgp bestpath peer-type multipath-relax" bgpd configuration to be
available.

Signed-off-by: Joanne Mikkelson [email protected]

- Why I did it

For the VoQ chassis system, the iBGP sessions between the ASIC Instances have substantially different configuration needs from the usual eBGP neighbors. Pushing those differences into the "general" templates would involve a lot of if-statements and the like, while the bgpcfgd system looks designed around keeping a set of templates per kind of neighbor. So this change adds a new set of templates, keeping the VoQ chassis peers out of the other templates.

See the VoQ chassis BGP HLD for additional details, particularly of the contents of the new templates: sonic-net/SONiC#674

- How I did it

Added new voq_chassis directory in the fpm-frr docker image. In order to make these templates selectable, changed bgpcfgd to look at a new BGP_VOQ_CHASSIS_NEIGHBOR table in config_db. To get those entries into config_db, added a new element to BGPSession in the minigraph and code to honor it when parsing the minigraph.

- How to verify it

Boot a VoQ chassis system with VoQChassisInternal BGPSessions (minigraph) or BGP_VOQ_CHASSIS_NEIGHBOR config_db entries, and check that all the peerings in the iBGP mesh are up. Advertise routes from an eBGP neighbor and confirm they are learned by the iBGP peers.

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006

- Description for the changelog

Add support for configuring VoQ chassis iBGP peers

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

* Add support to convert a new VoQChassisInternal element in the
  BGPSession element of the minigraph to a new BGP_VOQ_CHASSIS_NEIGHBOR
  table in CONFIG_DB.
* Add a new set of "voq_chassis" templates to docker-fpm-frr
* Add a new BGP peer manager to bgpcfgd to add neighbors from the
  BGP_VOQ_CHASSIS_NEIGHBOR table using the voq_chassis templates.
* Add a test case for minigraph.py, making sure the VoQChassisInternal
  element creates a BGP_VOQ_CHASSIS_NEIGHBOR entry, but not if its
  value is "false".
* Add a set of test cases for the new voq_chassis templates in
  sonic-bgpcfgd tests.

Note that the templates expect the new
"bgp bestpath peer-type multipath-relax" bgpd configuration to be
available.

Signed-off-by: Joanne Mikkelson <[email protected]>
@jmmikkel
Copy link
Contributor Author

retest baseimage please

@jmmikkel jmmikkel marked this pull request as ready for review October 14, 2020 21:35
@lguohan lguohan requested a review from rlhui October 18, 2020 08:20
minionatwork
minionatwork previously approved these changes Oct 27, 2020
This includes a bit of refactoring of parse_cpg in
src/sonic-config-engine/minigraph.py, to avoid having 3
copies of the code that creates the bgp_*sessions tables.
- Remove the require_intf_in_config_db workaround in bgpcfgd. The
  VOQ_INBAND_INTERFACE table is now in config_db and adding an
  InterfaceMgr works, so now we do not need to exempt VOQ chassis
  neighbors from the interface check.
- Remove the BackEnd sub_role from the voq_chassis bgpcfgd test
  inputs in response to a comment on the PR. It was copy-pasted from
  the general templates and had no effect on the test, but since it
  does not belong, it is gone now.
vganesan-nokia
vganesan-nokia previously approved these changes Feb 23, 2021
voq_chassis = session.find(str(QName(ns, "VoQChassisInternal")))
if voq_chassis is not None and voq_chassis.text == "true":
table = bgp_voq_chassis_sessions
admin_status = None
Copy link
Contributor

Choose a reason for hiding this comment

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

should the internal sessions be up by default, if not the admin status will be changed by CLI ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was agreed the sessions would be up by default so that routing works as intended as soon as any eBGP session comes up. The admin status should be changeable with the CLI though this PR does not cover the CLI. Are you saying admin_status should be 'up', rather than not-specified? Not-specified is not 'down' so the sessions would be configured up either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point was should we set the admin_status to up here as we want these session to up by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have changed it to 'up'. Please take a look!

minionatwork
minionatwork previously approved these changes Mar 13, 2021
eswaranb
eswaranb previously approved these changes Mar 30, 2021
arlakshm
arlakshm previously approved these changes Mar 30, 2021
Previously admin_status was not populated for these peers.
@jmmikkel jmmikkel dismissed stale reviews from arlakshm and eswaranb via 705529c March 30, 2021 22:31
@arlakshm
Copy link
Contributor

@jmmikkel
Copy link
Contributor Author

@jmmikkel, the unit tests are failing. Can you take a look
Log file
https://dev.azure.com/mssonic/build/_build/results?buildId=8902&view=logs&j=83516c17-6666-5250-abde-63983ce72a49&t=c10d5f44-55ce-55d7-7975-407ed75d9a96&l=1861

Ugh, how embarrassing, I had tested it to work in our local build system, then copied over the change wrong.

@jmmikkel
Copy link
Contributor Author

@arlakshm I don't think this failure is due to my changes.
"FAIL: advanced-reboot.ReloadTest",
"FAILED:dut:Error collecting current date from DUT: empty value returned",

Is there a way to retry this test or does it need a full rebuild? Thanks

@arlakshm arlakshm merged commit 43342b3 into sonic-net:master Apr 16, 2021
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-buildimage that referenced this pull request May 23, 2021
…onic-net#5622)

This commit has following changes:

* Add templates and code to support VoQ chassis iBGP peers

* Add support to convert a new VoQChassisInternal element in the
   BGPSession element of the minigraph to a new BGP_VOQ_CHASSIS_NEIGHBOR 
   table in CONFIG_DB.
* Add a new set of "voq_chassis" templates to docker-fpm-frr
* Add a new BGP peer manager to bgpcfgd to add neighbors from the
  BGP_VOQ_CHASSIS_NEIGHBOR table using the voq_chassis templates.
* Add a test case for minigraph.py, making sure the VoQChassisInternal
  element creates a BGP_VOQ_CHASSIS_NEIGHBOR entry, but not if its
  value is "false".
* Add a set of test cases for the new voq_chassis templates in
  sonic-bgpcfgd tests.

Note that the templates expect the new
"bgp bestpath peer-type multipath-relax" bgpd configuration to be
available.

Signed-off-by: Joanne Mikkelson <[email protected]>
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…onic-net#5622)

This commit has following changes:

* Add templates and code to support VoQ chassis iBGP peers

* Add support to convert a new VoQChassisInternal element in the
   BGPSession element of the minigraph to a new BGP_VOQ_CHASSIS_NEIGHBOR 
   table in CONFIG_DB.
* Add a new set of "voq_chassis" templates to docker-fpm-frr
* Add a new BGP peer manager to bgpcfgd to add neighbors from the
  BGP_VOQ_CHASSIS_NEIGHBOR table using the voq_chassis templates.
* Add a test case for minigraph.py, making sure the VoQChassisInternal
  element creates a BGP_VOQ_CHASSIS_NEIGHBOR entry, but not if its
  value is "false".
* Add a set of test cases for the new voq_chassis templates in
  sonic-bgpcfgd tests.

Note that the templates expect the new
"bgp bestpath peer-type multipath-relax" bgpd configuration to be
available.

Signed-off-by: Joanne Mikkelson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chassis 🤖 Modular chassis support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants