Add ContainerStateExited and OCI delete() in cleanup()

To work better with Kata containers, we need to delete() from the
OCI runtime as a part of cleanup, to ensure resources aren't
retained longer than they need to be.

To enable this, we need to add a new state to containers,
ContainerStateExited. Containers transition from
ContainerStateStopped to ContainerStateExited via cleanupRuntime
which is invoked as part of cleanup(). A container in the Exited
state is identical to Stopped, except it has been removed from
the OCI runtime and thus will be handled differently when
initializing the container.

Signed-off-by: Matthew Heon <matthew.heon@gmail.com>

cmd/podman/cleanup.go

@@ -46,6 +46,8 @@ func cleanupCmd(c *cli.Context) error {
 
 	args := c.Args()
 
+	ctx := getContext()
+
 	var lastError error
 	var cleanupContainers []*libpod.Container
 	if c.Bool("all") {
@@ -80,7 +82,7 @@ func cleanupCmd(c *cli.Context) error {
 		}
 	}
 	for _, ctr := range cleanupContainers {
-		if err = ctr.Cleanup(); err != nil {
+		if err = ctr.Cleanup(ctx); err != nil {
 			if lastError != nil {
 				fmt.Fprintln(os.Stderr, lastError)
 			}

cmd/podman/pod_stop.go

@@ -50,9 +50,11 @@ func podStopCmd(c *cli.Context) error {
 	// in which case the following loop will be skipped.
 	pods, lastError := getPodsFromContext(c, runtime)
 
+	ctx := getContext()
+
 	for _, pod := range pods {
 		// set cleanup to true to clean mounts and namespaces
-		ctr_errs, err := pod.Stop(true)
+		ctr_errs, err := pod.Stop(ctx, true)
 		if ctr_errs != nil {
 			for ctr, err := range ctr_errs {
 				if lastError != nil {

cmd/podman/run.go

@@ -140,7 +140,7 @@ func runCmd(c *cli.Context) error {
 		return runtime.RemoveContainer(ctx, ctr, true)
 	}
 
-	if err := ctr.Cleanup(); err != nil {
+	if err := ctr.Cleanup(ctx); err != nil {
 		// If the container has been removed already, no need to error on cleanup
 		// Also, if it was restarted, don't error either
 		if errors.Cause(err) == libpod.ErrNoSuchCtr ||

cmd/podman/start.go

@@ -81,6 +81,9 @@ func startCmd(c *cli.Context) error {
 		}
 		args = append(args, lastCtr.ID())
 	}
+
+	ctx := getContext()
+
 	var lastError error
 	for _, container := range args {
 		ctr, err := runtime.LookupContainer(container)
@@ -121,14 +124,14 @@ func startCmd(c *cli.Context) error {
 				exitCode = int(ecode)
 			}
 
-			return ctr.Cleanup()
+			return ctr.Cleanup(ctx)
 		}
 		if ctrRunning {
 			fmt.Println(ctr.ID())
 			continue
 		}
 		// Handle non-attach start
-		if err := ctr.Start(getContext()); err != nil {
+		if err := ctr.Start(ctx); err != nil {
 			if lastError != nil {
 				fmt.Fprintln(os.Stderr, lastError)
 			}

libpod/container.go

@@ -36,6 +36,9 @@ const (
 	ContainerStateStopped ContainerStatus = iota
 	// ContainerStatePaused indicates that the container has been paused
 	ContainerStatePaused ContainerStatus = iota
+	// ContainerStateExited indicates the the container has stopped and been
+	// cleaned up
+	ContainerStateExited ContainerStatus = iota
 )
 
 // CgroupfsDefaultCgroupParent is the cgroup parent for CGroupFS in libpod
@@ -354,9 +357,11 @@ func (t ContainerStatus) String() string {
 	case ContainerStateRunning:
 		return "running"
 	case ContainerStateStopped:
-		return "exited"
+		return "stopped"
 	case ContainerStatePaused:
 		return "paused"
+	case ContainerStateExited:
+		return "exited"
 	}
 	return "bad state"
 }

libpod/container_api.go

@@ -32,7 +32,8 @@ func (c *Container) Init(ctx context.Context) (err error) {
 	}
 
 	if !(c.state.State == ContainerStateConfigured ||
-		c.state.State == ContainerStateStopped) {
+		c.state.State == ContainerStateStopped ||
+		c.state.State == ContainerStateExited) {
 		return errors.Wrapf(ErrCtrExists, "container %s has already been created in runtime", c.ID())
 	}
 
@@ -50,7 +51,7 @@ func (c *Container) Init(ctx context.Context) (err error) {
 	}
 	defer func() {
 		if err != nil {
-			if err2 := c.cleanup(); err2 != nil {
+			if err2 := c.cleanup(ctx); err2 != nil {
 				logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
 			}
 		}
@@ -84,7 +85,8 @@ func (c *Container) Start(ctx context.Context) (err error) {
 	// Container must be created or stopped to be started
 	if !(c.state.State == ContainerStateConfigured ||
 		c.state.State == ContainerStateCreated ||
-		c.state.State == ContainerStateStopped) {
+		c.state.State == ContainerStateStopped ||
+		c.state.State == ContainerStateExited) {
 		return errors.Wrapf(ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID())
 	}
 
@@ -102,7 +104,7 @@ func (c *Container) Start(ctx context.Context) (err error) {
 	}
 	defer func() {
 		if err != nil {
-			if err2 := c.cleanup(); err2 != nil {
+			if err2 := c.cleanup(ctx); err2 != nil {
 				logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
 			}
 		}
@@ -113,8 +115,9 @@ func (c *Container) Start(ctx context.Context) (err error) {
 		if err := c.reinit(ctx); err != nil {
 			return err
 		}
-	} else if c.state.State == ContainerStateConfigured {
+	} else if c.state.State == ContainerStateConfigured ||
-		// Or initialize it for the first time if necessary
+		c.state.State == ContainerStateExited {
+		// Or initialize it if necessary
 		if err := c.init(ctx); err != nil {
 			return err
 		}
@@ -147,7 +150,8 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams,
 	// Container must be created or stopped to be started
 	if !(c.state.State == ContainerStateConfigured ||
 		c.state.State == ContainerStateCreated ||
-		c.state.State == ContainerStateStopped) {
+		c.state.State == ContainerStateStopped ||
+		c.state.State == ContainerStateExited) {
 		return nil, errors.Wrapf(ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID())
 	}
 
@@ -165,7 +169,7 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams,
 	}
 	defer func() {
 		if err != nil {
-			if err2 := c.cleanup(); err2 != nil {
+			if err2 := c.cleanup(ctx); err2 != nil {
 				logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
 			}
 		}
@@ -176,8 +180,9 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams,
 		if err := c.reinit(ctx); err != nil {
 			return nil, err
 		}
-	} else if c.state.State == ContainerStateConfigured {
+	} else if c.state.State == ContainerStateConfigured ||
-		// Or initialize it for the first time if necessary
+		c.state.State == ContainerStateExited {
+		// Or initialize it if necessary
 		if err := c.init(ctx); err != nil {
 			return nil, err
 		}
@@ -202,26 +207,8 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams,
 // Default stop timeout is 10 seconds, but can be overridden when the container
 // is created
 func (c *Container) Stop() error {
-	if !c.batched {
+	// Stop with the container's given timeout
-		c.lock.Lock()
+	return c.StopWithTimeout(c.config.StopTimeout)
-		defer c.lock.Unlock()
-
-		if err := c.syncContainer(); err != nil {
-			return err
-		}
-	}
-
-	if c.state.State == ContainerStateConfigured ||
-		c.state.State == ContainerStateUnknown ||
-		c.state.State == ContainerStatePaused {
-		return errors.Wrapf(ErrCtrStateInvalid, "can only stop created, running, or stopped containers")
-	}
-
-	if c.state.State == ContainerStateStopped {
-		return ErrCtrStopped
-	}
-
-	return c.stop(c.config.StopTimeout)
 }
 
 // StopWithTimeout is a version of Stop that allows a timeout to be specified
@@ -243,7 +230,8 @@ func (c *Container) StopWithTimeout(timeout uint) error {
 		return errors.Wrapf(ErrCtrStateInvalid, "can only stop created, running, or stopped containers")
 	}
 
-	if c.state.State == ContainerStateStopped {
+	if c.state.State == ContainerStateStopped ||
+		c.state.State == ContainerStateExited {
 		return ErrCtrStopped
 	}
 
@@ -431,7 +419,8 @@ func (c *Container) Attach(streams *AttachStreams, keys string, resize <-chan re
 	}
 
 	if c.state.State != ContainerStateCreated &&
-		c.state.State != ContainerStateRunning {
+		c.state.State != ContainerStateRunning &&
+		c.state.State != ContainerStateExited {
 		return errors.Wrapf(ErrCtrStateInvalid, "can only attach to created or running containers")
 	}
 
@@ -626,7 +615,7 @@ func (c *Container) WaitWithInterval(waitTimeout time.Duration) (int32, error) {
 
 // Cleanup unmounts all mount points in container and cleans up container storage
 // It also cleans up the network stack
-func (c *Container) Cleanup() error {
+func (c *Container) Cleanup(ctx context.Context) error {
 	if !c.batched {
 		c.lock.Lock()
 		defer c.lock.Unlock()
@@ -645,7 +634,7 @@ func (c *Container) Cleanup() error {
 		return errors.Wrapf(ErrCtrStateInvalid, "container %s has active exec sessions, refusing to clean up", c.ID())
 	}
 
-	return c.cleanup()
+	return c.cleanup(ctx)
 }
 
 // Batch starts a batch operation on the given container
@@ -800,7 +789,7 @@ func (c *Container) Refresh(ctx context.Context) error {
 
 	// Fire cleanup code one more time unconditionally to ensure we are good
 	// to refresh
-	if err := c.cleanup(); err != nil {
+	if err := c.cleanup(ctx); err != nil {
 		return err
 	}
 

libpod/container_internal.go

@@ -150,7 +150,8 @@ func (c *Container) syncContainer() error {
 	// If runtime knows about the container, update its status in runtime
 	// And then save back to disk
 	if (c.state.State != ContainerStateUnknown) &&
-		(c.state.State != ContainerStateConfigured) {
+		(c.state.State != ContainerStateConfigured) &&
+		(c.state.State != ContainerStateExited) {
 		oldState := c.state.State
 		// TODO: optionally replace this with a stat for the exit file
 		if err := c.runtime.ociRuntime.updateContainerStatus(c); err != nil {
@@ -528,6 +529,8 @@ func (c *Container) init(ctx context.Context) error {
 
 	logrus.Debugf("Created container %s in OCI runtime", c.ID())
 
+	c.state.ExitCode = 0
+	c.state.Exited = false
 	c.state.State = ContainerStateCreated
 
 	if err := c.save(); err != nil {
@@ -537,11 +540,14 @@ func (c *Container) init(ctx context.Context) error {
 	return c.completeNetworkSetup()
 }
 
-// Reinitialize a container
+// Clean up a container in the OCI runtime.
-// Deletes and recreates a container in the runtime
+// Deletes the container in the runtime, and resets its state to Exited.
-// Should only be done on ContainerStateStopped containers
+// The container can be restarted cleanly after this.
-func (c *Container) reinit(ctx context.Context) error {
+func (c *Container) cleanupRuntime(ctx context.Context) error {
-	logrus.Debugf("Recreating container %s in OCI runtime", c.ID())
+	// If the container is not ContainerStateStopped, do nothing
+	if c.state.State != ContainerStateStopped {
+		return nil
+	}
 
 	// If necessary, delete attach and ctl files
 	if err := c.removeConmonFiles(); err != nil {
@@ -552,19 +558,30 @@ func (c *Container) reinit(ctx context.Context) error {
 		return err
 	}
 
-	// Our state is now Configured, as we've removed ourself from
+	// Our state is now Exited, as we've removed ourself from
-	// the runtime
+	// the runtime.
-	// Set and save now to make sure that, if the init() below fails
+	c.state.State = ContainerStateExited
-	// we still have a valid state
-	c.state.State = ContainerStateConfigured
-	c.state.ExitCode = 0
-	c.state.Exited = false
 	if err := c.save(); err != nil {
 		return err
 	}
 
 	logrus.Debugf("Successfully cleaned up container %s", c.ID())
 
+	return nil
+}
+
+// Reinitialize a container.
+// Deletes and recreates a container in the runtime.
+// Should only be done on ContainerStateStopped containers.
+// Not necessary for ContainerStateExited - the container has already been
+// removed from the runtime, so init() can proceed freely.
+func (c *Container) reinit(ctx context.Context) error {
+	logrus.Debugf("Recreating container %s in OCI runtime", c.ID())
+
+	if err := c.cleanupRuntime(ctx); err != nil {
+		return err
+	}
+
 	// Initialize the container again
 	return c.init(ctx)
 }
@@ -592,7 +609,7 @@ func (c *Container) initAndStart(ctx context.Context) (err error) {
 	}
 	defer func() {
 		if err != nil {
-			if err2 := c.cleanup(); err2 != nil {
+			if err2 := c.cleanup(ctx); err2 != nil {
 				logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
 			}
 		}
@@ -603,28 +620,11 @@ func (c *Container) initAndStart(ctx context.Context) (err error) {
 	if c.state.State == ContainerStateStopped {
 		logrus.Debugf("Recreating container %s in OCI runtime", c.ID())
 
-		// If necessary, delete attach and ctl files
+		if err := c.reinit(ctx); err != nil {
-		if err := c.removeConmonFiles(); err != nil {
 			return err
 		}
-
+	} else if c.state.State == ContainerStateConfigured ||
-		// Delete the container in the runtime
+		c.state.State == ContainerStateExited {
-		if err := c.runtime.ociRuntime.deleteContainer(c); err != nil {
-			return errors.Wrapf(err, "error removing container %s from runtime", c.ID())
-		}
-
-		// Our state is now Configured, as we've removed ourself from
-		// the runtime
-		// Set and save now to make sure that, if the init() below fails
-		// we still have a valid state
-		c.state.State = ContainerStateConfigured
-		if err := c.save(); err != nil {
-			return err
-		}
-	}
-
-	// If we are ContainerStateConfigured we need to init()
-	if c.state.State == ContainerStateConfigured {
 		if err := c.init(ctx); err != nil {
 			return err
 		}
@@ -705,7 +705,7 @@ func (c *Container) restartWithTimeout(ctx context.Context, timeout uint) (err e
 	}
 	defer func() {
 		if err != nil {
-			if err2 := c.cleanup(); err2 != nil {
+			if err2 := c.cleanup(ctx); err2 != nil {
 				logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
 			}
 		}
@@ -716,8 +716,9 @@ func (c *Container) restartWithTimeout(ctx context.Context, timeout uint) (err e
 		if err := c.reinit(ctx); err != nil {
 			return err
 		}
-	} else if c.state.State == ContainerStateConfigured {
+	} else if c.state.State == ContainerStateConfigured ||
-		// Initialize the container if it has never been initialized
+		c.state.State == ContainerStateExited {
+		// Initialize the container
 		if err := c.init(ctx); err != nil {
 			return err
 		}
@@ -826,7 +827,7 @@ func (c *Container) cleanupStorage() error {
 }
 
 // Unmount the a container and free its resources
-func (c *Container) cleanup() error {
+func (c *Container) cleanup(ctx context.Context) error {
 	var lastError error
 
 	logrus.Debugf("Cleaning up container %s", c.ID())
@@ -845,6 +846,15 @@ func (c *Container) cleanup() error {
 		}
 	}
 
+	// Remove the container from the runtime, if necessary
+	if err := c.cleanupRuntime(ctx); err != nil {
+		if lastError != nil {
+			logrus.Errorf("Error removing container %s from OCI runtime: %v", c.ID(), err)
+		} else {
+			lastError = err
+		}
+	}
+
 	return lastError
 }
 

libpod/pod_api.go

@@ -77,7 +77,7 @@ func (p *Pod) Start(ctx context.Context) (map[string]error, error) {
 // containers. The container ID is mapped to the error encountered. The error is
 // set to ErrCtrExists
 // If both error and the map are nil, all containers were stopped without error
-func (p *Pod) Stop(cleanup bool) (map[string]error, error) {
+func (p *Pod) Stop(ctx context.Context, cleanup bool) (map[string]error, error) {
 	p.lock.Lock()
 	defer p.lock.Unlock()
 
@@ -118,7 +118,7 @@ func (p *Pod) Stop(cleanup bool) (map[string]error, error) {
 		}
 
 		if cleanup {
-			if err := ctr.cleanup(); err != nil {
+			if err := ctr.cleanup(ctx); err != nil {
 				ctrErrors[ctr.ID()] = err
 			}
 		}

libpod/runtime_ctr.go

@@ -311,7 +311,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool)
 	c.valid = false
 
 	// Clean up network namespace, cgroups, mounts
-	if err := c.cleanup(); err != nil {
+	if err := c.cleanup(ctx); err != nil {
 		if cleanupErr == nil {
 			cleanupErr = err
 		} else {
@@ -335,7 +335,8 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool)
 	// Delete the container
 	// Only do this if we're not ContainerStateConfigured - if we are,
 	// we haven't been created in the runtime yet
-	if c.state.State != ContainerStateConfigured {
+	if c.state.State != ContainerStateConfigured &&
+		c.state.State != ContainerStateExited {
 		if err := c.delete(ctx); err != nil {
 			if cleanupErr == nil {
 				cleanupErr = err

libpod/runtime_pod_linux.go

@@ -222,7 +222,7 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool)
 	// As we have guaranteed their dependencies are in the pod
 	for _, ctr := range ctrs {
 		// Clean up network namespace, cgroups, mounts
-		if err := ctr.cleanup(); err != nil {
+		if err := ctr.cleanup(ctx); err != nil {
 			return err
 		}
 
@@ -233,7 +233,8 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool)
 
 		// Delete the container from runtime (only if we are not
 		// ContainerStateConfigured)
-		if ctr.state.State != ContainerStateConfigured {
+		if ctr.state.State != ContainerStateConfigured &&
+			ctr.state.State != ContainerStateExited {
 			if err := ctr.delete(ctx); err != nil {
 				return err
 			}

pkg/varlinkapi/pods.go

@@ -125,7 +125,7 @@ func (i *LibpodAPI) StopPod(call iopodman.VarlinkCall, name string) error {
 	if err != nil {
 		return call.ReplyPodNotFound(name)
 	}
-	ctrErrs, err := pod.Stop(true)
+	ctrErrs, err := pod.Stop(getContext(), true)
 	callErr := handlePodCall(call, pod, ctrErrs, err)
 	if callErr != nil {
 		return err