Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

runtime: add linux default fs validation #121

Merged
merged 2 commits into from
Jun 30, 2016
Merged

runtime: add linux default fs validation #121

merged 2 commits into from
Jun 30, 2016

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

wking
wking reviewed Jun 23, 2016
View reviewed changes
cmd/runtimetest/main.go

for fs, fstype := range defaultFS {
if !(mountsMap[fs] == fstype) {
return fmt.Errorf("%v must exists and expected type is %v", fs, fstype)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: “exists” → “exist”.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks.

@wking
Copy link
Contributor

wking commented Jun 23, 2016

On Thu, Jun 23, 2016 at 09:11:21AM -0700, Ma Shimiao wrote:

  • runtime: add linux default fs validation

This looks ok to me, although there are fuzzy corner cases if
config.json specifies a different type be mounted over one of these
locations. See the last point in 1 about when the runtime is
supposed to supply these mounts/devices, and we still have no wording
about that in the spec itself.

@Mashimiao Mashimiao force-pushed the runtime-test-test-linux-default-fs branch from e2b3180 to 258adc8 Compare June 24, 2016 01:28
@Mashimiao
Copy link
Author

As smoe validations related to linux os, splited linux-specific valdiations

@wking
Copy link
Contributor

wking commented Jun 25, 2016

On Fri, Jun 24, 2016 at 07:16:22PM -0700, Ma Shimiao wrote:

As smoe validations related to linux os, splited linux-specific
valdiations

258adc8 itself looks good for me (except for “valdations” →
“validations” in the commit message).

I'm still not convinced that the current spec wording is enforcable
1, but I'm ok with punting on that for now (until someone asks us to
test the case where a config.json asks for a bind mount over /tmp? ;).

@Mashimiao Mashimiao force-pushed the runtime-test-test-linux-default-fs branch from 258adc8 to 798d84f Compare June 25, 2016 08:12
@Mashimiao
Copy link
Author

fixed valdations->validations

@wking
Copy link
Contributor

wking commented Jun 25, 2016

On Sat, Jun 25, 2016 at 01:12:52AM -0700, Ma Shimiao wrote:

fixed valdations->validations

798d84f looks good to me if we're punting on the corner cases 1.

liangchenye
liangchenye reviewed Jun 26, 2016
View reviewed changes
cmd/runtimetest/main.go
defaultFS = map[string]string{
"/proc": "proc",
"/sys": "sysfs",
"dev/pts": "devpts",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "dev/pts" to /dev/pts ?

same with "dev/shm"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Ma Shimiao added 2 commits June 26, 2016 22:15
runtime: add linux default fs validation
4cfd018 
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
split linux-specific validations
e4b2183 
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao force-pushed the runtime-test-test-linux-default-fs branch from 798d84f to e4b2183 Compare June 26, 2016 14:18
@Mashimiao
Copy link
Author

ping @liangchenye @mrunalp

@liangchenye
Copy link
Member

e4b2183 LGTM

@mrunalp
Copy link
Contributor

mrunalp commented Jun 30, 2016

LGTM

@mrunalp mrunalp merged commit 239e8e9 into opencontainers:master Jun 30, 2016
@Mashimiao Mashimiao deleted the runtime-test-test-linux-default-fs branch November 14, 2016 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mashimiao @wking @liangchenye @mrunalp