add sidctl "tree" command to display resource tree in JSON #89

trgill · 2021-03-09T21:38:03Z

No description provided.

trgill · 2021-03-11T13:02:40Z

src/resource/ubridge.c

+		return -ENOMEM;
+	}
+
+	sid_resource_dump_all_in_dot(sid_resource_search(res, SID_RESOURCE_SEARCH_TOP, NULL, NULL));


@prajnoha the data that is traversed in sid_resource_dump_all_in_dot() is what I should pack up to send to sidctl, right?

Currently it looks like this:

[27456:27464]:resource.c:1038:sid_resource_dump_all_in_dot <DOT> digraph resources { [27456:27464]:resource.c:1039:sid_resource_dump_all_in_dot <DOT> "worker/a29cafe8-ea1f-4850-8ccf-4877927e1a10"; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "aggregate/modules"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "worker/a29cafe8-ea1f-4850-8ccf-4877927e1a10" -> "aggregate/modules" [dir=both]; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "module-registry/block"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "aggregate/modules" -> "module-registry/block" [dir=both] [color=red]; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "module/blkid"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "module-registry/block" -> "module/blkid" [dir=both] [color=red]; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "module/dummy_block"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "module-registry/block" -> "module/dummy_block" [dir=both] [color=red]; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "module-registry/type"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "aggregate/modules" -> "module-registry/type" [dir=both] [color=red]; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "module/dummy_type"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "module-registry/type" -> "module/dummy_type" [dir=both] [color=red]; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "kv-store/main"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "worker/a29cafe8-ea1f-4850-8ccf-4877927e1a10" -> "kv-store/main" [dir=forward]; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "connection"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "worker/a29cafe8-ea1f-4850-8ccf-4877927e1a10" -> "connection" [dir=both]; [27456:27464]:resource.c:1007:_dump_children_recursively_in_dot <DOT> "command/27464/tree"; [27456:27464]:resource.c:1024:_dump_children_recursively_in_dot <DOT> "connection" -> "command/27464/tree" [dir=both]; [27456:27464]:resource.c:1041:sid_resource_dump_all_in_dot <DOT> }

Yes, indeed, that's the core information we have about resources:

resource type

id

parent/children dependency

flags (for now it's various SID_RESOURCE_RESTRICT_... flags and SID_RESOURCE_DISALLOW_ISOLATION flag)

We could possibly add more insights from the core struct sid_resource like:

indicator of event loop existence for the resource

number of event sources attached

PID where the resource got created

(...that's what can be read from looking at struct sid_resource directly)

And then it would be great if we had a possibility (or at least count with this for the future) for each resource to send its own extra information in some predefined format (I think a simple key-value should suffice, but also supporting arrays). Each resource could have a hook that we can call to do the per-resource custom dump (the declaration of new dump hook would go into typedef struct sid_resource_type besides init and destroy functions we already have there). We don't need to do this in one go, of course, just counting with this for the future that there might be more fields to add in the dump...

All of this this functionality should be mainly for us to have a better base for debugging and seeing state at certain point in time - like an introspection on demand without a need to run gdb or parsing through tons of logs.

(Of course, the dot is just an output format to view this graphically - but it's one of many possible. If we had some standard format for the resource dump - like json (we could reuie existing functions to print the json we have in usid now), then we could easily translate to specific output formats like dot or whatever else we needed...)

(...going even deeper with this later, we can even consider making the database dump just a custom kv-store resource dump that's simply added as a "subgraph" to the whole resource-tree dump)

@prajnoha something like this look ok to you?

I have some questions for you about how it should be implemented. I'm currently generating the JSON in sid - and sending it to sidctl as JSON. Wasn't sure what you'd think of that.... I'll push the code ASAP.

I still need to clean up the commit log etc.

# sidctl tree --format json { "ID": "worker/ca13a375-a97d-4dbf-9701-4b56ce9f5140", "type": "worker", "direction": "[dir=both]", "event-sources": [ { "name": "main" }, { "name": "worker_signal_handler" } ], "pid-created": 368550, "flags": 0, "prio": 0, "color": "", "children": [ { "ID": "aggregate/modules", "type": "aggregate", "direction": "[dir=both]", "pid-created": 368543, "flags": 0, "prio": 0, "color": "", "children": [ { "ID": "module-registry/block", "type": "module-registry", "direction": "[dir=both]", "pid-created": 368543, "flags": 4, "prio": 0, "color": " [color=red]", "children": [ { "ID": "module/blkid", "type": "module", "direction": "[dir=both]", "pid-created": 368543, "flags": 4, "prio": 0, "color": " [color=red]" }, { "ID": "module/dummy_block", "type": "module", "direction": "[dir=both]", "pid-created": 368543, "flags": 4, "prio": 1, "color": " [color=red]" } ] }, { "ID": "module-registry/type", "type": "module-registry", "direction": "[dir=both]", "pid-created": 368543, "flags": 4, "prio": 0, "color": " [color=red]", "children": [ { "ID": "module/dummy_type", "type": "module", "direction": "[dir=both]", "pid-created": 368543, "flags": 4, "prio": 1, "color": " [color=red]" } ] } ] }, { "ID": "kv-store/main", "type": "kv-store", "direction": " [dir=forward]", "pid-created": 368543, "flags": 1, "prio": 0, "color": "" }, { "ID": "connection", "type": "connection", "direction": "[dir=both]", "event-sources": [ { "name": "client connection" } ], "pid-created": 368550, "flags": 0, "prio": 0, "color": "", "children": [ { "ID": "command/368550/tree", "type": "command", "direction": "[dir=both]", "event-sources": [ { "name": "command handler" } ], "pid-created": 368550, "flags": 0, "prio": 0, "color": "" } ] } ] }

Sending the JSON directly from sid to sidctl is a possibility. We just need to take into account two things:

JSON is just one of possible representations and in the future, we may support more formats. We'd either need to be able to send those different formats OR take the JSON as primary and do the translation on sidctl side to any other format. But I think that if we can format in JSON on sid side, then we can do any other format there as well. So I think then we need some bits of protocol parts that handle this - "please send me the result in format XYZ". This will probably make it easier for us to support other clients than sidctl itself.

We'll be sending a bit more data over the socket with JSON as that is text format (compared to binary). But on the other hand, I think we'll save some cycles elsewhere with this - we'll format this just once and then send over to sidctl for display (...compared to formatting to binary format on sid side, then sending it over to sidctl, then formatting it again to target format there).

So I think formatting on sid side is a good idea, just needs the extra bits in the protocol so we can select the format in which sid will send us the result.

Looking at the example output - maybe we don't need to send/print the "direction" as that is already encoded in "flags" field? Also, for those flags, it would probably be better if we printed the flag names (NO_FLAGS/RESTRICT_WALK_UP/RESTRICT_WALK_DOWN/DISALLOW_ISOLATION). This could prevent confusion if we needed to reorder the flags later...

@prajnoha Ok - I removed the direction and color fields from the output.

I haven't added the bit of the protocol parts. I was thinking we might want to consider using a JSON API. Maybe make the socket that SID listens the API for SID? Does that seem like a reasonable idea to you?

Sure, I'm counting with a connection point/socket for the API (now I mean the other API for all the other commands and queries, not the API used to construct SID ucmd modules - these are executed inside SID daemon directly and they communicate through the DB itself).

As for JSON API - I think it's going to be fine for almost 99% of our needs. The only area where I'm a bit wary is sending more data, like the db dump which can grow easily with the number of devices growing on the system. But on the other hand - it's just a global whole db dump used for debug, not the usual use case. The usual use cases are not, and I believe won't be, formatting and transferring so much data (instead, working on subset of the information recorded in db, for example: "send me all devices affected by change on device XYZ in the stack").

The other thing is also a possibility to encode binary data (e.g. the database can contain binary data, we're not bound only to strings... though again, they're strings most of the time.). I'm not sure right now how JSON format handles such cases.

In summary, I'm definitely fine with JSON API and surely sounds reasonable - it will make the connection with other services much easier. We just need to be aware of those two things mentioned above...

It looks like we could encode binary data via base64. base64 would increase the data size by ~ 33%.

Add buffer-type.h to the same directory as the other base type. Signed-off-by: Todd Gill <tgill@redhat.com>

…tter Move to facilitate shared JSON generation code with the SID process. Update commands to write output to a buffer for output. Signed-off-by: Todd Gill <tgill@redhat.com>

trgill · 2021-03-25T01:01:03Z

src/resource/ubridge.c

 		return -1;
 	}

-	if ((r = _export_kv_store(cmd_res)) < 0) {


@prajnoha am I missing something here? It looks like we append the kv store even when responding to commands other than "dump"?

Ah yes, this is something I've also noticed recently - it's actually a bug!

The _export_kv_store is supposed to export the KV store snapshot diffs from SID worker process and export it to main process for syncing with main DB. Of course, this (currently) makes sense only for the usid scan command and we shouldn't be calling _export_kv_store for any other cases, including any of sidctl commands, because they do not change the database in any way, just peeking at it.

(The dump command just takes what is recorded in SID db, so not syncing any changes with main SID db. That means, the _export_kv_store call should be also skipped for the dump command. The dump cmd on SID side then collects formatted copy of the DB into result buffer which is then send over to the client...)

I have a patch for this in my queue, will commit that...

...well, for records marked as belonging to KV_NS_UDEV namespace, the _export_kv_store exports to udev db actually, not the SID db. But all the other namespaces (KV_NS_DEVICE, KV_NS_MODULE, KV_NS_GLOBAL) do sync with main SID db. So from this perspective, the SID db is extension to udev db in a way and KV_NS_UDEV is just for accessing what would normally be available in udev environment.

Ok - thanks for confirming. It causes problems with the JSON response. When sidctl gets the response, the size includes the data that is added via _export_kv_store().

@trgill This should be patched within PR #96. Please try with that patch...

This is working.

I pushed 54f03a2 to this PR

(edit: deleted previous incorrect comment)

prajnoha

OK, the patchset looks good. Just these trivia:

ubridge.c:1716:43: warning: unused variable 'data' [-Wunused-variable]
        char *               resource_tree_data, data;
                                                 ^
ubridge.c:1717:29: warning: unused variable 'buf_size' [-Wunused-variable]
        size_t               size, buf_size;
                                   ^

...and the comments in the code.

As for binary data - yes, we could use base64 which is pretty standard and it's easy to decode. And also we could simply print those values - we'd need some representation of those binary data anyway for printing. I don't expect we'll ever store huge amounts of binary data, they're usually short chunks of direct binary structures so the extra cost of ~33% is OK I think.

prajnoha · 2021-03-30T11:22:10Z

src/tools/sidctl/sidctl.c


 	if (r == 0) {
-		buffer_get_data(buf, (const void **) &hdr, &size);
+		buffer_get_data(outbuf, (const void **) &hdr, &size);


When I tried sidctl version, I got incorrect output for SID version - it's just this line that has typo, I think. It should be readbuf instead of outbuf here.

Thanks. Fixed.

prajnoha · 2021-03-30T11:31:42Z

src/tools/sidctl/sidctl.c

+		buffer_get_data(readbuf, (const void **) &msg, &size);
+		if (size < USID_MSG_HEADER_SIZE || msg->status & COMMAND_STATUS_FAILURE) {
+			buffer_destroy(readbuf);
+			return -1;
+		}
+		size -= USID_MSG_HEADER_SIZE;
+		buffer_add(outbuf, msg->data, size, &r);


Hmm, we're reading almost the whole complete and formatted message here, just skipping the header. Then we're copying all the rest to the output buffer. It works fine, but would be nicer if we could avoid the extra copy.

Maybe we could define the output buffer as BUFFER_TYPE_VECTOR in which case we could add the pointer to the msg->data + size directly into the vector buffer as element. But right now, formatting is done on sidctl side for other command now and they use the output buffer for formatting so there the BUFFER_TYPE_VECTOR is not ideal. But this is caused by the fact that tree command is formatted on SID side while the rest is formatted on sidctl side at the moment.

So maybe we can keep this as it is for now (copying the msg->data to outbuf which is BUFFER_TYPE_LINEAR), later we can optimize and/or move the rest of commands to SID-side formatting.

Ok. I'll open an issue to fix.

Fixed the warnings. Sorry for that mistake.

The "sidctl tree" command prints the resource tree in JSON. For each resource the event-sources, pid-created, flags, priority and children are included in the output. The tree command is intended for debugging. In the future, it may make sense to only include it in debug builds. Signed-off-by: Todd Gill <tgill@redhat.com>

prajnoha · 2021-03-31T13:12:59Z

OK, merged, thanks! There's just one thing - SID_SESSION_ID=... added at the very end of output, but I think I know why it's there - I need to exclude the sidctl tree cmd from the ones that generate/can use the session id - I'll patch that, should be one liner.

prajnoha · 2021-03-31T13:29:13Z

That was only missing the patch 7e8864a, I appended it...

trgill · 2021-03-31T13:47:20Z

👍

trgill force-pushed the resource_traverse branch from 8796490 to 4c6b65b Compare March 11, 2021 12:59

trgill commented Mar 11, 2021

View reviewed changes

trgill force-pushed the resource_traverse branch 6 times, most recently from 7294ae3 to df4d665 Compare March 23, 2021 19:41

trgill added 2 commits March 24, 2021 20:42

base: move buffer-type.h from /src/base to src/include/base

9ad02e4

Add buffer-type.h to the same directory as the other base type. Signed-off-by: Todd Gill <tgill@redhat.com>

sidctl: move JSON generation code out of sidctl and into common forma…

24e57b7

…tter Move to facilitate shared JSON generation code with the SID process. Update commands to write output to a buffer for output. Signed-off-by: Todd Gill <tgill@redhat.com>

trgill force-pushed the resource_traverse branch from df4d665 to c223d84 Compare March 25, 2021 00:48

trgill changed the title ~~start of adding sidctl command to display resource tree - not ready for merge~~ add sidctl "tree" command to display resource tree in JSON Mar 25, 2021

trgill commented Mar 25, 2021

View reviewed changes

trgill force-pushed the resource_traverse branch from c223d84 to 54f03a2 Compare March 26, 2021 14:12

prajnoha mentioned this pull request Mar 30, 2021

resource: ubridge: define and use flags for _cmd_regs representing ucmd commands #96

Closed

prajnoha reviewed Mar 30, 2021

View reviewed changes

trgill force-pushed the resource_traverse branch from 54f03a2 to 9869d39 Compare March 30, 2021 21:08

prajnoha merged commit 949f519 into sid-project:main Mar 31, 2021

trgill mentioned this pull request Apr 1, 2021

Support resource introspection/dump and display it by sidctl command #79

Open

trgill mentioned this pull request May 7, 2021

public API for SID #103

Open

add sidctl "tree" command to display resource tree in JSON #89

add sidctl "tree" command to display resource tree in JSON #89

Uh oh!

Conversation

trgill commented Mar 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prajnoha Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trgill Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prajnoha Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prajnoha Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prajnoha Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trgill Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prajnoha left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prajnoha commented Mar 31, 2021

Uh oh!

prajnoha commented Mar 31, 2021

Uh oh!

trgill commented Mar 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

prajnoha Mar 11, 2021 •

edited

Loading

trgill Mar 19, 2021 •

edited

Loading

prajnoha Mar 25, 2021 •

edited

Loading

prajnoha Mar 25, 2021 •

edited

Loading

prajnoha Mar 26, 2021 •

edited

Loading

trgill Mar 26, 2021 •

edited

Loading

prajnoha left a comment •

edited

Loading