Feature #8465

[Crunch2] Support stdin/stderr redirection

Added by Peter Amstutz almost 2 years ago. Updated 8 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
04/07/2017
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0
Release:
Release relationship:
Auto

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 libraryResolved2017-03-08

Associated revisions

Revision 95ef2a6f
Added by Radhika Chippada 8 months ago

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

Revision a7347892
Added by Peter Amstutz 8 months ago

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

History

#1 Updated by Brett Smith over 1 year ago

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

#2 Updated by Radhika Chippada over 1 year ago

  • Assigned To set to Radhika Chippada

#3 Updated by Tom Morris 10 months ago

  • Target version set to Arvados Future Sprints

#4 Updated by Peter Amstutz 10 months ago

  • Description updated (diff)
  • Assigned To deleted (Radhika Chippada)

#5 Updated by Tom Morris 9 months ago

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

#6 Updated by Radhika Chippada 9 months ago

  • Assigned To set to Radhika Chippada

#7 Updated by Peter Amstutz 9 months ago

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

#8 Updated by Radhika Chippada 8 months ago

  • Status changed from New to In Progress

#9 Updated by Radhika Chippada 8 months 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 8 months 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 8 months 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 8 months ago

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

#13 Updated by Radhika Chippada 8 months ago

  • Status changed from New to In Progress

#14 Updated by Peter Amstutz 8 months ago

8465-stderr-redirection @ c1ea9a2fdccc5e24554f8cf28296cbc0764c8bbc LGTM, please merge

#15 Updated by Radhika Chippada 8 months ago

Branch 8465-cwl-containers-stdin-stderr LGTM

#16 Updated by Peter Amstutz 8 months 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