Added missing error check that could cause kubelet to crash #60962

technicianted · 2018-03-09T05:45:32Z

What this PR does / why we need it:
Adds missing error check. An error can happen due to a race condition when watched files change, or become inaccessible. This can happen if a file was added to the driver directory then quickly removed, in which case the callback will be called with non-nil err and nil info, which is not checked, causing kubelet to crash.

Which issue(s) this PR fixes:
Fixes #60861

Special notes for your reviewer:

Release note:

Fixed missing error checking that could cause kubelet to crash in a race condition.

k8s-ci-robot · 2018-03-09T05:45:35Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

dims · 2018-03-09T11:15:14Z

/ok-to-test

dims · 2018-03-09T11:15:23Z

/sig storage

dims · 2018-03-09T11:15:40Z

/milestone v1.10

(per @saad-ali 's comment in issue)

dims · 2018-03-09T11:16:17Z

@technicianted hmm, you have signed the CLA already?

BenTheElder · 2018-03-09T17:19:48Z

Hi, GitHub doesn't seem to associate the commit with your account, have you verified the email address used in the commit with GitHub?

technicianted · 2018-03-09T21:27:44Z

@BenTheElder seems like it. Not sure how to fix the commit with the correct email.

BenTheElder · 2018-03-09T21:36:25Z

FWIW GitHub has an article here:
https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/
I checked out your PR and the commit seems to have an intel.com email which I'd guess is correct, but your email isn't public so I can't be sure.

dims · 2018-03-10T01:11:41Z

@technicianted it's easy to switch the email address for example see https://gist.github.com/trey/9588090

verult · 2018-03-10T01:37:06Z

pkg/volume/flexvolume/probe.go

-			if err := prober.watcher.AddWatch(path); err != nil {
-				glog.Errorf("Error recursively adding watch: %v", err)
+		if err == nil {
+			if info.IsDir() {


What about err == nil && info.IsDir()? Reduces nesting

technicianted · 2018-03-10T06:12:44Z

@dims I signed the CLA. I made sure my commits have the correct GitHub verified email.

dims · 2018-03-10T12:05:11Z

/test all

dims · 2018-03-10T13:50:51Z

@technicianted please reach out to the helpdesk - #27796 (comment)

here's what i see in the PR:

commit 0fb8072b1956cec112f9b961e324a21d69131d39 (HEAD)
Author: technicianted <osama.sorour@eformations.net>
Date:   Fri Mar 9 21:03:22 2018 -0800

    reduce nesting

commit 659d9df117531069026a2c31310702d6dee5d9f0
Author: technicianted <osama.sorour@eformations.net>
Date:   Thu Mar 8 21:39:22 2018 -0800

    added missing error check

technicianted · 2018-03-12T20:35:36Z

/retest

saad-ali · 2018-03-12T22:00:36Z

/lgtm
/approve

k8s-github-robot · 2018-03-12T22:07:36Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@saad-ali @technicianted @verult

Pull Request Labels

sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

saad-ali · 2018-03-12T22:14:48Z

Once this is merged, please make sure to cherry pick it back to relevant prior release branches (1.9, etc.)

k8s-github-robot · 2018-03-12T22:24:56Z

/test all [submit-queue is verifying that this PR is safe to merge]

verult · 2018-03-12T22:47:14Z

/lgtm

k8s-ci-robot · 2018-03-12T22:47:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saad-ali, technicianted, verult

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:

~~pkg/volume/flexvolume/OWNERS~~ [saad-ali]

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

k8s-github-robot · 2018-03-12T23:17:02Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

saad-ali · 2018-03-13T18:45:42Z

@technicianted please remember to cherry pick this PR back to relevant branches (1.9, etc.)

technicianted · 2018-03-13T22:17:10Z

/cherrypick-candidate

technicianted · 2018-03-13T22:18:38Z

@saad-ali please help with cherrypicking

dims · 2018-03-13T23:24:39Z

@technicianted can you please add a user facing release note in the body of the issue under "Release note:" section above? (currently has NONE there). the branch managers will ask for details.

technicianted · 2018-03-15T22:29:42Z

@dims added

dims · 2018-03-15T22:39:58Z

@technicianted thanks. i lined up the cherry picks for you. for next time, cherry pick process is here - https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md

…pstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #60962: added missing error check Cherry pick of #60962 on release-1.8. #60962: added missing error check

…pstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #60962: added missing error check Cherry pick of #60962 on release-1.9. #60962: added missing error check

added missing error check

659d9df

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 9, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 9, 2018

k8s-ci-robot requested review from rootfs and sjenning March 9, 2018 05:45

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 9, 2018

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Mar 9, 2018

k8s-ci-robot added this to the v1.10 milestone Mar 9, 2018

k8s-github-robot added the milestone/incomplete-labels label Mar 9, 2018

technicianted force-pushed the flex_race_fix branch from ce01ff7 to 659d9df Compare March 9, 2018 21:23

verult reviewed Mar 10, 2018

View reviewed changes

reduce nesting

0fb8072

technicianted mentioned this pull request Mar 10, 2018

Umbrella issue for the Linux Foundation (CNCF) CLA system #27796

Closed

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 12, 2018

k8s-ci-robot added this to the v1.10 milestone Mar 12, 2018

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/bug Categorizes issue or PR as related to a bug. labels Mar 12, 2018

k8s-github-robot added milestone/needs-approval and removed milestone/removed labels Mar 12, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 12, 2018

saad-ali mentioned this pull request Mar 12, 2018

FlexVolume probe race condition potentially crashes kubelet #60861

Closed

saad-ali added the status/approved-for-milestone label Mar 12, 2018

k8s-github-robot removed the milestone/needs-approval label Mar 12, 2018

k8s-github-robot merged commit 6571be1 into kubernetes:master Mar 12, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 13, 2018

This was referenced Mar 15, 2018

Automated cherry pick of #60962: added missing error check #61244

Merged

Automated cherry pick of #60962: added missing error check #61245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added missing error check that could cause kubelet to crash #60962

Added missing error check that could cause kubelet to crash #60962

technicianted commented Mar 9, 2018 •

edited

Loading

k8s-ci-robot commented Mar 9, 2018

dims commented Mar 9, 2018

dims commented Mar 9, 2018

dims commented Mar 9, 2018

dims commented Mar 9, 2018

BenTheElder commented Mar 9, 2018

technicianted commented Mar 9, 2018

BenTheElder commented Mar 9, 2018

dims commented Mar 10, 2018

verult Mar 10, 2018

technicianted commented Mar 10, 2018

dims commented Mar 10, 2018

dims commented Mar 10, 2018

technicianted commented Mar 12, 2018

saad-ali commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

saad-ali commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

verult commented Mar 12, 2018

k8s-ci-robot commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

saad-ali commented Mar 13, 2018

technicianted commented Mar 13, 2018

technicianted commented Mar 13, 2018

dims commented Mar 13, 2018

technicianted commented Mar 15, 2018

dims commented Mar 15, 2018 •

edited

Loading

Added missing error check that could cause kubelet to crash #60962

Added missing error check that could cause kubelet to crash #60962

Conversation

technicianted commented Mar 9, 2018 • edited Loading

k8s-ci-robot commented Mar 9, 2018

dims commented Mar 9, 2018

dims commented Mar 9, 2018

dims commented Mar 9, 2018

dims commented Mar 9, 2018

BenTheElder commented Mar 9, 2018

technicianted commented Mar 9, 2018

BenTheElder commented Mar 9, 2018

dims commented Mar 10, 2018

verult Mar 10, 2018

Choose a reason for hiding this comment

technicianted commented Mar 10, 2018

dims commented Mar 10, 2018

dims commented Mar 10, 2018

technicianted commented Mar 12, 2018

saad-ali commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

saad-ali commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

verult commented Mar 12, 2018

k8s-ci-robot commented Mar 12, 2018

k8s-github-robot commented Mar 12, 2018

saad-ali commented Mar 13, 2018

technicianted commented Mar 13, 2018

technicianted commented Mar 13, 2018

dims commented Mar 13, 2018

technicianted commented Mar 15, 2018

dims commented Mar 15, 2018 • edited Loading

technicianted commented Mar 9, 2018 •

edited

Loading

dims commented Mar 15, 2018 •

edited

Loading