container stop: release lock before calling the runtime

Podman defers stopping the container to the runtime, which can take some
time.  Keeping the lock while waiting for the runtime to complete the
stop procedure, prevents other commands from acquiring the lock as shown
in #8501.

To improve the user experience, release the lock before invoking the
runtime, and re-acquire the lock when the runtime is finished.  Also
introduce an intermediate "stopping" to properly distinguish from
"stopped" containers etc.

Fixes: #8501
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

4 files changed, 56 insertions, 4 deletions

diff --git a/libpod/container_api.go b/libpod/container_api.go
index 2c7dc79c9..12207ed0a 100644
--- a/libpod/container_api.go
+++ b/libpod/container_api.go
@@ -210,7 +210,13 @@ func (c *Container) Kill(signal uint) error {
 	}
 
 	// TODO: Is killing a paused container OK?
-	if c.state.State != define.ContainerStateRunning {
+	switch c.state.State {
+	case define.ContainerStateRunning, define.ContainerStateStopping:
+		// Note that killing containers in "stopping" state is okay.
+		// In that state, the Podman is waiting for the runtime to
+		// stop the container and if that is taking too long, a user
+		// may have decided to kill the container after all.
+	default:
 		return errors.Wrapf(define.ErrCtrStateInvalid, "can only kill running containers. %s is in state %s", c.ID(), c.state.State.String())
 	}
 
@@ -539,7 +545,7 @@ func (c *Container) Cleanup(ctx context.Context) error {
 	}
 
 	// Check if state is good
-	if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateExited) {
+	if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateStopping, define.ContainerStateExited) {
 		return errors.Wrapf(define.ErrCtrStateInvalid, "container %s is running or paused, refusing to clean up", c.ID())
 	}
 
diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index 540230c26..028710849 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -758,7 +758,7 @@ func (c *Container) isStopped() (bool, error) {
 		return true, err
 	}
 
-	return !c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused), nil
+	return !c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused, define.ContainerStateStopping), nil
 }
 
 // save container state to the database
@@ -1284,10 +1284,49 @@ func (c *Container) stop(timeout uint) error {
 		return err
 	}
 
+	// Set the container state to "stopping" and unlock the container
+	// before handing it over to conmon to unblock other commands.  #8501
+	// demonstrates nicely that a high stop timeout will block even simple
+	// commands such as `podman ps` from progressing if the container lock
+	// is held when busy-waiting for the container to be stopped.
+	c.state.State = define.ContainerStateStopping
+	if err := c.save(); err != nil {
+		return errors.Wrapf(err, "error saving container %s state before stopping", c.ID())
+	}
+	if !c.batched {
+		c.lock.Unlock()
+	}
+
 	if err := c.ociRuntime.StopContainer(c, timeout, all); err != nil {
 		return err
 	}
 
+	if !c.batched {
+		c.lock.Lock()
+		if err := c.syncContainer(); err != nil {
+			switch errors.Cause(err) {
+			// If the container has already been removed (e.g., via
+			// the cleanup process), there's nothing left to do.
+			case define.ErrNoSuchCtr, define.ErrCtrRemoved:
+				return nil
+			default:
+				return err
+			}
+		}
+	}
+
+	// Since we're now subject to a race condition with other processes who
+	// may have altered the state (and other data), let's check if the
+	// state has changed.  If so, we should return immediately and log a
+	// warning.
+	if c.state.State != define.ContainerStateStopping {
+		logrus.Warnf(
+			"Container %q state changed from %q to %q while waiting for it to be stopped: discontinuing stop procedure as another process interfered",
+			c.ID(), define.ContainerStateStopping, c.state.State,
+		)
+		return nil
+	}
+
 	c.newContainerEvent(events.Stop)
 
 	c.state.PID = 0
@@ -2094,7 +2133,7 @@ func (c *Container) sortUserVolumes(ctrSpec *spec.Spec) ([]*ContainerNamedVolume
 // Check for an exit file, and handle one if present
 func (c *Container) checkExitFile() error {
 	// If the container's not running, nothing to do.
-	if !c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused) {
+	if !c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused, define.ContainerStateStopping) {
 		return nil
 	}
 
diff --git a/libpod/define/containerstate.go b/libpod/define/containerstate.go
index 825e77387..5d2bc9099 100644
--- a/libpod/define/containerstate.go
+++ b/libpod/define/containerstate.go
@@ -28,6 +28,9 @@ const (
 	// ContainerStateRemoving indicates the container is in the process of
 	// being removed.
 	ContainerStateRemoving ContainerStatus = iota
+	// ContainerStateStopping indicates the container is in the process of
+	// being stopped.
+	ContainerStateStopping ContainerStatus = iota
 )
 
 // ContainerStatus returns a string representation for users
@@ -50,6 +53,8 @@ func (t ContainerStatus) String() string {
 		return "exited"
 	case ContainerStateRemoving:
 		return "removing"
+	case ContainerStateStopping:
+		return "stopping"
 	}
 	return "bad state"
 }
diff --git a/libpod/rootless_cni_linux.go b/libpod/rootless_cni_linux.go
index ce8a87759..9a980750f 100644
--- a/libpod/rootless_cni_linux.go
+++ b/libpod/rootless_cni_linux.go
@@ -110,6 +110,8 @@ func DeallocRootlessCNI(ctx context.Context, c *Container) error {
 			logrus.Warn(err)
 		}
 		logrus.Debugf("rootless CNI: removing infra container %q", infra.ID())
+		infra.lock.Lock()
+		defer infra.lock.Unlock()
 		if err := c.runtime.removeContainer(ctx, infra, true, false, true); err != nil {
 			return err
 		}