config: platform dependent reference source #310

vbatts · 2016-01-13T23:53:11Z

This introduces verbiage of fields that may occur in json (technically
optional), but is required on certain platforms (e.g. Linux).

Not adding a "name" string, as that is not a requirement yet.

In the event a windows runtime shows up, I could imagine an sid, but
we'll get to that when it happens.

Closes #135
Related to #166

Signed-off-by: Vincent Batts [email protected]

vishh · 2016-01-13T23:57:52Z

config.go

+// User specifies linux specific user and group information for the container's
+// main process.
+type User struct {
+	// UID is the user ID the Process is executed as. (this field is platform dependent)


Should we use a json tag to indicate platform dependency? It will help with tooling as well.

Good idea

On Wed, Jan 13, 2016, 15:57 Vish Kannan [email protected] wrote:

In config.go
#310 (comment):

@@ -33,6 +33,17 @@ type Process struct {
Cwd string json:"cwd"
}

+// User specifies linux specific user and group information for the container's
+// main process.
+type User struct {

// UID is the user ID the Process is executed as. (this field is platform dependent)

Should we use a json tag to indicate platform dependency? It will help
with tooling as well.

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/310/files#r49670377.

Should these tags be added to all Linux specific fields? It is perfectly fine to tackle that in a separate PR, if you agree.
These tags can help with validation.

vishh · 2016-01-13T23:58:03Z

LGTM

wking · 2016-01-14T00:04:31Z

On Wed, Jan 13, 2016 at 03:53:12PM -0800, Vincent Batts wrote:

In the event a windows runtime shows up, I could imagine an sid,
but we'll get to that when it happens.

+1 on this PR and its punt, but until we have no platform-specific
build decisions I think #135 should stay open. #190 implies that
having *_linux.go is enough to trigger platform-specific build
decisions.

vbatts · 2016-01-14T21:56:26Z

This PR now addresses all the platform dependent source, not just the User struct.
And added field tags that we can build tooling around.

vbatts · 2016-01-14T21:56:32Z

PTAL

duglin · 2016-01-14T22:15:51Z

looks nice

wking · 2016-01-14T23:09:30Z

config.md

-        "name": "proc",
-        "path": "/proc"
+      "name": "proc",
+      "path": "/proc"


Most of our other JSON examples use four-space indents, so we probably just want a blanket s/^/ / on this block instead of dropping to two-space indents.

And either way, this seems orthogonal to platform-dependent Go types ;).

wking · 2016-01-14T23:22:49Z

On Thu, Jan 14, 2016 at 01:56:28PM -0800, Vincent Batts wrote:

This PR now addresses all the platform dependent source…

You squashed config-linux.md into config.md but left
runtime-config-linux.md as its own file. I'm fine either going with
platform-specific files or per-config files, but I think we should
pick one way or the other.

hqhq · 2016-01-15T09:16:38Z

config.md

-* **`additionalGids`** (array of ints, optional) specifies additional group ids to be added to the process.
+* **`uid`** (int, required on Linux) specifies the user id.
+* **`gid`** (int, required on Linux) specifies the group id.
+* **`additionalGids`** (array of ints, optional on Linux) specifies additional group ids to be added to the process.


It's already said "For Linux-based systems the user structure has the following fields", isn't this additional change kind of redundant?

I'll drop that part of the sentence

hqhq · 2016-01-15T09:28:58Z

Is the plan to remove *-linux.md and won't add any *-windows.md but all combined into config.md? It's gonna make it really fat, isn't there other way we can do to make specs platforms agnostic?

vbatts · 2016-01-15T16:37:36Z

@hqhq that's a fair point

wking · 2016-01-15T17:23:45Z

On Fri, Jan 15, 2016 at 01:29:00AM -0800, Qiang Huang wrote:

Is the plan to remove *-linux.md and won't add any *-windows.md
but all combined into config.md? It's gonna make it really fat,
isn't there other way we can do to make specs platforms agnostic?

Combining the Go (or just changing __linux.go to something else. Does
Go treat linux/_.go as platform-specific?) seems orthogonal to
combining the Markdown docs. We can leave *-linux.md and still have
cross-platform Go.

wking · 2016-01-15T17:25:31Z

On the other hand, lots of folks want a single-page spec (#263), and a
single-page Markdown would make that easier (fixing links 1, etc.).

duglin · 2016-01-15T18:19:31Z

I have a preference for one file with sections for platform specific stuff. Less of a chance of having to duplicate things (and keep them in-sync). And it easier to then compare the differences between what each platform expects. Besides, the size of these platform specific bits should be small.

vbatts · 2016-01-15T18:34:18Z

I started to do runtime*.md, but it was much longer. It was really an arbitrary decision.

vishh · 2016-01-15T18:36:13Z

config.go

@@ -33,6 +36,17 @@ type Process struct {
 	Cwd string `json:"cwd"`
 }

+// User specifies linux specific user and group information for the container's
+// main process.
+type User struct {


Should this type be named LinuxUser for the sake of readability?

No, because the intention is that as other platforms have their own fields, they still fall in here.

+1 and while in there perhaps renaming "Linux" above too - when I first saw Linux Linux ... it made me laugh.

wking · 2016-01-15T18:38:20Z

On Fri, Jan 15, 2016 at 10:19:34AM -0800, Doug Davis wrote:

Besides, the size of these platform specific bits should be small.

Heh.

$ git describe
v0.2.0
$ wc -l config.md
39 config-linux.md
130 config.md
535 runtime-config-linux.md
122 runtime-config.md
826 total

so 70% of our config docs are Linux specific, and that's not even
counting Linux-specific examples like the process entry in config.md
and the mount entry in runtime-config.md. Of course, 1 would bring
that number down a lot, but since we still haven't landed an opt-out
like #237, I'd be surprised if cgroups are moved outside of this spec
before summer (if they get moved out at all).

 Subject: removal of cgroups from the OCI Linux spec
 Date: Wed, 28 Oct 2015 17:01:59 +0000
 Message-ID: <CAD2oYtO1RMCcUp52w-xXemzDTs+J6t4hS5Mm4mX+uBnVONGDfA@mail.gmail.com>

vishh · 2016-01-15T18:38:23Z

runtime_config.go

+	// SelinuxProcessLabel specifies the selinux context that the container process is run as.
+	SelinuxProcessLabel string `json:"selinuxProcessLabel"`
+	// Seccomp specifies the seccomp security settings for the container.
+	Seccomp Seccomp `json:"seccomp"`


nit: Make this optional.

and a pointer. got it.

wking · 2016-01-15T19:09:22Z

On Fri, Jan 15, 2016 at 10:34:21AM -0800, Vincent Batts wrote:

I started to do runtime*.md, but it was much longer.

If we're not comfortable rolling that in, that's fine with me 1, and
if we don't want to develop in a single page, but still provide a
single page view that's also fine with me 2. But if we don't want
squash *-linux.md for the runtime.json spec, I'd suggest rerolling
this series to avoid squashing *-linux.md for the config.json spec.
Otherwise we'll get odd half-squashing if/when we unify runtime.json
and config.json (#284).

vbatts · 2016-03-02T20:27:24Z

will need more rebasing, with #329

vbatts · 2016-03-09T16:13:31Z

Rebased

This introduces verbiage of fields that may occur in json (technically optional), but is required on certain platforms (e.g. Linux). The JSON document will look the same as it presently does, but now the reference source compiles regardless of platform. Not adding a "name" string to the user sturct, as that is not a requirement yet. In the event a windows runtime shows up, I could imagine an `sid` on the user struct, but we'll get to that when it happens. Closes opencontainers#135 Related to opencontainers#166 Signed-off-by: Vincent Batts <[email protected]>

vbatts · 2016-03-09T20:12:35Z

Rebased

vbatts · 2016-03-09T20:13:21Z

@crosbymichael if you would give this a once-over. It ought not be a code/structure change at all.

crosbymichael · 2016-03-09T20:35:06Z

LGTM

config: platform dependent reference source

Through eea2a6c (Merge pull request opencontainers#310 from vbatts/multi-platform, 2016-03-09). Signed-off-by: W. Trevor King <[email protected]>

These comments first landed in 820131d (*: flatten platform dependent source, 2016-03-08, opencontainers#310). But you can tell they're platform dependent by the platform:"..." tags. The Go comment doesn't add any additional information. Signed-off-by: W. Trevor King <[email protected]>

vishh reviewed Jan 13, 2016
View reviewed changes

vbatts force-pushed the multi-platform branch from a23a5c8 to 4d1b2d2 Compare January 14, 2016 20:38

vbatts changed the title ~~config: platform dependent user attributes~~ config: platform dependent reference source Jan 14, 2016

vbatts force-pushed the multi-platform branch from 4d1b2d2 to 49a6f9d Compare January 14, 2016 21:55

wking reviewed Jan 14, 2016
View reviewed changes

hqhq reviewed Jan 15, 2016
View reviewed changes

vbatts force-pushed the multi-platform branch from 49a6f9d to ba06e26 Compare January 15, 2016 18:35

vishh reviewed Jan 15, 2016
View reviewed changes

vbatts force-pushed the multi-platform branch 2 times, most recently from 3878a8d to bd8af9a Compare January 15, 2016 18:41

hqhq mentioned this pull request Feb 25, 2016

Minimum bar - compile on Windows (containerd dependency) #327

Closed

vbatts force-pushed the multi-platform branch from bd8af9a to 833e6d7 Compare March 9, 2016 16:13

vbatts added this to the v0.5.0 milestone Mar 9, 2016

vbatts force-pushed the multi-platform branch from 833e6d7 to 820131d Compare March 9, 2016 20:12

vbatts modified the milestones: v0.4.0, v0.5.0 Mar 9, 2016

vbatts added a commit that referenced this pull request Mar 9, 2016

Merge pull request #310 from vbatts/multi-platform

eea2a6c

config: platform dependent reference source

vbatts merged commit eea2a6c into opencontainers:master Mar 9, 2016

vbatts deleted the multi-platform branch March 9, 2016 20:47

vbatts mentioned this pull request Mar 9, 2016

[Tracker] Platform/Compiler agnostic schema #166

Closed

This was referenced Sep 14, 2016

specs-go/config: Drop "this field is platform dependent" #568

Merged

Aggressive namespacing #567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: platform dependent reference source #310

config: platform dependent reference source #310

vbatts commented Jan 13, 2016

vishh Jan 13, 2016

vbatts Jan 14, 2016

vishh Jan 15, 2016

vishh commented Jan 13, 2016

wking commented Jan 14, 2016

vbatts commented Jan 14, 2016

vbatts commented Jan 14, 2016

duglin commented Jan 14, 2016

wking Jan 14, 2016

wking Jan 14, 2016

wking commented Jan 14, 2016

hqhq Jan 15, 2016

vbatts Jan 15, 2016

hqhq commented Jan 15, 2016

vbatts commented Jan 15, 2016

wking commented Jan 15, 2016

wking commented Jan 15, 2016

duglin commented Jan 15, 2016

vbatts commented Jan 15, 2016

vishh Jan 15, 2016

vbatts Jan 15, 2016

duglin Jan 15, 2016

wking commented Jan 15, 2016

vishh Jan 15, 2016

vbatts Jan 15, 2016

wking commented Jan 15, 2016

vbatts commented Mar 2, 2016

vbatts commented Mar 9, 2016

vbatts commented Mar 9, 2016

vbatts commented Mar 9, 2016

crosbymichael commented Mar 9, 2016

config: platform dependent reference source #310

config: platform dependent reference source #310

Conversation

vbatts commented Jan 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Jan 13, 2016

wking commented Jan 14, 2016

vbatts commented Jan 14, 2016

vbatts commented Jan 14, 2016

duglin commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hqhq commented Jan 15, 2016

vbatts commented Jan 15, 2016

wking commented Jan 15, 2016

wking commented Jan 15, 2016

duglin commented Jan 15, 2016

vbatts commented Jan 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Jan 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Jan 15, 2016

vbatts commented Mar 2, 2016

vbatts commented Mar 9, 2016

vbatts commented Mar 9, 2016

vbatts commented Mar 9, 2016

crosbymichael commented Mar 9, 2016