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 nsenter GetFileType issue in containerized kubelet #62467

Merged
merged 1 commit into from Apr 13, 2018

Conversation

@andyzhangx
Member

andyzhangx commented Apr 12, 2018

What this PR does / why we need it:
UnmountDevice would fail in containerized kubelet in v1.9.x, and in the end, pod with volume mount could not be scheduled from one node to another since the original volume will never be released. This PR fixed this issue.

In nsenter_mount.GetFileType func, return error of following code will be different than mount_linux.GetFileType func

outputBytes, err := mounter.ne.Exec("stat", []string{"-L", `--printf "%F"`, pathname}).CombinedOutput()

Return error and output would be like following in nsenter_mount.GetFileType func:
error: exit status 1
output: /usr/bin/stat: cannot stat '2': No such file or directory

This PR makes the return error consistent as mount_linux.GetFileType func, and finally makes isDeviceOpened func work.

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 #62282

Special notes for your reviewer:
/assign @dixudx

Release note:

fix nsenter GetFileType issue in containerized kubelet

/sig node

@dixudx

This comment has been minimized.

Member

dixudx commented Apr 12, 2018

Dup of #61802?

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 12, 2018

@dixudx Actually this PR contains #61802, it also has the error translation in nsenter_mount.GetFileType func. BTW, I would suggest return outputBytes string if there is error since the error var will always return string like exit status 1 which does not make any sense, outputBytes return the actually error message.

@dixudx

This comment has been minimized.

Member

dixudx commented Apr 12, 2018

func isDeviceOpened(deviceToDetach AttachedVolume, mounter mount.Interface) (bool, error) {
isDevicePath, devicePathErr := mounter.PathIsDevice(deviceToDetach.DevicePath)
var deviceOpened bool
var deviceOpenedErr error
if !isDevicePath && devicePathErr == nil ||
(devicePathErr != nil && strings.Contains(devicePathErr.Error(), "does not exist")) {
// not a device path or path doesn't exist
//TODO: refer to #36092
glog.V(3).Infof("The path isn't device path or doesn't exist. Skip checking device path: %s", deviceToDetach.DevicePath)
deviceOpened = false

Currently method isDeviceOpened will check hard-coded string does not exist, which is hard to notice this.

Thanks @andyzhangx fixing such an annoying bug. Really appreciate.

/lgtm

fix nsenter GetFileType issue
use outputBytes as return error

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 12, 2018

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 12, 2018

@dixudx
I changed the PR a little, always use string(outputBytes) as return error since the original err ( like exit status 1) does not make any sense

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

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 12, 2018

/assign @jsafrane

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Apr 12, 2018

/test pull-kubernetes-e2e-kops-aws

if err != nil {
if strings.Contains(string(outputBytes), "No such file") {

This comment has been minimized.

@msau42

msau42 Apr 12, 2018

Member

Is it possible to check for error codes instead of strings?

This comment has been minimized.

@andyzhangx

andyzhangx Apr 13, 2018

Member

@msau42 I have tried, if nsenter exec fails, error msg will always be like exit status 1
So the answer is No

@jsafrane

This comment has been minimized.

Member

jsafrane commented Apr 13, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, dixudx, feiskyer, jsafrane

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 13, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit f5e8dca into kubernetes:master Apr 13, 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 Apr 15, 2018

Merge pull request #62547 from andyzhangx/automated-cherry-pick-of-#6…
…2467-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #62467: fix nsenter GetFileType issue

Cherry pick of #62467 on release-1.9.

#62467: fix nsenter GetFileType issue

k8s-merge-robot added a commit that referenced this pull request Apr 20, 2018

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

Automatic merge from submit-queue.

Automated cherry pick of #62467

Cherry pick of #62467 on release-1.10.

#62467: fix nsenter GetFileType issue

@andyzhangx andyzhangx referenced this pull request May 18, 2018

Closed

PVC Failing to mount #376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment