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

sonic-utilities: port to libyang3 #3766

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bradh352
Copy link
Contributor

@bradh352 bradh352 commented Feb 13, 2025

What I did

remove direct dependencies on libyang, and instead rely on functionality provided by sonic-yang-mgmt.

This has dependencies on other PRs in other repositories, see primary tracking PR sonic-net/sonic-buildimage#21679

How I did it

Modifications to sonic-yang-mgmt in sonic-net/sonic-buildimage#21716 added public functions and enhancements to allow us to not need to call into libyang, thus making this code not reliant on a given version of libyang (however the new sonic-yang-mgmt code does depend on libyang3, so this is not compatible with libyang1, the goal is to limit the number of packages directly using libyang to make future upgrading easier).
The find_data_dependencies() logic has been simplified due to enhancements in sonic-yang-mgmt in specifying schema or partial paths rather than data node paths able to return all nodes with leafrefs to the specified path. This is not only more simple, but a lot faster.

How to verify it

Pull all PRs from sonic-net/sonic-buildimage#21679 into a local build system and build and test

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -615,11 +614,7 @@ def _find_leafref_paths(self, path, config):

xpath = self.convert_path_to_xpath(path, config, sy)

leaf_xpaths = self._get_inner_leaf_xpaths(xpath, sy)

ref_xpaths = []
Copy link
Contributor

Choose a reason for hiding this comment

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

why removing this line?

Copy link
Contributor Author

@bradh352 bradh352 Feb 17, 2025

Choose a reason for hiding this comment

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

previously sonic-yang-mgmt's find_data_dependencies() expected an exact node to find dependencies for ... however the new version allows you to specify an ancestor node in the hierarchy and it will return all nodes that reference child nodes of the provided path.

That means that this code is no longer needing to do a complex tree recursion and never calls into libyang directly (which is what sy.root.tree_for() and node.tree_dfs() were doing), but instead only uses functionality provided by sonic-yang-mgmt.

This is orders of magnitude more efficient due to it being implemented directly in sonic-yang-mgmt, and also removes dependencies on the libyang version in use within sonic-utilities (which is bad, shouldn't use libyang directly since the API isn't exactly stable)

Copy link
Contributor Author

@bradh352 bradh352 Feb 17, 2025

Choose a reason for hiding this comment

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

See sonic-yang-mgmt PR: sonic-net/sonic-buildimage#21716

And in particular you can see the new test case added in https://github.com/sonic-net/sonic-buildimage/pull/21716/files#diff-fe704fab7611ef139a60d6d1b0c03dfde3ba4c09d7326a9c8f10132808614a99

(under "dependencies" when using the ancestor node xpath "/test-acl:acl/ACL_TABLE" which tests this functionality, and confirms this behavior was moved into sonic-yang-mgmt and doesn't need to be in sonic-utilities)

@bradh352 bradh352 force-pushed the bradh352/libyang3 branch from c490a3a to 70edc0b Compare March 2, 2025 11:43
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bradh352 bradh352 force-pushed the bradh352/libyang3 branch from 70edc0b to c143b99 Compare March 7, 2025 00:53
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

bradh352 added 2 commits March 7, 2025 06:17
remove direct dependencies on libyang, and instead rely on
functionality provided by sonic-yang-mgmt.

also, find_data_dependencies() from sonic-yang-mgmt has been enhanced
to be able to take a partial path and return all possible data nodes
that have leafrefs that point to the partial path location (including /).
@bradh352 bradh352 force-pushed the bradh352/libyang3 branch from c143b99 to b5ef0c6 Compare March 7, 2025 11:17
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

3 participants