Skip to content

schema: helpers for tracking backlinks #2351

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

Closed
wants to merge 2 commits into from

Conversation

bradh352
Copy link
Contributor

@bradh352 bradh352 commented Feb 15, 2025

In libyang v1, each schema node had a backlinks object that would allow a caller to quickly see if the node was referenced by another node. Since this functionality was removed, it takes quite a bit of code to accomplish the same thing, including full tree scans.

This adds these primary new helpers:

  • lys_find_backlinks() scan the entire schema tree (all modules) and locate any node that contains leafrefs (including in unions), possibly filtered to a single target node or ancestor thereof.
  • lysc_node_find_lref_targets() for a given node, return a set of leafref target nodes. This is an enhanced version of lysc_node_lref_target(), however this returns a set instead of a single node as it also returns targets associated with unions (which there may be more than one leafref). This does not return recursive results, meaning if the node provided isn't a leaf or leaflist, it will not return any results.

It also adds a couple of new helpers used by the above new functions that may be found useful so they were made public:

  • lysc_node_has_ancestor() determine if the ancestor node exists as a parent, grandparent, etc within the node schema for the specified node. This simply scans up the tree using node->parent looking for a match.
  • lysc_type_lref_target() similar to lysc_node_lref_target() except it can return the target node referenced in a leafref for a struct lysc_type. This is used internally by lysc_node_find_lref_targets().

This functionality was determined to be needed when porting SONiC from libyang v1 to v3. SONiC uses libyang-python, and the API does not currently expose some of the underlying functions used by these helpers. Instead of simply modifying the libyang-python to add CFFI wrappers it was determined it would be cost prohibitive to support lysc_module_dfs_full() in python due to generating a Python SNode for each node in the tree, of which most nodes are not needed to be evaluated by Python.

It is not unlikely that other users may have the same need to track backlinks so adding these helpers would also benefit other users.

@bradh352 bradh352 changed the base branch from master to devel February 15, 2025 13:31
@bradh352
Copy link
Contributor Author

bradh352 commented Feb 15, 2025

This PR is due to feedback on CESNET/libyang-python#132 which suggested this functionality should be in the core libyang rather than implemented in libyang-python (granted, libyang-python would still need modifications to import these functions via CFFI, but that would be a thin wrapper).

I've tried to make the APIs as similar to others in libyang as possible. I'm definitely willing to make changes, but I'd like to move quickly so we don't merge something in SONiC that isn't going to be accepted upstream

@bradh352 bradh352 force-pushed the backlinks branch 7 times, most recently from dc38d8f to 9b20027 Compare February 16, 2025 03:05
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
CESNET/libyang#2351

Backlinks were removed between libyang1 and libyang2, this adds
helper functions for enumerating backlinks.
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Feb 16, 2025
In libyang v1 the schema nodes had a backlinks member to be able to
look up dependents of the node.  SONiC depends on this to provide
functionality it uses and it needs to be exposed via the python
module.

In theory, exposing the 'dfs' functions could make this work, but
it would likely be cost prohibitive since walking the tree would
be expensive to create a python node for evaluation in native
python.

Instead this PR depends on the this libyang PR:
CESNET/libyang#2351
And adds thin wrappers.

This implementation provides 2 python functions:
 * Context.find_backlinks_paths() - This function can
   take the path of the base node and find all dependents.  If
   no path is specified, then it will return all nodes that contain
   a leafref reference.
 * Context.find_leafref_path_target_paths() - This function takes
   an xpath, then returns all target nodes the xpath may reference.
   Typically only one will be returned, but multiples may be in the
   case of a union.

A user can build a cache by combining Context.find_backlinks_paths()
with no path set and building a reverse table using
Context.find_leafref_path_target_paths()

Signed-off-by: Brad House <[email protected]>
In libyang v1, each schema node had a backlinks object that would
allow a caller to quickly see if the node was referenced by another
node.  Since this functionality was removed, it takes quite a bit
of code to accomplish the same thing, including full tree scans.

This adds these primary new helpers:
 * lys_find_backlinks() scan the entire schema tree (all modules)
   and locate any node that contains leafrefs (including in unions),
   possibly filtered to a single target node or ancestor.
 * lysc_node_find_lref_targets() for a given node, return a set
   of leafref target nodes.  This returns a set instead of a single
   node like lysc_node_lref_target() as it also returns targets
   associated with unions (which there may be more than one).

It also adds a couple of new helpers used by the above new functions
that may be found useful so they were made public:
 * lysc_node_has_ancestor() determine if the ancestor node exists
   as a parent, grandparent, etc within the node schema for the
   specified node.
 * lysc_type_lref_target() similar to lysc_node_lref_target() except
   it can return the target node referenced in a leafref for a
   `struct lysc_type`

This functionality was determined to be needed when porting SONiC from
libyang v1 to v3.  SONiC uses libyang-python, and the API does not
currently expose some of the underlying functions used by these helpers.
Instead of simply modifying the libyang-python to add CFFI wrappers
it was determined it would be cost prohibitive to support
lysc_module_dfs_full() in python due to generating a Python SNode for
each node in the tree, of which most nodes are not needed to be
evaluated by Python.

It is not unlikely that other users may have the same need to track
backlinks so adding these helpers would also benefit other users.

Signed-off-by: Brad House <[email protected]>
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Feb 16, 2025
In libyang v1 the schema nodes had a backlinks member to be able to
look up dependents of the node.  SONiC depends on this to provide
functionality it uses and it needs to be exposed via the python
module.

In theory, exposing the 'dfs' functions could make this work, but
it would likely be cost prohibitive since walking the tree would
be expensive to create a python node for evaluation in native
python.

Instead this PR depends on the this libyang PR:
CESNET/libyang#2351
And adds thin wrappers.

This implementation provides 2 python functions:
 * Context.find_backlinks_paths() - This function can
   take the path of the base node and find all dependents.  If
   no path is specified, then it will return all nodes that contain
   a leafref reference.
 * Context.find_leafref_path_target_paths() - This function takes
   an xpath, then returns all target nodes the xpath may reference.
   Typically only one will be returned, but multiples may be in the
   case of a union.

A user can build a cache by combining Context.find_backlinks_paths()
with no path set and building a reverse table using
Context.find_leafref_path_target_paths()

Signed-off-by: Brad House <[email protected]>

fix

fix

fix
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Feb 16, 2025
In libyang v1 the schema nodes had a backlinks member to be able to
look up dependents of the node.  SONiC depends on this to provide
functionality it uses and it needs to be exposed via the python
module.

In theory, exposing the 'dfs' functions could make this work, but
it would likely be cost prohibitive since walking the tree would
be expensive to create a python node for evaluation in native
python.

Instead this PR depends on the this libyang PR:
CESNET/libyang#2351
And adds thin wrappers.

This implementation provides 2 python functions:
 * Context.find_backlinks_paths() - This function can
   take the path of the base node and find all dependents.  If
   no path is specified, then it will return all nodes that contain
   a leafref reference.
 * Context.find_leafref_path_target_paths() - This function takes
   an xpath, then returns all target nodes the xpath may reference.
   Typically only one will be returned, but multiples may be in the
   case of a union.

A user can build a cache by combining Context.find_backlinks_paths()
with no path set and building a reverse table using
Context.find_leafref_path_target_paths()

Signed-off-by: Brad House <[email protected]>
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
CESNET/libyang#2351

Backlinks were removed between libyang1 and libyang2, this adds
helper functions for enumerating backlinks.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
CESNET/libyang#2351

Backlinks were removed between libyang1 and libyang2, this adds
helper functions for enumerating backlinks.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
CESNET/libyang#2351

Backlinks were removed between libyang1 and libyang2, this adds
helper functions for enumerating backlinks.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
CESNET/libyang#2351

Backlinks were removed between libyang1 and libyang2, this adds
helper functions for enumerating backlinks.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
CESNET/libyang#2351

Backlinks were removed between libyang1 and libyang2, this adds
helper functions for enumerating backlinks.
@michalvasko
Copy link
Member

You could have started with asking about this functionality because it has already been implemented, by a PR. Look at how LY_CTX_LEAFREF_LINKING context flag works.

@bradh352
Copy link
Contributor Author

bradh352 commented Feb 17, 2025

@michalvasko

You could have started with asking about this functionality because it has already been implemented, by a PR. Look at how LY_CTX_LEAFREF_LINKING context flag works.

Its not immediately obvious to me how that context flag accomplishes the same thing. Doesn't that strictly work on data not schema? The routines I added were strictly operating on the schema.

Since the schema is a lot smaller and should be quick to scan, I don't honestly see a reason to form a whole cross reference scheme like LY_CTX_LEAFREF_LINKING does.

@michalvasko
Copy link
Member

Right, sorry. But it will take considerable effort to get it to a state when it can be merged, not even talking about the coding style. I think it may even be faster if I just adjust the code but I can also just iteratively provide feedback, up to you. Generally regarding the code, I would definitely not add functions into API that are not needed (lysc_node_has_ancestor(), also lysc_node_find_lref_targets(), lysc_type_lref_target(), and lysc_node_lref_target() have too similar functionality) and would make sure the API is suitable for a library (lys_find_backlinks() match_ancestors parameter seems to be required for your specific use-case but not sure it should be supported by the library).

@bradh352
Copy link
Contributor Author

bradh352 commented Feb 17, 2025

match_ancestors in lys_find_backlinks() is a very strong requirement for what we are doing, without it we'd need to maintain external patches which is non-optimal. I don't see the harm in this function having another argument like that, but if you want to turn it into a flags type argument instead for future possible capabilities, that's fine with me. The only use-case in SONiC right now that utilizes it is through libyang-python, but of course that brings up other issues since even less is exposed in python so we can't implement similar functionality there ... it really needs to be in C libyang.

Also, lysc_node_find_lref_targets() is needed to be public as well since it is needed to support the union usecase and greatly simplifies what is otherwise a non-obvious lookup operation.

The other two functions don't really matter to me if they are public or not.

@bradh352
Copy link
Contributor Author

@michalvasko

it will take considerable effort to get it to a state when it can be merged, not even talking about the coding style. I think it may even be faster if I just adjust the code but I can also just iteratively provide feedback, up to you.

I'm definitely ok if you want to take the code and adjust it directly if you think that is more efficient. I think at this point with my last reply you understand the requirements in SONiC. This PR is currently integrated as a PR into SONiC being reviewed, but it shouldn't be a problem to modify that SONiC PR with whatever you come up with as long as it meets the aforementioned needs.

@michalvasko
Copy link
Member

Please look at the PR #2352 I created and test it.

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.

2 participants