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_internal.go | 86 ++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 38 deletions(-) (limited to 'libpod/container_internal.go') 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 } -- 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/container_internal.go') 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/container_internal.go') 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