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

job: make eventlog contexts json objects instead of free form text #2085

Merged
merged 7 commits into from Mar 20, 2019

Conversation

@chu11
Copy link
Contributor

commented Mar 20, 2019

Per discussion in #2076. A few points worth mentioning:

  • notes are no longer extra text, they are a key:value in the json object. Typically listed with "note=..." format from flux job.
  • flux job eventlog and flux job wait-event now have a --context-format option to output in raw json or nicer key=value output. It defaults to key=value, so that most tests continue to work the same.
  • when doing json_dumps() on a json string, it defaults to add quotes around the string. In order to keep output identical to what was there before (with no quotes), I had to check if it was a string value first.
            json_object_foreach (o, key, value) {
                const char *sval;
                char *ptr = NULL;
                if (json_is_string (value))
                    sval = json_string_value (value);
                else {
                    ptr = json_dumps (value, JSON_ENCODE_ANY|JSON_COMPACT);
                    sval = ptr;
                }
                printf (" %s=%s", key, sval);
                free (ptr);
            }

ehhhh, maybe not worth it? Could we just accept quotes around any strings? Will have to adjust a number of tests as a result (i.e. type=cancel -> type="cancel")

  • I renamed event_job_post_fmt() to event_job_post_pack() b/c it now takes json_pack() like syntax for contexts.
Per RFC21 changes, update job-ingest module to format context data
in a json object instead of key=value pairs.
@chu11 chu11 changed the title job: eventlog contexts are now json objects instead of free form text job: make eventlog contexts json objects instead of free form text Mar 20, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

looking through diff coverage at 75.96%, all important parts hit, just missed error paths

Copy link
Member

left a comment

Thanks - looks good! I did find a few things I think could be changed to improve the code. If I strayed into personal preference territory LMK, I'll bend.

On the string quotes/no quotes question, I'm fine as you have it, but don't feel strongly. Maybe @grondo has an opinion?

@@ -240,29 +243,46 @@ int event_job_update (struct job *job, const char *event)
if (job->state != FLUX_JOB_NEW)
goto inval;
job->t_submit = timestamp;
if (util_int_from_context (context, "priority", &priority) < 0)
if (!(o = json_loads (context, 0, NULL))) {

This comment has been minimized.

Copy link
@garlick

garlick Mar 20, 2019

Member

This function is going to get quite long as we add events. Could we perhaps create functions to decode each event that has a context, e.g. for this one, maybe

int event_submit_context_decode (const char *event, int *priority, uint32_t *userid, int *flags);
int event_job_post (struct event_ctx *ctx, struct job *job,
flux_continuation_f cb, void *arg,
const char *name, const char *context)
static int post (struct event_ctx *ctx, struct job *job,

This comment has been minimized.

Copy link
@garlick

garlick Mar 20, 2019

Member

What's the reason for this change? It seems like we've created the post() function without gaining anything?

This comment has been minimized.

Copy link
@chu11

chu11 Mar 20, 2019

Author Contributor

doh, I had changed event_job_post() at one point, thus making the function wrapper for post().

return event_job_post (ctx, job, cb, arg, name, context);
if (!(context = json_dumps (o, JSON_COMPACT))) {
errno = EINVAL;
goto error;

This comment has been minimized.

Copy link
@garlick

garlick Mar 20, 2019

Member

I think you may get into trouble with unbalanced va_start() / va_end() here?

}
return event_job_post (ctx, job, cb, arg, name, context);
if (!(context = json_dumps (o, JSON_COMPACT))) {

This comment has been minimized.

Copy link
@garlick

garlick Mar 20, 2019

Member

Need to check that encoded string doesn't exceed RFC 18 limit for context length.

This comment has been minimized.

Copy link
@chu11

chu11 Mar 20, 2019

Author Contributor

Oh yeah, in this case the check will "fallthrough" to the eventual flux_kvs_event_encode() call in event_job_post(). I'll add a comment though.

char *str = NULL;
int save_errno, rv = -1;

if (!(o = json_pack ("{ s:i s:i s:i }",

This comment has been minimized.

Copy link
@garlick

garlick Mar 20, 2019

Member

Suggestion: move event encoding to a separate function and just call flux_kvs_event_encode_timestamp() rather than encoding manually.

};

void output_event (const char *event)
int context_format (optparse_t *p)

This comment has been minimized.

Copy link
@garlick

garlick Mar 20, 2019

Member

I don't think converting to an enum is really buying us anything over a strcmp on the original option value.

This comment has been minimized.

Copy link
@chu11

chu11 Mar 20, 2019

Author Contributor

I think I was overthinking future support, where there would be other future output formats. It is a bit overkill.

return -1;
}

void output_event (const char *event, int format)

This comment has been minimized.

Copy link
@garlick

garlick Mar 20, 2019

Member

Suggestion: refactor to output_event_json() and output_event_text() and leave the format option checking to the main function for the subcommand?

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

I guess one question is how far we want to go with making the event context readable in "text" mode? Can we get rid of the note= prefix for what used to be free form strings at the end of the context?

@grondo

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

I guess one question is how far we want to go with making the event context readable in "text" mode? Can we get rid of the note= prefix for what used to be free form strings at the end of the context?

Yeah, I had the same thought, but I'm not sure I have any good insight. For now we should probably keep things simple, but I could see a point in the future where we might want to share the per-event "decode" functions between the job-manager and the flux-job eventlog command, so that wait-event or other utilities could be extended to search and/or wait based on not only the event but the context as @chu11 had mentioned before.

(Sorry, that isn't necessarily germane to the discussion about "text" mode, except that I was thinking that perhaps each event could have a custom method for formatting...)

FWIW, it is probably fine if JSON strings are quoted in "human readable" text mode.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

FWIW, it is probably fine if JSON strings are quoted in "human readable" text mode.

Ok, lets go with them quoted.

chu11 added 6 commits Mar 18, 2019
Per RFC21 changes, update job-manager to format context data
in a json object instead of key=value pairs.

As a consequence, change event_job_post_fmt() to
event_job_post_pack(), so event context data can be created
similarly to how json_pack() is used.
Remove util.[ch] and util unit tests.  None of these utility functions
are used anymore.
Update flux job eventlog and flux job wait-event for recent updates
to eventlog contexts stored in json format.

Support new --context-format option for both subcommands, which allow
user to output the context in raw json format or in a more readable
key=value output.
Strings in the default flux job eventlog output are now quoted, so adjust
tests that previously did not assume quotes around strings.

Additional notes in an event are now listed via a key of "note" and
not just additional text at the end of a line.  Update tests
accordingly to check for "note=".
@chu11 chu11 force-pushed the chu11:issue2076 branch from 267d00e to 6fef113 Mar 20, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

re-pushed with suggested changes, default output of strings now includes quotes.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 20, 2019

Codecov Report

Merging #2085 into master will decrease coverage by 0.04%.
The diff coverage is 74.38%.

@@            Coverage Diff             @@
##           master    #2085      +/-   ##
==========================================
- Coverage   80.44%   80.39%   -0.05%     
==========================================
  Files         193      192       -1     
  Lines       30532    30564      +32     
==========================================
+ Hits        24560    24571      +11     
- Misses       5972     5993      +21
Impacted Files Coverage Δ
src/modules/job-manager/restart.c 84.05% <ø> (ø) ⬆️
src/modules/job-manager/raise.c 79.54% <100%> (ø) ⬆️
src/modules/job-manager/alloc.c 74.61% <100%> (+0.12%) ⬆️
src/modules/job-manager/priority.c 89.65% <100%> (ø) ⬆️
src/modules/job-manager/submit.c 65% <66.66%> (-2.25%) ⬇️
src/modules/job-manager/event.c 70.74% <68.75%> (-2.81%) ⬇️
src/modules/job-ingest/job-ingest.c 73.72% <77.77%> (+0.38%) ⬆️
src/cmd/flux-job.c 84.72% <83.87%> (-0.33%) ⬇️
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/modules/barrier/barrier.c 76.55% <0%> (-2.07%) ⬇️
... and 3 more
@garlick

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Thanks for the fixes. Let me poke at this a bit and then I think it can go in.

@grondo, I can do the rebase of #2077 and force push, once this is merged, if that works for you.

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

This looks great @chu11, nice work as usual. Merging.

@garlick garlick merged commit 4e44bc0 into flux-framework:master Mar 20, 2019
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 74.38% of diff hit (target 80.44%)
Details
Mergify — Summary 1 potential rule
Details
codecov/project 80.39% (-0.05%) compared to 761b475
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@grondo, I can do the rebase of #2077 and force push, once this is merged, if that works for you.

Do you have pending work on that branch? I'm currently adding a test, so it would be easier for me to rebase?

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

No pending work here. If you want to do it that's OK, but I'm happy to help if needed.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

but I'm happy to help if needed.

Ah, now that I've started on the rebase I totally understand why you offered to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.