-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Don't attempt to make and chmod subPath if it already exists #45623
Conversation
Hi @wongma7. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
@k8s-bot ok to test |
@kubernetes/sig-node-pr-reviews pretty severe bug preventing pods from starting, please review :) |
/sig sig/node zzz |
safe guard lgtm |
This LGTM, i'll ask someone from node to approve. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, wongma7
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue |
Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
Volume mounting logic introduced in kubernetes#43775 and kubernetes#45623 checks for subPath existence before attempting to create a directory, should subPath not be present. This breaks if subPath is a dangling symlink, os.Stat returns "do not exist" status, yet `os.MkdirAll` can't create directory as symlink is present at the given path. This patch makes existence check to use os.Lstat which works for normal files/directories as well as doesn't not attempt to follow symlink, therefore it's "do not exist" status is more reliable when making a decision whether to create directory or not. subPath symlinks can be dangling in situations where kubelet is running in a container itself with access to docker socket, such as CoreOS's kubelet-wrapper script
Automatic merge from submit-queue Fix subPath existence check to not follow symlink **What this PR does / why we need it**: Volume mounting logic introduced in #43775 and #45623 checks for subPath existence before attempting to create a directory, should subPath not be present. This breaks if subPath is a dangling symlink, os.Stat returns "do not exist" status, yet `os.MkdirAll` can't create directory as symlink is present at the given path. This patch makes existence check to use os.Lstat which works for normal files/directories as well as doesn't not attempt to follow symlink, therefore it's "do not exist" status is more reliable when making a decision whether to create directory or not. subPath symlinks can be dangling in situations where kubelet is running in a container itself with access to docker socket, such as CoreOS's kubelet-wrapper script **Release note**: ```release-note Fix pods failing to start when subPath is a dangling symlink from kubelet point of view, which can happen if it is running inside a container ```
Volume mounting logic introduced in kubernetes#43775 and kubernetes#45623 checks for subPath existence before attempting to create a directory, should subPath not be present. This breaks if subPath is a dangling symlink, os.Stat returns "do not exist" status, yet `os.MkdirAll` can't create directory as symlink is present at the given path. This patch makes existence check to use os.Lstat which works for normal files/directories as well as doesn't not attempt to follow symlink, therefore it's "do not exist" status is more reliable when making a decision whether to create directory or not. subPath symlinks can be dangling in situations where kubelet is running in a container itself with access to docker socket, such as CoreOS's kubelet-wrapper script
fixes #45613
#43775 fixed one bug and introduced another... I overlooked that subPaths can be files, in which case MkDirAll will simply fail and the pod will not able to start.
Regardless of whether it is a directory or a file, there is no need to introduce the MkdirAll->Chmod procedure if it exists, because if it exists, it should already have the correct permissions.
This needs to be cherry-picked into 1.6.