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.

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 cinder volume dir umount issue #24717 #24718

Merged
merged 1 commit into from
May 6, 2016

Conversation

chengyli
Copy link
Contributor

fix #24717
If kubelet root-dir is a symlink, the pod's cinder volume dir can't be
umounted even after pod is deleted.
This patch reads target path of symlink before comparing with entries in
proc mounts.

@k8s-bot
Copy link

k8s-bot commented Apr 24, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented Apr 24, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Apr 24, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Apr 24, 2016
@zhouhaibing089
Copy link
Contributor

https://github.com/kubernetes/kubernetes/blob/master/pkg/util/mount/mount.go#L112 should the mountPath here be evaluated as symlinks as well?

@chengyli
Copy link
Contributor Author

@zhouhaibing089 Thanks. Yes, this mountPath need do EvalSymlinks as well.

@bgrant0607 bgrant0607 assigned saad-ali and unassigned bgrant0607 Apr 25, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 25, 2016
@chengyli
Copy link
Contributor Author

Could anyone guide me how to find the detail errors for below FAILURE?

TeamCity OSS :: Kubernetes Mesos :: 4 - Smoke Tests Build 22537 outcome was FAILURE
Summary: Exit code 1 Build time: 00:14:04

I can only see below error message in build log, but no much clue

[19:06:36]E:     [Step 6/6] Step Clean, Build, Kube-Up, Smoke Test, Kube-Down (Command Line) failed

@zhouhaibing089
Copy link
Contributor

@chengyli you could rebase your code to get a clean test again. I thought the test has failed for a long time, though not the way failed like you met.

fix kubernetes#24717
If kubelet root-dir is a symlink, the pod's cinder volume dir can't be
umounted even after pod is deleted.
This patch reads target path of symlink before comparing with entries in
proc mounts.
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 29, 2016
@eparis eparis removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 29, 2016
@chengyli
Copy link
Contributor Author

chengyli commented May 1, 2016

@saad-ali any actions reuiqred from my side? And who could help to review the code?

@saad-ali
Copy link
Member

saad-ali commented May 2, 2016

I'll take a look. CC @kubernetes/sig-storage

@saad-ali
Copy link
Member

saad-ali commented May 2, 2016

LGTM

@saad-ali saad-ali added 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. and removed release-note-label-needed labels May 2, 2016
@saad-ali
Copy link
Member

saad-ali commented May 2, 2016

@k8s-bot ok to test

@chengyli
Copy link
Contributor Author

chengyli commented May 4, 2016

@saad-ali , is the test failure caused by this issue #25037?

@saad-ali
Copy link
Member

saad-ali commented May 4, 2016

Yes, that does appear to be the case.

@k8s-github-robot
Copy link

@saad-ali
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2016
@saad-ali
Copy link
Member

saad-ali commented May 4, 2016

@k8s-bot test this issue: #25037

@saad-ali saad-ali added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2016
@k8s-bot
Copy link

k8s-bot commented May 5, 2016

GCE e2e build/test passed for commit 1fb33aa.

@chengyli
Copy link
Contributor Author

chengyli commented May 6, 2016

@saad-ali , could you please help to merge the pr, thanks!

@saad-ali
Copy link
Member

saad-ali commented May 6, 2016

It will automatically be merged by the merge-bot. The submit queue is pretty backed up, so it might take a few days.

@chengyli
Copy link
Contributor Author

chengyli commented May 6, 2016

@saad-ali Got it, thanks!

@roberthbailey
Copy link
Contributor

@chengyli I'm merging a bunch of small PRs to clear out the queue. In it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cinder volume can't be unmounted when pod is deleted
9 participants