-
Notifications
You must be signed in to change notification settings - Fork 349
Ensure the mount point is propagated #230
Ensure the mount point is propagated #230
Conversation
d8ca3ff
to
19ed6fe
Compare
The CI failed because we have not used the real path. |
0c13442
to
d391d7b
Compare
@miaoyq Will review this PR after Thursday. BTW, would you mind me mention ZTE is contributing to |
Of course not, i really appreciate that. : ) |
I've manually made unit test locally, and it works, but the travis-ci always failed. |
Related to containerd#230 Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>
d391d7b
to
fca9631
Compare
@Random-Liu The travis-ci run unit test as non root user, but mount operation need root user. I have create a pr for this, see #246, PTAL, Thanks :) |
pkg/server/container_create_test.go
Outdated
@@ -48,8 +50,57 @@ func checkMount(t *testing.T, mounts []runtimespec.Mount, src, dest, typ string, | |||
assert.True(t, found, "mount from %q to %q not found", src, dest) | |||
} | |||
|
|||
func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandboxConfig, | |||
*imagespec.ImageConfig, func(*testing.T, string, uint32, *runtimespec.Spec)) { | |||
func createHostPath(t *testing.T, mounts []*runtime.Mount) { |
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.
Don't think we should test this in unit test.
We have replaced or os
with FakeOS
. This test won't work.
If we really want to test, we should leverage the FakeOS
to test.
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'll add our own integration test framework, we could add this kind of test there.
pkg/server/container_create.go
Outdated
optsSplit := strings.Split(optionalOpts, " ") | ||
for _, opt := range optsSplit { | ||
if strings.HasPrefix(opt, "shared:") { | ||
sharedMount = true |
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.
return nil directly.
pkg/server/container_create.go
Outdated
sharedMount = true | ||
break | ||
} else if strings.HasPrefix(opt, "master:") { | ||
slaveMount = true |
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.
return nil directly.
pkg/server/container_create.go
Outdated
slaveMount = true | ||
break | ||
} | ||
} |
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.
return error here.
pkg/server/container_create.go
Outdated
|
||
func getSourceMount(source string) (string, string, error) { | ||
// Ensure any symlinks are resolved. | ||
sourcePath, err := filepath.EvalSymlinks(source) |
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.
No need to resolve symlink. We've done that in addOCIBindMounts
.
pkg/server/container_create.go
Outdated
return "", "", err | ||
} | ||
|
||
mountinfos, err := mount.GetMounts() |
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.
Add GetMounts
to os package, so that you could use fake os in unit test, and please add unit test based on that.
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.
That's better. 👍
pkg/server/container_create.go
Outdated
optsSplit := strings.Split(optionalOpts, " ") | ||
for _, opt := range optsSplit { | ||
if strings.HasPrefix(opt, "shared:") { | ||
sharedMount = true |
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.
return nil here.
pkg/server/container_create.go
Outdated
break | ||
} | ||
} | ||
|
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.
return error.
pkg/server/container_create.go
Outdated
return nil | ||
} | ||
|
||
func getSourceMount(source string) (string, string, error) { |
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.
Probably move this to helper.
@Random-Liu Thank you for review, I'm addressing the comments. |
fca9631
to
b59c02e
Compare
@Random-Liu Have addressed, PTAL, Thanks! |
@@ -384,3 +385,35 @@ func isInCRIMounts(dst string, mounts []*runtime.Mount) bool { | |||
} | |||
return false | |||
} | |||
|
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.
Add TODO to replace with mount.Lookup()
in containerd.
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.
And please replace with mount.Loopup()
after #257 is merged.
pkg/server/container_create.go
Outdated
} | ||
} | ||
|
||
return fmt.Errorf("path %s is mounted on %s but it is not a shared mount", path, sourceMount) |
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.
Use %q
for string.
pkg/server/container_create.go
Outdated
return nil | ||
} | ||
} | ||
return fmt.Errorf("path %s is mounted on %s but it is not a shared or slave mount", path, sourceMount) |
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.
ditto.
pkg/server/container_create_test.go
Outdated
imagespec "github.com/opencontainers/image-spec/specs-go/v1" | ||
runtimespec "github.com/opencontainers/runtime-spec/specs-go" | ||
"github.com/opencontainers/runtime-tools/generate" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
"k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" | ||
|
||
testing2 "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" |
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.
ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing"
pkg/server/container_create_test.go
Outdated
}, | ||
fakeGetMountsFn: nil, | ||
optionsCheck: []string{"rbind", "rprivate"}, | ||
}, |
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.
Should add error test case, which host path propagation is not properly set, we should expect an error from addOCIBindMounts
.
b59c02e
to
24930a2
Compare
@Random-Liu Thanks for review, Have addressed. |
24930a2
to
f750aa5
Compare
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.
LGTM with one nit. Good to go after address the nit.
} | ||
|
||
for desc, test := range map[string]struct { | ||
criMount *runtime.Mount |
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.
Add expectErr bool
.
We could:
- assert error, if
expectErr==true
. - assert no error and check options, if
expectErr==false
.
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.
OK, I will change according to your instructions, thanks!
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.
Done!
f750aa5
to
49eb38a
Compare
mount with `rshared`, the host path should be shared. mount with `rslave`, the host pash should be shared or slave. Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>
LGTM |
The test failure feels like a flake, filed #259 for it. Will retrigger the test. |
mount with
rshared
, the host path should be shared.mount with
rslave
, the host pash should be shared or slave.Based on #246
Signed-off-by: Yanqiang Miao miao.yanqiang@zte.com.cn
/cc @Random-Liu