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

bgpcfgd: add support for software bfd sessions #20981

Merged
merged 6 commits into from
Feb 6, 2025

Conversation

abdbaig
Copy link
Contributor

@abdbaig abdbaig commented Dec 1, 2024

Why I did it

As part of this HLD, need to monitor a new STATE_DB table called SOFTWARE_BFD_SESSION_TABLE and add/remove BFD sessions in FRR accordingly: https://github.com/kperumalbfn/SONiC/blob/kperumal/bfd/doc/smart-switch/BFD/SmartSwitchDpuLivenessUsingBfd.md

Work item tracking
  • Microsoft ADO (number only):

How I did it

-Added a new manager managers_bfd.py which is used by bgpcfgd if switch_type is dpu. This manager receives notification any time there is a change on SOFTWARE_BFD_SESSION_TABLE entries, and it calls FRR config CLI to add/remove/update BFD sessions accordingly. Both active and passive sessions along with single-hop or multihop sessions are supported.

How to verify it

  • Added pytest testcases to verify the functionality.
  • Tested on vs platform by forcing switch_type to dpu

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)

@abdbaig abdbaig changed the title add support for software bfd sessions in bgpcfgd bgpcfgd: add support for software bfd sessions Dec 1, 2024
table,
)

self.check_and_start_bfdd()
Copy link
Contributor

Choose a reason for hiding this comment

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

Check the return status and load_bfd_sessions only if the BFDD is running?

Copy link

Choose a reason for hiding this comment

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

Updated


ret_code, out, err = run_command(command)
if ret_code != 0:
log_err("Can't add bfd session: %s" % err)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add BFD session key details in the error log?

Copy link

Choose a reason for hiding this comment

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

Done

if command != "":
ret_code, out, err = run_command(command)
if ret_code != 0:
log_err("Can't update bfd session: %s" % err)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, please add BFD session details in the log

Copy link

Choose a reason for hiding this comment

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

Done

'multihop': False,
'local': '',
'detect-multiplier': self.MULTIPLIER,
'receive-interval': self.RX_INTERVAL,
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the RX/TX INTERVAL units? in micro or milliseconds? I believe FRR configs take interval in milliseconds.

Copy link

Choose a reason for hiding this comment

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

They are in ms

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the units in the variable or a comment.

Copy link

Choose a reason for hiding this comment

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

Done

'local': '',
'detect-multiplier': self.MULTIPLIER,
'receive-interval': self.RX_INTERVAL,
'transmit-interval': self.TX_INTERVAL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the unit, could you update the variable name to reflect the units.

Copy link

Choose a reason for hiding this comment

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

Done

'receive-interval': self.RX_INTERVAL,
'transmit-interval': self.TX_INTERVAL,
'passive-mode': True,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you check the default DSCP value of BFD packets from FRR? If it is not 48, could we check if there is any FRR config to update this DSCP?

Copy link

Choose a reason for hiding this comment

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

It is 48 by default

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @dypet. Could you add a comment about the DSCP value in this file?

Copy link

Choose a reason for hiding this comment

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

Added

'passive-mode': False,
'receive-interval': 300,
'transmit-interval': 100
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add BFD V6 sessions config?

Copy link

Choose a reason for hiding this comment

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

Done

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@KrisNey-MSFT
Copy link

hi @kperumalbfn , is there an ETA to close this one? We would want it complete before @r12f gets to HA_Mgr please :)

switch_type = device_info.get_localhost_info("switch_type")
if switch_type and switch_type == "dpu":
log_notice("switch type is dpu, starting bfd manager")
#TODO: use swsscommon.STATE_BFD_SOFTWARE_SESSION_TABLE_NAME onces definition is in master
Copy link
Contributor

Choose a reason for hiding this comment

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

@dypet could you update this PR to use swsscommon table as sonic-common PR has been merged.

Copy link

Choose a reason for hiding this comment

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

Updated

@kperumalbfn
Copy link
Contributor

@dypet @abdbaig please let me know once the changes are made, we will merge the changes soon.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dypet
Copy link

dypet commented Jan 30, 2025

@dypet @abdbaig please let me know once the changes are made, we will merge the changes soon.

@kperumalbfn changes have been made, not sure if sonic-net/sonic-swss#3406 needs to be merged first or if this one can be merged independently.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kperumalbfn
Copy link
Contributor

@dypet @abdbaig let me know if there is a dependency between this PR and the other swss PR. We can merge this soon.

@kperumalbfn kperumalbfn merged commit a35e23c into sonic-net:master Feb 6, 2025
21 checks 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