Core Page on Web Stats#101
Conversation
CI MessageYour results will arrive shortly |
CI MessageRun successful see results: Linter Passed |
|
Everything is minified and web stuff is rebuilt, I'll test later today |
|
Awesome thanks, I tried to limit the changes in c code, and there is no added code to any loops so performance shouldn't drop a result (most of the changes are initialization). |
koolzz
left a comment
koolzz
left a comment
There was a problem hiding this comment.
Lets try to make things more generic, adding new arguments into the call isn't the cleanest solution.
|
|
||
| void | ||
| onvm_stats_add_event(const char *msg, struct onvm_nf_info *nf_info) { | ||
| onvm_stats_add_event(const char *msg, struct onvm_nf_info *nf_info, unsigned core) { |
There was a problem hiding this comment.
Should this be more generic? I don't like adding a unsigned core argument as its not always used and we might need to support other arguments later. Could we provide an arg struct, with different IDs?
struct event_info {
uint16_t type;
const char *msg;
void *data;
};
We should also provide helper functions that generate this struct. For example smth like gen_event_info(msg) for our simple messages, that way we still have the oneline onvm_stats_add_event calls, but we could also do gen_event_info_with_core_arg(msg, core) (might need a better name) when we need more arguments.
And then the onvm_stats_add_event based on the event can parse arguments appropriately.
(this way we also combine msg + any other possible args)
There was a problem hiding this comment.
Ya that's a better way of doing it, I'll work on these fixes when I finish my other work this weekend
|
Semi related question - do we currently show any info about what core an NF is running on in the text console stats? If not, we should |
|
@twood2 We don't but that's definitely something we should include for this release |
|
check it out @onvm |
CI MessageYour results will arrive shortly |
CI MessageRun successful see results: Linter Failedexamples/aes_decrypt/aes.h:176: #endif line should be "#endif // AES_H" [build/header_guard] [5] |
|
Welp it seems nicer when there's no errors but whatever. |
I prefer a separate PR, the reason being is we want to make the stats code prettier (currently its a bit ugly). I was thinking of changing the description and value lines to |
| typedef enum { ONVM_STATS_NONE = 0, ONVM_STATS_STDOUT, ONVM_STATS_STDERR, ONVM_STATS_WEB } ONVM_STATS_OUTPUT; | ||
|
|
||
| struct onvm_event { | ||
| const char *type; |
There was a problem hiding this comment.
type should be a numeric value, uint8_t would work.
Then you can define a few different types like ONVM_EVENT_NF_INFO, ONVM_EVENT_MGR_TX_CORE_INFO, ONVM_EVENT_MGR_RX_CORE_INFO smth like that
| cJSON_AddStringToObject(source, "type", (const char *)event_info->type); | ||
|
|
||
| if (event_info->data != NULL) { | ||
| if (strcmp(event_info->type, "NF") == 0) { |
There was a problem hiding this comment.
And if you make type an int this will be a simple == check
| cJSON_AddNumberToObject(source, "instance_id", (int16_t)nf_info->instance_id); | ||
| cJSON_AddNumberToObject(source, "service_id", (int16_t)nf_info->service_id); | ||
| cJSON_AddNumberToObject(source, "core", (int16_t)nf_info->core); | ||
| } else { |
There was a problem hiding this comment.
Also be exhaustive, do
if(event_A)
bla
else if (event B)
blablabla
.
.
.
.
else
error as we don't support anything else
| time_t time_raw_format; | ||
| struct tm *ptr_time; | ||
| struct onvm_nf_info *nf_info; | ||
|
|
| cJSON_AddStringToObject(source, "type", "NF"); | ||
| cJSON_AddStringToObject(new_event, "message", event_info->msg); | ||
|
|
||
| if (type == 0) { |
There was a problem hiding this comment.
Skimmed through, this is the part that needs to be changed, avoid magic numbers. Always do #define COOL_MSG_NAME 2
| #define ONVM_STATS_FILE ONVM_STATS_PATH_BASE "onvm_stats.txt" | ||
|
|
||
| #define ONVM_EVENT_MGR_INFO "Manager" | ||
| #define ONVM_EVENT_PORT_INFO "Port" |
There was a problem hiding this comment.
You already have the names so just bundle it up with msg IDs
|
@onvm make sure we're all good |
CI MessageYour results will arrive shortly |
CI MessageRun successful see results: Linter Failedexamples/aes_decrypt/aes.h:176: #endif line should be "#endif // AES_H" [build/header_guard] [5] |
CI MessageYour results will arrive shortly |
CI MessageRun successful see results: Linter Failedexamples/aes_decrypt/aes.h:176: #endif line should be "#endif // AES_H" [build/header_guard] [5] |
| void | ||
| onvm_stats_add_event(const char *msg, struct onvm_nf_info *nf_info) { | ||
| if (msg == NULL || stats_destination != ONVM_STATS_WEB) { | ||
| gen_event_info(const char *msg, uint8_t type, void *data) { |
There was a problem hiding this comment.
public api funcs should be prefixed by filename, onvm_stats_gen_event_info
| } | ||
|
|
||
| void | ||
| gen_event_nf_info(const char *msg, struct onvm_nf_info *nf_info) { |
koolzz
left a comment
koolzz
left a comment
There was a problem hiding this comment.
Looks a lot better, pointed out some naming nits. Also never use magic numbers, if you defined the ONVM_EVENT_WITH_CORE to 0 and then use the gen_event function, always use the macro and not 0 when passing in args `gen_event_info("name thingy", ONVM_EVENT_WITH_CORE , &core)
| gen_event_nf_info("NF Stopping", nf); | ||
|
|
||
| /* Cleanup the allocated tag */ | ||
| if (nf->tag) { |
There was a problem hiding this comment.
tag cleanup is already in onvm_nf_stop, remove this
There was a problem hiding this comment.
well at least in the nf tag pr, not sure why its removed here
There was a problem hiding this comment.
I removed it from onvm_nf_stop. The reason is because when calling the json for the react app, on cleanup, when you call onvm_nf_stop, nf->tag is NULL. Thus, there is no way of knowing which nf (on the web side) needs to be removed from the display. So I just called it after the event info, as the only time onvm_nf_stop is called, is here.
There was a problem hiding this comment.
Not sure if I understand, why would the tag be required? you have the data from the onvm_nf_info right? How did we stop this before because tags were broken
There was a problem hiding this comment.
Also is actually dangerous anyway as we're freeing the memory and then trying to use it again.
I propose we rewrite this a little, add a new event macro ONVM_EVENT_NF_STOPPING, and do the message will just have the instance_id value of the NF that was stopped(save instance id as a temp value before calling stop). Does that make sense?
There was a problem hiding this comment.
It's because Aaron's code assumes that there is no tag at all. So in the code, NF <instance_id> is just passed around in the js. But now, I fixed the javascript so that we pass around the tag, so we can see it on every page (like a new id). But when we're cleaning up, in Stats NF conditonal, we pass the javascript NF as the name, instead of nf->tag if tag doesn't exist. So on start, an NF with tag will get a custom type, but on termination, it will be falsely called with NF <instance_id>, so we can't find it.
There was a problem hiding this comment.
Tag for id seems like a bad idea though, its not unique
There was a problem hiding this comment.
The uniqueness comes with <tag> <instance_id>
There was a problem hiding this comment.
Yeah but tag is just a pretty name, instance id is what we should actually use. Is there a benefit of including the tag there that I'm missing? Should we just pass it as extra data?
And as this usage in unsafe we need to rewrite this a little, do you agree with my earlier comment on that?
There was a problem hiding this comment.
Ya I agree and I'm working on changing it now. I'll figure out a way to fix the js code to work with it
| return; | ||
| } | ||
|
|
||
| event->type = 2; |
There was a problem hiding this comment.
magic number 2 replace with macro
| cJSON_AddNumberToObject(source, "service_id", (int16_t)nf_info->service_id); | ||
| cJSON_AddNumberToObject(source, "core", (int16_t)nf_info->core); | ||
| } else { | ||
| perror("Invalid stats event type"); |
There was a problem hiding this comment.
rte_exit is what we typically use
|
@koolzz We should be good now. I made a fix in the javascript so we don't even need a nf stopping flag in |
| nf = (struct onvm_nf_info *)msg->msg_data; | ||
| if (onvm_nf_stop(nf) == 0) { | ||
| onvm_stats_add_event("NF Stopping", nf); | ||
| onvm_stats_gen_event_nf_info("NF Stopping", nf); |
There was a problem hiding this comment.
Not safe as memory might be cleaned up, can we create a custom event and only pass an instance_id value?
| nf_status = nf_info->status; | ||
|
|
||
| /* Cleanup the allocated tag */ | ||
| if (nf_info->tag) { |
There was a problem hiding this comment.
Not sure why this is an addition its already in the dev branch. Can you pull upstream dev?
| #define ONVM_STATS_FILE ONVM_STATS_PATH_BASE "onvm_stats.txt" | ||
|
|
||
| /* | ||
| * Handle types of web stats events |
There was a problem hiding this comment.
A bit overkill, for short comments (not method stub comments)
/* Handle types of web stats events */
| /* | ||
| * Interface called by manager when a new event should be created. | ||
| * Interfaces called by manager when a new event should be created. | ||
| * |
| break; | ||
|
|
||
| /* Get instance_id before tearing down nf */ | ||
| stop_nf_id = (void *)&(nf->instance_id); |
There was a problem hiding this comment.
Yeah we can't do this, the assumption is the nf memory can be freed, thus we need to save instance_id in a temp value, you're pointing to a value which doesn't quite cut it as it might get freed.
Instead just try doing
/* Saved as onvm_nf_stop frees the memory*/
uint16_t stop_nf_id = (nf->instance_id);
onvm_stats_gen_event_info("NF Stopping", ONVM_EVENT_NF_STOP, &stop_nf_id);
I'm assuming that will work although double check
|
@kevindweb Tested, core page is nice. Everything seems to work as expected. MGR thread proly needs to be set to something like mgr aux(stats) or something. Might be cool to actually show cores allocated from the onvm_mgr and say that they are idle rn. Also we would want to know which cpu socket the core is on (basically a few future improvements, probably not in this pr) Also noticed possibly a small js bug (the highlight under the 3 different tabs isn't under the right tab) See screenschot (See some of my code comments above) |
|
Thanks for reviewing, ya we can work on some of those functionality updates this summer. I've looked into the tab issue before when testing the old web stats. You need to click somewhere else and it will update. I'll see if there is an easy fix. Also, I'm making the updates you requested and pulling develop so I'll push again in a minute. |
|
@onvm Sanity check |
CI MessageYour results will arrive shortly |
CI MessageRun successful see results: Linter Failedexamples/arp_response/arp_response.c:284: Lines should be <= 120 characters long [whitespace/line_length] [5] |
4 participants
Added a core page to view where all the threads are located.
Summary:
In the web stats view of onvm, seen through
./onvm/go.sh 0,1,2 1 0xF8 -s web, previously, the page was empty. Now, we have information about the cores, which can be seen in card format. Right now each core only runs one process, but with shared cpu in the future, will show multiple items on the same core.Usage:
Run onvm web stats as shown above, and click on the Core Mappings page. This will be in the localhost (port 8080) of the node.
Merging notes:
TODO before merging :
Review:
Sanity checks, assigned to @koolzz @dennisafa
Run linter
Check for memory leaks
Performance, assigned to @koolzz @dennisafa
Documentation, assigned to @koolzz @dennisafa