Platform annotations on `Process` struct #564

lowenna · 2016-09-14T20:24:25Z

Signed-off-by: John Howard jhoward@microsoft.com

Extracting pieces from the proof of concept PR for Windows OCI support at #504. This PR tidies up the Process structure by putting annotations for platform support on Capabilities, Rlimits and NoNewPrivileges, none of which are used on Windows. In addition, it removes what appears to be an erroneous blank line that appeared to serve no purpose.

Note I'm assuming (probably incorrectly 😇) , that Rlimits and NoNewPrivileges are also relevant on Solaris too, but I'm not sure. If that is incorrect, I will update the PR to remove the solaris annotation.

wking · 2016-09-14T20:48:31Z

specs-go/config.go

@@ -40,13 +40,12 @@ type Process struct {
 	// Cwd is the current working directory for the process and must be
 	// relative to the container's root.
 	Cwd string `json:"cwd"`
-	// Capabilities are Linux capabilities that are kept for the container.
+	// Capabilities are Linux capabilities that are kept for the container. (this field is platform dependent)
 	Capabilities []string `json:"capabilities,omitempty" platform:"linux"`


Doesn't the platform:"linux" tag already cover the “this field is platform dependent” idea?

Consistency. It's all over this file whenever a platform annotation is present.

wking · 2016-09-14T20:49:55Z

specs-go/config.go

+	// Rlimits specifies rlimit options to apply to the process. (this field is platform dependent)
+	Rlimits []Rlimit `json:"rlimits,omitempty" platform:"linux,solaris"`
+	// NoNewPrivileges controls whether additional privileges could be gained by processes in the container. (this field is platform dependent)
+	NoNewPrivileges bool `json:"noNewPrivileges,omitempty" platform:"linux,solaris"`


I don't think that these Rlimits or NoNewPrivileges are on Solaris. And we can probably wait for Solaris folks to pipe up if they are, instead of adding them here and waiting for Solaris folks to pipe up to re-remove them ;).

@anuthan - Could you comment on the use (or lack of) of Rlimits and NoNewCapabilities on Solaris? Thanks 😄

Solaris doesn't have Rlimits and NoNewPrivileges . Safe to remove :)

@anuthan Thanks for confirming! Removed.

wking · 2016-09-14T21:07:42Z

On Wed, Sep 14, 2016 at 01:51:28PM -0700, John Howard wrote:

// Capabilities are Linux capabilities that are kept for the container.

// Capabilities are Linux capabilities that are kept for the container. (this field is platform dependent)
Capabilities []string json:"capabilities,omitempty" platform:"linux"

Consistency. It's all over this file whenever a platform annotation is present.

+1 for consistency. How do folks feel about consistently removing
this from the comment when a platform field exists? And there are
fields like mounts[]options with different rules depending on the
platform, but I expect that folks will recognize that without having a
Go comment here to remind them. Are people really reading the Go
comments and skipping the Markdown spec?

lowenna · 2016-09-14T21:44:52Z

@wking - Yes, it all does seem more than a tad superfluous IMO. But that said, it's still a little orthogonal to this PR itself 😇

wking · 2016-09-14T22:04:44Z

On Wed, Sep 14, 2016 at 02:44:53PM -0700, John Howard wrote:

@wking - Yes, it all does seem more than a tad superfluous IMO. But
that said, it's still a little orthogonal to this PR itself
😇

This PR does two things (as of 103926f):

a. Add ‘solaris’ to some ‘platform’ tags.
b. Add “(this field is platform dependent)” to some Go comments.

I think we want to drop (a) 1, but am fine waiting on Solaris-dev
feedback there. And I think we want to go the other way with (b).

If this PR is just about (a), I'm happy to file my other-(b) as a
separate PR.

lowenna · 2016-09-14T23:11:25Z

@wking - Let's try and keep these two things independent, and handle the go comments as a separate issue/PR. I think that will be cleaner in the long run.

wking · 2016-09-14T23:26:37Z

On Wed, Sep 14, 2016 at 04:11:27PM -0700, John Howard wrote:

@wking - Let's try and keep these two things independent, and handle
the go comments as a separate issue/PR. I think that will be cleaner
in the long run.

Sounds good. I spun off the comment removal into #568.

vbatts · 2016-09-15T14:28:18Z

LGTM

vbatts · 2016-09-15T14:28:45Z

(side note: we ought to define these tags somewhere)

wking · 2016-09-15T14:40:47Z

On Thu, Sep 15, 2016 at 07:28:19AM -0700, Vincent Batts wrote:

LGTM

Really? Solaris does use these fields?

And I agree we should define the tags. We already have the
enum here 1, just need to link it with the JSON tags.

vbatts · 2016-09-15T14:44:41Z

@wking chill.
Solaris supports more of the Linux API than I realize. I'll leave it up to folks like @JLB13 to correct me.

Signed-off-by: John Howard <jhoward@microsoft.com>

lowenna · 2016-09-15T17:25:58Z

@vbatts @wking Removed the annotation for solaris

wking · 2016-09-15T17:31:35Z

On Thu, Sep 15, 2016 at 10:25:59AM -0700, John Howard wrote:

@vbatts @wking Removed the annotation for solaris

ce3ac33 looks good to me :).

hqhq · 2016-09-18T02:29:39Z

LGTM

lowenna · 2016-09-20T02:28:47Z

What's the process for full approval and merging in this repo? There's two LGTMs above, but I'm not sure if more are still required - it appears it does from above, but I'm not sure why. Forgive my ignorance 😇

hqhq · 2016-09-20T02:57:30Z

@jhowardmsft We have a .pullapprove.yml which set reset_on_push: true and reviewers: required: 2, so after you force pushed, we still need two LGTMs before merging.

lowenna · 2016-09-20T02:58:46Z

Ah, I see. Thanks for confirming 😄

mrunalp · 2016-09-20T21:18:42Z

LGTM

Rlimits do not need either ordering or repeat entries for a single type. While come JSON libraries preserve object key order or allow repeats, there are many JSON libraries which do not (e.g. Python and JavaScript both parse JSON objects into hash tables). Using objects here reinforces the unimportance of ordering or repeated entries. Also add Solaris support. I'm not entirely clear on this, because while Solaris is POSIX-certified system and there is a Solaris man page for setrlimit, Abhijeeth claims no Solaris support for rlimits [1]. The additionalProperties object bit comes from [2], although it is not documented in draft 4 of the JSON Schema RFC [3]. [1]: opencontainers#564 (comment) [2]: https://spacetelescope.github.io/understanding-json-schema/reference/object.html#properties [3]: https://tools.ietf.org/html/draft-zyp-json-schema-04 Signed-off-by: W. Trevor King <wking@tremily.us>

Rlimits do not need either ordering or repeat entries for a single type. Using an object leans on the new wording from eeaccfa (glossary: Make objects explicitly unordered and forbid duplicate names, 2016-09-27, opencontainers#584) to make both of those points explicit. Also add Solaris support. I'm not entirely clear on this, because while Solaris is POSIX-certified system and there is a Solaris man page for setrlimit, Abhijeeth claims no Solaris support for rlimits [1]. The additionalProperties object bit comes from [2,3], although it is not documented in draft 4 of the JSON Schema RFC [4]. [1]: opencontainers#564 (comment) [2]: https://spacetelescope.github.io/understanding-json-schema/reference/object.html#properties [3]: https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-5.18 [4]: https://tools.ietf.org/html/draft-zyp-json-schema-04 Signed-off-by: W. Trevor King <wking@tremily.us>

wking suggested changes Sep 14, 2016

View reviewed changes

wking mentioned this pull request Sep 14, 2016

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

Merged

wking mentioned this pull request Sep 15, 2016

config: Link platform:"…" JSON tags with ~~protocol~~ platform slugs #570

Merged

Platform markers on Process struct

ce3ac33

Signed-off-by: John Howard <jhoward@microsoft.com>

lowenna force-pushed the jjh/processplatformtags branch from 103926f to ce3ac33 Compare September 15, 2016 17:24

lowenna mentioned this pull request Sep 20, 2016

Windows: OCI HVRuntime and LayerPaths to options moby/moby#26746

Merged

mrunalp merged commit 7bce59f into opencontainers:master Sep 20, 2016

lowenna deleted the jjh/processplatformtags branch September 20, 2016 21:53

wking mentioned this pull request Sep 27, 2016

config: Convert process.rlimits from an array to an object #583

Closed

vbatts mentioned this pull request Dec 7, 2016

Version bump for v1.0.0-rc3 #642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform annotations on `Process` struct #564

Platform annotations on `Process` struct #564

lowenna commented Sep 14, 2016

wking Sep 14, 2016

lowenna Sep 14, 2016

wking Sep 14, 2016

lowenna Sep 14, 2016

anuthan Sep 15, 2016 •

edited

Loading

lowenna Sep 15, 2016

wking commented Sep 14, 2016

lowenna commented Sep 14, 2016

wking commented Sep 14, 2016

lowenna commented Sep 14, 2016

wking commented Sep 14, 2016

vbatts commented Sep 15, 2016 •

edited by caniszczyk

Loading

vbatts commented Sep 15, 2016

wking commented Sep 15, 2016

vbatts commented Sep 15, 2016

lowenna commented Sep 15, 2016

wking commented Sep 15, 2016

hqhq commented Sep 18, 2016 •

edited by caniszczyk

Loading

lowenna commented Sep 20, 2016

hqhq commented Sep 20, 2016

lowenna commented Sep 20, 2016

mrunalp commented Sep 20, 2016 •

edited by caniszczyk

Loading

Platform annotations on Process struct #564

Platform annotations on Process struct #564

Conversation

lowenna commented Sep 14, 2016

wking Sep 14, 2016

Choose a reason for hiding this comment

lowenna Sep 14, 2016

Choose a reason for hiding this comment

wking Sep 14, 2016

Choose a reason for hiding this comment

lowenna Sep 14, 2016

Choose a reason for hiding this comment

anuthan Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

lowenna Sep 15, 2016

Choose a reason for hiding this comment

wking commented Sep 14, 2016

lowenna commented Sep 14, 2016

wking commented Sep 14, 2016

lowenna commented Sep 14, 2016

wking commented Sep 14, 2016

vbatts commented Sep 15, 2016 • edited by caniszczyk Loading

vbatts commented Sep 15, 2016

wking commented Sep 15, 2016

vbatts commented Sep 15, 2016

lowenna commented Sep 15, 2016

wking commented Sep 15, 2016

hqhq commented Sep 18, 2016 • edited by caniszczyk Loading

lowenna commented Sep 20, 2016

hqhq commented Sep 20, 2016

lowenna commented Sep 20, 2016

mrunalp commented Sep 20, 2016 • edited by caniszczyk Loading

Platform annotations on `Process` struct #564

Platform annotations on `Process` struct #564

anuthan Sep 15, 2016 •

edited

Loading

vbatts commented Sep 15, 2016 •

edited by caniszczyk

Loading

hqhq commented Sep 18, 2016 •

edited by caniszczyk

Loading

mrunalp commented Sep 20, 2016 •

edited by caniszczyk

Loading