From 5f8bf3d07d94d58465dfa494f7720ca23b8d4c6f Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 28 Oct 2019 13:09:01 -0400 Subject: Add ensureState helper for checking container state We have a lot of checks for container state scattered throughout libpod. Many of these need to ensure the container is in one of a given set of states so an operation may safely proceed. Previously there was no set way of doing this, so we'd use unique boolean logic for each one. Introduce a helper to standardize state checks. Note that this is only intended to replace checks for multiple states. A simple check for one state (ContainerStateRunning, for example) should remain a straight equality, and not use this new helper. Signed-off-by: Matthew Heon --- libpod/container.go | 9 +++++++++ libpod/container_api.go | 44 +++++++++++++------------------------------ libpod/container_internal.go | 45 +++++++++++++++++++++++--------------------- 3 files changed, 46 insertions(+), 52 deletions(-) (limited to 'libpod') diff --git a/libpod/container.go b/libpod/container.go index 7be73b3c3..fc9ef0c86 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -1185,3 +1185,12 @@ func (c *Container) HasHealthCheck() bool { func (c *Container) HealthCheckConfig() *manifest.Schema2HealthConfig { return c.config.HealthCheckConfig } + +// AutoRemove indicates whether the container will be removed after it is executed +func (c *Container) AutoRemove() bool { + spec := c.config.Spec + if spec.Annotations == nil { + return false + } + return c.Spec().Annotations[InspectAnnotationAutoremove] == InspectResponseTrue +} diff --git a/libpod/container_api.go b/libpod/container_api.go index 759a7067e..1b2d52ce3 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -32,9 +32,7 @@ func (c *Container) Init(ctx context.Context) (err error) { } } - if !(c.state.State == define.ContainerStateConfigured || - c.state.State == define.ContainerStateStopped || - c.state.State == define.ContainerStateExited) { + if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateStopped, define.ContainerStateExited) { return errors.Wrapf(define.ErrCtrStateInvalid, "container %s has already been created in runtime", c.ID()) } @@ -176,15 +174,12 @@ func (c *Container) StopWithTimeout(timeout uint) error { } } - if c.state.State == define.ContainerStateConfigured || - c.state.State == define.ContainerStateUnknown || - c.state.State == define.ContainerStatePaused { - return errors.Wrapf(define.ErrCtrStateInvalid, "can only stop created, running, or stopped containers. %s is in state %s", c.ID(), c.state.State.String()) + if c.ensureState(define.ContainerStateStopped, define.ContainerStateExited) { + return define.ErrCtrStopped } - if c.state.State == define.ContainerStateStopped || - c.state.State == define.ContainerStateExited { - return define.ErrCtrStopped + if !c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning) { + return errors.Wrapf(define.ErrCtrStateInvalid, "can only stop created or running containers. %s is in state %s", c.ID(), c.state.State.String()) } return c.stop(timeout, false) @@ -201,6 +196,7 @@ func (c *Container) Kill(signal uint) error { } } + // TODO: Is killing a paused container OK? if c.state.State != define.ContainerStateRunning { return errors.Wrapf(define.ErrCtrStateInvalid, "can only kill running containers. %s is in state %s", c.ID(), c.state.State.String()) } @@ -234,10 +230,7 @@ func (c *Container) Exec(tty, privileged bool, env map[string]string, cmd []stri } } - conState := c.state.State - - // TODO can probably relax this once we track exec sessions - if conState != define.ContainerStateRunning { + if c.state.State != define.ContainerStateRunning { return define.ExecErrorCodeCannotInvoke, errors.Wrapf(define.ErrCtrStateInvalid, "cannot exec into container that is not running") } @@ -391,11 +384,10 @@ func (c *Container) Attach(streams *AttachStreams, keys string, resize <-chan re c.lock.Unlock() } - if c.state.State != define.ContainerStateCreated && - c.state.State != define.ContainerStateRunning && - c.state.State != define.ContainerStateExited { + if !c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning) { return errors.Wrapf(define.ErrCtrStateInvalid, "can only attach to created or running containers") } + defer c.newContainerEvent(events.Attach) return c.attach(streams, keys, resize, false, nil) } @@ -432,7 +424,7 @@ func (c *Container) Unmount(force bool) error { return errors.Wrapf(err, "can't determine how many times %s is mounted, refusing to unmount", c.ID()) } if mounted == 1 { - if c.state.State == define.ContainerStateRunning || c.state.State == define.ContainerStatePaused { + if c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused) { return errors.Wrapf(define.ErrCtrStateInvalid, "cannot unmount storage for container %s as it is running or paused", c.ID()) } if len(c.state.ExecSessions) != 0 { @@ -574,7 +566,7 @@ func (c *Container) Cleanup(ctx context.Context) error { } // Check if state is good - if c.state.State == define.ContainerStateRunning || c.state.State == define.ContainerStatePaused { + if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateExited) { return errors.Wrapf(define.ErrCtrStateInvalid, "container %s is running or paused, refusing to clean up", c.ID()) } @@ -652,9 +644,7 @@ func (c *Container) Sync() error { // If runtime knows about the container, update its status in runtime // And then save back to disk - if (c.state.State != define.ContainerStateUnknown) && - (c.state.State != define.ContainerStateConfigured) && - (c.state.State != define.ContainerStateExited) { + if c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning, define.ContainerStatePaused, define.ContainerStateStopped) { oldState := c.state.State if err := c.ociRuntime.UpdateContainerStatus(c); err != nil { return err @@ -666,6 +656,7 @@ func (c *Container) Sync() error { } } } + defer c.newContainerEvent(events.Sync) return nil } @@ -840,12 +831,3 @@ func (c *Container) Restore(ctx context.Context, options ContainerCheckpointOpti defer c.newContainerEvent(events.Restore) return c.restore(ctx, options) } - -// AutoRemove indicates whether the container will be removed after it is executed -func (c *Container) AutoRemove() bool { - spec := c.config.Spec - if spec.Annotations == nil { - return false - } - return c.Spec().Annotations[InspectAnnotationAutoremove] == InspectResponseTrue -} diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 0043c9651..028d7601d 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -328,7 +328,7 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (restarted bool, er // Is the container running again? // If so, we don't have to do anything - if c.state.State == define.ContainerStateRunning || c.state.State == define.ContainerStatePaused { + if c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused) { return false, nil } else if c.state.State == define.ContainerStateUnknown { return false, errors.Wrapf(define.ErrInternal, "invalid container state encountered in restart attempt!") @@ -359,8 +359,7 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (restarted bool, er if err := c.reinit(ctx, true); err != nil { return false, err } - } else if c.state.State == define.ContainerStateConfigured || - c.state.State == define.ContainerStateExited { + } else if c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited) { // Initialize the container if err := c.init(ctx, true); err != nil { return false, err @@ -372,6 +371,18 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (restarted bool, er return true, nil } +// Ensure that the container is in a specific state or state. +// Returns true if the container is in one of the given states, +// or false otherwise. +func (c *Container) ensureState(states ...define.ContainerStatus) bool { + for _, state := range states { + if state == c.state.State { + return true + } + } + return false +} + // Sync this container with on-disk state and runtime status // Should only be called with container lock held // This function should suffice to ensure a container's state is accurate and @@ -382,9 +393,7 @@ 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 != define.ContainerStateUnknown) && - (c.state.State != define.ContainerStateConfigured) && - (c.state.State != define.ContainerStateExited) { + if c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning, define.ContainerStateStopped, define.ContainerStatePaused) { oldState := c.state.State if err := c.checkExitFile(); err != nil { @@ -516,7 +525,7 @@ func (c *Container) setupStorage(ctx context.Context) error { // Tear down a container's storage prior to removal func (c *Container) teardownStorage() error { - if c.state.State == define.ContainerStateRunning || c.state.State == define.ContainerStatePaused { + if c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused) { return errors.Wrapf(define.ErrCtrStateInvalid, "cannot remove storage for container %s as it is running or paused", c.ID()) } @@ -721,10 +730,7 @@ func (c *Container) save() error { // Otherwise, this function will return with error if there are dependencies of this container that aren't running. func (c *Container) prepareToStart(ctx context.Context, recursive bool) (err error) { // Container must be created or stopped to be started - if !(c.state.State == define.ContainerStateConfigured || - c.state.State == define.ContainerStateCreated || - c.state.State == define.ContainerStateStopped || - c.state.State == define.ContainerStateExited) { + if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateExited) { return errors.Wrapf(define.ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID()) } @@ -755,8 +761,7 @@ func (c *Container) prepareToStart(ctx context.Context, recursive bool) (err err if err := c.reinit(ctx, false); err != nil { return err } - } else if c.state.State == define.ContainerStateConfigured || - c.state.State == define.ContainerStateExited { + } else if c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited) { // Or initialize it if necessary if err := c.init(ctx, false); err != nil { return err @@ -987,7 +992,7 @@ func (c *Container) cleanupRuntime(ctx context.Context) error { // If the container is not ContainerStateStopped or // ContainerStateCreated, do nothing. - if c.state.State != define.ContainerStateStopped && c.state.State != define.ContainerStateCreated { + if !c.ensureState(define.ContainerStateStopped, define.ContainerStateCreated) { return nil } @@ -1078,8 +1083,7 @@ func (c *Container) initAndStart(ctx context.Context) (err error) { if err := c.reinit(ctx, false); err != nil { return err } - } else if c.state.State == define.ContainerStateConfigured || - c.state.State == define.ContainerStateExited { + } else if c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited) { if err := c.init(ctx, false); err != nil { return err } @@ -1205,7 +1209,7 @@ func (c *Container) unpause() error { // Internal, non-locking function to restart a container func (c *Container) restartWithTimeout(ctx context.Context, timeout uint) (err error) { - if c.state.State == define.ContainerStateUnknown || c.state.State == define.ContainerStatePaused { + if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateRunning, define.ContainerStateStopped, define.ContainerStateExited) { return errors.Wrapf(define.ErrCtrStateInvalid, "unable to restart a container in a paused or unknown state") } @@ -1733,9 +1737,8 @@ func (c *Container) checkReadyForRemoval() error { return errors.Wrapf(define.ErrCtrStateInvalid, "container %s is in invalid state", c.ID()) } - if c.state.State == define.ContainerStateRunning || - c.state.State == define.ContainerStatePaused { - return errors.Wrapf(define.ErrCtrStateInvalid, "cannot remove container %s as it is %s - running or paused containers cannot be removed", c.ID(), c.state.State.String()) + if c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused) { + return errors.Wrapf(define.ErrCtrStateInvalid, "cannot remove container %s as it is %s - running or paused containers cannot be removed without force", c.ID(), c.state.State.String()) } if len(c.state.ExecSessions) != 0 { @@ -1816,7 +1819,7 @@ func (c *Container) sortUserVolumes(ctrSpec *spec.Spec) ([]*ContainerNamedVolume // Check for an exit file, and handle one if present func (c *Container) checkExitFile() error { // If the container's not running, nothing to do. - if c.state.State != define.ContainerStateRunning && c.state.State != define.ContainerStatePaused { + if !c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused) { return nil } -- cgit v1.2.3-54-g00ecf