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

operational-state (datastore) change notifications #17796

Merged

Conversation

choppsv1
Copy link
Contributor

@choppsv1 choppsv1 commented Jan 8, 2025

Add framework for notifications sent to front-end clients for changes to operational state (datastore). This is to support various YANG push RFCs such as:

  • RFC7923: Requirements for Subscription to YANG Datastores
  • RFC8639: Subscription to YANG Notifications
  • RFC8640: Dynamic Subscription to YANG Events and Datastores over NETCONF
  • RFC8641: Subscription to YANG Notifications for Datastore Updates
  • RFC8650: Dynamic Subscription to YANG Events and Datastores over RESTCONF

@frrbot frrbot bot added libfrr mgmt FRR Management Infra tests Topotests, make check, etc labels Jan 8, 2025
@choppsv1 choppsv1 force-pushed the chopps/datastore-notifications branch 7 times, most recently from d9ed722 to dc66226 Compare January 8, 2025 17:43
@choppsv1 choppsv1 marked this pull request as draft January 8, 2025 20:30
@choppsv1 choppsv1 force-pushed the chopps/datastore-notifications branch 2 times, most recently from d8c15b9 to 94fe28d Compare January 10, 2025 05:30
@choppsv1 choppsv1 marked this pull request as ready for review January 10, 2025 05:30
@choppsv1 choppsv1 force-pushed the chopps/datastore-notifications branch 2 times, most recently from 66e4ed0 to 275b1e0 Compare January 10, 2025 12:03
@frrbot frrbot bot added the bugfix label Jan 10, 2025

nb_notif_add(abs_path);

if (abs_path != path)
Copy link
Member

Choose a reason for hiding this comment

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

What is the chance that abs_path and path are equal and path was passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

abs_path starts out assigned as NULL, it is then either assigned the value path or it is a new allocation from lyd_path(). path is not currently allowed to be NULL (notice we use path[0] without checking for path == NULL). So path can only ever equal abs_path when we set it that way (which is what our equality check is deciding, did we set it or newly allocate it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I don't see a reason we couldn't also support a NULL path for updating the path/value of the tree node itself, so I'll modify the code to also handle path == NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually nm, this gets tricky deeper in the implementation, i'll leave extended functionality for later if we need it.

@choppsv1 choppsv1 force-pushed the chopps/datastore-notifications branch 3 times, most recently from 26f0f0a to f9eac62 Compare January 13, 2025 17:20
Signed-off-by: Christian Hopps <chopps@labn.net>
Signed-off-by: Christian Hopps <chopps@labn.net>
Signed-off-by: Christian Hopps <chopps@labn.net>
Signed-off-by: Christian Hopps <chopps@labn.net>
- Additionally push the selectors down to the backends

Signed-off-by: Christian Hopps <chopps@labn.net>
Signed-off-by: Christian Hopps <chopps@labn.net>
Signed-off-by: Christian Hopps <chopps@labn.net>
@choppsv1 choppsv1 force-pushed the chopps/datastore-notifications branch from f9eac62 to c88b489 Compare January 14, 2025 04:40
@donaldsharp donaldsharp merged commit 5f35096 into FRRouting:master Jan 14, 2025
11 checks passed
donaldsharp added a commit that referenced this pull request Jan 15, 2025
@choppsv1 choppsv1 deleted the chopps/datastore-notifications branch January 15, 2025 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix libfrr master mgmt FRR Management Infra size/XXL tests Topotests, make check, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants