Aggressive namespacing

[lowenna]

Signed-off-by: John Howard [email protected]

@wking As per comment #504 (comment) on the proof of concept PR for adding Windows support to the OCI runtime spec, this does the "aggressive namespacing" for all Linux and Solaris structures. Fortunately it doesn't change any of the JSON in the documentation, but will unfortunately have a big knock-on effect to downstream consumers of config.go through the specs package.

[wking]

On Wed, Sep 14, 2016 at 04:09:17PM -0700, John Howard wrote:

Fortunately it doesn't change any of the JSON in the documentation,
but will unfortunately have a big knock-on effect to downstream
consumers of config.go.

This is going to hurt, but it will only hurt more if we do it later
;). Downstream consumers should be pinning to released versions of
runtime-spec, and they can always not bump their pin until they have
time to upgrade. Alternatively, we could keep the old names around as
aliases for a release or too, but that seems too conservative for a
pre-1.0 spec to me.

So b88b924 looks good to me. I expect we will want to do some similar
namespacing in the JSON Schema, but that can happen in follow-up work
if it's too much to bite off here.

[lowenna]

Let's try and keep to smaller incremental changes and handle namespacing in the JSON schema separately 😄 Thanks for all your quick reviews, @wking 👍

[vbatts]

gotcha. though that does not affect the field names in the JSON. Which is both convenient for the time being, but also seems counter to the theme of what you're trying to accomplish, no?

[wking]

On Thu, Sep 15, 2016 at 07:27:31AM -0700, Vincent Batts wrote:

gotcha. though that does not affect the field names in the
JSON. Which is both convenient for the time being, but also seems
counter to the theme of what you're trying to accomplish, no?

JSON keys are already namespaced (e.g. ‘"linux": {"resources": …}’).
You can namespace the Go types by either:

a. using LinuxResources in an all-platform config.go package (what
this PR does), or
b. using Resources in a Linux-specific package (#310 dropped something
that was almost this, but it was just using separate files and not
separate packages).

I don't see a lot of meaningful difference between (a) and (b), and
(a) seems like and easier migration for downstream consumers.

[anuthan]

All for this , and agree it should be done sooner rather than later.

[hqhq]

We used to have config-linux.go, and it's removed in #310 , I personally don't quite like combining all platform-specific configurations in one file. Maybe we can consider separating them since we are now hitting windows configurations.

[lowenna]

@hqhq - There's two solutions, I'm just trying to follow the pattern that is in the current implementation. I honestly don't think there's going to be consensus in either direction, it's just necessary to stick to one and be done with it. We are already at a single file for Linux and Solaris. If the consensus was to have a _solaris.go and _linux.go file, that would already be in master. FWIW, I personally prefer a homogeneous single file as it makes the overall thing easier to comprehend without needing to redirect.

[cyphar]

I don't think it'd be a good idea to use _linux.go and such files -- it would restrict consumers to only being able to use the structures that are defined for their respective GOOS. On the other hand, I don't really like having a single file for everything and actually prefer config-linux.go. Now that we're aggressively namespacing things it should be obvious which structure belongs in which config-*.go file.

[lowenna]

I'm really sorry but I don't understand what you are suggesting. A - rather than _ is really no different. Regardless, it's orthogonal to this PR - I'm merely keeping to an existing behaviour and constructs to allow for a third platform where there IS a clash.

[cyphar]

@jhowardmsft In Go, _linux.go is equivalent to having a // +build linux at the top of the file.

[wking]

On Sat, Sep 17, 2016 at 10:32:02PM -0700, Aleksa Sarai wrote:

On the other hand, I don't really like having a single file for
everything and actually prefer config-linux.go.

I floated the hyphen form a while back and was told it was too
un-idiomatic ;) 1. @vbatts wasn't quite putting his foot down then
though, so it may be possible to get this pushed through now. Unless
we go with per-platform packages (specs-go/config/linux/linux.go? Not
sure if Go would build that for other GOOS), I don't have a preference
on one file vs. per-platform files.

[lowenna]

Yup, I know about build vs _. - just seems odd to me.

[wking]

On Sat, Sep 17, 2016 at 10:36:45PM -0700, John Howard wrote:

Regardless, it's orthogonal to this PR - I'm merely keeping to an
existing behaviour and constructs to allow for a third platform
where there IS a clash.

I'm +1 to “any agressive namespacing is better than no agressive
namespacing”. And shuffling between files in this directory isn't a
breaking change for consumers, so that can get punted. The only thing
I think we need to settle here is whether or not we want per-platform
subpackages (and while I like them, I don't know if they also have the
Go-build issues and I doubt they'd carry the consensus even if they do
build on all platforms ;). If the consensus is “we only want one
package”, then I'm +1 on punting a per-package-file split to a follow
up PR/discussion.

[lowenna]

So it still sounds like this PR is good as it is and the tagging/file-naming/other re-org is a discussion for another PR. Or am I missing something still? Thanks.

[mrunalp]

I feel that doing on per collision might be better. Maybe discuss this in tomorrows call?

[wking]

You probably want NamespaceType → LinuxNamespaceType too.

[lowenna]

Fixed in e918daa

[wking]

You probably want to namespace Action → LinuxSeccompAction (or maybe SeccompAction if we don't namespace Seccomp). Operator and Arg are in the same boat.

[lowenna]

Fixed in e918daa, along with Operator and Arg

[wking]

On Tue, Sep 20, 2016 at 02:21:47PM -0700, Mrunal Patel wrote:

I feel that doing on per collision might be better. Maybe discuss
this in tomorrows call?

Discussing on the call is fine. The balance is between the pain of
translating consumers vs. the likelyhood of a collision. Where a
collision is likely, it's better to namespace now when we have fewer
consumers. Where a collision is unlikely, we may be able to escape
namespacing forever. Looking at the changes here, I'd only put
Seccomp and DeviceCgroup in the “unlikely to ever collide” category.
And with only two in that category (and a ones that the current PR
doesn't namespace [1,2]), it seems easier to avoid crystal-ball gazing
and just namespace them all ;).

[lowenna]

@mrunalp @wking As for all or nothing, or as things arise, my personal preference (obviously holding no weight in the decisions made in this repo) is that all now is a more prudent approach. Have a one-time hit to downstream consumers, with little likelyhood for future issues as new structures are added on a per-platform basis seems sensible. However, I'm looking forward to hearing the results of tomorrows call ☎️

[RobDolinMS]

Tagging @opencontainers/runtime-spec-maintainers to take a look per discussion on OCI Dev ConCall

[vbatts]

LGTM

[RobDolinMS]

@mrunalp Would you please give this a look?

[mrunalp]

LGTM (but would be open to any alternatives if possible).
cc: @crosbymichael

[crosbymichael]

Since all the windows code is behind _windows.go files and will only be build on windows, why do we have to do types like this?

Isn't this the same problem as SysProcAttr in Go's stdlib? You don't have LinuxSysProcAttr and Plan9SysProcAttr, you just have one type and depending on the compile target it has the fields localized for that target.

[wking]

On Mon, Sep 26, 2016 at 10:45:56AM -0700, Michael Crosby wrote:

Since all the windows code is behind _windows.go files and will
only be build on windows, why do we have to do types like this?

We had a structure like that before. It was flattened in #310 to
allow one platform to handle configurations targeting other platforms
(#135, #166). For example, an oci-runtime-tool instance built for
Linux should still be able to parse and validate a runtime
configuration targetting Windows. You obviously have more difficulty
running host-specific checks, but you can still validate the
Windows-targetting config against all of the specs' MUSTs and SHOULDs.

[vbatts]

@crosbymichael you're right but we want the reference code in the specs to not compile platform-dependently.

[tianon]

Indeed, I think a strong use case would be a tool that operates on bundles in general (but doesn't ever execute them) being able to read/write for any platform.

This PR seems sane IMO.

[RobDolinMS]

@crosbymichael Given @vbatts's comment above, is this ready to be merged?