From 2c7f97d5a782d35efc195baf7a7ca9016ca05409 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Sun, 23 Sep 2018 18:04:29 -0400 Subject: 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 --- libpod/container.go | 7 +++- libpod/container_api.go | 59 +++++++++++++----------------- libpod/container_internal.go | 86 ++++++++++++++++++++++++-------------------- libpod/pod_api.go | 4 +-- libpod/runtime_ctr.go | 5 +-- libpod/runtime_pod_linux.go | 5 +-- 6 files changed, 86 insertions(+), 80 deletions(-) (limited to 'libpod') diff --git a/libpod/container.go b/libpod/container.go index 0b1879208..55a0f3a2c 100644 --- a/libpod/container.go +++ b/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" } diff --git a/libpod/container_api.go b/libpod/container_api.go index fc2058de6..192ccd347 100644 --- a/libpod/container_api.go +++ b/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 { - // Or initialize it for the first time if necessary + } else if c.state.State == ContainerStateConfigured || + 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 { - // Or initialize it for the first time if necessary + } else if c.state.State == ContainerStateConfigured || + 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 { - c.lock.Lock() - 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) + // Stop with the container's given timeout + return c.StopWithTimeout(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 } diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 0642c5aad..60e83576a 100644 --- a/libpod/container_internal.go +++ b/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 -// Deletes and recreates a container in the runtime -// Should only be done on ContainerStateStopped containers -func (c *Container) reinit(ctx context.Context) error { - logrus.Debugf("Recreating container %s in OCI runtime", c.ID()) +// Clean up a container in the OCI runtime. +// Deletes the container in the runtime, and resets its state to Exited. +// The container can be restarted cleanly after this. +func (c *Container) cleanupRuntime(ctx context.Context) error { + // 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 - // 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 - c.state.ExitCode = 0 - c.state.Exited = false + // Our state is now Exited, as we've removed ourself from + // the runtime. + c.state.State = ContainerStateExited 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.removeConmonFiles(); err != nil { - return err - } - - // Delete the container in the runtime - 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 { + if err := c.reinit(ctx); err != nil { return err } - } - - // If we are ContainerStateConfigured we need to init() - if c.state.State == ContainerStateConfigured { + } else if c.state.State == ContainerStateConfigured || + c.state.State == ContainerStateExited { 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 { - // Initialize the container if it has never been initialized + } else if c.state.State == ContainerStateConfigured || + 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 } diff --git a/libpod/pod_api.go b/libpod/pod_api.go index 0c518da0d..3d5512e8c 100644 --- a/libpod/pod_api.go +++ b/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 } } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 6c487e367..b8a8b6c20 100644 --- a/libpod/runtime_ctr.go +++ b/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 diff --git a/libpod/runtime_pod_linux.go b/libpod/runtime_pod_linux.go index dd57007e0..974cd2b68 100644 --- a/libpod/runtime_pod_linux.go +++ b/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 } -- cgit v1.2.3-54-g00ecf From 29dbab64407d11f314b894c37deb4419faeefc56 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 24 Sep 2018 09:53:08 -0400 Subject: Address review comments and fix ps output Signed-off-by: Matthew Heon --- cmd/podman/ps.go | 2 ++ libpod/runtime_ctr.go | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'libpod') diff --git a/cmd/podman/ps.go b/cmd/podman/ps.go index d36c929e8..e53afe1bf 100644 --- a/cmd/podman/ps.go +++ b/cmd/podman/ps.go @@ -548,6 +548,8 @@ func getTemplateOutput(psParams []psJSONParams, opts shared.PsOptions) ([]psTemp labels := formatLabels(psParam.Labels) switch psParam.Status { + case libpod.ContainerStateExited.String(): + fallthrough case libpod.ContainerStateStopped.String(): exitedSince := units.HumanDuration(time.Since(psParam.ExitedAt)) status = fmt.Sprintf("Exited (%d) %s ago", psParam.ExitCode, exitedSince) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index b8a8b6c20..0fe896e5f 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -332,9 +332,9 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool) label.ReleaseLabel(c.ProcessLabel()) r.reserveLabels() } - // Delete the container - // Only do this if we're not ContainerStateConfigured - if we are, - // we haven't been created in the runtime yet + // Delete the container. + // Not needed in Configured and Exited states, where the container + // doesn't exist in the runtime if c.state.State != ContainerStateConfigured && c.state.State != ContainerStateExited { if err := c.delete(ctx); err != nil { -- cgit v1.2.3-54-g00ecf From 39d7c869ea9291352e836f3c170d9bd801f9c35c Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 2 Oct 2018 12:07:23 -0400 Subject: Fix bug with exited state and container remove Signed-off-by: Matthew Heon --- libpod/runtime_ctr.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'libpod') diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 0fe896e5f..4256a84a0 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -262,7 +262,8 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool) } } else 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, "cannot remove container %s as it is %s - running or paused containers cannot be removed", c.ID(), c.state.State.String()) } -- cgit v1.2.3-54-g00ecf From 7e23fb6c5d252c61125388ec3c7e1313b7f6bb85 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 2 Oct 2018 13:39:33 -0400 Subject: Fix cleanupRuntime to only save if container is valid We call cleanup() (which calls cleanupRuntime()) as part of removing containers, after the container has already been removed from the database. cleanupRuntime() tries to update and save the state, which obviously fails if the container no longer exists. Make the save() conditional on the container not being in the process of being removed. Signed-off-by: Matthew Heon --- libpod/container_internal.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'libpod') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 60e83576a..c2ed67a82 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -561,8 +561,11 @@ func (c *Container) cleanupRuntime(ctx context.Context) error { // Our state is now Exited, as we've removed ourself from // the runtime. c.state.State = ContainerStateExited - if err := c.save(); err != nil { - return err + + if c.valid { + if err := c.save(); err != nil { + return err + } } logrus.Debugf("Successfully cleaned up container %s", c.ID()) -- cgit v1.2.3-54-g00ecf From b7c5fa70ab858a32432a653674efcf10531048ff Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 2 Oct 2018 14:26:19 -0400 Subject: Fix Wait() to allow Exited state as well as Stopped Signed-off-by: Matthew Heon --- libpod/container_internal.go | 2 +- libpod/oci.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'libpod') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index c2ed67a82..f6c8781e0 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -423,7 +423,7 @@ func (c *Container) isStopped() (bool, error) { if err != nil { return true, err } - return c.state.State == ContainerStateStopped, nil + return (c.state.State == ContainerStateStopped || c.state.State == ContainerStateExited), nil } // save container state to the database diff --git a/libpod/oci.go b/libpod/oci.go index 3838394cb..61ae4b46e 100644 --- a/libpod/oci.go +++ b/libpod/oci.go @@ -457,7 +457,7 @@ func (r *OCIRuntime) updateContainerStatus(ctr *Container) error { if err != nil { if strings.Contains(string(out), "does not exist") { ctr.removeConmonFiles() - ctr.state.State = ContainerStateConfigured + ctr.state.State = ContainerStateExited return nil } return errors.Wrapf(err, "error getting container %s state. stderr/out: %s", ctr.ID(), out) -- cgit v1.2.3-54-g00ecf