mgmt get-data improvements #15246

idryzhov · 2024-01-28T23:15:39Z

No description provided.

idryzhov · 2024-01-29T00:57:47Z

[CI] Verify Source fails because I commented out the code that is not used anymore, but may be needed someday. I can just delete it or we can ignore the failure. Anyway, waiting for the review before doing anything.

choppsv1 · 2024-01-29T01:26:57Z

lib/mgmt_msg_native.h

+ */
+#define MGMT_MSG_FORMAT_XML    0
+#define MGMT_MSG_FORMAT_JSON   1
+#define MGMT_MSG_FORMAT_BINARY 2 /* non-standard libyang internal format */


I don't agree with changing away from LYD_FORMAT. This is introducing an unnecessary indirection and possible diverging errors. Now whenever we need to pass a LYD_FORMAT we have to do a mapping, or assume they are the same. I don't want to build assumptions into the code -- or unneeded mappings.

mgmtd frontend API is a public API intended to be used by people for configuring FRR from scripts/tools/etc. We can't ensure backwards compatibility if we rely on libyang-defined variables. What happens if libyang decides to change these values in a new major release? All user scripts would just break. We can use LYD_FORMAT between mgmtd and backends, but not on the frontend.

Also, when someone external to FRR developments wants to write a script to configure FRR through mgmtd, how do they even know what LYD_FORMAT is? Do they need to download libyang sources to figure out which number to set?

I didn't notice that this was exclusively for the frontend. Your argument makes sense.

I changed the values to use LYD_FORMAT values directly and added _Static_asserts.

choppsv1 · 2024-01-29T01:29:40Z

lib/mgmt_msg_native.h

+#define GET_DATA_FLAG_WD_TRIM	  0x08
+#define GET_DATA_FLAG_WD_ALL_TAG  0x10
+#define GET_DATA_FLAG_WD_ALL	  0x18
+#define GET_DATA_FLAG_WD_MASK	  0x18


Same comment as above. We are tightly coupled to libyang. There's no reason not to use the libyang constants. In fact these values here appear to be different from the exact same symbolic constants in libyang which is even worse b/c now we have to have a mapping. Please use libyang constants and types instead of creating new aliases.

The reasoning is explained in the previous comment. In this particular case I don't like libyang flags for an additional reason - they are mutually exclusive, but can be set simultaneously. It is possible set WD_TRIM (remove all defaults) and WD_ALL (print all defaults) at the same time. I don't even know what will happen, but this is not a good API.

I think these flags values are incorrect _ALL here doesn't mean all settings, and a flag should be a bit not multiple bits. The three values of EXPLICIT (only present nodes), TRIM (only nodes different from default value) and ALL (all nodes present or not) are mutually exclusive; and that's just the way it is, but we can't (and shouldn't try) to use the flag values to enforce that here. Rename ALL, ALL_NODES to make this more clear maybe.

Using the libyang values works with our other flags, and eliminates the need for mapping. So here's my suggestion. For both sets of libyang mapped values, use the libyang values, write 2 mapping inline functions that do nothing but return the value (b/c the values are the same) and _Static_assert the fact that the values are indeed the same (using libyang symbols). The latter can be done anywhere (including a .c file) and just refer back to the mapping functions in a comment to keep the header cleaner I suppose.

Right I would rename ALL to ALL_NODES and I would rename ALL_TAG to TAG_ALL, makes it a lot more clear what's going on IMO. Also TRIM_DEFAULTS and we need to put EXPLICIT_NODES with it's 0 value back I think.

We have 4 different values here - TRIM, EXPLICIT, ALL and ALL_TAG. So we just need two bits to encode them. That's why I programmed it like that. I agree that it's a bit misleading that it's called a flag when it actually represents a value. How about we keep the encoding as-is, but change the semantics of the flags field to a bitfield:

uint8_t state : 1; uint8_t config : 1; uint8_t exact : 1; uint8_t with_defaults : 2; uint8_t rsvd : 3;

I really don't want to use libyang values directly here for two resons:

as I wrote above, with-default is four distinct values, so exposing them as flags makes an impression that they are not mutually exclusive and can be used simultaneously

we'll need at least uint16_t to store libyang values directly, because the largest value is 0x80, and it's just to store values that can be encoded as two bits

Regarding the rename, the current names reflect values from the RFC 6243: trim, explicit, report-all, report-all-tagged. I can rename ALL and ALL_TAG to REPORT_ALL and REPORT_ALL_TAGGED to make it even more similar. But I don't want to use some custom names here.

What do you think?

BTW, I think this is a regular thing to encode values like this. Just one example from an RFC: https://datatracker.ietf.org/doc/html/rfc4191#section-2

Ok fine we have 4 distinct values in 2 bits, but please don't call them flags then. So, remove _FLAG_ from their symbolic names and put them in another uint8 field maybe called withdef or something -- we have 5 unused uint8s currently.

I don't care very much about the RFC names here b/c I don't think people are going to be coming from the RFC when they read the code, they are just going to be looking at the code, and importantly the RFC names are confusing sounding,

If I read REPORT_ALL_TAGGED I'm going to think, rightly so, that this is going to report all nodes that are tagged. Instead what this value means is Return all values and add tags to them so ALL_ADD_TAG would be a much better name.

You can mention the RFC name in a comment on the #define for the symbol if you think there's some value there.

Implemented using a new defaults field in the message. Definitions renamed according to the request.

choppsv1 · 2024-01-29T01:30:16Z

lib/vty.c

@@ -3673,6 +3673,7 @@ static int vty_mgmt_get_data_result_notified(
 	return 0;
 }

+#if 0


This is going to get hit by checkpatch.pl, it doesn't like #if 0'd code.

Let's please ignore this error for now. I don't want to spend time moving this anywhere. We have other #if 0 code in the codebase, so it's not something strictly prohibited.

My overall idea is to start returning errors in this JSON/XML format in mgmt_msg_error message. I think this makes sense, because user requests a response in a specific format and we're returning a simple string in case of an error. This code may be reused for this, so I don't want to delete it completely.

OK I think you can trick checkpatch.pl then to pass by using a symbolic constant that i undefined

#ifdef DONT_DEFINE_UNTIL_NEEDED

choppsv1 · 2024-01-29T01:31:47Z

mgmtd/mgmt_fe_adapter.c

+		goto done;
+	}
+
+	switch (msg->format) {


This is exactly the code that we don't need -- just use the libyang type.

I removed the translation as it's not needed now, but I preserved the validation of the supplied value, so there's not much less LOC here.

choppsv1 · 2024-01-29T01:32:42Z

mgmtd/mgmt_txn.c

 	int ret;

 	txn = mgmt_txn_id2ctx(txn_id);
 	if (!txn)
 		return -1;

+	switch (flags & GET_DATA_FLAG_WD_MASK) {


Again unnecessary code, just use the libyang type.

As discussed above, we're using own values for the defaults parameter, so the translation is needed.

choppsv1 · 2024-01-29T10:45:38Z

[CI] Verify Source fails because I commented out the code that is not used anymore, but may be needed someday. I can just delete it or we can ignore the failure. Anyway, waiting for the review before doing anything.

Is this error to XML code provided by libyang anywhere? If not perhaps the functions should be converted to return an allocated string (e.g., using darr_sprintf) put in a different file, and made non-static.

choppsv1 · 2024-01-29T11:30:43Z

lib/mgmt_msg_native.h

@@ -237,19 +252,33 @@ _Static_assert(sizeof(struct mgmt_msg_tree_data) ==
 #define GET_DATA_FLAG_STATE	0x01	/* get only "config false" data */
 #define GET_DATA_FLAG_CONFIG	0x02	/* get only "config true" data */
 #define GET_DATA_FLAG_EXACT	0x04	/* get exact data node instead of the full tree */
+/*


Also while in here, let's remove only from the comments above since that's not what is actually happening. These are flags and you can have both set in which case both "config" and "data" are returned not "only data" and "only config" :)

github-actions · 2024-01-29T12:50:48Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

choppsv1 · 2024-01-30T09:48:43Z

lib/vty.c

@@ -3782,9 +3783,6 @@ static int vty_mgmt_get_tree_result_notified(
 	size_t len, int partial_error)
 {
 	struct vty *vty;
-	struct lyd_node *dnode;


As discussed in slack, these changes need to be reverted so that we support using LYB up to right before converting to send to the user.

choppsv1 · 2024-01-30T09:49:24Z

lib/vty.c

 	vty->mgmt_req_id++;

 	if (mgmt_fe_send_get_data_req(mgmt_fe_client, vty->mgmt_session_id,
-				      vty->mgmt_req_id, datastore,
-				      intern_format, flags, defaults, xpath)) {


please leave the intern_format name as it is highlighting that it can be different (LYD_LYB) from the users requested format.

These models are needed to use LYD_PRINT_WD_ALL_TAG flag of libyang. Signed-off-by: Igor Ryzhov <[email protected]>

Signed-off-by: Igor Ryzhov <[email protected]>

We don't need to create an actual tree to print an empty tree, libyang handles NULL just fine. The actual problem is that `yang_dnode_new` creates a tree by validating it, and the validation creates all implicit default nodes. Therefore, when called with "with-default" flags, instead of getting an empty tree, we get a tree with all top-level default set. Signed-off-by: Igor Ryzhov <[email protected]>

It allows people not familiar with libyang and FRR internals to use mgmtd FE API by looking only at `mgmt_msg_native.h` header. We still use the same values to avoid a lot of mapping code, and ensure that any change doesn't slip unnoticed by using static asserts. Signed-off-by: Igor Ryzhov <[email protected]>

Signed-off-by: Igor Ryzhov <[email protected]>

frrbot bot added bugfix mgmt FRR Management Infra yang labels Jan 28, 2024

github-actions bot added master size/XXL labels Jan 28, 2024

idryzhov force-pushed the mgmt-get-data-improvements branch from 20afefd to b899aec Compare January 28, 2024 23:26

idryzhov force-pushed the mgmt-get-data-improvements branch from b899aec to 842fec9 Compare January 29, 2024 01:13

choppsv1 requested changes Jan 29, 2024

View reviewed changes

choppsv1 reviewed Jan 29, 2024

View reviewed changes

github-actions bot added the conflicts label Jan 29, 2024

idryzhov force-pushed the mgmt-get-data-improvements branch from 842fec9 to 6fbf670 Compare January 29, 2024 22:34

frrbot bot added the tests Topotests, make check, etc label Jan 29, 2024

github-actions bot removed the conflicts label Jan 29, 2024

choppsv1 reviewed Jan 30, 2024

View reviewed changes

Jafaral added this to the 10.0 milestone Jan 30, 2024

idryzhov added 6 commits January 31, 2024 02:20

yang: add more ietf models

e2caf64

These models are needed to use LYD_PRINT_WD_ALL_TAG flag of libyang. Signed-off-by: Igor Ryzhov <[email protected]>

mgmtd: add support for with-defaults parameter to get-data

4317c8f

Signed-off-by: Igor Ryzhov <[email protected]>

mgmtd: add ability to choose datastore to get data from

e13c590

Signed-off-by: Igor Ryzhov <[email protected]>

tests: add tests for mgmt get-data with-defaults parameter

3afea9c

Signed-off-by: Igor Ryzhov <[email protected]>

idryzhov force-pushed the mgmt-get-data-improvements branch from 6fbf670 to 3afea9c Compare January 31, 2024 00:21

choppsv1 approved these changes Jan 31, 2024

View reviewed changes

choppsv1 merged commit 25d3086 into FRRouting:master Jan 31, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgmt get-data improvements #15246

mgmt get-data improvements #15246

idryzhov commented Jan 28, 2024

idryzhov commented Jan 29, 2024

choppsv1 Jan 29, 2024

idryzhov Jan 29, 2024

idryzhov Jan 29, 2024

choppsv1 Jan 29, 2024

idryzhov Jan 29, 2024

choppsv1 Jan 29, 2024

idryzhov Jan 29, 2024

choppsv1 Jan 29, 2024 •

edited

Loading

choppsv1 Jan 29, 2024 •

edited

Loading

idryzhov Jan 29, 2024

idryzhov Jan 29, 2024

choppsv1 Jan 29, 2024 •

edited

Loading

idryzhov Jan 29, 2024

choppsv1 Jan 29, 2024

idryzhov Jan 29, 2024

choppsv1 Jan 30, 2024

choppsv1 Jan 29, 2024

idryzhov Jan 29, 2024

choppsv1 Jan 29, 2024

idryzhov Jan 29, 2024

choppsv1 commented Jan 29, 2024

choppsv1 Jan 29, 2024

idryzhov Jan 29, 2024

github-actions bot commented Jan 29, 2024

choppsv1 Jan 30, 2024

choppsv1 Jan 30, 2024

mgmt get-data improvements #15246

mgmt get-data improvements #15246

Conversation

idryzhov commented Jan 28, 2024

idryzhov commented Jan 29, 2024

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 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

choppsv1 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choppsv1 Jan 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

choppsv1 commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choppsv1 Jan 29, 2024 •

edited

Loading

choppsv1 Jan 29, 2024 •

edited

Loading

choppsv1 Jan 29, 2024 •

edited

Loading