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 <mheon@redhat.com>
This commit is contained in:
Matthew Heon
2019-10-28 13:09:01 -04:00
parent ac73fd3fe5
commit 5f8bf3d07d
3 changed files with 46 additions and 52 deletions

View File

@ -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
}

View File

@ -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
}

View File

@ -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
}