Remove platform #409
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -588,6 +588,8 @@ func validate(context *cli.Context) error { | |
| return err | ||
| } | ||
|
|
||
| platform := context.String("platform") | ||
|
Member
There was a problem hiding this comment. We also need to pass this to
Contributor
Author
There was a problem hiding this comment. Done, but I don't think we use it in generator.
Member
There was a problem hiding this comment. I mean we should change: to since you add a platform option to runtimetest command.
Member
There was a problem hiding this comment. Yes, we don't use it anymore in generator.
Contributor
Author
There was a problem hiding this comment.
I know what you mean, here the process is |
||
|
|
||
| defaultValidations := []validation{ | ||
| { | ||
| test: validateRootFS, | ||
|
|
@@ -666,7 +668,7 @@ func validate(context *cli.Context) error { | |
| } | ||
| } | ||
|
|
||
| if platform == "linux" { | ||
| for _, v := range linuxValidations { | ||
| err := v.test(spec) | ||
| t.Ok(err == nil, v.description) | ||
|
|
@@ -695,7 +697,12 @@ func main() { | |
| cli.StringFlag{ | ||
| Name: "path", | ||
| Value: ".", | ||
| 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?
Contributor
Author
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. |
||
| Value: "linux", | ||
| Usage: "Platform the runtime runs on", | ||
| }, | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,15 +47,21 @@ type Validator struct { | |
| spec *rspec.Spec | ||
| bundlePath string | ||
| HostSpecific bool | ||
| platform string | ||
| } | ||
|
|
||
| // NewValidator creates a Validator | ||
| func NewValidator(spec *rspec.Spec, bundlePath string, hostSpecific bool, platform string) Validator { | ||
| return Validator{ | ||
| spec: spec, | ||
| bundlePath: bundlePath, | ||
| HostSpecific: hostSpecific, | ||
| platform: platform, | ||
| } | ||
| } | ||
|
|
||
| // NewValidatorFromPath creates a Validator with specified bundle path | ||
| func NewValidatorFromPath(bundlePath string, hostSpecific bool, platform string) (Validator, error) { | ||
| if bundlePath == "" { | ||
| return Validator{}, fmt.Errorf("Bundle path shouldn't be empty") | ||
| } | ||
|
|
@@ -77,18 +83,17 @@ func NewValidatorFromPath(bundlePath string, hostSpecific bool) (Validator, erro | |
| return Validator{}, err | ||
| } | ||
|
|
||
| return NewValidator(&spec, bundlePath, hostSpecific, platform), nil | ||
| } | ||
|
|
||
| // CheckAll checks all parts of runtime bundle | ||
| func (v *Validator) CheckAll() (msgs []string) { | ||
| msgs = append(msgs, v.CheckPlatform()...) | ||
| msgs = append(msgs, v.CheckRootfsPath()...) | ||
| msgs = append(msgs, v.CheckMandatoryFields()...) | ||
| msgs = append(msgs, v.CheckSemVer()...) | ||
| msgs = append(msgs, v.CheckMounts()...) | ||
| msgs = append(msgs, v.CheckProcess()...) | ||
| msgs = append(msgs, v.CheckHooks()...) | ||
| if v.spec.Linux != nil { | ||
| msgs = append(msgs, v.CheckLinux()...) | ||
|
|
@@ -131,9 +136,9 @@ func (v *Validator) CheckRootfsPath() (msgs []string) { | |
| msgs = append(msgs, fmt.Sprintf("root.path is %q, but it MUST be a child of %q", v.spec.Root.Path, absBundlePath)) | ||
| } | ||
|
|
||
| if v.platform == "windows" { | ||
| if v.spec.Root.Readonly { | ||
| msgs = append(msgs, "root.readonly field MUST be omitted or false when target platform is windows") | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -156,37 +161,6 @@ func (v *Validator) CheckSemVer() (msgs []string) { | |
| return | ||
| } | ||
|
|
||
| // CheckHooks check v.spec.Hooks | ||
| func (v *Validator) CheckHooks() (msgs []string) { | ||
| logrus.Debugf("check hooks") | ||
|
|
@@ -271,8 +245,7 @@ func (v *Validator) CheckProcess() (msgs []string) { | |
| } | ||
| msgs = append(msgs, v.CheckRlimits()...) | ||
|
|
||
| if v.platform == "linux" { | ||
| if len(process.ApparmorProfile) > 0 { | ||
| profilePath := filepath.Join(v.bundlePath, v.spec.Root.Path, "/etc/apparmor.d", process.ApparmorProfile) | ||
| _, err := os.Stat(profilePath) | ||
|
|
@@ -288,7 +261,7 @@ func (v *Validator) CheckProcess() (msgs []string) { | |
| // CheckCapabilities checks v.spec.Process.Capabilities | ||
| func (v *Validator) CheckCapabilities() (msgs []string) { | ||
| process := v.spec.Process | ||
| if v.platform == "linux" { | ||
| var effective, permitted, inheritable, ambient bool | ||
| caps := make(map[string][]string) | ||
|
|
||
|
|
@@ -336,7 +309,7 @@ func (v *Validator) CheckCapabilities() (msgs []string) { | |
| } | ||
| } | ||
| } else { | ||
| logrus.Warnf("process.capabilities validation not yet implemented for OS %q", v.platform) | ||
| } | ||
|
|
||
| return | ||
|
|
@@ -402,7 +375,7 @@ func supportedMountTypes(OS string, hostSpecific bool) (map[string]bool, error) | |
| func (v *Validator) CheckMounts() (msgs []string) { | ||
| logrus.Debugf("check mounts") | ||
|
|
||
| supportedTypes, err := supportedMountTypes(v.platform, v.HostSpecific) | ||
| if err != nil { | ||
| msgs = append(msgs, err.Error()) | ||
| return | ||
|
|
@@ -423,25 +396,30 @@ func (v *Validator) CheckMounts() (msgs []string) { | |
| return | ||
| } | ||
|
|
||
| // CheckPlatform checks v.platform | ||
| 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?
Contributor
Author
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
Member
There was a problem hiding this comment. I think we should check right after
Contributor
Author
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 Plus I moved |
||
| msgs = append(msgs, fmt.Sprintf("platform %q is not supported", v.platform)) | ||
| return | ||
| } | ||
|
|
||
| if v.platform != "linux" { | ||
| if v.spec.Linux != nil { | ||
| msgs = append(msgs, fmt.Sprintf("'linux' MUST NOT be set when target platform is %q", v.platform)) | ||
| } | ||
| } | ||
|
|
||
| if v.platform != "solaris" { | ||
| if v.spec.Solaris != nil { | ||
| msgs = append(msgs, fmt.Sprintf("'solaris' MUST NOT be set when target platform is %q", v.platform)) | ||
| } | ||
| } | ||
|
|
||
| if v.platform != "windows" { | ||
| if v.spec.Windows != nil { | ||
| msgs = append(msgs, fmt.Sprintf("'windows' MUST NOT be set when target platform is %q", v.platform)) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -502,7 +480,7 @@ func (v *Validator) CheckLinux() (msgs []string) { | |
| } | ||
| } | ||
|
|
||
| if v.platform == "linux" && !typeList[rspec.UTSNamespace].newExist && v.spec.Hostname != "" { | ||
| msgs = append(msgs, fmt.Sprintf("On Linux, hostname requires a new UTS namespace to be specified as well")) | ||
| } | ||
|
|
||
|
|
@@ -684,15 +662,15 @@ func (v *Validator) rlimitValid(rlimit rspec.LinuxRlimit) (msgs []string) { | |
| msgs = append(msgs, fmt.Sprintf("hard limit of rlimit %s should not be less than soft limit", rlimit.Type)) | ||
| } | ||
|
|
||
| if v.platform == "linux" { | ||
| for _, val := range defaultRlimits { | ||
| if val == rlimit.Type { | ||
| return | ||
| } | ||
| } | ||
| msgs = append(msgs, fmt.Sprintf("rlimit type %q is invalid", rlimit.Type)) | ||
| } else { | ||
| logrus.Warnf("process.rlimits validation not yet implemented for platform %q", v.platform) | ||
| } | ||
|
|
||
| return | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also need to remove options os and arch from cli.Flag and manpage