Skip to content

Commit

Permalink
Add ContainerStateRemoving
Browse files Browse the repository at this point in the history 
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 containers#3906

Downstream-patch: podman-1775647.patch
Signed-off-by: Matthew Heon <[email protected]>
Signed-off-by: Valentin Rothberg <[email protected]>
  • Loading branch information
@mheon @vrothberg
mheon authored and vrothberg committed Jun 28, 2022
1 parent 23a870b commit 2c8c02a
Show file tree
Hide file tree
Showing 8 changed files with 105 additions and 95 deletions.
2 changes: 2 additions & 0 deletions cmd/podman/shared/container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand Down
17 changes: 15 additions & 2 deletions libpod/container_api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,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()
}
Expand Down Expand Up @@ -479,7 +484,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)
}

Expand Down Expand Up @@ -670,6 +680,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
Expand Down Expand Up @@ -810,7 +824,6 @@ func (c *Container) Checkpoint(ctx context.Context, options ContainerCheckpointO
return err
}
}
defer c.newContainerEvent(events.Checkpoint)
return c.checkpoint(ctx, options)
}

Expand Down
5 changes: 4 additions & 1 deletion libpod/container_internal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1050,6 +1051,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
Expand Down
5 changes: 4 additions & 1 deletion libpod/container_internal_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -698,6 +699,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
Expand Down Expand Up @@ -769,7 +772,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())
}

Expand Down
7 changes: 7 additions & 0 deletions libpod/define/containerstate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -45,6 +48,8 @@ func (t ContainerStatus) String() string {
return "paused"
case ContainerStateExited:
return "exited"
case ContainerStateRemoving:
return "removing"
}
return "bad state"
}
Expand All @@ -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)
}
Expand Down
84 changes: 14 additions & 70 deletions libpod/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,16 +769,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()
Expand All @@ -797,16 +789,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()
Expand All @@ -825,22 +809,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
Expand All @@ -857,16 +833,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 {
Expand All @@ -889,16 +857,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()
Expand All @@ -918,16 +878,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()
Expand All @@ -946,16 +898,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()
Expand Down
55 changes: 34 additions & 21 deletions libpod/runtime_ctr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
25 changes: 25 additions & 0 deletions libpod/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,3 +203,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
}

0 comments on commit 2c8c02a

Please sign in to comment.