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] Re-add dependency in managers_bgp for bgpcfgd #19088

Merged
merged 1 commit into from
May 28, 2024

Conversation

yaqiangz
Copy link
Contributor

@yaqiangz yaqiangz commented May 27, 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.

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)

@yaqiangz
Copy link
Contributor Author

/azpw ms_conflict

@yaqiangz yaqiangz changed the title [bgp] Add dependency in bgpcfgd [bgp] Re-add dependency in bgpcfgd May 27, 2024
@yaqiangz yaqiangz changed the title [bgp] Re-add dependency in bgpcfgd [bgp] Re-add dependency in managers_bgp for bgpcfgd May 27, 2024
@yaqiangz yaqiangz marked this pull request as ready for review May 28, 2024 02:20
@StormLiangMS
Copy link
Contributor

hi @yaqiangz I remember you mentioned this interface would be monitored in the bgp manager file? why we need to add these two back?

@StormLiangMS
Copy link
Contributor

hi @yaqiangz I remember you mentioned this interface would be monitored in the bgp manager file? why we need to add these two back?

src/sonic-bgpcfgd/bgpcfgd/main.py

@yaqiangz
Copy link
Contributor Author

yaqiangz commented May 28, 2024

hi @yaqiangz I remember you mentioned this interface would be monitored in the bgp manager file? why we need to add these two back?

@StormLiangMS Yes interface change will be monitored by https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-bgpcfgd/bgpcfgd/main.py#L50

The dependency is to make sure Loopback interface exist. With this dependency adding:
If interested table changed, but CONFIG_DB entry LOOPBACK_INTERFACE|Loopback0 doesn't exist, it will not invoke handler for the table change, but record it until the entry LOOPBACK_INTERFACE|Loopback0 appear.

I removed them previously because I thought these dependencies require LOOPBACK_INTERFACE|Loopback0|<ipv4_address> should always exist, which would make bgp cannot work well in Loopback IPv6 only scenario. But actually these dependencies require LOOPBACK_INTERFACE|Loopback0 should always exist, it treats Loopback interface existence as a must have for SONiC BGP, I think it's reasonable to have it, so added them back

@StormLiangMS
Copy link
Contributor

After discussion, if we configured IPv6 or IPv4 on the loopback interface, it will create LOOPBACK_INTERFACE|Loopback0 in the config_db, no matter a IPv6 or IPv4 addressed configured. In previous assumption, the device could be IPv6 only later, and in either case, there should at least one of them (IPv6 or IPv4) available, no harm to add the dependency on LOOPBACK_INTERFACE|Loopback0 back.

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.

@StormLiangMS StormLiangMS merged commit d1875c6 into sonic-net:master May 28, 2024
20 checks passed
@bingwang-ms
Copy link
Contributor

Hi @yaqiangz , @StormLiangMS Is this change required for 202405 branch?

@yaqiangz
Copy link
Contributor Author

yaqiangz commented Nov 7, 2024

Hi @yaqiangz , @StormLiangMS Is this change required for 202405 branch?

Hi @bingwang-ms it's included in 202405 by this commit d1875c6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants