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

[bgp] Add support to explicitly specify bgp router id for single asice device #18727

Merged
merged 7 commits into from
May 13, 2024

Conversation

yaqiangz
Copy link
Contributor

@yaqiangz yaqiangz commented Apr 19, 2024

Why I did it

HLD: sonic-net/SONiC#1643
Add support to explictly specify bgp router id for single asic device

Work item tracking
  • Microsoft ADO (number only): 27674485

How I did it

  1. When bgp_router_id configured in DEVICE_METADATA, use it as bgp router-id.
  2. Remove the hard dependency on loopback0 ipv4 address when adding bgp peer.
  3. Add UTs.

Behavior of set bgp router-id

To be clarified that when bgp router-id hasn't been explicitly set, bgp actions would totally like previous

Loopback0 IPv4 address exists Loopback0 IPv4 address doesn't exist
bgp_router_id configured Honor bgp_router_id Honor bgp_router_id
bgp_router_id doesn't be configured Honor Loopback0 IPv4 address FRR default router ID value is selected as the largest IP address of the device. When router zebra is not enabled bgpd can’t get interface information, so router-id is set to 0.0.0.0

Behavior of add bgp peer

To be clarified that when bgp router-id hasn't been explicitly set, bgp actions would totally like previous

Loopback0 IPv4 address exists Loopback0 IPv4 address doesn't exist
bgp_router_id configured Add BGP peer Add BGP peer
bgp_router_id doesn't be configured Add BGP peer Do not add BGP peer

How to verify it

UT passed.
Run bgp related test in sonic-mgmt

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

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

@mssonicbld
Copy link
Collaborator

@yaqiangz PR: #18727 is conflict with MS internal repo
Please push fix commit to sonicbld/precheck/head/18727 and approve
https://msazure.visualstudio.com/One/_git/Networking-acs-buildimage/pullrequest/9928963
After ms PR is merged, comment "/azpw ms_conflict" to rerun PR checker.

@yaqiangz yaqiangz force-pushed the master_bgp_router_id branch from 8ad2aef to 5fcb722 Compare April 19, 2024 07:16
@mssonicbld
Copy link
Collaborator

@yaqiangz PR: #18727 is conflict with MS internal repo
Please push fix commit to sonicbld/precheck/head/18727 and approve
https://msazure.visualstudio.com/One/_git/Networking-acs-buildimage/pullrequest/9928963
After ms PR is merged, comment "/azpw ms_conflict" to rerun PR checker.

@yaqiangz yaqiangz changed the title [bgp] Add support to explictly specify bgp router id for single asice device [bgp] Add support to explicitly specify bgp router id for single asice device Apr 25, 2024
@yaqiangz
Copy link
Contributor Author

Hi @StormLiangMS Could you please help to review this PR?

@StormLiangMS
Copy link
Contributor

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StormLiangMS
Copy link
Contributor

  1. first one is a question, what's the issue this design is to address? To avoid bgp breakage due to the loopback0 ip is not configured? What's the use case? @yaqiangz Why we assume loopback0 ip is not configured, but bgp_router_id is configured?

@StormLiangMS
Copy link
Contributor

In this PR, no multi asic, it would be a separate PR?

@yaqiangz
Copy link
Contributor Author

In this PR, no multi asic, it would be a separate PR?

Yes

@yaqiangz
Copy link
Contributor Author

yaqiangz commented May 6, 2024

  1. first one is a question, what's the issue this design is to address? To avoid bgp breakage due to the loopback0 ip is not configured? What's the use case? @yaqiangz Why we assume loopback0 ip is not configured, but bgp_router_id is configured?

As HLD said, we want to remove the strong coupling of configuring router id and Loopback ipv4 address.

Copy link
Contributor

@StormLiangMS StormLiangMS left a comment

Choose a reason for hiding this comment

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

LGTM

@liushilongbuaa
Copy link
Contributor

/azpw ms_conflict

1 similar comment
@liushilongbuaa
Copy link
Contributor

/azpw ms_conflict

@lguohan lguohan merged commit 3c1e31b into sonic-net:master May 13, 2024
19 checks passed
StormLiangMS pushed a commit that referenced this pull request May 28, 2024
Why I did it
These 2 dependencies were removed by #18764 and #18727 for decoupling hard dependency between Loopback IPv4 address and BGP.
Actually, these dependencies are to make sure existence of Loopback interface, no need to remove.

Work item tracking
Microsoft ADO (number only): 28210625
How I did it
Add dependency back

How to verify it
UT passed.
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.

5 participants