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: prevent stdout from disappearing in script templates. Fixes #11330 #11368

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

boiledfroginthewell
Copy link
Contributor

@boiledfroginthewell boiledfroginthewell commented Jul 15, 2023

Fixes #11330

Motivation

Outputs of the executed command are asynchronously read and copied to cmd.Stdout. cmd.Stdout must be closed after copies are done otherwise command outputs are gone.

As described below, however, copies can never be end because sub-processes can keep running and can write to stdout indefinitely.

func Wait(process *os.Process) error {
// We must copy the behaviour of Kubernetes in how we handle sub-processes.
// Kubernetes only waits on PID 1, not on any sub-process that process might fork.
// The only way for those forked processes to run in the background is to background the
// sub-process by calling Process.Release.
// Background processes always become zombies when they exit.
// Because the sub-process is now running in the background it will become a zombie,
// so we must wait for it.
// Because we run the process in the background, we cannot Process.Wait for it to get the exit code.
// Instead, we can reap it to get the exit code
pid := process.Pid
if err := process.Release(); err != nil {

Modifications

Wait() is added to wait the copy.
WaitDelay is also added. It will close the pipes (stdout/stderr) of the executed command to stop sub-processes writing any more texts.

Verification

Tests were added.
I also ran the following command for more than an hour.

$ docker run \
    --rm -it \
    --cpus 0.5 \
    -e ARGO_CONTAINER_NAME=main \
    -e ARGO_INCLUDE_SCRIPT_OUTPUT=true \
    -v $PWD/dist/argoexec:/var/run/argo/argoexec \
    busybox:1.35 \
    sh
$ echo '{}' > /var/run/argo/template
$ while :; do
    rm -f /var/run/argo/ctr/main/*
    < /dev/null /var/run/argo/argoexec emissary --loglevel info --log-format text -- echo hello
    ls -l /var/run/argo/ctr/main
    [ ! -s /var/run/argo/ctr/main/stdout ] && break
    [ ! -s /var/run/argo/ctr/main/combined ] && break
done

Signed-off-by: boiledfroginthewell <boiledfroginthewell@users.noreply.github.com>
@boiledfroginthewell boiledfroginthewell marked this pull request as ready for review July 15, 2023 15:53
@terrytangyuan terrytangyuan requested a review from alexec July 16, 2023 01:38
@terrytangyuan
Copy link
Member

@alexec Could you help review this when you get a chance?

@terrytangyuan terrytangyuan merged commit 66e78a5 into argoproj:master Aug 10, 2023
terrytangyuan pushed a commit that referenced this pull request Aug 11, 2023
… (#11368)

Signed-off-by: boiledfroginthewell <boiledfroginthewell@users.noreply.github.com>
@sonbui00
Copy link
Member

I got a fail test

--- FAIL: TestSimpleStartCloser (0.43s)
    /Users/sonbui/go/src/github.com/argoproj/argo-workflows/workflow/executor/os-specific/command_test.go:31: 
        	Error Trace:	/Users/sonbui/go/src/github.com/argoproj/argo-workflows/workflow/executor/os-specific/command_test.go:31
        	Error:      	Not equal: 
        	            	expected: "A123456789B123456789C123456789D123456789E123456789"
        	            	actual  : "-n A123456789B123456789C123456789D123456789E123456789\n"

From man page

Most notably, the builtin echo in sh(1) does not accept the -n option.  Consult the builtin(1) manual page.

sonbui00 added a commit to sonbui00/argo-workflows that referenced this pull request Aug 15, 2023
Signed-off-by: Son Bui <sonbv00@gmail.com>
terrytangyuan pushed a commit that referenced this pull request Aug 15, 2023
cbuchli added a commit to helio/argo-workflows that referenced this pull request Oct 4, 2023
terrytangyuan pushed a commit that referenced this pull request Oct 19, 2023
dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
…proj#11330 (argoproj#11368)

Signed-off-by: boiledfroginthewell <boiledfroginthewell@users.noreply.github.com>
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

result output parameter of script templates occasionally become empty
4 participants