Update all eventlog entries to be full JSON objects #2128
Conversation
|
hmmm, haven't seen a hang in awhile. Restarting builder. |
|
rebased |
|
So the context and event name max lengths went away in the RFC 18 updates. How would you feel about adding the next PR to this one? Since we create our release notes from the merge commits, it's nice if a PR completely handles one thing rather than being split over several PRs. Is there a good reason to split it here? (Apologies if we already discussed this and you made a good point that I've now forgotten!) @grondo suggests if we agree that practice is what we want, we could add it to RFC 1. |
Yup.
I just felt it was two different changes. Converting to RFC18 felt like one change (i.e. nothing wrong with keeping the code the way it is and can keep the I can go either way on this. |
|
Sorry I was a bit terse above. I meant the PR doesn't quite get the code in sync with RFC 18 because the old API enforces the old length limitations. The argument for combining the PR's is that it accomplishes the entire "sync with RFC 18 changes" task in one go (one release notes entry), and some intermediate changes can be avoided (e.g. in a combined PR: add new libeventlog api + tests, convert users, drop old kvs/eventlog api + tests, thus avoiding updates the the old api and tests). If thinking in the granularity of release notes, this seems like one part of a whole change, not an independent change. Having said that, you've got this PR all prepared and the button is green. If you prefer, I'll hit the button and we can split hairs on the next topic :-) |
Ahhh. I get it now. I'll go ahead and push the bigger overall change. |
|
re-pushed with the new |
|
Lots of builders failed. All tests pass but I clearly screwed up something in the Makefile.am that travis is expecting with my new unit tests. Hmmm. |
|
hmmm, Possible b/c the logs have bad output? I'll re-work and try again. |
|
oh wait I have > 54 unit tests. Hmmmm, but little info to go off of. |
|
Yeah, that's weird. The fact that the test got a What does the (Of course, there is also a chance there's a problem with our custom tap-driver script...) |
|
This branch fails for me with just plain under gdb in case it is not reproducing for you |
Don't think so. The main routine is pretty normal:
Now that you mention it, there is an odd thing in Note that it indicates EDIT: i now clearly see these are XFAILS. |
Thanks. I'd been concentrating on mem corruption around test 55, but perhaps it's way earlier given you're seeing it at 90. |
|
Here's valgrind output from my system in case it helps: |
|
Hmmm. What version of jansson is on your machine @grondo? I'm wondering if my tests like: are creating problems b/c the |
Ubuntu 18.04: This matches what we've got in the while CentOS 7 has 2.10. Maybe that is why the centos builds seem to succeed? |
Yeah. It appears my guess that the One builder hit a valgrind issue, but no info beyond just this. Hmmm. |
|
re-pushed, for whatever reason, jansson 2.10 was able to correctly tell that a format like |
There was a problem hiding this comment.
Nice work Al! Some minor comments inline.
I think you can squash out of existence the changes to libkvs/eventlog.[ch] and its test by reordering commits?
Note that newlines in the name or in strings embedded in context are no longer a problem since they will be escaped when serialized. (And the RFC no longer explicitly forbids them)
src/common/libeventlog/eventlog.c
Outdated
| if (name) | ||
| (*name) = n; | ||
|
|
||
| if (!json_unpack (entry, "{ s:o }", "context", &c)) { |
There was a problem hiding this comment.
There probably needs to be a check that 'o' is a JSON object and not an array.
src/common/libeventlog/eventlog.c
Outdated
| } | ||
|
|
||
| if (!(copy = strdup (s))) | ||
| goto enomem; |
There was a problem hiding this comment.
strdup sets ENOMEM so just return NULL?
src/common/libeventlog/eventlog.c
Outdated
| return true; | ||
| } | ||
|
|
||
| static json_t *eventlog_entry_decode_wrapper (const char *entry, |
There was a problem hiding this comment.
Different name? It's not a wrapper for event_decode(), it's the other way around right?
src/common/libeventlog/eventlog.c
Outdated
| if (timestamp < 0. | ||
| || !name | ||
| || !strlen (name) | ||
| || strchr (name, ' ') != NULL |
There was a problem hiding this comment.
RFC 18 doesn't actually impose any restrictions on what is in the "name" string anymore, so prob OK to skip these checks for white space. (note embedded newlines are escaped in JSON strings and cannot be confused with the event end of line)
src/common/libeventlog/eventlog.c
Outdated
| int save_errno; | ||
|
|
||
| if (context) { | ||
| if (strchr (context, '\n')) { |
There was a problem hiding this comment.
embedded newline check not needed.
src/common/libeventlog/eventlog.c
Outdated
| int save_errno; | ||
|
|
||
| if (context_fmt) { | ||
| if (strchr (context_fmt, '\n')) { |
There was a problem hiding this comment.
check for embedded newline not needed
src/common/libeventlog/eventlog.c
Outdated
| if (!(s = json_dumps (entry, JSON_COMPACT))) | ||
| goto enomem; | ||
| if (asprintf (&rv, "%s\n", s) < 0) | ||
| goto enomem; |
There was a problem hiding this comment.
asprintf sets errno so might want to not re-set it here.
Ahhh yes, now that I've put the two PRs into one, that initial change makes less sense. I'll see how I can piece things together into a more sensible commit series. |
|
just re-pushed with suggested fixes above and squashed changes into a smaller set of commits. Added new tests such as |
|
LGTM! Just needs a rebase. |
Library contains convenience functions for parsing eventlogs and eventlog entries per RFC18 changes, where eventlog entries are complete JSON objects.
Per changes in RFC18, events are now full JSON objects, so adjust all event creation to be compliant. When appropriate, utilize new functions in libeventlog. Rename several functions by adding pack/vpack suffix, so it is more clear that they take jansson style pack arguments.
Per changes in RFC18, events are now full JSON objects, so adjust appropriately by utilizing new libeventlog functions. As a side effect, json_t pointer representing an eventlog entry is now passed around between many functions instead of a const char * to an event. Collapse event_job_post() and event_job_post_pack() into a single function call. Update unit tests accordingly.
Per changes in RFC18, events are now full JSON objects, so adjust appropriately by utilizing new libeventlog functions.
Per changes in RFC18, events are now full JSON objects, so adjust appropriately by utilizing new libeventlog functions. As consequence, util.[ch] are no longer needed.
Per changes in RFC18, events are now full JSON objects, so adjust appropriately by utilizing new libeventlog functions. Add additional test for invalid format context.
With RFC18 changes that make all events JSON objects, update by using new libeventlog library. Rename the --context-format option to --format. When --format=json, output the entire event as JSON. Update tests.
Update tests per RFC18 changes, events are now full JSON objects.
With new libeventlog internal convenience library, there is no need for the public flux_kvs_eventlog library.
|
rebased |
Codecov Report
@@ Coverage Diff @@
## master #2128 +/- ##
==========================================
- Coverage 80.42% 80.38% -0.04%
==========================================
Files 201 200 -1
Lines 31742 31701 -41
==========================================
- Hits 25529 25484 -45
- Misses 6213 6217 +4
|
|
Awesome! |
Per RFC18 update, this changes all eventlog entries to be full JSON objects. This is the second part of #2106.
Note that there is no refactoring here, just the basics to get flux-core to be RFC18 compliant. Part 3 will include a new
libeventloglibrary to refactor things.