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

feat(frrcfgd): update for new locator config way #21843

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LARLSN
Copy link
Contributor

@LARLSN LARLSN commented Feb 24, 2025

since srv6 locator and sid config schema have changed as https://github.com/BYGX-wcr/SONiC/blob/srv6_static_config_hld/doc/srv6/srv6_static_config_hld.md, update corresponding frrcfgd code.

image

image

Why I did it

Work item tracking
  • Microsoft ADO (number only):

How I did it

How to verify it

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)

@LARLSN LARLSN requested a review from lguohan as a code owner February 24, 2025 13:47
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@eddieruan-alibaba
Copy link
Collaborator

The test config changes.

sonic-net/sonic-mgmt#17125 (review)

@@ -118,7 +118,9 @@ class BgpdClientMgr(threading.Thread):
'PIM_INTERFACE': ['pimd'],
'IGMP_INTERFACE': ['pimd'],
'IGMP_INTERFACE_QUERY': ['pimd'],
'SRV6_LOCATOR': ['zebra']
'SRV6_MY_LOCATORS': ['zebra'],
'SRV6_MY_SIDS': ['mgmtd']
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the damemon corresponding to MY_SIDS table is mgmtd? I thought that the static-sids command is handled by staticd?

Copy link
Contributor Author

@LARLSN LARLSN Feb 27, 2025

Choose a reason for hiding this comment

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

In the frr code, mgmtd calls the relevant vtysh cli func defined by staticd, so it's still staticd to handle the command

image

image

Since we use unified config mode for FRR, https://github.com/sonic-net/sonic-mgmt/blob/master/ansible/vars/configdb_jsons/7nodes_cisco/PE1.json#L130, this leads to use mgmtd as the entry point for FRR configurations.

@venkatmahalingam hi, could you please help to confirm this understand ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using client socket, we'll send the configs to corresponding FRR daemons, why would we send all FRR configurations to mgmtd, any new changes that I'm not aware of?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@venkatmahalingam we are using "docker_routing_config_mode": "unified". In my understanding, since it is unified mode, the configuration would go via mgmtd, then distribute to different protocol process. Is that the correct understanding?

I saw mgmtd https://docs.frrouting.org/en/latest/mgmtd.html

"The FRR Management Daemon (from now on referred to as MGMTd) is a new centralized entity representing the FRR Management Plane which can take management requests from any kind of UI/Frontend entity (e.g. CLI, Netconf, Restconf, Grpc etc.) over a new unified and common Frontend interface "

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, we send the configs to FRR daemons via client socket, executing the "subprocess.Popen(command, shell=True, stdout=subprocess.PIPE)" for each command causing delays in case of scaled FRR configs.

if not bgpd_client.run_vtysh_command(table, command, daemons) and not ignore_fail:

As much as possible we can use the FRR daemons's client socket directly, if this is restricted by FRR or mgmtd provides better configuration handling, we could migrate to use mgmtd for all commands, no need to map configs to FRR daemons.

Copy link

Choose a reason for hiding this comment

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

We can gradually migrate configs associated with staticd daemon to mgmtd. Starting with SRv6 now, then static routes, and so on. @venkatmahalingam @eddieruan-alibaba what do you think?

Copy link
Collaborator

@venkatmahalingam venkatmahalingam Feb 28, 2025

Choose a reason for hiding this comment

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

I've been hearing YANG validation issues that caused config failure with bgpcfgd because of mgmtd, we have no urgency to use mgmtd for frrcfgd, we can wait for mgmtd's stability and access the importance of adding extra hop (mgmtd) to config FRR daemons, I believe, direct client socket method to configure FRR deamon would continue to be there. @LARLSN If SRV6_MY_SIDS is working with staticd directly, please make change to reflect it in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahsalam What are the current configs using mgmtd? Could you please provide some details

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless there is a strong reason for us to go mgmtd (earlier it was vtysh that handles all config), we can continue to use individual client socket methods, my question was simple, should migrate all configs associated with staticd daemon to mgmtd now? wondering if anyone attempted to check static route config in SONiC after new FRR changes (with mgmtd) @eddieruan-alibaba anyone tried from Alibaba? @LARLSN did we try staticd for SRV6_MY_SIDS table configs?

We started with staticd, it doesn't work since frr has moved to use mgmtd for staticd.

Copy link
Contributor Author

@LARLSN LARLSN Mar 1, 2025

Choose a reason for hiding this comment

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

@venkatmahalingam As eddie said,We firstly used staticd,which can not handle this command. This is the shot of frrcfgd log file when used staticd
image

Signed-off-by: linsongnan <linsongnan.lsn@alibaba-inc.com>
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

@ahsalam ahsalam left a comment

Choose a reason for hiding this comment

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

Thanks for updating the frrcfgd code to align with the new schema for SRv6 Locator and SID config.

@kperumalbfn
Copy link
Contributor

@dgsudharsan could you review this PR? We have seen issues with mgmtd client(#21815)

@eddieruan-alibaba
Copy link
Collaborator

@dgsudharsan could you review this PR? We have seen issues with mgmtd client(#21815)

@kperumalbfn for mgmtd issue, I saw @dgsudharsan had started an email thread. That would require more efforts and is not related to the problem in this PR.

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.

8 participants