Remove platform #409
Remove platform #409
Conversation
|
ping @Mashimiao @liangchenye @mrunalp I know we have to update runtime-spec to fully remove platform in runtime-tools, I haven't done this because I'm not sure how much amends we have to do to adapt the latest runtime-spec, I might not have time for this for now, and it's safe we just remove checking platform in runtime-tools. |
|
CI failed, fixing. |
|
CI passed, PTAL. |
cmd/runtimetest/main.go
Outdated
| }, | ||
| cli.StringFlag{ | ||
| Name: "platform", | ||
| Value: "Linux", |
| } | ||
|
|
||
| g.SetPlatformOS(context.String("os")) | ||
| g.SetPlatformArch(context.String("arch")) |
There was a problem hiding this comment.
I think you also need to remove options os and arch from cli.Flag and manpage
| func (v *Validator) CheckPlatform() (msgs []string) { | ||
| logrus.Debugf("check platform") | ||
|
|
||
| if v.platform != "linux" && v.platform != "solaris" && v.platform != "windows" { |
There was a problem hiding this comment.
So, spec just supports linux, solaris and windows, all other platforms are invalid?
There was a problem hiding this comment.
For now, I guess the answer is YES, see https://github.com/opencontainers/runtime-spec/blob/master/spec.md#platforms
There was a problem hiding this comment.
I think we should check
if v.platform != "linux" && v.platform != "solaris" && v.platform != "windows" {
right after
platform := context.GlobalString("platform")
There was a problem hiding this comment.
I'm fine with either, if check platform right after got it from context, it would be like a runtime-tools limitation, while it's actually can be an OCI validation, so check it in Validator methods seems reasonable to me.
Plus I moved CheckPlatform to the top in the second commit, so we don't do much unnecessary check it platform is wrong.
|
@Mashimiao Fixed, thanks. |
As it's deleted from runtime-spec. Signed-off-by: Qiang Huang <[email protected]>
Signed-off-by: Qiang Huang <[email protected]>
| inputPath := context.String("path") | ||
| hostSpecific := context.GlobalBool("host-specific") | ||
| v, err := validate.NewValidatorFromPath(inputPath, hostSpecific) | ||
| platform := context.GlobalString("platform") |
There was a problem hiding this comment.
Add 'platform' to man page as well.
| return err | ||
| } | ||
|
|
||
| platform := context.String("platform") |
There was a problem hiding this comment.
We also need to pass this to validation_test.go#getDefaultGenerator and change the Makefile (by adding a PLATFORM for example).
There was a problem hiding this comment.
Done, but I don't think we use it in generator.
There was a problem hiding this comment.
I mean we should change:
g.SetProcessArgs([]string{"/runtimetest"})
to
g.SetProcessArgs([]string{"/runtimetest", "platform", platform})
since you add a platform option to runtimetest command.
There was a problem hiding this comment.
Yes, we don't use it anymore in generator.
There was a problem hiding this comment.
But platform is not a process arg. It should be a generator arg as it's a validate arg.
I know what you mean, here the process is runtimetest.
| Usage: "Path to the configuration", | ||
| }, | ||
| cli.StringFlag{ | ||
| Name: "platform", |
There was a problem hiding this comment.
Should the runtimetest automatically get which platform it is running on? I think it seems there is no need to specify it manunally. What do you think?
There was a problem hiding this comment.
@Mashimiao We can, but can we do that in a follow up PR, what we did in this PR looks consistent and we have the default value.
@liangchenye runc doesn't check platform at all, the spec author should be responsible to give runc the right config.
|
LGTM |
1 similar comment
|
LGTM |
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]>
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]>
3 participants
Fixes: #407