Respect the host platform #194

wking · 2016-08-17T05:19:08Z

Details in the commit messages, but the net effect is to respect
runtime.GOOS and runtime.GOARCH when --host-specific is set (for both
generate and validate). Also add an argument to generate.New to set
the target OS (falling back to runtime.GOOS).

Mashimiao · 2016-08-17T15:38:57Z

generate/generate.go

+	}
+
+	if *os == "linux" {
+		spec.Process.User = rspec.User{}


User is not only used for Linux, it's also used by solaris.

On Wed, Aug 17, 2016 at 08:38:58AM -0700, Ma Shimiao wrote:

}, Hostname: "mrsdalloway",

Mounts: []rspec.Mount{

}

if *os == "linux" {

spec.Process.User = rspec.User{}

User is not only used for Linux, it's also used by solaris.

We could handle that in two ways. If there is significant overlap
between some Linux and Solaris defaults, we'd want:

if *os == "linux" || *os == "solaris" {
spec.Process.User = rspec.User{}
…other common setup…
}

if *os == "linux" {
spec.Process.Capabilities = …
…other Linux-specific setup…
} else if *os == "solaris" {
spec.Solaris.Milestone = "svc:/milestone/container:default"
…other Solaris-specific setup…
} else {
return generator, fmt.Errorf("no defaults configured for %s", *os)
}

If there is not significant overlap, it's probably easier to have:

if *os == "linux" {
spec.Process.User = rspec.User{}
spec.Process.Capabilities = …
…other Linux-specific setup…
} else if *os == "solaris" {
spec.Process.User = rspec.User{}
spec.Solaris.Milestone = "svc:/milestone/container:default"
…other Solaris-specific setup…
} else {
return generator, fmt.Errorf("no defaults configured for %s", *os)
}

I think it will be easier to tell how much overlap there is once we
have a more detailed Solaris default. I don't think setting up an
empty struct for spec.Process.User is enough to be worth claiming
support for Solaris, though, so I'd rather leave this commit as it
stands until we do have a detailed Solaris default.

I think we can go with second approach for now. It is easier to follow the code if there is one path we walk down per platform.

wking · 2016-08-18T20:58:25Z

@cyphar had some concerns about this approach 1, but I'm not clear
on the details. Just pointing that out in case we want to wait and
hear what the concerns are before thinking about merging.

 Subject: Re: oci-image-tool create-runtime-bundle and Kernel Namespaces
 Date: Wed, 17 Aug 2016 15:25:26 +1000
 Message-ID: <CAOviyaienNi7B7zdv3A-3Ys4AFGgf4RHXjFhARPmYAOiV-H1eQ@mail.gmail.com>

cyphar · 2016-08-19T00:29:38Z

Right. So my concerns are basically that I don't see why we're setting any OS defaults with a plain ocitool generate invocation. I understand the argument (saner defaults) but the point of ocitool is that users have to configure things (and we generate a JSON blob that does what they asked for). I'd be fine with a --platform=<> flag which adds platform-specific defaults to the blob (you could even make it --platforms=solaris,linux,... which added platform-specific defaults for multiple platforms in the same blob). But in general, if someone wanted to create a config that is platform-independent (so they could pass it to other tools to add platform-dependant stuff) then they wouldn't have a nice way of doing it. Also, if you're generating configs for a different platform then things get a bit odd (and if we add a --platform option then I reckon the sane default should be none -- we could add a --platform=auto if you really wanted it).

wking · 2016-08-19T01:56:14Z

On Thu, Aug 18, 2016 at 05:29:39PM -0700, Aleksa Sarai wrote:

So my concerns are basically that I don't see why we're setting
any OS defaults with a plain ocitool generate invocation. I
understand the argument (saner defaults) but the point of ocitool
is that users have to configure things (and we generate a JSON blob
that does what they asked for).

Callers that want to configure everything explicitly can use:

$ ocitools generate --template <(echo '{}')

to bypass most defaults. Without template, you get the fuller set of
ocitools-maintainer suggestions.

I'd be fine with a --platform=<> flag which adds platform-specific
defaults to the blob (you could even make it
--platforms=,linux,... which added platform-specific defaults for
multiple platforms in the same blob).

I'm ok with this, although I'd call it --defaults and have it take a
JSON path (e.g. ‘--defaults platform,linux.resources,…’). But it
would be a fairly large shift from what we have now.

But in general, if someone wanted to create a config that is
platform-independent…

Do such configs exist? There aren't many platform independent
settings, and some of those (e.g. mounts) have a platform-independent
schema but not platform independent values. What would you put in a
platform-independent config?

Also, if you're generating configs for a different platform then
things get a bit odd (and I don't see why we need that).

One reason would be “I'm building an ocitools test suite and want to
make sure Windows default generation works even though I'll be running
the suite on Linux” ;).

cyphar · 2016-08-19T05:32:54Z

Also, if you're generating configs for a different platform then things get a bit odd (and I don't see why we need that).

One reason would be “I'm building an ocitools test suite and want to
make sure Windows default generation works even though I'll be running
the suite on Linux” ;).

That wording was incorrect. What I meant was that we want to be able to do that (create a Linux config on Solaris or whatever) but I'm not sure about the platform option being "current OS you're running on" by default -- how is that meant to interact with things like LX-branded zones (where you're running a Linux binary on illumos).

wking · 2016-08-19T17:02:41Z

On Thu, Aug 18, 2016 at 10:32:55PM -0700, Aleksa Sarai wrote:

What I meant was that we want to be able to do that (create a
Linux config on Solaris or whatever) but I'm not sure about the
platform option being "current OS you're running on" by default --
how is that meant to interact with things like LX-branded zones
(where you're running a Linux binary on illumos).

With this PR you can do that by setting --os, although we currently
only have defaults for ‘linux’. runtime.GOOS is just the default if
--os is not set.

mrunalp · 2016-08-22T17:03:34Z

I like the idea of defaulting to current OS or specifying alternative via --os. It covers both aspects of easy to use for default case as well as supporting cross platform spec generation.

Mashimiao · 2016-11-14T08:38:37Z

@wking any updates for this PR？

wking · 2016-11-14T15:07:17Z

On Mon, Nov 14, 2016 at 12:38:38AM -0800, Ma Shimiao wrote:

@wking any updates for this PR？

I'm not sure what you were looking for, but I've rebased this onto
master with 217c769 → 1ae6388.

mrunalp · 2016-11-14T19:26:44Z

Missing a signed-off?

wking · 2016-11-14T19:44:17Z

On Mon, Nov 14, 2016 at 11:26:44AM -0800, Mrunal Patel wrote:

Missing a signed-off?

My three commits are signed-off. I think PullApprove is confused by
my re-basing from v1.0.0.rc1 to master.

Mashimiao · 2016-11-15T01:21:26Z

I like the idea of defaulting to current OS or specifying alternative via --os. It covers both aspects of easy to use for default case as well as supporting cross platform spec generation.

As @mrunalp said, I also want to keep default current OS or specifying alternative via --os
What I'm looking for is ea326f04c391035e653c93e10739971a6aeb5b1c and e3df0d57d3ead77a9abec382672f74960cdc7c73

wking · 2016-11-15T04:43:47Z

On Mon, Nov 14, 2016 at 05:21:26PM -0800, Ma Shimiao wrote:

I like the idea of defaulting to current OS or specifying
alternative via --os. It covers both aspects of easy to use for
default case as well as supporting cross platform spec generation.

As @mrunalp said, I also want to keep default current OS or
specifying alternative via --os What I'm looking for is
ea326f04c391035e653c93e10739971a6aeb5b1c and
e3df0d57d3ead77a9abec382672f74960cdc7c73

I'm not convinced that's what @mrunalp was saying, but if there's a
consensus around ea326f0 and e3df0d5 I'm happy to have those landed.
I've punted the tip commit off of this PR (into #270), so we can get
this one landed sooner.

Mashimiao · 2016-11-15T06:36:45Z

validate/validate.go

 	}
 	msgs = append(msgs, fmt.Sprintf("Operation system %q of the bundle is not supported yet.", platform.OS))

+	if hostCheck {


hostCheck->v.HostSpecific

Mashimiao · 2016-11-15T06:41:46Z

cmd/oci-runtime-tool/generate.go

 	Action: func(context *cli.Context) error {
 		// Start from the default template.
-		specgen := generate.New()
+		specgen, err := generate.New(nil)


How about getting --os value before generate.New()?
Then we don't have to pass nil any more.

wking · 2016-11-15T15:03:19Z

On Mon, Nov 14, 2016 at 10:43:03PM -0800, Ma Shimiao wrote:

if hostCheck {

hostCheck->v.HostSpecific

Oops, fixed in ea326f0 → 2cb6c62.

specgen := generate.New()
specgen, err := generate.New(nil)
How about getting --os value before generate.New()?
Then we don't have to pass nil any more.

I liked it better when the caller didn't have to have an opinion (what
happens if/when we extend platform.os or platform.arch beyond
GOOS/GOARCH?). But whatever, also updated in ea326f0 → 2cb6c62.

Mashimiao · 2016-11-16T01:09:27Z

@opencontainers/runtime-tools-maintainers , @wking
I still have a question, which makes me a little confused. I think we need to make it clear.
If we used a template and also used some options and the values of options may conflict with values in template, should we respect values in template or let values of options to cover them?

wking · 2016-11-16T04:45:30Z

On Tue, Nov 15, 2016 at 05:09:28PM -0800, Ma Shimiao wrote:

If we used a template and also used some options and the values of
options may conflict with values in template, should we respect
values in template or let values of options to cover them?

Explicit command line options (e.g. ‘--os windows’) should always take
precedence. The template or default config comes through when
conflicting command line options are not set.

As it stands in this PR, the template is getting clobbered by the
implicit default for the --os option. That's what the tip commit
(now in #270) is fixing. With the commits remaining in this PR, we
start respecting explicit --os to select non-conflicting defaults for
settings besides platform.os (e.g. not filling in ‘linux’ when asked
to generate a new config with ‘--os windows’).

zhouhao3 · 2017-12-26T05:58:13Z

generate/generate.go

-		},
+		}
+	} else {
+		return generator, fmt.Errorf("no defaults configured for %s", os)


I think you need to generate the appropriate configuration for windows and solaris .

I think you need to generate the appropriate configuration for windows and solaris.

I'm creating a very slim Solaris now default with 984db97. I'm happy to add a Window default too if you'll tell me what it should be, but I don't know enough about Windows to be able to figure that out on my own. Since we don't currently support generating defaults for Windows configs, I don't see a problem with leaving that unimplemented for this PR and just adjusting the function signatures to allow it to be added in the future.

If --host-specific is set, but the host OS doesn't match the requested validation platform, add an error, unset HostSpecific, and carry on with the remaining host-agnostic checks. Signed-off-by: W. Trevor King <[email protected]>

wking · 2018-04-11T06:56:52Z

I see this is tagged v0.6.0, so I've rebased it onto master with 2cb6c62 → 984db97. Hopefully we can land it before too many more validation/*.go land, since they'll probably need rebasing now that generate.New(…) can return an error.

wking · 2018-04-11T07:10:19Z

I've pushed 984db97 → 2e6f6ab, restoring the generate --os option which had been removed by 597c7d4 (#409). Otherwise there's no way to generate default configs for other platforms.

Don't fill in a bunch of Linux stuff if runtime.GOOS isn't Linux ;). We don't have sensible defaults for other OSes yet, so error out in those cases. This commit restores the --os argument which had previously been removed in 597c7d4 (Remove platform, 2017-07-05, opencontainers#409). The diff here is fairly large, because many callers depend (directly or indirectly) on the generation code, and now all of those callers need to be on the lookout for errors. Generation will currently fail for all GOOS besides linux and solaris. I doubt the Solaris default is particularly useful either; it has all the POSIX settings from our Linux default, but I don't know enough about Solaris to know which Solaris-specific properties should get defaults. And while some rlimits are OS-specific, RLIMIT_NOFILE (the only one we set in our default config) is in POSIX [1], so I've put the rlimit config in the POSIX block. [1]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_resource.h.html Signed-off-by: W. Trevor King <[email protected]>

zhouhao3 · 2018-04-12T01:33:51Z

LGTM

Mashimiao reviewed Aug 17, 2016
View reviewed changes

wking mentioned this pull request Aug 20, 2016

Range-checking for generate options #186

Open

wking force-pushed the tk/validate-host-platform branch from 217c769 to 1ae6388 Compare November 14, 2016 15:03

wking changed the base branch from v1.0.0.rc1 to master November 14, 2016 15:04

wking mentioned this pull request Nov 15, 2016

generate: Respect --os when generating default templates #270

Closed

wking force-pushed the tk/validate-host-platform branch from 1ae6388 to ea326f0 Compare November 15, 2016 04:38

Mashimiao reviewed Nov 15, 2016

View reviewed changes

wking force-pushed the tk/validate-host-platform branch from ea326f0 to 2cb6c62 Compare November 15, 2016 14:59

Mashimiao added this to the v0.6.0 milestone Sep 26, 2017

zhouhao3 reviewed Dec 26, 2017

View reviewed changes

wking force-pushed the tk/validate-host-platform branch from 2cb6c62 to 984db97 Compare April 11, 2018 06:55

wking mentioned this pull request Apr 11, 2018

generate: Move Generator.spec to Generator.Config #266

Merged

wking force-pushed the tk/validate-host-platform branch from 984db97 to 4aecad2 Compare April 11, 2018 07:08

wking force-pushed the tk/validate-host-platform branch from 4aecad2 to 2e6f6ab Compare April 11, 2018 07:12

zhouhao3 merged commit 25ae2f4 into opencontainers:master Apr 12, 2018

wking deleted the tk/validate-host-platform branch April 12, 2018 04:25

zhouhao3 mentioned this pull request May 11, 2018

generate: sync with runc's default spec #477

Open

Respect the host platform #194

Respect the host platform #194

Uh oh!

Conversation

wking commented Aug 17, 2016

Uh oh!

Mashimiao Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

wking Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

mrunalp Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

wking commented Aug 18, 2016

Uh oh!

cyphar commented Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Aug 19, 2016

Uh oh!

cyphar commented Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Aug 19, 2016

Uh oh!

mrunalp commented Aug 22, 2016

Uh oh!

Mashimiao commented Nov 14, 2016

Uh oh!

wking commented Nov 14, 2016

Uh oh!

mrunalp commented Nov 14, 2016

Uh oh!

wking commented Nov 14, 2016

Uh oh!

Mashimiao commented Nov 15, 2016

Uh oh!

wking commented Nov 15, 2016

Uh oh!

Mashimiao Nov 15, 2016

Choose a reason for hiding this comment

Uh oh!

Mashimiao Nov 15, 2016

Choose a reason for hiding this comment

Uh oh!

wking commented Nov 15, 2016

Uh oh!

Mashimiao commented Nov 16, 2016

Uh oh!

wking commented Nov 16, 2016

Uh oh!

zhouhao3 Dec 26, 2017

Choose a reason for hiding this comment

Uh oh!

wking Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Apr 11, 2018

Uh oh!

wking commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouhao3 commented Apr 12, 2018 • 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

cyphar commented Aug 19, 2016 •

edited

Loading

cyphar commented Aug 19, 2016 •

edited

Loading

wking Apr 11, 2018 •

edited

Loading

wking commented Apr 11, 2018 •

edited

Loading

zhouhao3 commented Apr 12, 2018 •

edited by caniszczyk

Loading