Implement Container States #311

crosbymichael · 2015-10-02T20:15:12Z

One of the most complex parts of the container are the various states that it has. Pause, running, stopped, destroyed, checkpointed, resumed, etc. Transitioning to these various states are also complex. Right now we use various methods to figure out what state the container is in and not really where it was. There is a good pattern to follow when you have to deal with states and transitions.

For each of the container states there is a separate type. Right now most of the logic is around how to destroy the container and depending on the state what to remove on disk. By moving all the destroy logic to the state types and letting them handle what other states they are able to transition to this it reduces the complexity when figuring out the transitions even if it is a little more code and more types.

In the future we can move more of the container methods to the state types to better handle the error conditions and remove some of the if blocks in things like Start().

Let me know what you all think. This is part of the work making runc work with the new State type from the specs. Alot of the logic was spread out around Destory() in runc and libcontainer and this centralizes all that logic and handles all the changes properly no matter what combination.

Closes #218

mrunalp · 2015-10-02T21:54:29Z

Build failure libcontainer/container_linux.go:827: unreachable code

mrunalp · 2015-10-02T21:57:13Z

I like the idea of a state machine like you have 👍

rajasec · 2015-10-03T01:52:52Z

libcontainer/container_linux.go

@crosbymichael
Do we depend on config frozen state(paused), I thought checking cgroup subsystem for the specific container would get the right state for freezer subsystem ?
or
As with your new changes, you are transitioning the state, can we leverage the state that instead of checking configs ( where Frozen never set in our configs while starting the container)

ya, we need to get your commits in #218 to resolve this.

rajasec · 2015-10-03T02:01:39Z

@crosbymichael
I like state machine approach to fix the container status/states and container cleans ups.

rajasec · 2015-10-03T02:25:57Z

libcontainer/state_linux.go

If the user checkpointed in different directory, I'm not sure whether that will get cleaned. This forces to run the checkpoint and restore from the same place ( which is expected) but the user does some mistake if the checkpoint directory exists ( not cleaned) it will pick up that image for restoring ?
Few times I faced the problem in this area.

I believe this is correct. That is not the checkpointed files but just a marker that the container was checkpointed. It will be removed like all the state after the container stops.

crosbymichael · 2015-10-05T17:41:03Z

@rajasec i was actually going to either cherry-pick your commits in #218 to this PR or just rebase your PR for you and open one with your commits after this was merged.

mrunalp · 2015-10-08T17:07:30Z

libcontainer/container_linux.go

s/inprocess/in process/

crosbymichael · 2015-10-08T20:47:59Z

@mrunalp updated

mrunalp · 2015-10-09T21:11:10Z

libcontainer/state_linux.go

Why do we need the base here? From the patch it looks like we are using it for calling destroy on destroyed state. Could we have a common destroy helper function and call it from destroy() of runningState/destroyState instead?

crosbymichael · 2015-10-12T19:06:44Z

@mrunalp @LK4D4 PTAL

Updated with your suggestions.

crosbymichael · 2015-10-15T21:28:11Z

@LK4D4 ping

LK4D4 · 2015-10-15T21:29:35Z

libcontainer/container_linux.go

Looks like you don't need else here

LK4D4 · 2015-10-15T21:39:32Z

Code looks logical for me. I posted some style recommendations though.

crosbymichael · 2015-10-15T21:43:10Z

@LK4D4 updated thanks for the reivew

LK4D4 · 2015-10-15T21:45:24Z

@crosbymichael I think maybe it's possible to write some tests. Maybe simple unit-tests for transitions.

mrunalp · 2015-10-15T22:03:57Z

This looks fine to me. Could we rearrange the commits a bit before we merge (once all comments are addressed) ?

crosbymichael · 2015-10-16T18:21:33Z

@LK4D4 @mrunalp added tests for state transitions and status

mrunalp · 2015-11-02T23:00:26Z

Needs rebase. Otherwise looks fine.

LK4D4 · 2015-11-02T23:12:19Z

Yeah, LGTM in advance
Sorry, I missing so much in mail flow

crosbymichael · 2015-11-16T19:01:35Z

@LK4D4 updated and squashed

crosbymichael · 2015-11-16T19:07:20Z

@mrunalp i squashed the commits for you ;)

mrunalp · 2015-11-16T22:26:52Z

@crosbymichael LGTM. I didn't really want you to have to squash to 1 :D (Just meant no big addition deletion over the series).

LK4D4 · 2015-11-16T22:27:39Z

@mrunalp yeah, maybe unsquash now...

crosbymichael · 2015-11-16T22:45:38Z

noooooo

Signed-off-by: Michael Crosby <[email protected]> Add state status() method Signed-off-by: Michael Crosby <[email protected]> Allow multiple checkpoint on restore Signed-off-by: Michael Crosby <[email protected]> Handle leave-running state Signed-off-by: Michael Crosby <[email protected]> Fix state transitions for inprocess Because the tests use libcontainer in process between the various states we need to ensure that that usecase works as well as the out of process one. Signed-off-by: Michael Crosby <[email protected]> Remove isDestroyed method Signed-off-by: Michael Crosby <[email protected]> Handling Pausing from freezer state Signed-off-by: Rajasekaran <[email protected]> freezer status Signed-off-by: Rajasekaran <[email protected]> Fixing review comments Signed-off-by: Rajasekaran <[email protected]> Added comment when freezer not available Signed-off-by: Rajasekaran <[email protected]> Signed-off-by: Michael Crosby <[email protected]> Conflicts: libcontainer/container_linux.go Change checkFreezer logic to isPaused() Signed-off-by: Michael Crosby <[email protected]> Remove state base and factor out destroy func Signed-off-by: Michael Crosby <[email protected]> Add unit test for state transitions Signed-off-by: Michael Crosby <[email protected]>

crosbymichael · 2016-01-04T17:56:22Z

ping @mrunalp @LK4D4 any more comments because I don't know what else to do with the commits

LK4D4 · 2016-01-04T17:58:40Z

@crosbymichael Yeah, it was a joke about unsquash.
LGTM from me.

mrunalp · 2016-01-04T17:59:23Z

LGTM

Implement Container States

style: remove unicode character

GordonTheTurtle added the status/0-triage label Oct 2, 2015

crosbymichael force-pushed the destory-state branch from c3e7e40 to 1b014f6 Compare October 2, 2015 21:57

rajasec reviewed Oct 3, 2015
View reviewed changes

mrunalp reviewed Oct 8, 2015
View reviewed changes

libcontainer/container_linux.go Outdated

Copy link

Contributor

mrunalp Oct 8, 2015

Choose a reason for hiding this comment

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

s/inprocess/in process/

crosbymichael force-pushed the destory-state branch from 1b014f6 to a232e4f Compare October 8, 2015 20:47

crosbymichael mentioned this pull request Oct 8, 2015

Fixing container restore issue due to different IDs #284

Closed

mrunalp reviewed Oct 9, 2015
View reviewed changes

crosbymichael mentioned this pull request Oct 12, 2015

Removing pause state from container errors #294

Closed

LK4D4 mentioned this pull request Oct 12, 2015

Adding status command to container #246

Closed

crosbymichael force-pushed the destory-state branch from a232e4f to e797abd Compare October 12, 2015 18:58

crosbymichael mentioned this pull request Oct 12, 2015

Handling Pausing from freezer state #218

Closed

LK4D4 reviewed Oct 15, 2015
View reviewed changes

libcontainer/container_linux.go Outdated

Copy link

Contributor

LK4D4 Oct 15, 2015

Choose a reason for hiding this comment

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

Looks like you don't need else here

crosbymichael force-pushed the destory-state branch from c080be2 to 6afa5f3 Compare October 15, 2015 21:42

crosbymichael force-pushed the destory-state branch from 1ad4e22 to 31f7b06 Compare November 16, 2015 19:01

rajasec mentioned this pull request Nov 24, 2015

container pause status #217

Closed

crosbymichael force-pushed the destory-state branch 2 times, most recently from f8ac1b5 to 03f07c8 Compare December 9, 2015 22:00

crosbymichael force-pushed the destory-state branch from 03f07c8 to 4415446 Compare December 17, 2015 21:55

hqhq mentioned this pull request Dec 24, 2015

Some fixes about memory stats #418

Closed

mrunalp pushed a commit that referenced this pull request Jan 4, 2016

Merge pull request #311 from crosbymichael/destory-state

fa24ebf

Implement Container States

mrunalp merged commit fa24ebf into opencontainers:master Jan 4, 2016

crosbymichael deleted the destory-state branch January 4, 2016 18:01

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017

Merge pull request opencontainers#311 from vbatts/unicode

d61af70

style: remove unicode character

Implement Container States #311

Implement Container States #311

Uh oh!

Conversation

crosbymichael commented Oct 2, 2015

Uh oh!

mrunalp commented Oct 2, 2015

Uh oh!

mrunalp commented Oct 2, 2015

Uh oh!

rajasec Oct 3, 2015

Choose a reason for hiding this comment

Uh oh!

crosbymichael Oct 5, 2015

Choose a reason for hiding this comment

Uh oh!

rajasec commented Oct 3, 2015

Uh oh!

rajasec Oct 3, 2015

Choose a reason for hiding this comment

Uh oh!

crosbymichael Oct 5, 2015

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Oct 5, 2015

Uh oh!

mrunalp Oct 8, 2015

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Oct 8, 2015

Uh oh!

mrunalp Oct 9, 2015

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Oct 12, 2015

Uh oh!

crosbymichael commented Oct 15, 2015

Uh oh!

LK4D4 Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Oct 15, 2015

Uh oh!

crosbymichael commented Oct 15, 2015

Uh oh!

LK4D4 commented Oct 15, 2015

Uh oh!

mrunalp commented Oct 15, 2015

Uh oh!

crosbymichael commented Oct 16, 2015

Uh oh!

mrunalp commented Nov 2, 2015

Uh oh!

LK4D4 commented Nov 2, 2015

Uh oh!

crosbymichael commented Nov 16, 2015

Uh oh!

crosbymichael commented Nov 16, 2015

Uh oh!

mrunalp commented Nov 16, 2015

Uh oh!

LK4D4 commented Nov 16, 2015

Uh oh!

crosbymichael commented Nov 16, 2015

Uh oh!

crosbymichael commented Jan 4, 2016

Uh oh!

LK4D4 commented Jan 4, 2016

Uh oh!

mrunalp commented Jan 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants