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-exec: implement ability to execute job shells #2198

Merged
merged 14 commits into from Jun 23, 2019

Conversation

@grondo
Copy link
Contributor

commented Jun 19, 2019

This PR is still a work in progress, but I wanted to get a PR open in its current form to ensure I'm able to address likely feedback before I go away for a bit.

This PR adds the ability of the job-exec service to actually execute job shells for a job on receipt of a start request from the job manager. In the current implementation, the job shell used is set from configuration (the job-exec.job-shell broker attribute, falling back to /bin/true) overridden by the attributes.system.exec.job-shell key of submitted jobspec.

The refactoring of the job-exec module to support multiple execution "implementations" did not turn out as clean as I'd hoped, and it is still a bit rough around the edges. I tried to keep nice incremental commits as I worked on the refactor, but ended up throwing it away for one large garbage commit that changes a lot of code, so I apologize for that.

Existing tests for the job-exec module were adapted so that they drive the "test" exec mode, which allows testing of exception handling etc. Therefore, tests of actual execution still need to be added (along with fixes for the things found in testing).

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Oh, and I should warn I haven't fully tested real execution yet, that is also on todo list 😨 So don't be surprised if testing this out something doesn't work.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Ah, I also just realized there are some other sharness tests that need to be updated to use the "test" exec implementation. So some sharness tests will fail in travis...

@grondo grondo force-pushed the grondo:exec-no-really branch from db6cb48 to a01c9b8 Jun 19, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

I like the implementation abstraction. If we end up with something special for compute nodes that don't run full brokers, that will slot in nicely.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

If we end up with something special for compute nodes that don't run full brokers, that will slot in nicely.

I was also thinking we might need a slightly different exec implementation when we need to exec the IMP (thus the abstraction of bulk execution interface), however perhaps when we get to that point we'll see a simpler way.

@grondo grondo self-assigned this Jun 19, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Pushed some incremental updates and experimental tests. Nothing to see here yet -- still have to address @chu11's comments.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Ok, I think this is getting close to the amount of work I'll be able to get ready by tomorrow. I'll continue to do more testing and add more sharness tests (If I can think of any more)

After @chu11 at least has had another review, I'll squash this down to remove the incremental development and we could consider a merge.

One important note: currently the job-exec module kills all job shells on any job exception. This includes flux job cancel, so that is not the long term plan. However, for now we don't have the job signaling mechanism fully spec'd out, and also who initiates the signal to clean up on fatal errors, etc. Once we have that down we can re-architect exception handling here.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Other todo items in job-exec (off the top of my head). I'll try to get these into issues.

  • Epilog: no post-job-shell epilog is supported yet. Probably only would be used by the system instance. Parity with wrexec epilog could be handled in the job shell.
  • Partial release; This should be pretty easy to accomplish given the new structure, but perhaps isn't necessary until we have epilog or other cleanup support (we don't want a slow epilog to tie up all resources of a job)
  • Job shell output: currently any job shell output is directed to the flux log. We'll probably want to tie this to the same output service used by the job shell so it can optionally be "tailed" and saved to the job record.
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Just pushed a fix for a small race early in the t2402-job-exec-dummy.t tests.

I've also found a leak in the exec_kill handling. The continuation callback for the future returned by bulk_exec_kill() is never called. I've probably done something dumb in the composite "wait all" future handling, but I'm still looking.

Once I fix that and hopefully #2203 goes in, I'll rebase and squash this PR as a candidate for merge.

grondo added 4 commits Jun 14, 2019
Add jobinfo_emit_event_vpack_nowait() and call this from
jobinfo_event_pack_nowait. This will allow addition of other
function with varargs that call this event emitting funciton.
Allow caller of jobinfo_started() to append event context to
the exec "running" eventlog with a optional context args to
jobinfo_started() call.

Move call to jobinfo_started() into place where timer is started
Move clear of job->running flag into jobinfo_complete()
Force test exec mode by ensuring attributes.system.exec.test
is set in all jobspec used in this test.
@grondo grondo force-pushed the grondo:exec-no-really branch from 0d664d0 to 7bbb068 Jun 21, 2019
@grondo grondo changed the title WIP: job-exec: implement ability to execute job shells job-exec: implement ability to execute job shells Jun 21, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Ok, I've squashed this PR and rebased on current master.

There is still one issue with this implementation when a running job is killed due to an exception. The continuation for bulk_exec_kill() never runs, so the returned future is leaked. There is probably a bug in how the composite future is being set up.

I'm not sure I'll find a fix before I leave, though, so I'm posting this PR as-is as a candidate for another review and possible merge.

grondo added 10 commits Jun 17, 2019
Refactor job-exec module to allow multiple execution "implementations"
with test execution as the first implementation.

Also, lay the groundwork for supporting partial resource release by
allowing the implementation to mark tasks as complete and start
cleanup tasks on a subset of ranks (though for now this support is
disabled)
Add a simple "bulk execution" wrapper around libsubprocess API
and an (unused) test implementation of the API.
Add exec implementation to job-exec module using libsubprocess API.

This implementaiton will now be the default unless the
attributes.system.exec.test is set in the submitted jobspec.

Operation:

The bulk-exec implementation launches a configured (job-exec.job-shell
broker attribute) or selected (attributes.system.exec.job-shell in the
submitted jobspec) job shell, one per allocated rank, notifying the job-exec
module when all job shells have successfully started. An execution failure
of any job shell invokes a fatal exception and terminates the job.

The module does not currently support partial release of resources, though
the bulk execution API allows it. Instead, once all job shells have exited
the finish response is triggered.

The bulk-exec implementation does not currently support execution of an
epilog.
Number fake ranks in reosurce.hwloc.by_rank from 0 not 1.
Add the current working directory to emitted jobspec under the
attributes.system.cwd key.

Fixes #2199
Ensure that flux-jobspec sets the current working directory in the
attributes.system.cwd key of emitted jobspec.
Tests of flux-job attach should use testexec execution implementation
(for now at least) so that jobs run for set timelimit and behave in
an expected way.

Check for jq(1) during test startup, and if preset add the appropriate
key to emitted jobspec before submission to activate test execution.
This means that these tests are now skipped if jq is not present.
Test real job shell execution under the job-exec module using
a shell-based dummy job shell.
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Ok, found the leak and force-pushing the fixes.

@grondo grondo force-pushed the grondo:exec-no-really branch from 7bbb068 to 075e59e Jun 21, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Jun 21, 2019

Codecov Report

Merging #2198 into master will decrease coverage by <.01%.
The diff coverage is 78.65%.

@@            Coverage Diff             @@
##           master    #2198      +/-   ##
==========================================
- Coverage   80.87%   80.86%   -0.01%     
==========================================
  Files         199      202       +3     
  Lines       31825    32166     +341     
==========================================
+ Hits        25737    26010     +273     
- Misses       6088     6156      +68
Impacted Files Coverage Δ
src/modules/job-exec/bulk-exec.c 74.51% <74.51%> (ø)
src/modules/job-exec/exec.c 75.86% <75.86%> (ø)
src/modules/job-exec/job-exec.c 73.87% <82.14%> (-0.5%) ⬇️
src/modules/job-exec/testexec.c 85.84% <85.84%> (ø)
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/common/libflux/mrpc.c 87.79% <0%> (-1.19%) ⬇️
src/broker/modservice.c 72.86% <0%> (-0.78%) ⬇️
src/broker/module.c 78.03% <0%> (-0.26%) ⬇️
src/common/libflux/message.c 80.62% <0%> (-0.26%) ⬇️
src/cmd/flux-module.c 83.96% <0%> (-0.24%) ⬇️
... and 7 more
@garlick

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Just skimmed through the changes and LGTM! This will be a solid base to rebase and retool #2196 on. Thanks for all the hard work competing with packing for vacation!

@garlick garlick merged commit 216f191 into flux-framework:master Jun 23, 2019
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 78.65% of diff hit (target 80.87%)
Details
Summary 1 potential rule
Details
codecov/project 80.86% (-0.01%) compared to 075fa6d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

Thanks!

@grondo grondo deleted the grondo:exec-no-really branch Jun 23, 2019
garlick added a commit to garlick/flux-core that referenced this pull request Jul 4, 2019
As sketched in flux-framework#2186, flux-framework#2187, flux-framework#2198, add skeletal
job shell, with source located in src/shell,
installed to ${fluxlibexec}/flux-shell.

Usage: flux-shell [OPTIONS] JOBID

R, jobspec, and the local broker rank are fetched
from the job-info service, unless provided on the
command line.  A 'struct shell_info' created to hold
R, jobspec, and other parameters for use by shell
components.

Create flux handle and reactor capable of handling
SIGCHLD, and start tasks as subprocesses and capture
exit codes.  When the reactor finishes, all tasks have
exit.

Add PMI support using internal libpmi_server library.

Capture stdio of tasks and emit with labels on shell's
stdout, stderr, to be replaced with proper stdio
handling later.
garlick added a commit to garlick/flux-core that referenced this pull request Jul 5, 2019
As sketched in flux-framework#2186, flux-framework#2187, flux-framework#2198, add skeletal
job shell, with source located in src/shell,
installed to ${fluxlibexec}/flux-shell.

Usage: flux-shell [OPTIONS] JOBID

R, jobspec, and the local broker rank are fetched
from the job-info service, unless provided on the
command line.  A 'struct shell_info' created to hold
R, jobspec, and other parameters for use by shell
components.

Create flux handle and reactor capable of handling
SIGCHLD, and start tasks as subprocesses and capture
exit codes.  When the reactor finishes, all tasks have
exit.

Add PMI support using internal libpmi_server library.

Capture stdio of tasks and emit with labels on shell's
stdout, stderr, to be replaced with proper stdio
handling later.
garlick added a commit to garlick/flux-core that referenced this pull request Jul 10, 2019
As sketched in flux-framework#2186, flux-framework#2187, flux-framework#2198, add skeletal
job shell, with source located in src/shell,
installed to ${fluxlibexec}/flux-shell.

Usage: flux-shell [OPTIONS] JOBID

R, jobspec, and the local broker rank are fetched
from the job-info service, unless provided on the
command line.  A 'struct shell_info' created to hold
R, jobspec, and other parameters for use by shell
components.

Create flux handle and reactor capable of handling
SIGCHLD, and start tasks as subprocesses and capture
exit codes.  When the reactor finishes, all tasks have
exit.

Add PMI support using internal libpmi_server library.

Capture stdio of tasks and emit with labels on shell's
stdout, stderr, to be replaced with proper stdio
handling later.
garlick added a commit to garlick/flux-core that referenced this pull request Jul 14, 2019
As sketched in flux-framework#2186, flux-framework#2187, flux-framework#2198, add skeletal
job shell, with source located in src/shell,
installed to ${fluxlibexec}/flux-shell.

Usage: flux-shell [OPTIONS] JOBID

R, jobspec, and the local broker rank are fetched
from the job-info service, unless provided on the
command line.  A 'struct shell_info' created to hold
R, jobspec, and other parameters for use by shell
components.

Create flux handle and reactor capable of handling
SIGCHLD, and start tasks as subprocesses and capture
exit codes.  When the reactor finishes, all tasks have
exit.

Add PMI support using internal libpmi_server library.

Capture stdio of tasks and emit with labels on shell's
stdout, stderr, to be replaced with proper stdio
handling later.
garlick added a commit to garlick/flux-core that referenced this pull request Jul 14, 2019
As sketched in flux-framework#2186, flux-framework#2187, flux-framework#2198, add skeletal
job shell, with source located in src/shell,
installed to ${fluxlibexec}/flux-shell.

Usage: flux-shell [OPTIONS] JOBID

R, jobspec, and the local broker rank are fetched
from the job-info service, unless provided on the
command line.  A 'struct shell_info' created to hold
R, jobspec, and other parameters for use by shell
components.

Create flux handle and reactor capable of handling
SIGCHLD, and start tasks as subprocesses and capture
exit codes.  When the reactor finishes, all tasks have
exit.

Add PMI support using internal libpmi_server library.

Capture stdio of tasks and emit with labels on shell's
stdout, stderr, to be replaced with proper stdio
handling later.
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.