From 9e2e7f2a77ab81fdddf101d47c2ccc7c3fb5929d 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. Now I wrote automated test for both podman run and exec detach but this uncovered several larger issues: - detach sequence parsing is broken[1] - podman-remote exec detach is broken[2] - detach in general seems to be buggy/racy, seeing lot of flakes that fail to restore the terminal and get an EIO instead, i.e. "Unable to restore terminal: input/output error" Thus I cannot add tests for now but this commit should at least fix the obvoius case as reported by the user so I like to get this in regardless and I will work through the other issues once I have more time. Fixes #24895 [1] https://github.com/containers/common/pull/2302 [2] https://github.com/containers/podman/issues/25089 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 +++++++++++++++++++---- 4 files changed, 68 insertions(+), 14 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 }