chopps/mgmtd simplify xpaths #14525

choppsv1 · 2023-10-03T12:45:05Z

No description provided.

pushpasis

The simplification removing the granularity of what kind of subscription a specific client wants is not the right thing to do in my opinion. I think we should not do this much simplification in the name of KISS.

pushpasis · 2023-10-03T15:54:46Z

mgmtd/mgmt_be_adapter.c

-static struct mgmt_be_client_xpath staticd_xpaths[] = {
-	{
-		.xpath = "/frr-vrf:lib/*",
-		.subscribed = MGMT_SUBSCR_VALIDATE_CFG | MGMT_SUBSCR_NOTIFY_CFG,


Removing this kind of granularity will not suffice in scenarios like following:

A XPATH in one of the config datastore is NOT to be sent for validation/notifciation to the specific client, but the same needs to be retrieved through the operational datastore. For example lib-interface/* and lib-vhf/* are only to be retrieved as operational data from Clients like static or zebra. But the config is to be only passed to zebra. Rest of the other daemons get the interface and vrf information through lib/interface.c and lib/vrf.c libraries from zebra. You can refer to
We need at least this much granularity in the subscription map.

Ther operational datastore uses a different set of xpaths

static const char **be_client_oper_xpaths[] = {};

There's no reason to walk though all the xpaths for config looking for interleaved operation state xpaths. They are never needed together, we are iether dealing with op state or we are dealing with config state. So I have them in 2 different arrays.

The above statement is with the assumption that for the same item the config xpaths and operstate paths are different. However even though that is true for certain models e.g. Openconfig) we can't say the same about the current FRR models. To achieve the same a major rehaul would be necessary in quite a few of the Yang models in FRR. I haven't seen any example yet where the same item has separate config and overstate paths in FRR yang models.

The above statement makes no such assumption.

Perhaps you are not understanding my comment. I am saying that when mgmtd needs to get to op-state it has no interest in the config xpaths, likewise when mgmtd needs to match config state xpaths it has no interest in op-state matching. That is all, this has nothing to do with yang model design.

I mean look at the code you're commenting on, and you can see this.

Ok I missed that part in the diffs that you are introducing two different maps now.

Even then for Config we will probably need to differentiate between the client that will need to validate a configuration item from clients who just needs to be notified about the same (after the validator has validated). Today with limited clients we have support on, this scenario has not come up yet. But we may come across this in future. Now I know you this will be over-engineering for you and lot of other members. So I will not make a fuss now.

But my take on this PR is still that this simplification is really not that necessary. Keeping the map the way it is today, leaves a lot of scope for future enhancements. And we are really not gaining much with this simplification.

@pushpasis I believe the change request you made is resolved now.

idryzhov · 2023-10-13T18:09:27Z

Backend clients can send a list of xpaths they are interested in using MGMTD__BE_MESSAGE__MESSAGE_SUBSCR_REQ message, but processing this is currently unsupported by backend adapter. As you're changing a lot in this area, can we please implement dynamic xpath registration and get rid of backend-specific hardcoded values inside mgmtd?

choppsv1 · 2023-10-14T02:02:46Z

Igor Ryzhov ***@***.***> writes:

Backend clients can send a list of xpaths they are interested in using MGMTD__BE_MESSAGE__MESSAGE_SUBSCR_REQ message, but processing this is currently unsupported by backend adapter. As you're changing a lot in this area, can we please implement dynamic xpath registration and get rid of backend-specific hardcoded values inside mgmtd?

It would be easy to do this; however, when is this ever needed? I think it was Mark who pushed for the embedding of most things b/c we knew them at compile time. FRR isn't loading externally compiled stuff, it's all built at the same time in the same build. I mean we embed the entire CLI of a daemon into mgmtd, this is just a few strings. It might be better to get rid of the dynamic message to simplify the backend API. I've definitely considered it already. Having everything known at compile time has real advantages.

idryzhov · 2023-10-14T17:39:18Z

As an example, our company intends to connect our internal daemons to mgmtd as backends, to have a single point of control for the whole suite of daemons. Obviously, we can patch mgmtd to provide necessary xpaths, but having ability to pass xpaths dynamically would be easier. I understand that it probably shouldn't affect decisions made by FRR community, but it is still a use-case that may be needed by other companies as well.

choppsv1 · 2023-10-15T03:12:33Z

Igor Ryzhov ***@***.***> writes:

As an example, our company intends to connect our internal daemons to mgmtd as backends, to have a single point of control for the whole suite of daemons. Obviously, we can patch mgmtd to provide necessary xpaths, but having ability to pass xpaths dynamically would be easier.

Right, but it's not just xpaths, you'll need to add your daemons to reserve ID numbers and to the ID->name mapping array too. Might as well do the one extra step of adding a couple xpath strings.

idryzhov · 2023-10-15T07:57:25Z

Well, this can also be done dynamically, but I agree that it's too much work for some external use-case. If we don't do that, then I don't see any reason to keep the code for dynamic xpath subscription.

idryzhov · 2023-10-30T02:10:33Z

mgmtd/mgmt_be_adapter.c

-			"/frr-routing:routing/control-plane-protocols/control-plane-protocol/frr-staticd:staticd/*",
-		.subscribed = MGMT_SUBSCR_VALIDATE_CFG | MGMT_SUBSCR_NOTIFY_CFG,
-	},
+static const char *const staticd_xpaths[] = {


I see that you removed the asterisks from xpaths. I didn't look into the code yet, but want to understand how do you solve the problem of partial subscription only for a subtree? For example, OSPF and ISIS augment interfaces with their specific nodes. In my understanding, they would want to subscribe to the interface list itself and to their specific subtrees, but not to subtrees of another daemon. If asterisks are supported, it would look like this (OSPF for example):

"frr-interface:lib/interface" - without asterisk, subscribe to the list itself without children "frr-interface:lib/interface/osfp/*" - with asterisk, subscribe to the whole OSPF subtree

Is this solved somehow in this PR? Or should we subscribe to the whole interface tree in both daemons and just ignore the unknown nodes on the backend?

Just leave off the *. It's redundant. In your example:

"frr-interface:lib/interface/ospf"

Or in a local tree of mine: "/frr-interface:lib/interface/state/frr-ospfd-lite:ospf/state" for state.

If you look at the old code even the first thing it does is strip trailing /* from paths.

Just to confirm that I understand this correctly. If a daemon subscribes to subtree "frr-interface:lib/interface/ospf", it receives updates for all its parents (interface list in this case) as well as subtree itself with all its children. But no to other subtrees or leaf children of any parent, right?

I'm asking, because right now I see that daemons receive updates for the whole tree even if they subscribe only to a subtree. For example, if I subscribe to "frr-interface:lib/interface/ospf", I receive updates for all children of "frr-interface:lib/interface", including "frr-interface:lib/interface/isis", etc. Xpaths here are just examples, they are not real xpaths used in FRR.

I want to understand whether this is changed/fixed, or we expect daemons to filter unnecessary xpaths.

Yes, this is what I meant by the current code is buggy, and yes it fixes it. Daemons should always filter unnecessary xpaths too, but this is turning off the firehose that existed prior.

choppsv1 · 2023-10-30T04:37:50Z

mgmtd/mgmt_be_adapter.c

-	if (rexp_len && xpath_regexp[rexp_len-1] == '*')
-		rexp_len--;
-	if (xpath_len && xpath[xpath_len-1] == '*')
-		xpath_len--;


@idryzhov here it was stripping trailing *s. But if you look over the rest of this code, it's quite broken, the only reason things worked correctly is b/c the broken code mostly return "Yes!" for matches regardless of the actual comparison. The daemons would then just ignore what they didn't understand.

choppsv1 · 2023-10-30T11:10:38Z

The sole style check failure will not be fixed as it is incorrect, the macro in question would not function if it was wrapped in a do { } while(0).

idryzhov

Overall good, but I have a couple of comments.
One is required (in mgmt_register_client_xpath), others are optional.

lib/mgmt_be_client.h

lib/frrstr.h

mgmtd/mgmt_be_adapter.c

idryzhov

Need to fix some tab/space style errors, otherwise all good.

- move from client id indexed array of uints for register info per client to a u64 bitmask. - add bit walking FOREACH macro Walk the client IDs whose bits are set in a mask. Signed-off-by: Christian Hopps <[email protected]>

This also avoids a bug in the workaround function if the set variable wasn't set to NULL the Debug version of libyang would sigsegv. Signed-off-by: Christian Hopps <[email protected]>

Also update `checkpatch.sh` so it runs `checkpatch.pl` from the same directory it resides in. This allows copying them both somewhere else to use a specific version. Signed-off-by: Christian Hopps <[email protected]>

idryzhov · 2023-11-07T08:41:25Z

@pushpasis I want to merge this once CI passes. Do you have any objections? From your previous comments, I see that you believe we lose the ability to differentiate between clients who need to validate/be notified about the xpath. If we ever need this, it is easily solved by adding one more mgmt_be_xpath_map.

frrbot bot added libfrr mgmt FRR Management Infra labels Oct 3, 2023

github-actions bot added size/L master labels Oct 3, 2023

Jafaral requested a review from pushpasis October 3, 2023 15:39

pushpasis requested changes Oct 3, 2023

View reviewed changes

choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch 3 times, most recently from 429547e to fe5ff98 Compare October 9, 2023 22:01

github-actions bot added size/XL and removed size/L labels Oct 9, 2023

choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch 2 times, most recently from 08fa62d to 4aeeeb9 Compare October 13, 2023 10:35

choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch from 4aeeeb9 to 34025a1 Compare October 24, 2023 16:54

frrbot bot added the tools label Oct 24, 2023

choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch 2 times, most recently from cb3ce1f to 1285a65 Compare October 27, 2023 01:08

idryzhov reviewed Oct 30, 2023

View reviewed changes

choppsv1 commented Oct 30, 2023

View reviewed changes

choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch from 1285a65 to 63f08ec Compare October 31, 2023 06:39

idryzhov requested changes Nov 5, 2023

View reviewed changes

lib/mgmt_be_client.h Outdated Show resolved Hide resolved

lib/frrstr.h Show resolved Hide resolved

mgmtd/mgmt_be_adapter.c Outdated Show resolved Hide resolved

mgmtd/mgmt_be_adapter.c Outdated Show resolved Hide resolved

choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch from 63f08ec to 67c3be8 Compare November 6, 2023 17:07

github-actions bot added the rebase PR needs rebase label Nov 6, 2023

idryzhov approved these changes Nov 6, 2023

View reviewed changes

choppsv1 added 3 commits November 6, 2023 17:44

mgmtd: simplify xpath registries

a65cda1

- move from client id indexed array of uints for register info per client to a u64 bitmask. - add bit walking FOREACH macro Walk the client IDs whose bits are set in a mask. Signed-off-by: Christian Hopps <[email protected]>

lib: yang: restore old API use now that it's fixed

3184f63

This also avoids a bug in the workaround function if the set variable wasn't set to NULL the Debug version of libyang would sigsegv. Signed-off-by: Christian Hopps <[email protected]>

tools: checkpatch updated with darr_foreach iter macros

bec1091

Also update `checkpatch.sh` so it runs `checkpatch.pl` from the same directory it resides in. This allows copying them both somewhere else to use a specific version. Signed-off-by: Christian Hopps <[email protected]>

choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch from 67c3be8 to bec1091 Compare November 6, 2023 22:45

idryzhov merged commit 448f75e into FRRouting:master Nov 8, 2023
79 checks passed

choppsv1 deleted the chopps/mgmtd-simplify-xpaths branch November 10, 2023 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chopps/mgmtd simplify xpaths #14525

chopps/mgmtd simplify xpaths #14525

choppsv1 commented Oct 3, 2023

pushpasis left a comment

pushpasis Oct 3, 2023

choppsv1 Oct 3, 2023

choppsv1 Oct 3, 2023

choppsv1 Oct 3, 2023

pushpasis Oct 4, 2023

choppsv1 Oct 4, 2023

choppsv1 Oct 4, 2023

choppsv1 Oct 4, 2023 •

edited

Loading

pushpasis Oct 5, 2023 •

edited

Loading

choppsv1 Oct 24, 2023 •

edited

Loading

idryzhov commented Oct 13, 2023

choppsv1 commented Oct 14, 2023 via email

idryzhov commented Oct 14, 2023

choppsv1 commented Oct 15, 2023 via email

idryzhov commented Oct 15, 2023

idryzhov Oct 30, 2023 •

edited

Loading

choppsv1 Oct 30, 2023 •

edited

Loading

idryzhov Oct 30, 2023 •

edited

Loading

choppsv1 Oct 30, 2023

choppsv1 Oct 30, 2023

choppsv1 commented Oct 30, 2023

idryzhov left a comment

idryzhov left a comment

idryzhov commented Nov 7, 2023

chopps/mgmtd simplify xpaths #14525

chopps/mgmtd simplify xpaths #14525

Conversation

choppsv1 commented Oct 3, 2023

pushpasis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choppsv1 Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

pushpasis Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

choppsv1 Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

idryzhov commented Oct 13, 2023

choppsv1 commented Oct 14, 2023 via email

idryzhov commented Oct 14, 2023

choppsv1 commented Oct 15, 2023 via email

idryzhov commented Oct 15, 2023

idryzhov Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

choppsv1 Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

idryzhov Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choppsv1 commented Oct 30, 2023

idryzhov left a comment

Choose a reason for hiding this comment

idryzhov left a comment

Choose a reason for hiding this comment

idryzhov commented Nov 7, 2023

choppsv1 Oct 4, 2023 •

edited

Loading

pushpasis Oct 5, 2023 •

edited

Loading

choppsv1 Oct 24, 2023 •

edited

Loading

idryzhov Oct 30, 2023 •

edited

Loading

choppsv1 Oct 30, 2023 •

edited

Loading

idryzhov Oct 30, 2023 •

edited

Loading