From 61fd7a43e314dae4189936dffdca1eca13b8547c Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 22 Jan 2025 11:15:15 +0100 Subject: [PATCH] podman exec: correctly support detaching podman exec support detaching early via the detach key sequence. In that case the podman process should exit successfully but the container exec process keeps running. Given that I could not find any existing test for the detach key functionality not even for exec I added some. This seems to reveal more issues with on podman-remote, podman-remote run detach was broken which I fixed here as well but for podman-remote exec something bigger is needed. While I thought I fixed most problems there there was a strange race condition which caused the process to just hang. Thus I skipped the remote exec test for now and filled #25089 to track that. Fixes #24895 Signed-off-by: Paul Holzinger --- libpod/container_exec.go | 42 +++++++++++++++++--- libpod/util.go | 2 +- pkg/api/handlers/compat/exec.go | 2 +- pkg/domain/infra/tunnel/containers.go | 36 ++++++++++++++--- test/system/450-interactive.bats | 57 ++++++++++++++++++++++++++- 5 files changed, 124 insertions(+), 15 deletions(-) diff --git a/libpod/container_exec.go b/libpod/container_exec.go index 69a1c356d2..c9efc707f5 100644 --- a/libpod/container_exec.go +++ b/libpod/container_exec.go @@ -287,9 +287,14 @@ func (c *Container) ExecStart(sessionID string) error { // execStartAndAttach starts and attaches to an exec session in a container. // newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachStreams, newSize *resize.TerminalSize, isHealthcheck bool) error { + unlock := true if !c.batched { c.lock.Lock() - defer c.lock.Unlock() + defer func() { + if unlock { + c.lock.Unlock() + } + }() if err := c.syncContainer(); err != nil { return err @@ -344,6 +349,12 @@ func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachS } tmpErr := <-attachChan + // user detached + if errors.Is(tmpErr, define.ErrDetach) { + // ensure we the defer does not unlock as we are not locked here + unlock = false + return tmpErr + } if lastErr != nil { logrus.Errorf("Container %s exec session %s error: %v", c.ID(), session.ID(), lastErr) } @@ -431,9 +442,14 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w close(hijackDone) }() + unlock := true if !c.batched { c.lock.Lock() - defer c.lock.Unlock() + defer func() { + if unlock { + c.lock.Unlock() + } + }() if err := c.syncContainer(); err != nil { return err @@ -514,6 +530,12 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w } tmpErr := <-attachChan + // user detached + if errors.Is(tmpErr, define.ErrDetach) { + // ensure we the defer does not unlock as we are not locked here + unlock = false + return tmpErr + } if lastErr != nil { logrus.Errorf("Container %s exec session %s error: %v", c.ID(), session.ID(), lastErr) } @@ -765,11 +787,14 @@ func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resi if err != nil { return -1, err } + cleanup := true defer func() { - if err := c.ExecRemove(sessionID, false); err != nil { - if retErr == nil && !errors.Is(err, define.ErrNoSuchExecSession) { - exitCode = -1 - retErr = err + if cleanup { + if err := c.ExecRemove(sessionID, false); err != nil { + if retErr == nil && !errors.Is(err, define.ErrNoSuchExecSession) { + exitCode = -1 + retErr = err + } } } }() @@ -803,6 +828,11 @@ func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resi } if err := c.execStartAndAttach(sessionID, streams, size, isHealthcheck); err != nil { + // user detached, there will be no exit just exit without reporting an error + if errors.Is(err, define.ErrDetach) { + cleanup = false + return 0, nil + } return -1, err } diff --git a/libpod/util.go b/libpod/util.go index 49c9f10c27..67ba151c04 100644 --- a/libpod/util.go +++ b/libpod/util.go @@ -149,7 +149,7 @@ func checkDependencyContainer(depCtr, ctr *Container) error { // hijackWriteError writes an error to a hijacked HTTP session. func hijackWriteError(toWrite error, cid string, terminal bool, httpBuf *bufio.ReadWriter) { - if toWrite != nil { + if toWrite != nil && !errors.Is(toWrite, define.ErrDetach) { errString := []byte(fmt.Sprintf("Error: %v\n", toWrite)) if !terminal { // We need a header. diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go index 7c28458d57..a83bf749a9 100644 --- a/pkg/api/handlers/compat/exec.go +++ b/pkg/api/handlers/compat/exec.go @@ -200,7 +200,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) { t := r.Context().Value(api.IdleTrackerKey).(*idle.Tracker) defer t.Close() - if err != nil { + if err != nil && !errors.Is(err, define.ErrDetach) { // Cannot report error to client as a 500 as the Upgrade set status to 101 logErr(err) } diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index daee6a8219..99a9c6510a 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/pkg/domain/infra/tunnel/containers.go @@ -666,6 +666,7 @@ func startAndAttach(ic *ContainerEngine, name string, detachKeys *string, sigPro go func() { err := containers.Attach(ic.ClientCtx, name, input, output, errput, attachReady, options) attachErr <- err + close(attachErr) }() // Wait for the attach to actually happen before starting // the container. @@ -683,13 +684,36 @@ func startAndAttach(ic *ContainerEngine, name string, detachKeys *string, sigPro return -1, err } - // call wait immediately after start to avoid racing against container removal when it was created with --rm - exitCode, err := containers.Wait(cancelCtx, name, nil) - if err != nil { - return -1, err - } - code = int(exitCode) + // Call wait immediately after start to avoid racing against container removal when it was created with --rm. + // It must be run in a separate goroutine to so we do not block when attach returns early, i.e. user + // detaches in which case wait would not return. + waitChan := make(chan error) + go func() { + defer close(waitChan) + + exitCode, err := containers.Wait(cancelCtx, name, nil) + if err != nil { + waitChan <- fmt.Errorf("wait for container: %w", err) + return + } + code = int(exitCode) + }() + select { + case err := <-waitChan: + if err != nil { + return -1, err + } + case err := <-attachErr: + if err != nil { + return -1, err + } + // also wait for the wait to be complete in this case + err = <-waitChan + if err != nil { + return -1, err + } + } case err := <-attachErr: return -1, err } diff --git a/test/system/450-interactive.bats b/test/system/450-interactive.bats index c9f8d2942d..12efba6533 100644 --- a/test/system/450-interactive.bats +++ b/test/system/450-interactive.bats @@ -39,7 +39,7 @@ function teardown() { kill $PODMAN_SOCAT_PID PODMAN_SOCAT_PID= fi - rm -f $PODMAN_TEST_PTY $PODMAN_DUMMY_PTY + rm -f $PODMAN_TEST_PTY $PODMAN_DUMMY basic_teardown } @@ -124,4 +124,59 @@ function teardown() { is "$output" "$expected_tty" "passthrough-tty: tty matches" } +@test "podman run detach keys" { + local cname1=c1-$(safename) + local cname2=c2-$(safename) + + local msg=$(random_string) + # First "type" a command then send CTRL-p,CTRL-q sequence to the run command. + # We must send that sequence in two echo commands if I use a single echo it + # doesn't work, I don't know why. + # If the detach does not work podman run will hang and timeout. + # The sleep is needed to ensure the output can be printed before we detach. + (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cp' > $PODMAN_DUMMY; echo -n $'\cq' > $PODMAN_DUMMY ) & + run_podman run -it --name $cname1 $IMAGE sh <$PODMAN_TEST_PTY + # Because we print to a terminal it appends "\r" + assert "$output" =~ "$msg"$'\r' "run output contains message" + + # The container should still be running + run_podman inspect --format {{.State.Status}} $cname1 + assert "$output" == "running" "container status" + + # Now a second test with --detach-keys to make sure the cli option works + (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cc' > $PODMAN_DUMMY; echo -n $'J' > $PODMAN_DUMMY ) & + run_podman run -it --name $cname2 --detach-keys ctrl-c,J $IMAGE sh <$PODMAN_TEST_PTY + assert "$output" =~ "$msg"$'\r' "run output with --detach-keys contains message" + + run_podman rm -f -t0 $cname1 $cname2 +} + +@test "podman exec detach keys" { + skip_if_remote "FIXME #25089: podman-remote exec detach broken" + + local cname=c-$(safename) + run_podman run -d --name $cname $IMAGE sleep inf + + local msg=$(random_string) + # First "type" a command then send CTRL-p,CTRL-q sequence to the exec command. + # If the detach does not work podman exec will hang and timeout. + # The sleep is needed to ensure the output can be printed before we detach. + (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cp' > $PODMAN_DUMMY; echo -n $'\cq' > $PODMAN_DUMMY ) & + run_podman exec -it $cname sh <$PODMAN_TEST_PTY + # Because we print to a terminal it appends "\r" + assert "$output" =~ "$msg"$'\r' "exec output contains message" + + # The previous exec session/process should still be running + run_podman exec $cname ps aux + # Match PID=2 USER=root and COMMAND=sh from the ps output + assert "$output" =~ "2 root.*sh" "sh exec process still running" + + # Now a second test with --detach-keys to make sure the cli option works + (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\ct' > $PODMAN_DUMMY; echo -n $'f' > $PODMAN_DUMMY ) & + run_podman exec -it --detach-keys ctrl-t,f $cname sh <$PODMAN_TEST_PTY + assert "$output" =~ "$msg"$'\r' "exec output with --detach-keys contains message" + + run_podman rm -f -t0 $cname +} + # vim: filetype=sh