Add a 'status' field to our state struct by duglin · Pull Request #462 · opencontainers/runtime-spec

duglin · 2016-05-27T00:32:13Z

When thinking about kill() vs delete(), a client needs to know (gracefully - w/o errors) when to call each. Which means it needs to be able to query the container to know what its current state is (running vs stopped). This defines a property in our 'state' struct to convey that info.

Signed-off-by: Doug Davis dug@us.ibm.com

crosbymichael · 2016-05-27T00:34:08Z

 {
 "ociVersion": "0.2.0",
 "id": "oci-container1",
+	"status": "running",


formatting issue

tabs... the root of all evil

crosbymichael · 2016-05-27T00:53:30Z

LGTM

wking · 2016-05-27T20:48:12Z

I commented via email last night, but GitHub seems to have eaten my
comment. Here it is again:

I'm opposed.

There is no race-free way to do this gracefully without errors, since
“‘state’, process changes, take action based on stale state” is always
possible. If a client wants to start a container, just run ‘start’
and catch errors. The same approach works for the other commands,
with the additional complication of a wait-before-delete dance like
@julz outlined in 1.

duglin · 2016-05-27T21:08:01Z

@wking the intent isn't to remove any chance of an error, the intent is to make it so that an error isn't part of the normal workflow just to figure out the state of a container.

wking · 2016-05-27T21:27:48Z

On Fri, May 27, 2016 at 02:08:03PM -0700, Doug Davis wrote:

@wking the intent isn't to remove any chance of an error, the intent
is to make it so that an error isn't part of the normal workflow
just to figure out the state of a container.

So “decrease the likelyhood of generating errors” (and I agree that
this change would do that). Why is that useful? Is there a problem
with errors?

I think that the runtime needs to be careful in this case (e.g. I
don't like this risk of sending the ‘start’ signal to user-specified
code 1), but at the level of abstraction exposed by the spec
operations I see no such risks. That doesn't mean that the risk isn't
there, but I think we should identify it (or some other use-case) more
clearly before landing a new field in the state.

mrunalp · 2016-05-31T16:40:04Z

I would prefer a status operation over a field in the state file.

duglin · 2016-05-31T16:49:49Z

@mrunalp we already have one: https://github.com/duglin/runtime-spec/blob/master/runtime.md#query-state
The problem is that it doesn't return enough info and I doubt we want 2 ops that return almost the same info.

mrunalp · 2016-05-31T17:00:10Z

I meant status as a separate op that just returns the current run status of the container.

Sent from my iPhone

On May 31, 2016, at 9:49 AM, Doug Davis notifications@github.com wrote:
@mrunalp we already have one: https://github.com/duglin/runtime-spec/blob/master/runtime.md#query-state
The problem is that it doesn't return enough info and I doubt we want 2 ops that return almost the same info.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

dqminh · 2016-05-31T17:02:55Z

LGTM.

@duglin @wking AFAIK this is most useful when use for things like inspect, not really for When thinking about kill() vs delete(), a client needs to know (gracefully - w/o errors) when to call each. though, as runtime should try to make those operations idempotent.

crosbymichael · 2016-05-31T17:04:32Z

@mrunalp why would another op that just returned the status be better than the state op that returns status along with the other info?

wking · 2016-05-31T17:19:06Z

On Tue, May 31, 2016 at 10:02:56AM -0700, Daniel, Dao Quang Minh wrote:

@duglin @wking AFAIK this is most useful when use for things like
inspect…

This is getting into very fuzzy territory to me. “I want to know
$PROPERTY because I want to know $PROPERTY” is hard to argue
for/against ;). I'd rather keep the spec restricted to “I want to
know $PROPERTY because I neet it to take $ACTION” is much easier to
reason about (do we want to support $ACTION? Do you really need
$PROPERTY to do it?). So, what sort of actions does this information
let you take?

… not really for When thinking about kill() vs delete()… though,
as runtime should try to make those operations idempotent.

At least with the current spec, delete is not idempotent 1:

Attempting to delete a container that does not exist MUST generate
an error.

mrunalp · 2016-05-31T17:23:45Z

@crosbymichaelstate is static whereas the running status of a process is more dynamic. The information in the state file could get stale.

mrunalp · 2016-05-31T17:25:48Z

I think it will be hard to keep the status field in the state file correct and queries race free.

crosbymichael · 2016-05-31T17:26:57Z

@mrunalp we are not talking about cat'ing a file. The state response is dynamic as well and the status is correct it is returned. Nothing static about it in the spec or in the runc implementation.

duglin · 2016-05-31T17:28:26Z

the spec doesn't mention state files any more - we put it all behind an op.

mrunalp · 2016-05-31T17:32:30Z

@crosbymichael Agree if the state isn't persisted in a file in a implementation (but anyway that is up to the implementation).

mrunalp · 2016-05-31T17:33:07Z

@duglin Needs rebase.

duglin · 2016-05-31T17:35:40Z

I think git is acting up again - there were not conflicts at all so the rebase shouldn't have been necessary. Very odd.

wking · 2016-05-31T17:37:49Z

+The value MAY be one of:
+ * `created` : the container has been created but the user-specified process has not yet been executed
+ * `running` : the container has been created and the user-specified process is running
+ * `stopped` : the container has been created and the user-specified process has been executed but is no longer running


If we land a status property (I'm still opposed), I think we want to use dead here to mirror proc(5) and ps(1). More details on why I don't like “stopped” in #465.

Signed-off-by: Doug Davis <dug@us.ibm.com>

wking · 2016-05-31T17:42:28Z

+* **`status`**: (string) is the runtime state of the container.
+The value MAY be one of:
+ * `created` : the container has been created but the user-specified process has not yet been executed
+ * `running` : the container has been created and the user-specified process is running


I'd rather have this be “the container process has executed user-specified code has not exited”. In Linux, in any case “running” is a technical term for one possible process state, and the status you're shooting for here also covers “sleeping”, “waiting”, and “stopped”.

duglin · 2016-05-31T17:43:13Z

oh man - had to rebase again. Github is really acting up.

crosbymichael · 2016-05-31T17:46:39Z

LGTM

mrunalp · 2016-05-31T17:47:45Z

LGTM

…ecycle After unifying the pre- and post-split hook lifecycle information (this commit's first parent), merge master to pull in subsequent mainline evolution. Conflicts: runtime.md The conflict was a trivial collision with dd0cd21 (Add a 'status' field to our state struct, 2016-05-26, opencontainers#462). Signed-off-by: W. Trevor King <wking@tremily.us>

In [1], I'd proposed replacing our old "user-specified process" with "user-specified code" to help distinguish between 'create' (cloning the container process) and 'start' (signaling the container process to execve (or similar) the user-specified $STUFF_FROM_THE_process_CONFIG rejected, although the renaming proposed there had already landed via dd0cd21 (Add a 'status' field to our state struct, 2016-05-26, opencontainers#462). This PR attempts to find a common ground between "process" (preferred by maintainers in opencontainers#466 [2,3,4], but which I consider incorrect [5]) and "code" (which maintainers found confusing [3,4,6]). The Linux execve(2) has says "program" and unpacks that to "a binary executable, or a script starting with a [shebang]" [7]. proc(5) documents /proc/[pid]/exe by talking about "the executed command" [8]. The POSIX exec docs call this the "process image" and talk about loading it from the "new process image file" [9]. POSIX formally defines "command" [11], "executable file" [12], and "program" [13]. The only reference to "process image" in the definitions is in the "executable file" entry. The "command" definition is focused on the shell, the "executable file" definition is focused on files, and the "program" definition talks about a "prepared sequence of instructions to the system", so "program" seems like the best fit. [1]: opencontainers#466 Subject: runtime: Replace "user-specified process" with "user-specified code" in 'create' [2]: opencontainers#466 (comment) [3]: opencontainers#466 (comment) [4]: opencontainers#466 (comment) [5]: opencontainers#466 (comment) [6]: opencontainers#466 (comment) [7]: http://man7.org/linux/man-pages/man2/execve.2.html [8]: http://man7.org/linux/man-pages/man5/proc.5.html [9]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html [10]: https://git.kernel.org/cgit/docs/man-pages/man-pages.git/ [11]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_104 [12]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_154 [13]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_306 Signed-off-by: W. Trevor King <wking@tremily.us>

In [1], I'd proposed replacing our old "user-specified process" with "user-specified code" to help distinguish between 'create' (cloning the container process) and 'start' (signaling the container process to execve or similar the user-specified $STUFF_FROM_THE_process_CONFIG). That PR was rejected, although the renaming proposed there had already landed via dd0cd21 (Add a 'status' field to our state struct, 2016-05-26, opencontainers#462). This PR attempts to find a common ground between "process" (preferred by maintainers in opencontainers#466 [2,3,4], but which I consider incorrect [5]) and "code" (which maintainers found confusing [3,4,6]). The Linux execve(2) has says "program" and unpacks that to "a binary executable, or a script starting with a [shebang]" [7]. proc(5) documents /proc/[pid]/exe by talking about "the executed command" [8]. The POSIX exec docs call this the "process image" and talk about loading it from the "new process image file" [9]. POSIX formally defines "command" [11], "executable file" [12], and "program" [13]. The only reference to "process image" in the definitions is in the "executable file" entry. The "command" definition is focused on the shell, the "executable file" definition is focused on files, and the "program" definition talks about a "prepared sequence of instructions to the system", so "program" seems like the best fit. [1]: opencontainers#466 Subject: runtime: Replace "user-specified process" with "user-specified code" in 'create' [2]: opencontainers#466 (comment) [3]: opencontainers#466 (comment) [4]: opencontainers#466 (comment) [5]: opencontainers#466 (comment) [6]: opencontainers#466 (comment) [7]: http://man7.org/linux/man-pages/man2/execve.2.html [8]: http://man7.org/linux/man-pages/man5/proc.5.html [9]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html [10]: https://git.kernel.org/cgit/docs/man-pages/man-pages.git/ [11]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_104 [12]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_154 [13]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_306 Signed-off-by: W. Trevor King <wking@tremily.us>

In [1], I'd proposed replacing our old "user-specified process" with "user-specified code" to help distinguish between 'create' (cloning the container process) and 'start' (signaling the container process to execve or similar the user-specified $STUFF_FROM_THE_process_CONFIG). That PR was rejected, although the renaming proposed there had already landed via dd0cd21 (Add a 'status' field to our state struct, 2016-05-26, opencontainers#462). This PR attempts to find a common ground between "process" (preferred by maintainers in opencontainers#466 [2,3,4], but which I consider incorrect [5]) and "code" (which maintainers found confusing [3,4,6]). The Linux execve(2) says "program" and unpacks that to "a binary executable, or a script starting with a [shebang]" [7]. proc(5) documents /proc/[pid]/exe by talking about "the executed command" [8]. The POSIX exec docs call this the "process image" and talk about loading it from the "new process image file" [9]. POSIX formally defines "command" [11], "executable file" [12], and "program" [13]. The only reference to "process image" in the definitions is in the "executable file" entry. The "command" definition is focused on the shell, the "executable file" definition is focused on files, and the "program" definition talks about a "prepared sequence of instructions to the system", so "program" seems like the best fit. [1]: opencontainers#466 Subject: runtime: Replace "user-specified process" with "user-specified code" in 'create' [2]: opencontainers#466 (comment) [3]: opencontainers#466 (comment) [4]: opencontainers#466 (comment) [5]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_295 [6]: opencontainers#466 (comment) [7]: http://man7.org/linux/man-pages/man2/execve.2.html [8]: http://man7.org/linux/man-pages/man5/proc.5.html [9]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html [10]: https://git.kernel.org/cgit/docs/man-pages/man-pages.git/ [11]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_104 [12]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_154 [13]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_306 Signed-off-by: W. Trevor King <wking@tremily.us>

In [1], I'd proposed replacing our old "user-specified process" with "user-specified code" to help distinguish between 'create' (cloning the container process) and 'start' (signaling the container process to execve or similar the user-specified $STUFF_FROM_THE_process_CONFIG). That PR was rejected, although the renaming proposed there had already landed via dd0cd21 (Add a 'status' field to our state struct, 2016-05-26, opencontainers#462). This PR attempts to find a common ground between "process" (preferred by maintainers in opencontainers#466 [2,3,4], but which I consider incorrect [5]) and "code" (which maintainers found confusing [3,4,6]). The Linux execve(2) says "program" and unpacks that to "a binary executable, or a script starting with a [shebang]" [7]. proc(5) documents /proc/[pid]/exe by talking about "the executed command" [8]. The POSIX exec docs call this the "process image" and talk about loading it from the "new process image file" (although they also sprinkle in a number of “program” references, apparently interchangeably with “process image”) [9]. POSIX formally defines "command" [11], "executable file" [12], and "program" [13]. The only reference to "process image" in the definitions is in the "executable file" entry. The "command" definition is focused on the shell, the "executable file" definition is focused on files, and the "program" definition talks about a "prepared sequence of instructions to the system", so "program" seems like the best fit. [1]: opencontainers#466 Subject: runtime: Replace "user-specified process" with "user-specified code" in 'create' [2]: opencontainers#466 (comment) [3]: opencontainers#466 (comment) [4]: opencontainers#466 (comment) [5]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_295 [6]: opencontainers#466 (comment) [7]: http://man7.org/linux/man-pages/man2/execve.2.html [8]: http://man7.org/linux/man-pages/man5/proc.5.html [9]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html [10]: https://git.kernel.org/cgit/docs/man-pages/man-pages.git/ [11]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_104 [12]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_154 [13]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_306 Signed-off-by: W. Trevor King <wking@tremily.us>

…ecycle After unifying the pre- and post-split hook lifecycle information (this commit's first parent), merge master to pull in subsequent mainline evolution. Conflicts: runtime.md The conflicts were primarily due to: * dd0cd21 (Add a 'status' field to our state struct, 2016-05-26, opencontainers#462). * 98f0bdf (Add some related docs links, 2016-10-25, opencontainers#596). * c45ffb4 (*: Replace "user-specified code" with "user-specified program", 2016-11-18, opencontainers#629). Signed-off-by: W. Trevor King <wking@tremily.us>

crosbymichael reviewed May 27, 2016
View reviewed changes

duglin force-pushed the AddStatus branch from 356ba8d to ad82cffCompare May 27, 2016 00:35

duglin force-pushed the AddStatus branch from ad82cff to 94ef569Compare May 31, 2016 13:09

duglin force-pushed the AddStatus branch from 94ef569 to 42d3fd5Compare May 31, 2016 17:35

wking reviewed May 31, 2016
View reviewed changes

duglin force-pushed the AddStatus branch from 42d3fd5 to b378e9eCompare May 31, 2016 17:40

Add a 'status' field to our state struct
dd0cd21
Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the AddStatus branch from b378e9e to dd0cd21Compare May 31, 2016 17:42

wking reviewed May 31, 2016
View reviewed changes

mrunalp merged commit 8f02d00 into opencontainers:masterMay 31, 2016

This was referenced May 31, 2016
runtime: Replace "process is stopped" with "process exits" #465
Merged
Add 'status' to state.go #485
Merged
state's status detection (based on PID lookups) is imprecise opencontainers/runc#871
Closed

duglin deleted the AddStatus branch July 20, 2016 10:53

wking mentioned this pull request Nov 4, 2016
runtime: Replace "user-specified process" with "user-specified code" in 'create' #466
Closed

wking mentioned this pull request Nov 18, 2016
*: Replace “user-specified code” with “user-specified program” #629
Merged

wking mentioned this pull request Jan 23, 2017
runtime: Clarify "new runtime states" and the flexibility of state status? #669
Open

Uh oh!

Conversation

duglin commented May 27, 2016

Uh oh!

crosbymichaelMay 27, 2016

Choose a reason for hiding this comment

Uh oh!

duglinMay 27, 2016

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented May 27, 2016• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented May 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duglin commented May 27, 2016

Uh oh!

wking commented May 27, 2016

Uh oh!

mrunalp commented May 31, 2016

Uh oh!

duglin commented May 31, 2016

Uh oh!

mrunalp commented May 31, 2016

Uh oh!

dqminh commented May 31, 2016• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crosbymichael commented May 31, 2016

Uh oh!

wking commented May 31, 2016

Uh oh!

mrunalp commented May 31, 2016

Uh oh!

mrunalp commented May 31, 2016

Uh oh!

crosbymichael commented May 31, 2016

Uh oh!

duglin commented May 31, 2016

Uh oh!

mrunalp commented May 31, 2016

Uh oh!

mrunalp commented May 31, 2016

Uh oh!

duglin commented May 31, 2016

Uh oh!

wkingMay 31, 2016

Choose a reason for hiding this comment

Uh oh!

wkingMay 31, 2016

Choose a reason for hiding this comment

Uh oh!

duglin commented May 31, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crosbymichael commented May 31, 2016• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrunalp commented May 31, 2016• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

crosbymichael commented May 27, 2016•
edited by caniszczyk
Loading

wking commented May 27, 2016•
edited
Loading

dqminh commented May 31, 2016•
edited by caniszczyk
Loading

duglin commented May 31, 2016•
edited
Loading

crosbymichael commented May 31, 2016•
edited by caniszczyk
Loading

mrunalp commented May 31, 2016•
edited by caniszczyk
Loading