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

fix(docker): fix streaming of combined stdout/stderr #2368

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

tcolgate
Copy link
Contributor

@tcolgate tcolgate commented Mar 5, 2020

This PR fixes #2261. Multireader concactinates, not combines, the two
streams, this meant that we blocked on stdin if stderr was not
completely buffered.

In addition, this implementation ensures that we call Wait() on the
command, and any error is return by Close(). Godoc for
exec.Cmd.Std(out|err)Pipe, confirms that we do not need to close those
pipes.

This PR also ensures that we do not leak go routines in the even that
cmd.Start() fails.

workflow/executor/docker/docker.go Outdated Show resolved Hide resolved
workflow/executor/docker/docker.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #2368 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2368   +/-   ##
=======================================
  Coverage   13.28%   13.28%           
=======================================
  Files          72       72           
  Lines       24653    24653           
=======================================
  Hits         3274     3274           
  Misses      20966    20966           
  Partials      413      413

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9743831...5fda49b. Read the comment docs.

This PR fixes argoproj#2261. Multireader concactinates, not combines, the two
streams, this meant that we blocked on stdin if stderr was not
completely buffered.

In addition, this implementation ensures that we call Wait() on the
command, and any error is return by Close(). Godoc for
exec.Cmd.Std(out|err)Pipe, confirms that we do not need to close those
pipes.

This PR also ensures that we do not leak go routines in the even that
cmd.Start() fails.
@tcolgate
Copy link
Contributor Author

tcolgate commented Mar 5, 2020

tested with:

kind: Workflow
metadata:
  name:  wftestthing.3
  namespace: argo
spec:
  entrypoint: build
  templates:
  - container:
      args:
      - "for i in $(seq 100000); do date > /dev/stderr;  done "
      command:
      - sh
      - -c
      image: alpine:latest
    name: build```

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting fix. Thank you.

@alexec alexec merged commit 5b6b825 into argoproj:master Mar 5, 2020
@alexec alexec added this to the v2.7 milestone Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wait container stuck in multi-root workflows in 2.5-rc8 to 2.5.0
3 participants