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-manager: add job state transition announcements #2109

Merged
merged 7 commits into from Apr 1, 2019

Conversation

@garlick
Copy link
Member

commented Mar 28, 2019

As discussed in #2094, this PR adds a simple notification mechanism for bulk job state transitions based on published event messages. This is just the job-manager end. The events could be consumed by subscribing to job-state and interpreting each message (see below). I would expect the simulator front end to be the first customer of this feature.

Eventually we may want to put a nicer interface in front of this, but I think that could be a separate PR and will require some more design (and maybe some experience using the raw event interface first?)

Each job state transition is represented as a [jobid,state] tuple, and is batched up in array form for publication after the event that triggered the state transition has landed in the KVS. Each published message can included state transitions for multiple jobs, and multiple transitions per job. Here's a little snippet of flux event sub job-state while running sched-bench.sh:

job-state	{"transitions":[[612267720704,"D"],[612267720704,"S"],[612267720704,"R"],[612267720704,"C"],[612267720704,"I"]]}
job-state	{"transitions":[[677380096000,"D"],[677380096000,"S"],[677380096000,"R"],[677380096000,"C"],[677380096000,"I"]]}
job-state	{"transitions":[[1566656430080,"D"],[1566656430080,"S"],[1566572544000,"D"],[1566572544000,"S"],[1566572544000,"R"],[1566656430080,"R"],[1566572544000,"C"]]}
job-state	{"transitions":[[1566656430080,"C"],[1566840979456,"D"],[1566840979456,"S"],[1566757093376,"D"],[1566757093376,"S"],[1566757093376,"R"],[1567008751616,"D"],[1567008751616,"S"],[1566924865536,"D"],[1566924865536,"S"]]}
job-state	{"transitions":[[1566572544000,"I"],[1566840979456,"R"],[1566757093376,"C"],[1566656430080,"I"],[1566924865536,"R"]]}

I went with this form rather than a possibly more compact one (such as state => id array in an object) so that the order of all the transitions is preserved, in case that ends up being important.

Along the way I added the DEPEND state (which immediately triggers a depend event, which causes a transition to SCHED state), and updated tests accordingly. @chu11: I reworked that code in t2205-job-info-security.t for editing the eventlog in the KVS since what was there did not seem to be working after I added the depend event - let me know if those changes look OK to you (see bfcfde2)

I still need to add a test that subscribes to these events and ensures all the state transitions are published for a group of jobs, but wanted to get this posted for early review.

event_job_post() and event_job_post_pack() both include
a callback mechanism, so that the caller can be informed
upon completion of the batched KVS commit of eventlog entries.
This was originally intended for both error handling and
as a way to usefully tie state notification to KVS updates.

The errors are now handled directly in the callback.
We will generate state notification events directly as
well, so it is best for simplicity to remove this mechanism
that is otherwise not used.
@grondo

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

Nice!

Since the states are identified as strings anyway, did you consider using the RFC 21 state names instead of single character representations? In the end I suppose it doesn't matter because likely only code and scripts will be examining these messages, but some of the single character choices do conflict with existing resource manager "single character" states, so just wondering if saving a few characters is worth it.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

just wondering if saving a few characters is worth it.

Eh probably not. I'll push a change.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

Ok, looks like this now:

job-state	{"transitions":[[459578277888,"DEPEND"],[459578277888,"SCHED"],[459494391808,"DEPEND"],[459494391808,"SCHED"],[459494391808,"RUN"],[459578277888,"RUN"],[459494391808,"CLEANUP"]]}
job-state	{"transitions":[[459578277888,"CLEANUP"],[459762827264,"DEPEND"],[459762827264,"SCHED"],[459678941184,"DEPEND"],[459678941184,"SCHED"],[459678941184,"RUN"]]}
job-state	{"transitions":[[459494391808,"INACTIVE"],[459930599424,"DEPEND"],[459930599424,"SCHED"],[459846713344,"DEPEND"],[459846713344,"SCHED"],[459578277888,"INACTIVE"],[459762827264,"RUN"],[459678941184,"CLEANUP"],[459846713344,"RUN"],[459762827264,"CLEANUP"]]}
job-state	{"transitions":[[460115148800,"DEPEND"],[460115148800,"SCHED"],[460014485504,"DEPEND"],[460014485504,"SCHED"],[459930599424,"RUN"],[459846713344,"CLEANUP"],[459678941184,"INACTIVE"]]}
# string, so I do a classic trick of adding an extra char at the end
# to avoid newline stripping, then strip the extra char.

# Usage: submit_job [userid]

This comment has been minimized.

Copy link
@chu11

chu11 Mar 29, 2019

Contributor

minor nit, perhaps add a comment saying why you do the get --raw | sed | put --raw, it's b/c you want to preserve the trailing newline.

This comment has been minimized.

Copy link
@garlick

garlick Mar 31, 2019

Author Member

Agreed! I added this:

# This method of editing the eventlog preserves newline separators.
@garlick garlick force-pushed the garlick:job_state_bulk branch 2 times, most recently from 285c276 to 4a95fd6 Mar 30, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

Finally got the test coverage up to reasonable levels. I think this could probably go in.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

Oh right, except there's some incremental development that needs to be squashed. Will do that now.

@garlick garlick force-pushed the garlick:job_state_bulk branch from 4a95fd6 to a653a39 Mar 31, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

I think I will drop that last commit that tries to cover unloading the job manager with KVS commits/event pubs in flight as it's racy with respect to the timing of the KVS/rank 0 broker. For two codecov runs, I got coverage the first time and not the second time. If this goes in then people will waste time trying to find coverage drops that are just due to this race. I'll force a push and then I this is probably ready to go in @chu11.

@garlick garlick force-pushed the garlick:job_state_bulk branch from a653a39 to 27fa4d4 Apr 1, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Apr 1, 2019

Codecov Report

Merging #2109 into master will increase coverage by 0.02%.
The diff coverage is 77.67%.

@@            Coverage Diff             @@
##           master    #2109      +/-   ##
==========================================
+ Coverage   80.29%   80.31%   +0.02%     
==========================================
  Files         197      197              
  Lines       31483    31546      +63     
==========================================
+ Hits        25278    25337      +59     
- Misses       6205     6209       +4
Impacted Files Coverage Δ
src/modules/job-manager/submit.c 73.33% <100%> (+0.25%) ⬆️
src/modules/job-manager/raise.c 79.54% <100%> (ø) ⬆️
src/modules/job-manager/start.c 72.56% <100%> (ø) ⬆️
src/modules/job-manager/alloc.c 75.24% <100%> (ø) ⬆️
src/modules/job-manager/priority.c 89.65% <100%> (ø) ⬆️
src/cmd/flux-job.c 84.69% <100%> (+0.18%) ⬆️
src/modules/job-manager/event.c 74.3% <65.62%> (+1%) ⬆️
src/common/libjob/job.c 78.14% <90.62%> (+3.35%) ⬆️
src/common/libflux/message.c 81.15% <0%> (-0.37%) ⬇️
src/cmd/flux-module.c 83.72% <0%> (-0.24%) ⬇️
... and 3 more
Copy link
Contributor

left a comment

just saw some nits, but looks good to go

@@ -1,5 +1,4 @@
/************************************************************\
* Copyright 2018 Lawrence Livermore National Security, LLC
/************************************************************\ * Copyright 2018 Lawrence Livermore National Security, LLC

This comment has been minimized.

Copy link
@chu11

chu11 Apr 1, 2019

Contributor

accidental newline removal?

This comment has been minimized.

Copy link
@garlick

garlick Apr 1, 2019

Author Member

Gah, embarrassing!

/* N.B. any in-flight batches are destroyed here.
* If they are not yet fulfilled, user callbacks may synchronously block on
* flux_future_get().
/* Any in-flight KVS batche commits and their follow-on job-state.STATE event

This comment has been minimized.

Copy link
@chu11

chu11 Apr 1, 2019

Contributor

typo "batche"

This comment has been minimized.

Copy link
@garlick

garlick Apr 1, 2019

Author Member

That one was already fixed. Comment on an outdated branch?

This comment has been minimized.

Copy link
@chu11

chu11 Apr 1, 2019

Contributor

i was looking commit by commit, so didn't see that you had addressed it later

This comment has been minimized.

Copy link
@garlick

garlick Apr 1, 2019

Author Member

Oh sorry that was my bad - I must have squashed that into the wrong commit!

@@ -108,12 +108,17 @@ void event_batch_commit (struct event_ctx *ctx)
struct event_batch *batch = ctx->batch;

This comment has been minimized.

Copy link
@chu11

chu11 Apr 1, 2019

Contributor

commit message typo "seperately"

This comment has been minimized.

Copy link
@garlick

garlick Apr 1, 2019

Author Member

seperately/separately, my achilles heel.

garlick added 6 commits Mar 27, 2019
Add a JSON array to the KVS batch machinery in event.c
that tracks [jobid, state] tuples.  When the post function
handles an event that triggers a job state change, add
the job and its new state to this object.

Once the triggering event(s) are committed to the KVS,
publish a job-state event message, notifying interested
parties that a set of jobs have transitioned to a new state.
The fact that this comes after the KVS commit has completed
maintains the invariant from RFC 21 that notification of
certain state changes indicate that info, such as exception
events or exec finish events, is fetchable from the KVS.

The timed batch machinery, already there to mitigate
the load on the KVS from bursts of job events, now
helps reduce the number of published event messages.
Each message will contain more job ID's, and the latency
will increase a bit from event occurrence to notification.
Add functions for converting between strings and job
states.

Use flux_job_statetostr() instead of a local function
in 'flux job list'.

Add coverage for new functions to libjob unit test.
Transition NEW->DEPEND state on the submit event.
Then emit a depend event in DEPEND state that
transitions to SCHED state.

Update job unit test.

Update job-info-security sharness test:
- add a 'flux job wait-event depend' before
  reading the eventlog to rewrite it.
- use a method of editing eventlog in place
  that should be a bit more robust towards
  preserving formatting:
Refactor event_job_post() so that event_batch_pub_state()
can be called separately.  Then call it for the 'submit'
event to get the first state transition (NEW->DEPEND)
announced.
Problem: sched-bench.sh fails job submission due to
duration expressed as a string with units, instead
of the numeric type (implicit units=seconds) per
recent changes to RFC 14.

Change string to a number (thanks @grondo).
Add a python script that subscribes to job state change
event notifications, then submits a specified number of jobs,
and verifies that notifications are received indicating that
each job progressed through an expected set of states,
in an expected order.  Notifications from jobs not submitted
by the script are ignored.

Drive the sript from a sharness test, on rank0, and simultaneously
across all ranks.
@garlick garlick force-pushed the garlick:job_state_bulk branch from 27fa4d4 to 19cf1cf Apr 1, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

OK, addressed those comments. Thanks!

@chu11 chu11 merged commit 3746a1b into flux-framework:master Apr 1, 2019
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 77.67% of diff hit (target 80.29%)
Details
Mergify — Summary 1 potential rule
Details
codecov/project 80.28% (-0.01%) compared to 5a5fce4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

Thanks!

@garlick garlick deleted the garlick:job_state_bulk branch Apr 1, 2019
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.