Feature #8465

[Crunch2] Support stdin/stderr redirection

Added by Peter Amstutz about 1 year ago. Updated 16 days ago.

Status:ResolvedStart date:04/07/2017
Priority:NormalDue date:
Assignee:Radhika Chippada% Done:

100%

Category:-
Target version:2017-04-12 sprint
Story points1.0Remaining (hours)0.00 hour
Velocity based estimate-
ReleaseCrunch v2

Description

For stdin redirection:

  1. After arv-mount is started
  2. Look for "stdin" in mounts
  3. Open file with stdin content (must support kind: collection, ideally should support kind: json as well)
  4. Attach stdin to docker container, I think this is ContainerAttachOptions{Stdin: true}
  5. Copy input file to docker container stdin stream, I think this is a goroutine with io.Copy(infile, response.Conn)

For stderr redirection:

Copy/refactor code for existing stdout redirection.


Subtasks

Task #11385: Review branch 8465-stdin-redirectionResolvedPeter Amstutz

Task #11433: Review branch 8465-stderr-redirectionResolvedPeter Amstutz

Task #11448: Review 8465-cwl-containers-stdin-stderrResolvedRadhika Chippada

Task #11446: Update arvados-cwl-runnerResolvedPeter Amstutz


Related issues

Blocked by Arvados - Story #9132: [Crunch2] crunch-run uses official docker client library Resolved 03/08/2017

Associated revisions

Revision 95ef2a6f
Added by Radhika Chippada 17 days ago

refs #8465
Merge branch '8465-stderr-redirection'

Revision a7347892
Added by Peter Amstutz 16 days ago

Merge branch '8465-cwl-containers-stdin-stderr' closes #8465

History

#1 Updated by Brett Smith about 1 year ago

  • Subject changed from [Crunch2[ Support stdin redirection to [Crunch2] Support stdin redirection

#2 Updated by Radhika Chippada 11 months ago

  • Assignee set to Radhika Chippada

#3 Updated by Tom Morris 2 months ago

  • Target version set to Arvados Future Sprints

#4 Updated by Peter Amstutz 2 months ago

  • Description updated (diff)
  • Assignee deleted (Radhika Chippada)

#5 Updated by Tom Morris about 1 month ago

  • Target version changed from Arvados Future Sprints to 2017-04-12 sprint
  • Story points set to 1.0

#6 Updated by Radhika Chippada about 1 month ago

  • Assignee set to Radhika Chippada

#7 Updated by Peter Amstutz 28 days ago

  • Subject changed from [Crunch2] Support stdin redirection to [Crunch2] Support stdin/stderr redirection
  • Description updated (diff)

#8 Updated by Radhika Chippada 25 days ago

  • Status changed from New to In Progress

#9 Updated by Radhika Chippada 23 days ago

Branch 8465-stdin-redirection @ c3ae1e2

  • Added stdin redirection for collection and json mount points
  • Added tests
    • Currently, the tests provide stdin but do not do anything with it. It would be desirable that the command does "echo stdin-content" or something, but I could not (yet) figure out how to do this.

#10 Updated by Peter Amstutz 22 days ago

  • Status changed from In Progress to New

crunchrun.go L690:

stdinRdr, err = runner.Kc.CollectionFileReader(map[string]interface{}{"manifest_text": stdinColl.ManifestText}, stdinMnt.Path)

Can write this as:

stdinRdr, err = runner.Kc.ManifestFileReader(manifest.Manifest{Text: stdinColl.ManifestText}, stdinMnt.Path)

It looks like it waits for the entire contents to copied to stdin. That's not right, because AttachStreams() is called from CreateContainer(), but before StartContainer(), which means the container is not running yet (so there is nothing ready to accept the stdin stream.) You need to start the goroutine and then return from AttachStreams (so the goroutine continues running).

If there is an error from io.Copy(), it should be log the error using runner.CrunchLog and call runner.stop() to cancel the container.

The same applies to json content on stdin.

#11 Updated by Radhika Chippada 22 days ago

Can write this as: stdinRdr, err = runner.Kc.ManifestFileReader(manifest.Manifest{Text: stdinColl.ManifestText}, stdinMnt.Path)

Done (and removed unused CollectionFileReader references)

It looks like it waits for the entire contents to copied to stdin. That's not right, because AttachStreams() is called from CreateContainer(), but before StartContainer(), which means the container is not running yet (so there is nothing ready to accept the stdin stream.) You need to start the goroutine and then return from AttachStreams (so the goroutine continues running).

Updated accordingly

If there is an error from io.Copy(), it should be log the error using runner.CrunchLog and call runner.stop() to cancel the container.

Done

The same applies to json content on stdin

Done

#12 Updated by Radhika Chippada 21 days ago

New branch 8465-stderr-redirection implements stderr redirection. This branch is off of 8465-stdin-redirection. Thanks.

#13 Updated by Radhika Chippada 21 days ago

  • Status changed from New to In Progress

#14 Updated by Peter Amstutz 17 days ago

8465-stderr-redirection @ c1ea9a2fdccc5e24554f8cf28296cbc0764c8bbc LGTM, please merge

#15 Updated by Radhika Chippada 16 days ago

Branch 8465-cwl-containers-stdin-stderr LGTM

#16 Updated by Peter Amstutz 16 days ago

  • Status changed from In Progress to Resolved
  • % Done changed from 75 to 100

Applied in changeset arvados|commit:a734789218122d8ab0d8f766bac4d69c04db91bf.

Also available in: Atom PDF