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

Fix iSCSI panic in DetachDisk #69140

Merged
merged 2 commits into from
Oct 3, 2018
Merged

Fix iSCSI panic in DetachDisk #69140

merged 2 commits into from
Oct 3, 2018

Conversation

jsafrane
Copy link
Member

There are two commits there:

  • Fill iscsiDetacher.plugin so DetachDisk does not panic.
  • Don't unmount already unmounted directory.

Fixes #69137

Fixed panic on iSCSI volume tear down.

/sig storage
/priority important-soon
cc @humblec @bswartz @redbaron

@jsafrane
Fixed panic in iSCSI.UnmountDevice
ffaff4e 
Fill iscsiDetacher.plugin so iscsiDetacher.plugin.targetLocks.LockKey(iqn) does not
panic.
@jsafrane
Unmount iSCSI device only if it's mounted.
e5fbb35
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 27, 2018
redbaron
redbaron reviewed Sep 27, 2018
View reviewed changes
pkg/volume/iscsi/iscsi_util.go
return err
}
if !notMnt {
Copy link
Contributor

Choose a reason for hiding this comment

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

can it change between check and umount attempt? maybe unmount unconditionally but recognise a "not mounted" error ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Kubelet itself makes sure that only one UnmountDevice runs for a given volume. So it can't change unless something else (admin) unmounts things in parallel.

And even if the volume is unmounted by admin after the check, the volume plugin calls umount, which fails, and kubelet re-tries UnmountDevice in a short while, now with correct check.

Copy link
Contributor

Choose a reason for hiding this comment

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

It just feels akin to file exist check before deleting it, just deleting is more correct and no races

@bswartz
Copy link
Contributor

bswartz commented Sep 28, 2018

This is a good catch. I think it was my fix to prevent login/logout race conditions that created this bug, and I need to figure out why my testing didn't catch this. Sorry!

@jsafrane
Copy link
Member Author

jsafrane commented Oct 1, 2018

/retest

@jsafrane
Copy link
Member Author

jsafrane commented Oct 1, 2018

@rootfs @bswartz @humblec, can you please review it? @bswartz, don't be afraid of /lgtm :-)

bertinatto
bertinatto approved these changes Oct 1, 2018
View reviewed changes
Copy link
Member

@bertinatto bertinatto left a comment

Choose a reason for hiding this comment

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

/lgtm

(Although I'm not well versed in these bits).

pkg/volume/iscsi/iscsi_util.go
return err
}
if !notMnt {
if err := c.mounter.Unmount(mntPath); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to use k8s.io/kubernetes/pkg/volume/util's UnmountPath instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, fixed.

@k8s-ci-robot
Copy link
Contributor

@bertinatto: changing LGTM is restricted to assignees, and only kubernetes/kubernetes repo collaborators may be assigned issues.

In response to this:

/lgtm

(Although I'm not well versed in these bits).

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.

@bswartz
Copy link
Contributor

bswartz commented Oct 1, 2018

/lgtm

@k8s-ci-robot
Copy link
Contributor

@bswartz: changing LGTM is restricted to assignees, and only kubernetes/kubernetes repo collaborators may be assigned issues.

In response to this:

/lgtm

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.

@jsafrane
Copy link
Member Author

jsafrane commented Oct 2, 2018

I got second thoughts about using util.UnmountPath(), it will remove the directory where the volume was mounted. Kubelet will loose track about this volume if iscsi logout fails and kubelet is restarted - it uses the directory as a memento that there is not fully cleaned volume and some work needs to be done.

I restored the original behavior and the directory will be removed only when DetachDisk returns successfully.

@humblec
Copy link
Contributor

humblec commented Oct 3, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, bswartz, humblec, 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

@bertinatto
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 3, 2018
@k8s-ci-robot k8s-ci-robot merged commit 984b7c8 into kubernetes:master Oct 3, 2018
@jsafrane jsafrane mentioned this pull request Oct 3, 2018
Merged
k8s-ci-robot added a commit that referenced this pull request Oct 4, 2018
@k8s-ci-robot
Merge pull request #69365 from jsafrane/automated-cherry-pick-of-#691…
b885435 
…40-upstream-release-1.12

Automated cherry pick of #69140: Fixed panic in iSCSI.UnmountDevice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@redbaron redbaron redbaron left review comments

@bertinatto bertinatto bertinatto approved these changes

@humblec humblec Awaiting requested review from humblec

@mtanino mtanino Awaiting requested review from mtanino

Assignees

@humblec humblec

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/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jsafrane @bswartz @k8s-ci-robot @humblec @bertinatto @redbaron