runtime: Add 'creating' to state status

[wking]

To distinguish between "we're still setting this container up" and
"we're finished setting up; you can call 'start' if you like".

Also reference the lifecycle steps, because you can't be too explicit

Also relax the pid requirement from “always” to “only for created
and running”. During the creating phase we may not have a
container process yet (e.g. if we're still reading the configuration
or setting up cgroups), and in the stopped phase the PID is no
longer meaningful.

Some discussion with @mikebrow around this starting here.

This PR builds on #465, so that should get reviewed first.

[wking]

This is a breaking change for callers who depend on the initial status being created, so it might restart the rc counter (assuming that passes).

On the other hand, we allow runtimes to add other statuses as they see fit (which I don't like), so callers should arguably be (somehow) prepared to gracefully accept unknown statuses. In that case, we don't have a contract with the callers that this addition breaks, and the rc process can continue forward without restarting.

[mikebrow]

Looks good to me. However, this should be noted as a breaking change as creating is a newly discussed state. RunC 1.0rc does not currently include the creating state. Bit late in the release cycle to be adding new stuff.

[wking]

On Tue, Jul 12, 2016 at 07:29:24AM -0700, Mike Brown wrote:

However, this should be noted as a breaking change as creating is a
newly discussed state.

I agree 1, but I have trouble squaring that with our allowing
runtimes to add their own statuses [1,2].

RunC 1.0rc does not currently include the creating state. Bit late
in the release cycle to be adding new stuff.

If the consensus on state status extension is that this is a breaking
change, then I guess this would target 2.0. I'm not sure what the
planned timescale is for 2.0 (and we're still missing clarity on
exactly how versioning is supposed to work, opencontainers/tob#16),
but it seems unfortunate to push 1.0 through if we already know we
have breaking changes in the pipe.

[mikebrow]

By breaking I meant .. makes runC not compatible with the spec, not without a PR. I don't see this spec and runc as two different runtimes.

Maybe I'm reading it wrong. The text does say MAY be one of ... and does not say MUST include these..

[wking]

On Tue, Jul 12, 2016 at 01:20:38PM -0700, Mike Brown wrote:

By breaking I meant .. makes runC not compatible with the spec, not
without a PR.

This is probably true of many spec PRs ;). For instance, it is still
true of #164. The solution to that is to cut spec releases. Say this
lands in spec 1.0.0-rc2, runC just has to land support sometime
between now and when it claims to support 1.0.0-rc2.

I don't see this spec and runc as two different runtimes. Maybe I'm
reading it wrong. The text does say MAY be one of ... and does not
say MUST include these..

It does, I'm just not sure how clients are supposed to process the
state status (e.g. for compliance testing) if it MAY contain
unspecified strings [1,2]. The current spec wording is not clear
enough (to my eyes) on what “running”, etc., mean to push
runtime-extension statuses outside of what the compliance tester will
generate 3.

[tianon]

I think having a creating state of some kind makes some sense (although possibly more in the face of things like Hyper-V containers where resources take longer to allocate; cc @RobDolinMS), but I'm not sure what's actually going on in the PR contents (looks like there's quite a bit of unrelated change there).

I'd be curious about other maintainer's thoughts about whether a new state is warranted before refactoring though (is there an issue tracking this already?)

[crosbymichael]

@wking its hard to review any of your PRs because you cram so many changes into each one. Please start doing formatting and random changes in separate PRs so we can actually review the changes that you are proposing and not have to filter though all these random changes.

Seriously, just look at the diff and try to find the actual change for the new state.

[wking]

On Fri, Nov 04, 2016 at 03:48:28PM -0700, Tianon Gravi wrote:

… but I'm not sure what's actually going on in the PR contents
(looks like there's quite a bit of unrelated change there)…

I tried to break it up into reasonably atomic commits 1. I'm happy
rebase, reshuffle, or split them out into multiple PRs if any of the
ideas in there have legs.

I'd be curious about other maintainer's thoughts about whether a new
state is warranted before refactoring though (is there an issue
tracking this already?)

Not that I'm aware of.

[crosbymichael]

@wking we review PRs not commits as the PR is the unit that gets merged. Its good to split into separate commits to make view easier just as long as the commits are actually related to one another.

[wking]

On Fri, Nov 04, 2016 at 03:52PM -0700, Michael Crosby wrote:

Please start doing formatting and random changes in separate PRs…

I sat down to do this just now and noticed it was already done :p. As the topic post here points out, “This PR builds on #465, so that should get reviewed first”.

[wking]

Rebased onto the current #465 with 1a962c0 → d69038d.

[wking]

Rebased onto the current #465 with d69038d → cfc27a8.

[mrunalp]

We can address this post 1.0.

[wking]

Rebased onto master with 08d52a0 → 65d9d6b now that #465 has landed (so GitHub notices that the base commit is not part of this PR). I also dropped the tip commit since it's been spun off into #664. So the only think left in this PR is the commit adding the creating status.

[crosbymichael]

LGTM

[mrunalp]

LGTM