From 25cc43c376c5ddfa70a6009526f8f03b5235c2c6 Mon Sep 17 00:00:00 2001

From: Matthew Heon <mheon@redhat.com>

Date: Mon, 11 Nov 2019 09:52:13 -0500

Subject: [PATCH 1/2] Add ContainerStateRemoving

When Libpod removes a container, there is the possibility that

removal will not fully succeed. The most notable problems are

storage issues, where the container cannot be removed from

c/storage.

When this occurs, we were faced with a choice. We can keep the

container in the state, appearing in `podman ps` and available for

other API operations, but likely unable to do any of them as it's

been partially removed. Or we can remove it very early and clean

up after it's already gone. We have, until now, used the second

approach.

The problem that arises is intermittent problems removing

storage. We end up removing a container, failing to remove its

storage, and ending up with a container permanently stuck in

c/storage that we can't remove with the normal Podman CLI, can't

use the name of, and generally can't interact with. A notable

cause is when Podman is hit by a SIGKILL midway through removal,

which can consistently cause `podman rm` to fail to remove

storage.

We now add a new state for containers that are in the process of

being removed, ContainerStateRemoving. We set this at the

beginning of the removal process. It notifies Podman that the

container cannot be used anymore, but preserves it in the DB

until it is fully removed. This will allow Remove to be run on

these containers again, which should successfully remove storage

if it fails.

Fixes #3906

Signed-off-by: Matthew Heon <mheon@redhat.com>

---

 cmd/podman/shared/container.go     |  2 +

 libpod/container_api.go            | 17 +++++-

 libpod/container_internal.go       |  5 +-

 libpod/container_internal_linux.go |  5 +-

 libpod/define/containerstate.go    |  7 +++

 libpod/options.go                  | 84 +++++-------------------------

 libpod/runtime_ctr.go              | 55 +++++++++++--------

 libpod/util.go                     | 25 +++++++++

 8 files changed, 105 insertions(+), 95 deletions(-)

diff --git a/cmd/podman/shared/container.go b/cmd/podman/shared/container.go

index f49943477f..0a2e96cf7f 100644

--- a/cmd/podman/shared/container.go

+++ b/cmd/podman/shared/container.go

@@ -195,6 +195,8 @@ func NewBatchContainer(ctr *libpod.Container, opts PsOptions) (PsContainerOutput

 		status = "Paused"

 	case define.ContainerStateCreated.String(), define.ContainerStateConfigured.String():

 		status = "Created"

+	case define.ContainerStateRemoving.String():

+		status = "Removing"

 	default:

 		status = "Error"

 	}

diff --git a/libpod/container_api.go b/libpod/container_api.go

index b8cfe02f6f..153a1d628e 100644

--- a/libpod/container_api.go

+++ b/libpod/container_api.go

@@ -404,6 +404,11 @@ func (c *Container) Mount() (string, error) {

 			return "", err

 		}

 	}

+

+	if c.state.State == define.ContainerStateRemoving {

+		return "", errors.Wrapf(define.ErrCtrStateInvalid, "cannot mount container %s as it is being removed", c.ID())

+	}

+

 	defer c.newContainerEvent(events.Mount)

 	return c.mount()

 }

@@ -488,7 +493,12 @@ func (c *Container) Export(path string) error {

 			return err

 		}

 	}

-	defer c.newContainerEvent(events.Export)

+

+	if c.state.State == define.ContainerStateRemoving {

+		return errors.Wrapf(define.ErrCtrStateInvalid, "cannot mount container %s as it is being removed", c.ID())

+	}

+

+	defer c.newContainerEvent(events.Mount)

 	return c.export(path)

 }

@@ -674,6 +684,10 @@ func (c *Container) Refresh(ctx context.Context) error {

 		}

 	}

+	if c.state.State == define.ContainerStateRemoving {

+		return errors.Wrapf(define.ErrCtrStateInvalid, "cannot refresh containers that are being removed")

+	}

+

 	wasCreated := false

 	if c.state.State == define.ContainerStateCreated {

 		wasCreated = true

@@ -819,7 +833,6 @@ func (c *Container) Checkpoint(ctx context.Context, options ContainerCheckpointO

 			return err

 		}

 	}

-	defer c.newContainerEvent(events.Checkpoint)

 	return c.checkpoint(ctx, options)

 }

diff --git a/libpod/container_internal.go b/libpod/container_internal.go

index 4ff1913b52..1e8a8a5808 100644

--- a/libpod/container_internal.go

+++ b/libpod/container_internal.go

@@ -719,7 +719,8 @@ func (c *Container) isStopped() (bool, error) {

 	if err != nil {

 		return true, err

 	}

-	return c.state.State != define.ContainerStateRunning && c.state.State != define.ContainerStatePaused, nil

+

+	return !c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused), nil

 }

 // save container state to the database

@@ -1057,6 +1058,8 @@ func (c *Container) initAndStart(ctx context.Context) (err error) {

 	// If we are ContainerStateUnknown, throw an error

 	if c.state.State == define.ContainerStateUnknown {

 		return errors.Wrapf(define.ErrCtrStateInvalid, "container %s is in an unknown state", c.ID())

+	} else if c.state.State == define.ContainerStateRemoving {

+		return errors.Wrapf(define.ErrCtrStateInvalid, "cannot start container %s as it is being removed", c.ID())

 	}

 	// If we are running, do nothing

diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go

index 26d6771b0f..aca7bdc67a 100644

--- a/libpod/container_internal_linux.go

+++ b/libpod/container_internal_linux.go

@@ -21,6 +21,7 @@ import (

 	"github.com/containernetworking/plugins/pkg/ns"

 	"github.com/containers/buildah/pkg/secrets"

 	"github.com/containers/libpod/libpod/define"

+	"github.com/containers/libpod/libpod/events"

 	"github.com/containers/libpod/pkg/annotations"

 	"github.com/containers/libpod/pkg/apparmor"

 	"github.com/containers/libpod/pkg/cgroups"

@@ -695,6 +696,8 @@ func (c *Container) checkpoint(ctx context.Context, options ContainerCheckpointO

 		return err

 	}

+	defer c.newContainerEvent(events.Checkpoint)

+

 	if options.TargetFile != "" {

 		if err = c.exportCheckpoint(options.TargetFile, options.IgnoreRootfs); err != nil {

 			return err

@@ -766,7 +769,7 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti

 		return err

 	}

-	if (c.state.State != define.ContainerStateConfigured) && (c.state.State != define.ContainerStateExited) {

+	if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited) {

 		return errors.Wrapf(define.ErrCtrStateInvalid, "container %s is running or paused, cannot restore", c.ID())

 	}

diff --git a/libpod/define/containerstate.go b/libpod/define/containerstate.go

index ab2527b3ee..e7d258e214 100644

--- a/libpod/define/containerstate.go

+++ b/libpod/define/containerstate.go

@@ -25,6 +25,9 @@ const (

 	// ContainerStateExited indicates the the container has stopped and been

 	// cleaned up

 	ContainerStateExited ContainerStatus = iota

+	// ContainerStateRemoving indicates the container is in the process of

+	// being removed.

+	ContainerStateRemoving ContainerStatus = iota

 )

 // ContainerStatus returns a string representation for users

@@ -45,6 +48,8 @@ func (t ContainerStatus) String() string {

 		return "paused"

 	case ContainerStateExited:

 		return "exited"

+	case ContainerStateRemoving:

+		return "removing"

 	}

 	return "bad state"

 }

@@ -67,6 +72,8 @@ func StringToContainerStatus(status string) (ContainerStatus, error) {

 		return ContainerStatePaused, nil

 	case ContainerStateExited.String():

 		return ContainerStateExited, nil

+	case ContainerStateRemoving.String():

+		return ContainerStateRemoving, nil

 	default:

 		return ContainerStateUnknown, errors.Wrapf(ErrInvalidArg, "unknown container state: %s", status)

 	}

diff --git a/libpod/options.go b/libpod/options.go

index bfbbb9e2da..19c776cf06 100644

--- a/libpod/options.go

+++ b/libpod/options.go

@@ -768,16 +768,8 @@ func WithIPCNSFrom(nsCtr *Container) CtrCreateOption {

 			return define.ErrCtrFinalized

 		}

-		if !nsCtr.valid {

-			return define.ErrCtrRemoved

-		}

-

-		if nsCtr.ID() == ctr.ID() {

-			return errors.Wrapf(define.ErrInvalidArg, "must specify another container")

-		}

-

-		if ctr.config.Pod != "" && nsCtr.config.Pod != ctr.config.Pod {

-			return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, nsCtr.ID())

+		if err := checkDependencyContainer(nsCtr, ctr); err != nil {

+			return err

 		}

 		ctr.config.IPCNsCtr = nsCtr.ID()

@@ -796,16 +788,8 @@ func WithMountNSFrom(nsCtr *Container) CtrCreateOption {

 			return define.ErrCtrFinalized

 		}

-		if !nsCtr.valid {

-			return define.ErrCtrRemoved

-		}

-

-		if nsCtr.ID() == ctr.ID() {

-			return errors.Wrapf(define.ErrInvalidArg, "must specify another container")

-		}

-

-		if ctr.config.Pod != "" && nsCtr.config.Pod != ctr.config.Pod {

-			return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, nsCtr.ID())

+		if err := checkDependencyContainer(nsCtr, ctr); err != nil {

+			return err

 		}

 		ctr.config.MountNsCtr = nsCtr.ID()

@@ -824,22 +808,14 @@ func WithNetNSFrom(nsCtr *Container) CtrCreateOption {

 			return define.ErrCtrFinalized

 		}

-		if !nsCtr.valid {

-			return define.ErrCtrRemoved

-		}

-

-		if nsCtr.ID() == ctr.ID() {

-			return errors.Wrapf(define.ErrInvalidArg, "must specify another container")

+		if err := checkDependencyContainer(nsCtr, ctr); err != nil {

+			return err

 		}

 		if ctr.config.CreateNetNS {

 			return errors.Wrapf(define.ErrInvalidArg, "cannot join another container's net ns as we are making a new net ns")

 		}

-		if ctr.config.Pod != "" && nsCtr.config.Pod != ctr.config.Pod {

-			return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, nsCtr.ID())

-		}

-

 		ctr.config.NetNsCtr = nsCtr.ID()

 		return nil

@@ -856,16 +832,8 @@ func WithPIDNSFrom(nsCtr *Container) CtrCreateOption {

 			return define.ErrCtrFinalized

 		}

-		if !nsCtr.valid {

-			return define.ErrCtrRemoved

-		}

-

-		if nsCtr.ID() == ctr.ID() {

-			return errors.Wrapf(define.ErrInvalidArg, "must specify another container")

-		}

-

-		if ctr.config.Pod != "" && nsCtr.config.Pod != ctr.config.Pod {

-			return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, nsCtr.ID())

+		if err := checkDependencyContainer(nsCtr, ctr); err != nil {

+			return err

 		}

 		if ctr.config.NoCgroups {

@@ -888,16 +856,8 @@ func WithUserNSFrom(nsCtr *Container) CtrCreateOption {

 			return define.ErrCtrFinalized

 		}

-		if !nsCtr.valid {

-			return define.ErrCtrRemoved

-		}

-

-		if nsCtr.ID() == ctr.ID() {

-			return errors.Wrapf(define.ErrInvalidArg, "must specify another container")

-		}

-

-		if ctr.config.Pod != "" && nsCtr.config.Pod != ctr.config.Pod {

-			return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, nsCtr.ID())

+		if err := checkDependencyContainer(nsCtr, ctr); err != nil {

+			return err

 		}

 		ctr.config.UserNsCtr = nsCtr.ID()

@@ -917,16 +877,8 @@ func WithUTSNSFrom(nsCtr *Container) CtrCreateOption {

 			return define.ErrCtrFinalized

 		}

-		if !nsCtr.valid {

-			return define.ErrCtrRemoved

-		}

-

-		if nsCtr.ID() == ctr.ID() {

-			return errors.Wrapf(define.ErrInvalidArg, "must specify another container")

-		}

-

-		if ctr.config.Pod != "" && nsCtr.config.Pod != ctr.config.Pod {

-			return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, nsCtr.ID())

+		if err := checkDependencyContainer(nsCtr, ctr); err != nil {

+			return err

 		}

 		ctr.config.UTSNsCtr = nsCtr.ID()

@@ -945,16 +897,8 @@ func WithCgroupNSFrom(nsCtr *Container) CtrCreateOption {

 			return define.ErrCtrFinalized

 		}

-		if !nsCtr.valid {

-			return define.ErrCtrRemoved

-		}

-

-		if nsCtr.ID() == ctr.ID() {

-			return errors.Wrapf(define.ErrInvalidArg, "must specify another container")

-		}

-

-		if ctr.config.Pod != "" && nsCtr.config.Pod != ctr.config.Pod {

-			return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, nsCtr.ID())

+		if err := checkDependencyContainer(nsCtr, ctr); err != nil {

+			return err

 		}

 		ctr.config.CgroupNsCtr = nsCtr.ID()

diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go

index 7069d34940..ae401013c8 100644

--- a/libpod/runtime_ctr.go

+++ b/libpod/runtime_ctr.go

@@ -489,32 +489,19 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool,

 		}

 	}

-	var cleanupErr error

-	// Remove the container from the state

-	if c.config.Pod != "" {

-		// If we're removing the pod, the container will be evicted

-		// from the state elsewhere

-		if !removePod {

-			if err := r.state.RemoveContainerFromPod(pod, c); err != nil {

-				cleanupErr = err

-			}

-		}

-	} else {

-		if err := r.state.RemoveContainer(c); err != nil {

-			cleanupErr = err

-		}

+	// Set ContainerStateRemoving and remove exec sessions

+	c.state.State = define.ContainerStateRemoving

+	c.state.ExecSessions = nil

+

+	if err := c.save(); err != nil {

+		return errors.Wrapf(err, "unable to set container %s removing state in database", c.ID())

 	}

-	// Set container as invalid so it can no longer be used

-	c.valid = false

+	var cleanupErr error

 	// Clean up network namespace, cgroups, mounts

 	if err := c.cleanup(ctx); err != nil {

-		if cleanupErr == nil {

-			cleanupErr = errors.Wrapf(err, "error cleaning up container %s", c.ID())

-		} else {

-			logrus.Errorf("cleanup network, cgroups, mounts: %v", err)

-		}

+		cleanupErr = errors.Wrapf(err, "error cleaning up container %s", c.ID())

 	}

 	// Stop the container's storage

@@ -540,6 +527,29 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool,

 		}

 	}

+	// Remove the container from the state

+	if c.config.Pod != "" {

+		// If we're removing the pod, the container will be evicted

+		// from the state elsewhere

+		if !removePod {

+			if err := r.state.RemoveContainerFromPod(pod, c); err != nil {

+				if cleanupErr == nil {

+					cleanupErr = err

+				} else {

+					logrus.Errorf("Error removing container %s from database: %v", c.ID(), err)

+				}

+			}

+		}

+	} else {

+		if err := r.state.RemoveContainer(c); err != nil {

+			if cleanupErr == nil {

+				cleanupErr = err

+			} else {

+				logrus.Errorf("Error removing container %s from database: %v", c.ID(), err)

+			}

+		}

+	}

+

 	// Deallocate the container's lock

 	if err := c.lock.Free(); err != nil {

 		if cleanupErr == nil {

@@ -549,6 +559,9 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool,

 		}

 	}

+	// Set container as invalid so it can no longer be used

+	c.valid = false

+

 	c.newContainerEvent(events.Remove)

 	if !removeVolume {

diff --git a/libpod/util.go b/libpod/util.go

index bae2f4eb83..30e5cd4c33 100644

--- a/libpod/util.go

+++ b/libpod/util.go

@@ -206,3 +206,28 @@ func DefaultSeccompPath() (string, error) {

 	}

 	return config.SeccompDefaultPath, nil

 }

+

+// CheckDependencyContainer verifies the given container can be used as a

+// dependency of another container.

+// Both the dependency to check and the container that will be using the

+// dependency must be passed in.

+// It is assumed that ctr is locked, and depCtr is unlocked.

+func checkDependencyContainer(depCtr, ctr *Container) error {

+	state, err := depCtr.State()

+	if err != nil {

+		return errors.Wrapf(err, "error accessing dependency container %s state", depCtr.ID())

+	}

+	if state == define.ContainerStateRemoving {

+		return errors.Wrapf(define.ErrCtrStateInvalid, "cannot use container %s as a dependency as it is being removed", depCtr.ID())

+	}

+

+	if depCtr.ID() == ctr.ID() {

+		return errors.Wrapf(define.ErrInvalidArg, "must specify another container")

+	}

+

+	if ctr.config.Pod != "" && depCtr.PodID() != ctr.config.Pod {

+		return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, depCtr.ID())

+	}

+

+	return nil

+}

From 6c405b5fbcc83ba49c187087eb4e1ccc1a7ff147 Mon Sep 17 00:00:00 2001

From: Matthew Heon <mheon@redhat.com>

Date: Mon, 11 Nov 2019 14:36:00 -0500

Subject: [PATCH 2/2] Error on netns not exist only when ctr is running

If the container is running and we need to get its netns and

can't, that is a serious bug deserving of errors.

If it's not running, that's not really a big deal. Log an error

and continue.

Signed-off-by: Matthew Heon <mheon@redhat.com>

---

 libpod/boltdb_state_linux.go | 8 +++++++-

 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libpod/boltdb_state_linux.go b/libpod/boltdb_state_linux.go

index 09a9be6067..6ccda71bd5 100644

--- a/libpod/boltdb_state_linux.go

+++ b/libpod/boltdb_state_linux.go

@@ -3,6 +3,8 @@

 package libpod

 import (

+	"github.com/containers/libpod/libpod/define"

+	"github.com/pkg/errors"

 	"github.com/sirupsen/logrus"

 )

@@ -25,8 +27,12 @@ func replaceNetNS(netNSPath string, ctr *Container, newState *ContainerState) er

 			if err == nil {

 				newState.NetNS = ns

 			} else {

+				if ctr.ensureState(define.ContainerStateRunning, define.ContainerStatePaused) {

+					return errors.Wrapf(err, "error joning network namespace of container %s", ctr.ID())

+				}

+

 				logrus.Errorf("error joining network namespace for container %s: %v", ctr.ID(), err)

-				ctr.valid = false

+				ctr.state.NetNS = nil

 			}

 		}

 	} else {