Skip to content

Add ensureState helper for checking container state #4355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Oct 28, 2019
Merged

Add ensureState helper for checking container state #4355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions libpod/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
44 changes: 13 additions & 31 deletions libpod/container_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}

Expand Down Expand Up @@ -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)
Expand All @@ -201,6 +196,7 @@ func (c *Container) Kill(signal uint) error {
}
}

// TODO: Is killing a paused container OK?
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A paused container shouldn't be running, so I'm not sure how you'd stop the process there. If you could, I don't think you should unless there was some kind of --force option added to the kill command.

Copy link
Member Author

@mheon mheon Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paused processes can still receive signals, which has some interesting applications - you can pause a container, SIGKILL every process in it, and unpause it, to cause it to immediately exit without any possibility of a race condition around fork()

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the 411, I wasn't aware that paused containers could receive signals still.

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())
}
Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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())
}

Expand Down Expand Up @@ -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
Expand All @@ -666,6 +656,7 @@ func (c *Container) Sync() error {
}
}
}

defer c.newContainerEvent(events.Sync)
return nil
}
Expand Down Expand Up @@ -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
}
45 changes: 24 additions & 21 deletions libpod/container_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!")
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice little convenience function!


// 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
Expand All @@ -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 {
Expand Down Expand Up @@ -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())
}

Expand Down Expand Up @@ -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())
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down