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.

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

fix local volume absolute path issue on Windows #62018

Merged
merged 2 commits into from Apr 10, 2018

Conversation

@andyzhangx
Member

andyzhangx commented Apr 2, 2018

What this PR does / why we need it:
remove IsAbs validation on local volume since it does not work on windows cluster, Windows absolute path D: is not allowed in local volume, the validation happens on both master and agent node, while for windows cluster, the master is Linux and agent is Windows, so path.IsAbs() func will not work all in both nodes.
Instead, this PR use MakeAbsolutePath func to convert local.path value in kubelet, it supports both linux and windows styple.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #62016

Special notes for your reviewer:

Release note:

fix local volume absolute path issue on Windows

/sig storage
/sig windows

@andyzhangx andyzhangx changed the title from remove IsAbs validation on local volume to fix local volume absolute path issue on Windows Apr 2, 2018

@msau42

This comment has been minimized.

Member

msau42 commented Apr 2, 2018

I think similarly to #55665, we need to make absolute path on the kubelet (local.go) side, otherwise the user could be allowed to mount a relative path.

/assign

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 4, 2018

@msau42 I have moved MakeAbsolutePath func from kubelet_pods.go to volume\utils.go, and use same logic as #55665, PTAL, thanks.

@@ -311,7 +311,8 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
}
glog.V(4).Infof("attempting to mount %s", dir)
err = m.mounter.Mount(m.globalPath, dir, "", options)
globalPath := util.MakeAbsolutePath(runtime.GOOS, m.globalPath)

This comment has been minimized.

@msau42

msau42 Apr 4, 2018

Member

SetUpDevice() also needs to be updated

This comment has been minimized.

@andyzhangx
@msau42

This comment has been minimized.

Member

msau42 commented Apr 4, 2018

Can you add unit tests in the local volume plugin?

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 5, 2018

@msau42 I just found there are no local volume tests for windows, I will enable those tests on windows also.

// MakeAbsolutePath convert path to absolute path according to GOOS
func MakeAbsolutePath(goos, path string) string {
if goos != "windows" {
return "/" + path

This comment has been minimized.

@msau42

msau42 Apr 5, 2018

Member

Can you also call filepath.Clean() on the path, so things like "//path" are simplified?

This comment has been minimized.

@andyzhangx
remove IsAbs validation on local volume
use MakeAbsolutePath to convert path in Windows

fix test error: allow relative path for local volume

fix comments

fix comments and add windows unit tests
@andyzhangx

@msau42 I have splitted the unit tests of local volume, most of them should also run on Windows too. PTAL, thanks.

@@ -311,7 +311,8 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
}
glog.V(4).Infof("attempting to mount %s", dir)
err = m.mounter.Mount(m.globalPath, dir, "", options)
globalPath := util.MakeAbsolutePath(runtime.GOOS, m.globalPath)

This comment has been minimized.

@andyzhangx
// MakeAbsolutePath convert path to absolute path according to GOOS
func MakeAbsolutePath(goos, path string) string {
if goos != "windows" {
return "/" + filepath.Clean(path)

This comment has been minimized.

@msau42

msau42 Apr 6, 2018

Member

Clean should be called after prepending "/". Ie, if the user already specified an absolute path. A test case should be added too

This comment has been minimized.

@andyzhangx

andyzhangx Apr 8, 2018

Member

@msau42 thanks, fixed, and added a new unit test

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 8, 2018

/test pull-kubernetes-bazel-test

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 8, 2018

/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-e2e-gce-device-plugin-gpu

@msau42

This comment has been minimized.

Member

msau42 commented Apr 9, 2018

LGTM please squash

/assign @thockin
for approval

@thockin

This comment has been minimized.

Member

thockin commented Apr 9, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 9, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 9, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 10, 2018

@andyzhangx: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-kubemark-e2e-gce-big d736986 link /test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 10, 2018

/test pull-kubernetes-integration
/test pull-kubernetes-kubemark-e2e-gce-big

@fejta-bot

This comment has been minimized.

fejta-bot commented Apr 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 10, 2018

Automatic merge from submit-queue (batch tested with PRs 61147, 62236, 62018). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 865d3cf into kubernetes:master Apr 10, 2018

15 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation andyzhangx authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-merge-robot added a commit that referenced this pull request May 15, 2018

Merge pull request #62615 from andyzhangx/automated-cherry-pick-of-#6…
…2018-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #62018: remove IsAbs validation on local volume

Cherry pick of #62018 on release-1.10.

#62018: remove IsAbs validation on local volume
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment