Add runtime state configuration and structs #87
Conversation
|
On Wed, Jul 29, 2015 at 02:10:46PM -0700, Michael Crosby wrote:
I think closing #41 (the state spec) is important, but I'm still not Can we restrict this PR to just address the spec content (#41), which |
| The directory structure for a container is `<root>/<containerID>/state.json`. | ||
|
|
||
| * **id** (string) ID is the container's ID. | ||
| * **pid** (int) Pid is the main processes id within the container. |
There was a problem hiding this comment.
“processes” → “process's”. And also probably “Pid” → “PID” (at least, that's what kill(1) uses) and “id” → “ID” (that's what chown(1) uses).
|
The state content here looks good to me (modulo a few minor copy-edits
Folks might also be interested in things like the TTY associated with |
|
@wking We are being conservative in what we add to the state right now. |
|
On Wed, Jul 29, 2015 at 03:59:11PM -0700, Mrunal Patel wrote:
That makes sense, but then why not leave cgroups and namespaces out of |
|
On Wed, Jul 29, 2015 at 04:11:49PM -0700, W. Trevor King wrote:
And the external file descriptors, since ‘lsof -p $PID’ should be able |
| On Linux based systems the state information is stored in `/run/oci`. | ||
| The directory structure for a container is `<root>/<containerID>/state.json`. | ||
|
|
||
| * **id** (string) ID is the container's ID. |
There was a problem hiding this comment.
id is a bit under specified; are there any restrictions on the format of the string? what is the uniqueness domain?
There was a problem hiding this comment.
How about all lowercase alphabets and digits?
There was a problem hiding this comment.
On Wed, Aug 05, 2015 at 09:19:02AM -0700, Mrunal Patel wrote:
+* id (string) ID is the container's ID.
How about all lowercase alphabets and digits?
That sounds good to me, although we might want to allow hyphens for
folks who want to use UUIDs. And currently runC defaults to the
bundle's directory name, so we'd need to figure out how we wanted to
map other characters into the valid character set.
Do we want to add requirements for the uniqueness domain? Unique to
the host seems like a minimum, but we can require universal uniqueness
(e.g. “container IDs must be UUIDs 1”) or leave that up to the
implementation. I'd rather leave it up to the implementation, since
as long as UUIDs are valid container identifiers, runtime callers
that want universal uniqueness are free to use them.
There was a problem hiding this comment.
We need to add a comment that the reason we have an ID is because the hooks only get an open fd to this json document so they won't know the unique ID which may be useful for removal, etc.
There was a problem hiding this comment.
Also need to note that containerID MUST match the containerID subdirectory field.
There was a problem hiding this comment.
On Wed, Aug 26, 2015 at 10:12:52AM -0700, Brandon Philips wrote:
+* id (string) ID is the container's ID.
We need to add a comment that the reason we have an ID is because
the hooks only get an open fd to this json document so they won't
know the unique ID which may be useful for removal, etc.
I wasn't clear on how it would be useful for removal. Everyone got on
board when someone mentioned network teardown, but the connection
between that and the container ID wasn't clear to me. Not that we
have to be completely specific when motivating this entry. I'm just
pointing out that if we want to explain why we need the container ID
in a post-stop hook, we might want to provide more detail than we had
in today's meeting.
|
any other comments, questions, concerns? |
|
On Tue, Aug 04, 2015 at 11:23:02AM -0700, Michael Crosby wrote:
If that's “can we merge this now?”, I'd suggest at least addressing I still think the issue of “how much more than the main PID do we |
|
@wking Yes, I know a little bit about contributing to open source, I like to get additional feedback before I start addressing the comments so I don't have to update, push, feedback, update, push, feedback multiple times. |
|
|
||
| * **id** (string) ID is the container's ID. | ||
| * **pid** (int) Pid is the main processes id within the container. | ||
| * **root** (string) Root is the path to the container's root filesystem specified in the configuration. |
There was a problem hiding this comment.
Maybe replace “$ENTRY ($TYPE) $ENTRY is the …” with “$ENTRY ($TYPE) The …”.
|
On Tue, Aug 04, 2015 at 12:40:41PM -0700, Michael Crosby wrote:
That makes sense, although rebase/push is reasonably cheap when While I'm summarizing issues that could be addressed in a reroll 1, |
|
I updated this PR addressing comments and rebasing. |
|
On Wed, Aug 05, 2015 at 01:09:49PM -0700, Michael Crosby wrote:
Do we have a reason for not moving LinuxState from spec_linux.go to |
|
@wking because I don't want alot of files |
|
On Wed, Aug 05, 2015 at 01:23:35PM -0700, W. Trevor King wrote:
And I didn't mention this explicitly on the first pass, but I'd move |
|
On Wed, Aug 05, 2015 at 01:09:49PM -0700, Michael Crosby wrote:
Also no longer visible after the rebase 1: Maybe replace “$ENTRY ($TYPE) $ENTRY is the …” with “$ENTRY ($TYPE) The …”. |
|
On Wed, Aug 05, 2015 at 01:27:58PM -0700, Michael Crosby wrote:
Then why not squash state.go into spec.go? I think “we don't separate |
| @@ -78,3 +78,13 @@ type Hook struct { | |||
| Args []string `json:"args"` | |||
| Env []string `json:"env"` | |||
| } | |||
|
|
|||
| // State holds information about the runtime state of the container | |||
There was a problem hiding this comment.
We introduce the Spec entry with:
Spec is the base configuration for the container. It specifies platform independent configuration.
Since the State type is the root of a new JSON structure, independent of Spec, we may want to echo that here with something like:
State is the base type for the container's runtime state. It specifies platform independent state.
Or something like that to help folks avoid confusing the spec types and the state types.
|
Yes, i moved state into the spec.go file now |
|
|
||
| * **namespaces** Paths to the Linux namespaces setup for the container. | ||
| * **cgroups** Paths to the container's cgroups. | ||
| * **externalFds** Paths to the container's open file descriptors. |
There was a problem hiding this comment.
what is the utility of this? Why can't it be derived from the pid?
There was a problem hiding this comment.
On Wed, Aug 05, 2015 at 03:38:33PM -0700, Brandon Philips wrote:
+Linux systems add some platform specific information to the state.
+
+* namespaces Paths to the Linux namespaces setup for the container.
+* cgroups Paths to the container's cgroups.
+* externalFds Paths to the container's open file descriptors.what is the utility of this? Why can't it be derived from the pid?
+1 to dropping these (previous discussion in [1,2,3,4]).
There was a problem hiding this comment.
@philips @crosbymichael @avagin
For checkpoint restore, we need to pass the external pathnames to CRIU via --ext-mount-map.
Docker containers, for example, currently have two sets of external paths: (1) statically named external bind mounts /etc/hostname, /etc/hosts, and /etc/resolv.conf which are in config.json as HostnamePath, HostsPath, and ResolvConfPath (2) dynamically named external volumes (which may or may not exist for a container) and are listed under Volumes.
In short, for checkpoint restore via CRIU, the runtime state configuration has to have all external pathnames.
This adds runtime state information for oci container's so that it can be persisted and used by external tools. Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
|
Updated |
|
On Wed, Sep 02, 2015 at 11:16:05AM -0700, Michael Crosby wrote:
With a79d565→180df9d, LinuxState and ExternalFds have been punted.
All the other changes I'm interested in are wording changes that don't |
|
LGTM |
Add runtime state configuration and structs
Michael was adding these to existing Go files to avoid having a lot of files [1]. But 7232e4b (specs: introduce the concept of a runtime.json, 2015-07-30, opencontainers#88) split the runtime-configuration into two file sets (one rooted in config.go for config.json and another rooted in runtime_config.go for runtime.json). In the face of a two-set split for a single task (feeding the runtime while it manipulates the container lifecycle), it seems odd to push the type definition for a completely different task (sharing container/application state with other tools) into an existing file set. [1]: opencontainers#87 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
The version field was added while 180df9d (Add runtime state configuration and structs, 2015-07-29, opencontainers#87) was in-flight [1], and it missed getting documented in the example. [1]: opencontainers#87 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
Allow the runtime to use it's own scheme, but let the caller use UUIDs if they want. Jonathan asked for clarification as part of opencontainers#87, but didn't suggest a particular approach [1]. When we discussed it in the 2015-08-26 meeting [2], the consensus was to just allow everything. With container IDs like 'a/b/c' leading to state entries like '/var/oci/containers/a/b/c/state.json'. But that could get ugly with container IDs that contain '../' etc. And perhaps there are some filesystems out there that cannot represent ASCII characters (actually, I'm not even sure what charset our JSON is in ;). I'd rather pick this minimal charset which can handle UUIDs, and make life easy for runtime implementers and safe for bundle consumers at a slight cost of flexibility for bundle-authors. [1]: opencontainers#87 (comment) [2]: https://github.com/opencontainers/specs/wiki/Meeting-Minutes-2015-08-26 Reported-by: Jonathan Boulle <jonathanboulle@gmail.com> Signed-off-by: W. Trevor King <wking@tremily.us>
Allow the runtime to use it's own scheme, but let the caller use UUIDs if they want. Jonathan asked for clarification as part of opencontainers#87, but didn't suggest a particular approach [1]. When we discussed it in the 2015-08-26 meeting [2], the consensus was to just allow everything. With container IDs like 'a/b/c' leading to state entries like '/var/oci/containers/a/b/c/state.json'. But that could get ugly with container IDs that contain '../' etc. And perhaps there are some filesystems out there that cannot represent non-ASCII characters (actually, I'm not even sure what charset our JSON is in ;). I'd rather pick this minimal charset which can handle UUIDs, and make life easy for runtime implementers and safe for bundle consumers at a slight cost of flexibility for bundle-authors. [1]: opencontainers#87 (comment) [2]: https://github.com/opencontainers/specs/wiki/Meeting-Minutes-2015-08-26 Reported-by: Jonathan Boulle <jonathanboulle@gmail.com> Signed-off-by: W. Trevor King <wking@tremily.us>
Allow the runtime to use it's own scheme, but let the caller use UUIDs if they want. Jonathan asked for clarification as part of opencontainers#87, but didn't suggest a particular approach [1]. When we discussed it in the 2015-08-26 meeting [2], the consensus was to just allow everything. With container IDs like 'a/b/c' leading to state entries like '/var/oci/containers/a/b/c/state.json'. But that could get ugly with container IDs that contain '../' etc. And perhaps there are some filesystems out there that cannot represent non-ASCII characters (actually, I'm not even sure what charset our JSON is in ;). I'd rather pick this minimal charset which can handle UUIDs, and make life easy for runtime implementers and safe for bundle consumers at a slight cost of flexibility for bundle-authors. There was some confusion on the list about what "ASCII letters" meant [3], so I've explicitly listed the allowed character ranges. Here's a Python 3 script that shows the associated Unicode logic: import unicodedata # http://www.unicode.org/reports/tr44/tr44-4.html#GC_Values_Table category = { 'Ll': 'lowercase letter', 'Lu': 'uppercase letter', 'Nd': 'decimal number', 'Pd': 'dash punctuation', } for i in range(1<<7): char = chr(i) abbr = unicodedata.category(char) if abbr[0] in ['L', 'N'] or abbr == 'Pd': cat = category[abbr] print('{:02x} {} {}'.format(i, char, cat)) [1]: opencontainers#87 (comment) [2]: https://github.com/opencontainers/specs/wiki/Meeting-Minutes-2015-08-26 [3]: https://groups.google.com/a/opencontainers.org/d/msg/dev/P9gZBYhiqDE/-ptpOcQ5FwAJ Message-Id: <7ec9cff6-c1a6-4beb-82de-16eb412bf2f8@opencontainers.org> Reported-by: Jonathan Boulle <jonathanboulle@gmail.com> Signed-off-by: W. Trevor King <wking@tremily.us>
The version field was added while 180df9d (Add runtime state configuration and structs, 2015-07-29, opencontainers#87) was in-flight [1], and it missed getting documented in the example. [1]: opencontainers#87 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
The version field was added while 180df9d (Add runtime state configuration and structs, 2015-07-29, opencontainers#87) was in-flight [1], and it missed getting documented in the example. [1]: opencontainers#87 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
This gives us an easy way to share state JSON (because sometimes the PID is insufficient, e.g. on Linux you may need externalFds for efficient checkpointing [1,2]) [3]. Possible alternatives for transmitting state information, and why I feel this approach is superior: * Writing a state file from a pre-start hook [4]. This is pretty close to the --state option, but the --state option allows callers to recover the JSON via a named pipe like /dev/fd/3. That sort of direct connection would be trickier to setup with a hook-based approach. Pre-start and post-stop hooks may still be the best solution for (de)registering a container with a monitoring service, but that's not quite the same application. * Writing a state file to a global directory [5]. Atomic changes, change-monitoring, garbage collection, and ownership/access issues on a global directory are all hard to get right (or have ambiguous values of "right") [6]. * Requiring runtimes to maintain an internal registry of containers they launch [7]. This gives runtimes more flexibility than having a single, global directory. But ownership/access issues are still difficult (if one unprivileged user registers a container, can another unprivileged user see that entry? What elevated permissions would you need to see that entry? To remove that entry?). And the easiest way to get atomic changes and garbage-collection is by registering with a daemon, while not requiring a daemon is currently the #1 feature listed on [8]. In the event that any of those arguments seem leaky, callers that prefer a different approach can easily use hooks (without setting --state) or write wrappers that use a named pipe approach like (--state /dev/fd/3) to collect the JSON and then write it to their preferred registry. So the --state approach seems easy for the runtime to implement reliably, and also compatible with any of the suggested alternatives. The converse is not true; requiring a write to a global or per-runtime registry is not compatible with use-cases that prefer the anonymity of not writing the state at all (which is possible just by leaving off the --state option). [1]: opencontainers/runtime-spec#87 (comment) [2]: https://groups.google.com/a/opencontainers.org/d/msg/dev/z25xQsF3pHA/ixyeTrxyFwAJ Subject: Re: Drop /run/opencontainer entirely? Date: Fri, 4 Sep 2015 21:30:24 -0700 Message-ID: <20150905043024.GB25638@odin.tremily.us> [3]: https://groups.google.com/a/opencontainers.org/d/msg/dev/q6TYqVZOcX8/W1RVyCXCCQAJ Subject: Re: removal of /run/opencontainer/containers, add --state? Date: Tue, 8 Dec 2015 15:49:57 -0800 Message-ID: <20151208234957.GK2767@odin.tremily.us> [4]: https://groups.google.com/a/opencontainers.org/d/msg/dev/q6TYqVZOcX8/GQs0zkRHBwAJ Subject: Re: removal of /run/opencontainer/containers Date: Mon, 30 Nov 2015 13:55:40 -0800 Message-ID: <20151130215540.GF23595@odin.tremily.us> [5]: https://github.com/opencontainers/specs/blob/v0.1.1/runtime.md#state [6]: https://groups.google.com/a/opencontainers.org/d/msg/dev/q6TYqVZOcX8/JRm4auylBQAJ Subject: removal of /run/opencontainer/containers Date: Wed, 25 Nov 2015 14:29:35 +0000 Message-ID: <CAD2oYtNipt3i_C6=J4Bc-jwauo5YAvKXUqTROnPNP3vZ9+C5Vw@mail.gmail.com> [7]: https://groups.google.com/a/opencontainers.org/d/msg/dev/q6TYqVZOcX8/wHYucS-rBQAJ Subject: Re: removal of /run/opencontainer/containers Date: Wed, 25 Nov 2015 08:06:10 -0800 Message-ID: <CANEZBD7VCWj6w5dFAEbULrL6WsY=FSRhVsWFreYXUOHwsUJkwA@mail.gmail.com> [8]: http://runc.io/ Embeddable Containers are started as a child process of runC and can be embedded into various other systems without having to run a Docker daemon. Signed-off-by: W. Trevor King <wking@tremily.us>
The version field was added while 180df9d (Add runtime state configuration and structs, 2015-07-29, opencontainers#87) was in-flight [1], and it missed getting documented in the example. [1]: opencontainers#87 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
This adds runtime state information for oci container's so that it can
be persisted and used by external tools.
Closes #72
Closes #41
Signed-off-by: Michael Crosby crosbymichael@gmail.com