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

chopps/mgmtd simplify xpaths #14525

Merged

Conversation

choppsv1
Copy link
Contributor

@choppsv1 choppsv1 commented Oct 3, 2023

No description provided.

@frrbot frrbot bot added libfrr mgmt FRR Management Infra labels Oct 3, 2023
@Jafaral Jafaral requested a review from pushpasis October 3, 2023 15:39
Copy link
Contributor

@pushpasis pushpasis left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ther operational datastore uses a different set of xpaths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above statement makes no such assumption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@choppsv1 choppsv1 Oct 4, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@pushpasis pushpasis Oct 5, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@choppsv1 choppsv1 Oct 24, 2023

Choose a reason for hiding this comment

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

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

@choppsv1 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 github-actions bot added size/XL and removed size/L labels Oct 9, 2023
@choppsv1 choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch 2 times, most recently from 08fa62d to 4aeeeb9 Compare October 13, 2023 10:35
@idryzhov
Copy link
Contributor

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
Copy link
Contributor Author

choppsv1 commented Oct 14, 2023 via email

@idryzhov
Copy link
Contributor

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
Copy link
Contributor Author

choppsv1 commented Oct 15, 2023 via email

@idryzhov
Copy link
Contributor

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.

@frrbot frrbot bot added the tools label Oct 24, 2023
@choppsv1 choppsv1 force-pushed the chopps/mgmtd-simplify-xpaths branch 2 times, most recently from cb3ce1f to 1285a65 Compare October 27, 2023 01:08
"/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[] = {
Copy link
Contributor

@idryzhov idryzhov Oct 30, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@choppsv1 choppsv1 Oct 30, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@idryzhov idryzhov Oct 30, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

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).

Copy link
Contributor

@idryzhov idryzhov left a comment

Choose a reason for hiding this comment

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

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 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
@github-actions github-actions bot added the rebase PR needs rebase label Nov 6, 2023
Copy link
Contributor

@idryzhov idryzhov left a comment

Choose a reason for hiding this comment

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

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 <chopps@labn.net>
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 <chopps@labn.net>
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 <chopps@labn.net>
@idryzhov
Copy link
Contributor

idryzhov commented Nov 7, 2023

@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.

@idryzhov idryzhov merged commit 448f75e into FRRouting:master Nov 8, 2023
80 checks passed
@choppsv1 choppsv1 deleted the chopps/mgmtd-simplify-xpaths branch November 10, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libfrr master mgmt FRR Management Infra rebase PR needs rebase size/XL tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants