diff --git a/libpod/container_api.go b/libpod/container_api.go index 275f365b2f..300fe5ca20 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -84,27 +84,20 @@ func (c *Container) Init(ctx context.Context, recursive bool) error { // running before starting the container. The recursive parameter, if set, will start all // dependencies before starting this container. func (c *Container) Start(ctx context.Context, recursive bool) (finalErr error) { - defer func() { - if finalErr != nil { - // Have to re-lock. - // As this is the first defer, it's the last thing to - // happen in the function - so `defer c.lock.Unlock()` - // below already fired. - if !c.batched { - c.lock.Lock() - defer c.lock.Unlock() - } - - if err := saveContainerError(c, finalErr); err != nil { - logrus.Debug(err) - } - } - }() - if !c.batched { c.lock.Lock() defer c.lock.Unlock() + // defer's are executed LIFO so we are locked here + // as long as we call this after the defer unlock() + defer func() { + if finalErr != nil { + if err := saveContainerError(c, finalErr); err != nil { + logrus.Debug(err) + } + } + }() + if err := c.syncContainer(); err != nil { return err } @@ -147,27 +140,20 @@ func (c *Container) Update(resources *spec.LinuxResources, restartPolicy *string // ordering of the two such that no output from the container is lost (e.g. the // Attach call occurs before Start). func (c *Container) Attach(ctx context.Context, streams *define.AttachStreams, keys string, resize <-chan resize.TerminalSize, start bool) (retChan <-chan error, finalErr error) { - defer func() { - if finalErr != nil { - // Have to re-lock. - // As this is the first defer, it's the last thing to - // happen in the function - so `defer c.lock.Unlock()` - // below already fired. - if !c.batched { - c.lock.Lock() - defer c.lock.Unlock() - } - - if err := saveContainerError(c, finalErr); err != nil { - logrus.Debug(err) - } - } - }() - if !c.batched { c.lock.Lock() defer c.lock.Unlock() + // defer's are executed LIFO so we are locked here + // as long as we call this after the defer unlock() + defer func() { + if finalErr != nil { + if err := saveContainerError(c, finalErr); err != nil { + logrus.Debug(err) + } + } + }() + if err := c.syncContainer(); err != nil { return nil, err } @@ -270,27 +256,24 @@ func (c *Container) Stop() error { // manually. If timeout is 0, SIGKILL will be used immediately to kill the // container. func (c *Container) StopWithTimeout(timeout uint) (finalErr error) { - defer func() { - if finalErr != nil { - // Have to re-lock. - // As this is the first defer, it's the last thing to - // happen in the function - so `defer c.lock.Unlock()` - // below already fired. - if !c.batched { - c.lock.Lock() - defer c.lock.Unlock() - } - - if err := saveContainerError(c, finalErr); err != nil { - logrus.Debug(err) - } - } - }() - if !c.batched { c.lock.Lock() defer c.lock.Unlock() + // defer's are executed LIFO so we are locked here + // as long as we call this after the defer unlock() + defer func() { + // The podman stop command is idempotent while the internal function here is not. + // As such we do not want to log these errors in the state because they are not + // actually user visible errors. + if finalErr != nil && !errors.Is(finalErr, define.ErrCtrStopped) && + !errors.Is(finalErr, define.ErrCtrStateInvalid) { + if err := saveContainerError(c, finalErr); err != nil { + logrus.Debug(err) + } + } + }() + if err := c.syncContainer(); err != nil { return err } diff --git a/libpod/container_internal.go b/libpod/container_internal.go index d12b2809fc..50acaf3e7f 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1106,6 +1106,9 @@ func (c *Container) init(ctx context.Context, retainRetries bool) error { c.state.RestoreLog = "" c.state.ExitCode = 0 c.state.Exited = false + // Reset any previous errors as we try to init it again, either it works and we don't + // want to keep an old error around or a new error will be written anyway. + c.state.Error = "" c.state.State = define.ContainerStateCreated c.state.StoppedByUser = false c.state.RestartPolicyMatch = false diff --git a/test/e2e/inspect_test.go b/test/e2e/inspect_test.go index 202ca036ae..72b3c8b5cc 100644 --- a/test/e2e/inspect_test.go +++ b/test/e2e/inspect_test.go @@ -583,6 +583,15 @@ var _ = Describe("Podman inspect", func() { Expect(session).Should(ExitCleanly()) Expect(session.OutputToString()).To(BeEmpty()) + // Stopping the container should be a NOP and not log an error in the state here. + session = podmanTest.Podman([]string{"container", "stop", cid}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + session = podmanTest.Podman([]string{"container", "inspect", cid, "-f", "{{ .State.Error }}"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + Expect(session.OutputToString()).To(BeEmpty(), "state error after stop") + commandNotFound := "OCI runtime attempted to invoke a command that was not found" session = podmanTest.Podman([]string{"start", cid}) session.WaitWithDefaultTimeout()