Skip to content
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.

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

Add 'status' to state.go #485

Merged
merged 1 commit into from
Jun 3, 2016
Merged

Add 'status' to state.go #485

merged 1 commit into from
Jun 3, 2016

Conversation

duglin
Copy link
Contributor

@duglin duglin commented Jun 2, 2016

Forgot to do this in previous PR.

Signed-off-by: Doug Davis [email protected]

Forgot to do this in previous PR.

Signed-off-by: Doug Davis <[email protected]>
@wking
Copy link
Contributor

wking commented Jun 2, 2016 via email

@mrunalp
Copy link
Contributor

mrunalp commented Jun 3, 2016

Wondering if having this be an enum might be better.

@duglin
Copy link
Contributor Author

duglin commented Jun 3, 2016

can implementations still extend it?

@mrunalp
Copy link
Contributor

mrunalp commented Jun 3, 2016

@duglin I guess nothing is stopping implementations as long as they support at the least the ones that we have in the spec. That said, I don't see why we wouldn't add any reasonable statuses to the spec.

@duglin
Copy link
Contributor Author

duglin commented Jun 3, 2016

immediately runc will extend it with things like 'paused'.

@wking
Copy link
Contributor

wking commented Jun 3, 2016

On Thu, Jun 02, 2016 at 06:51:36PM -0700, Doug Davis wrote:

immediately runc will extend it with things like 'paused'.

runC has pause/resume, and in that context “paused” makes sense. But
without such operations in the spec, they're probably not “reasonable
statuses” to add here (how would you define them?). That is an
argument for “runC will probably want a different status set than we
have here”, and options to support that are probably:

a. Tell runC (and other runtimes) to put its augmented status
(e.g. including “paused”) in a different property, and stick to the
spec's vanilla statuses for the now-official ‘status’ property.
b. Allow runC (and other runtimes) to extend the ‘status’ property
with out-of-spec values.

(a) is unfortunately noisy, but workable in the face of lifecycle
extensions like pause/resume. I think (b) is a bad idea, because a
standards-oriented ‘state’ caller won't know how to handle “paused”
(does that mean I can call ‘start’?).

@mrunalp
Copy link
Contributor

mrunalp commented Jun 3, 2016

If a runtime can add states, then it can also add state transitions to support those states. If one is using runc per the standard, then they wouldn't get into a paused state, so they could stick with what is in the spec. I think this wouldn't be a big issue in practice.

@wking
Copy link
Contributor

wking commented Jun 3, 2016

On Fri, Jun 03, 2016 at 08:09:00AM -0700, Mrunal Patel wrote:

If one is using runc per the standard, then they wouldn't get into a
paused state, so they could stick with what is in the spec.

This assumes that the caller is always using OCI-oriented tools or
runC-oriented tools. If they mix and match, the runC-oriented tools
could pause the container, and the OCI-oriented tools would be
confused by the resulting status.

@crosbymichael
Copy link
Member

crosbymichael commented Jun 3, 2016

LGTM

Approved with PullApprove

@crosbymichael crosbymichael added this to the 1.0.0 milestone Jun 3, 2016
@vbatts
Copy link
Member

vbatts commented Jun 3, 2016

LGTM

Approved with PullApprove

@vbatts vbatts merged commit eeeecb0 into opencontainers:master Jun 3, 2016
@mrunalp
Copy link
Contributor

mrunalp commented Jun 3, 2016

We never did conclude whether to go with string or enum before merging...

@crosbymichael
Copy link
Member

I'd say string for now because we only define 3 in the spec and allow for runtime defined states

@duglin duglin deleted the FixStatus branch July 20, 2016 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants