From 9920290e2ccdbe7ef0a63d4bde2d15e9a94e8668 Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Sat, 10 Dec 2022 19:17:50 -0500 Subject: [PATCH] Add container error message to ContainerState This change aims to store an error message to the ContainerState struct when an error comes out of the OCI Runtime. Fixes: #13729 Signed-off-by: Jake Correnti --- libpod/container.go | 2 ++ libpod/container_api.go | 62 +++++++++++++++++++++++++++++++++--- libpod/container_inspect.go | 2 +- libpod/container_internal.go | 46 ++++++++++++++++++++++++-- test/e2e/start_test.go | 15 +++++++++ 5 files changed, 118 insertions(+), 9 deletions(-) diff --git a/libpod/container.go b/libpod/container.go index 2f5376649249..138ab95548b2 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -150,6 +150,8 @@ type ContainerState struct { ExitCode int32 `json:"exitCode,omitempty"` // Exited is whether the container has exited Exited bool `json:"exited,omitempty"` + // The error message produced by a failed container operation + Error string `json:"error,omitempty"` // OOMKilled indicates that the container was killed as it ran out of // memory OOMKilled bool `json:"oomKilled,omitempty"` diff --git a/libpod/container_api.go b/libpod/container_api.go index 35fed93e6447..bfead7731156 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -92,6 +92,9 @@ func (c *Container) Start(ctx context.Context, recursive bool) error { } } if err := c.prepareToStart(ctx, recursive); err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } return err } @@ -125,6 +128,9 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachSt } if err := c.prepareToStart(ctx, recursive); err != nil { + if e := saveContainerError(c, err); e != nil { + return nil, e + } return nil, err } attachChan := make(chan error) @@ -147,7 +153,11 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachSt opts.Started = startedChan if err := c.ociRuntime.Attach(c, opts); err != nil { - attachChan <- err + if e := saveContainerError(c, err); e != nil { + attachChan <- e + } else { + attachChan <- err + } } close(attachChan) }() @@ -237,6 +247,9 @@ func (c *Container) Kill(signal uint) error { // Hardcode all = false, we only use all when removing. if err := c.ociRuntime.KillContainer(c, signal, false); err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } return err } @@ -283,7 +296,13 @@ func (c *Container) Attach(streams *define.AttachStreams, keys string, resize <- go func() { <-attachRdy if err := c.ociRuntime.KillContainer(c, uint(signal.SIGWINCH), false); err != nil { - logrus.Warnf("Unable to send SIGWINCH to container %s after attach: %v", c.ID(), err) + killErr := fmt.Errorf("unable to send SIGWINCH to container %s after attach: %v", c.ID(), err) + + logrus.Warn(killErr.Error()) + + if e := saveContainerError(c, killErr); e != nil { + logrus.Warn(e.Error()) + } } }() } @@ -299,7 +318,16 @@ func (c *Container) Attach(streams *define.AttachStreams, keys string, resize <- opts.AttachReady = attachRdy c.newContainerEvent(events.Attach) - return c.ociRuntime.Attach(c, opts) + + err := c.ociRuntime.Attach(c, opts) + if err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } + return err + } + + return nil } // HTTPAttach forwards an attach session over a hijacked HTTP session. @@ -346,7 +374,16 @@ func (c *Container) HTTPAttach(r *http.Request, w http.ResponseWriter, streams * logrus.Infof("Performing HTTP Hijack attach to container %s", c.ID()) c.newContainerEvent(events.Attach) - return c.ociRuntime.HTTPAttach(c, r, w, streams, detachKeys, cancel, hijackDone, streamAttach, streamLogs) + + err := c.ociRuntime.HTTPAttach(c, r, w, streams, detachKeys, cancel, hijackDone, streamAttach, streamLogs) + if err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } + return err + } + + return nil } // AttachResize resizes the container's terminal, which is displayed by Attach @@ -367,7 +404,14 @@ func (c *Container) AttachResize(newSize resize.TerminalSize) error { logrus.Infof("Resizing TTY of container %s", c.ID()) - return c.ociRuntime.AttachResize(c, newSize) + err := c.ociRuntime.AttachResize(c, newSize) + if err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } + return err + } + return nil } // Mount mounts a container's filesystem on the host @@ -549,6 +593,9 @@ func (c *Container) WaitForExit(ctx context.Context, pollInterval time.Duration) case errors.Is(err, define.ErrNoSuchCtr): containerRemoved = true case err != nil: + if e := saveContainerError(c, err); e != nil { + return false, -1, e + } return false, -1, err case !conmonAlive: timerDuration := time.Second * 20 @@ -1035,3 +1082,8 @@ func (c *Container) Stat(ctx context.Context, containerPath string) (*define.Fil info, _, _, err := c.stat(mountPoint, containerPath) return info, err } + +func saveContainerError(c *Container, err error) error { + c.state.Error = err.Error() + return c.save() +} diff --git a/libpod/container_inspect.go b/libpod/container_inspect.go index 4dc1ca3a5d55..c81ef1348d34 100644 --- a/libpod/container_inspect.go +++ b/libpod/container_inspect.go @@ -129,7 +129,7 @@ func (c *Container) getContainerInspectData(size bool, driverData *define.Driver Pid: runtimeInfo.PID, ConmonPid: runtimeInfo.ConmonPID, ExitCode: runtimeInfo.ExitCode, - Error: "", // can't get yet + Error: runtimeInfo.Error, StartedAt: runtimeInfo.StartedTime, FinishedAt: runtimeInfo.FinishedTime, Checkpointed: runtimeInfo.Checkpointed, diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 13feb2ffeb2b..818a9df8be3e 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -154,12 +154,26 @@ func (c *Container) PreCheckPointPath() string { // AttachSocketPath retrieves the path of the container's attach socket func (c *Container) AttachSocketPath() (string, error) { - return c.ociRuntime.AttachSocketPath(c) + path, err := c.ociRuntime.AttachSocketPath(c) + if err != nil { + if e := saveContainerError(c, err); e != nil { + return "", e + } + return "", err + } + return path, nil } // exitFilePath gets the path to the container's exit file func (c *Container) exitFilePath() (string, error) { - return c.ociRuntime.ExitFilePath(c) + path, err := c.ociRuntime.ExitFilePath(c) + if err != nil { + if e := saveContainerError(c, err); e != nil { + return "", e + } + return "", err + } + return path, nil } // Wait for the container's exit file to appear. @@ -1209,6 +1223,10 @@ func (c *Container) start() error { } if err := c.ociRuntime.StartContainer(c); err != nil { + c.state.Error = err.Error() + if e := saveContainerError(c, err); e != nil { + return e + } return err } logrus.Debugf("Started container %s", c.ID()) @@ -1299,6 +1317,9 @@ func (c *Container) stop(timeout uint) error { if stopErr != nil { logrus.Errorf("Syncing container %s status: %v", c.ID(), err) + if e := saveContainerError(c, stopErr); e != nil { + return e + } return stopErr } return err @@ -1308,6 +1329,9 @@ func (c *Container) stop(timeout uint) error { // We have to check stopErr *after* we lock again - otherwise, we have a // change of panicking on a double-unlock. Ref: GH Issue 9615 if stopErr != nil { + if e := saveContainerError(c, stopErr); e != nil { + return e + } return stopErr } @@ -1331,6 +1355,9 @@ func (c *Container) stop(timeout uint) error { func (c *Container) waitForConmonToExitAndSave() error { conmonAlive, err := c.ociRuntime.CheckConmonRunning(c) if err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } return err } if !conmonAlive { @@ -1370,6 +1397,9 @@ func (c *Container) pause() error { } if err := c.ociRuntime.PauseContainer(c); err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } // TODO when using docker-py there is some sort of race/incompatibility here return err } @@ -1388,6 +1418,9 @@ func (c *Container) unpause() error { } if err := c.ociRuntime.UnpauseContainer(c); err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } // TODO when using docker-py there is some sort of race/incompatibility here return err } @@ -1979,7 +2012,11 @@ func (c *Container) stopPodIfNeeded(ctx context.Context) error { // hooks. func (c *Container) delete(ctx context.Context) error { if err := c.ociRuntime.DeleteContainer(c); err != nil { - return fmt.Errorf("removing container %s from runtime: %w", c.ID(), err) + err := fmt.Errorf("removing container %s from runtime: %w", c.ID(), err) + if e := saveContainerError(c, err); e != nil { + return e + } + return err } if err := c.postDeleteHooks(ctx); err != nil { @@ -2363,6 +2400,9 @@ func (c *Container) extractSecretToCtrStorage(secr *ContainerSecret) error { // update calls the ociRuntime update function to modify a cgroup config after container creation func (c *Container) update(resources *spec.LinuxResources) error { if err := c.ociRuntime.UpdateContainer(c, resources); err != nil { + if e := saveContainerError(c, err); e != nil { + return e + } return err } logrus.Debugf("updated container %s", c.ID()) diff --git a/test/e2e/start_test.go b/test/e2e/start_test.go index db6f87ac0fc6..ca539b00c697 100644 --- a/test/e2e/start_test.go +++ b/test/e2e/start_test.go @@ -249,4 +249,19 @@ var _ = Describe("Podman start", func() { Expect(session1).Should(Exit(0)) Expect(session1.OutputToString()).To(BeEquivalentTo(cid2)) }) + + It("podman start container with bad args", func() { + session := podmanTest.Podman([]string{"container", "create", ALPINE, "efcho", "Hello World"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(Exit(0)) + cid := session.OutputToString() + + session = podmanTest.Podman([]string{"start", cid}) + session.WaitWithDefaultTimeout() + Expect(session).Should(Exit(125)) + session = podmanTest.Podman([]string{"container", "inspect", cid, "-f", "'{{ .State.Error }}"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(Exit(0)) + Expect(session.OutputToString()).Should(ContainSubstring("crun: executable file `efcho` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found")) + }) })