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

If mountPath is missing, prefix with root dir. #55665

Merged
merged 2 commits into from
Nov 18, 2017
Merged

If mountPath is missing, prefix with root dir. #55665

merged 2 commits into from
Nov 18, 2017

Conversation

brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Nov 14, 2017 •
edited
Loading

If mountPath is not absolute, add the os-specific root directory as a prefix.

Ref: #51240
https://groups.google.com/forum/#!topic/kubernetes-sig-storage/k_0Wr2kYkpU

@thockin @saad-ali @andyzhangx

Note to @thockin I left the validation in place, in seems like it is a net win, since it will give a decent
error message to most people, but the fall-back defaulting is there if it doesn't catch 'c:'

I'm happy to rip out the validation if that is preferable to everyone. Let me know.

If a non-absolute mountPath is passed to the kubelet, prefix it with the appropriate root path.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 14, 2017
@brendandburns brendandburns added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 14, 2017
@saad-ali
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2017
@saad-ali
Copy link
Member

/approve no-issue

thockin
thockin reviewed Nov 14, 2017
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Need to remove all the apiserver validation, too.

And update API docs (comments)

And regenerate all the crap

pkg/kubelet/kubelet_pods.go
@@ -111,6 +111,23 @@ func (kl *Kubelet) makeDevices(pod *v1.Pod, container *v1.Container) ([]kubecont
return devices, nil
}

func makeAbsolutePath(goos, path string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Should this just be in kubelet_windows.go and kubelet_linux.go files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it here better, since that way it will run in the unit tests on either platform.

@thockin
Copy link
Member

thockin commented Nov 15, 2017 via email

@brendandburns
Copy link
Contributor Author

No, if you notice, the absolute path function takes the goos as a parameter, and I pass strings in the test, so I test most of the GOOS=windows path even on Linux.

@brendandburns
Copy link
Contributor Author

Also, can I get a decision between @thockin and @saad-ali about whether or not to rip out the validation?

@thockin
Copy link
Member

thockin commented Nov 15, 2017 via email

@brendandburns
Copy link
Contributor Author

fwiw, it never fires wrongly. it sometimes doesn't fire when it should...

Again, don't feel strongly either way, just looking for consensus.

@brendandburns
Copy link
Contributor Author

Friendly ping on this. Given @thockin's desire to get this in for 1.9, I need a quick consensus...

thanks
--brendan

@saad-ali
Copy link
Member

I think it's simpler just to remove it. @saad-ali - do you disagree?

I agree, let's pull it out.

@brendandburns
Copy link
Contributor Author

ok, will do.

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 16, 2017
@brendandburns
Copy link
Contributor Author

Validation removed, documentation updated, please take another look...

I tried to regenerate things, but none of the scripts I ran seemed to actually make any changes.

I guess I'll see if automated unit tests pass, and fix any problems that might occur...

@dims
Copy link
Member

dims commented Nov 16, 2017

/retest

@saad-ali
Copy link
Member

Unit test needs to be updated for removal of validation:

--- FAIL: TestValidateVolumeMounts (0.00s)
	validation_test.go:3862: expected failure for windows absolute subpath

Otherwise LGTM

@brendandburns
Copy link
Contributor Author

@saad-ali test fixed (that's what I get for not re-running tests after a rebase sigh)

ptal, thanks!

@brendandburns
Copy link
Contributor Author

@thockin tests are passing, @saad-ali LGTM'd any further comments?

Thanks
--brendan

@dims
Copy link
Member

dims commented Nov 17, 2017

LGTM 👍

@andyzhangx
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2017
@saad-ali
Copy link
Member

/lgtm
/approve

@saad-ali
Copy link
Member

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andyzhangx, brendandburns, saad-ali

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@brendandburns brendandburns added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55908, 55829, 55293, 55653, 55665). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 01c7414 into kubernetes:master Nov 18, 2017
@andyzhangx andyzhangx mentioned this pull request Nov 18, 2017
Closed
@andyzhangx
Copy link
Member

@brendanburns so will this PR cherry-pick to v1.8? I can help do that.

@andyzhangx andyzhangx mentioned this pull request Nov 27, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Nov 30, 2017
Merge pull request #56396 from andyzhangx/automated-cherry-pick-of-#5…
241db53 
…5665-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #55665

Cherry pick of #55665 on release-1.8.

#55665: If mountPath is missing, prefix with root dir.
@msau42 msau42 mentioned this pull request Apr 2, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

Assignees

@andyzhangx andyzhangx

@thockin thockin

@saad-ali saad-ali

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@brendandburns @saad-ali @thockin @dims @andyzhangx @k8s-github-robot @k8s-ci-robot