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 volume detach while mount in progress #71145

Merged
merged 1 commit into from
Nov 23, 2018
Merged

Fix volume detach while mount in progress #71145

merged 1 commit into from
Nov 23, 2018

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Nov 16, 2018 •
edited

Fix volume getting detach while being mounted or formatted.

Fixes #70319

Do not detach volume if mount in progress

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/kubelet needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 16, 2018
@gnufied
Copy link
Member Author

gnufied commented Nov 17, 2018

/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 Nov 17, 2018
@gnufied gnufied changed the title Test mounted detach Fix volume detach while mount in progress Nov 17, 2018
@gnufied gnufied mentioned this pull request Nov 17, 2018
Closed
@saad-ali saad-ali added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 17, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 17, 2018
@saad-ali saad-ali added this to the v1.13 milestone Nov 17, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 18, 2018
@AishSundar
Copy link
Contributor

@saad-ali waiting on LGTM ^^

jingxu97
jingxu97 reviewed Nov 19, 2018
View reviewed changes
pkg/kubelet/volumemanager/cache/actual_state_of_world.go Outdated

// GetAttachedVolumes returns a list of volumes that is known to be attached
// to the node. This list can be used to determine volumes that are either in-use
// or have an operation pending.
Copy link
Contributor

Choose a reason for hiding this comment

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

// or might have a mount/unmount operation pending

@gnufied
Copy link
Member Author

gnufied commented Nov 20, 2018

/test pull-kubernetes-local-e2e-containerized

@saad-ali
Copy link
Member

@saad-ali waiting on LGTM ^^

@AishSundar as soon as @gnufied and @jingxu97 give me a thumbs up, I'll take a look and approve.

@gnufied
Copy link
Member Author

gnufied commented Nov 20, 2018

/test pull-kubernetes-integration

1 similar comment
@gnufied
Copy link
Member Author

gnufied commented Nov 20, 2018

/test pull-kubernetes-integration

jingxu97
jingxu97 reviewed Nov 20, 2018
View reviewed changes
test/e2e/testing-manifests/flexvolume/attachable-with-long-format
@@ -0,0 +1,145 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

how about name it to attachable-with-long-mount?
Also each time we want to modify the volume plugin behavior, we have to create a new file with almost the same code?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can rename in a follow up PR, since this is very close to 1.13. I will follow up this in - #71243

What kind of behaviour modification to volume plugin you are talking about? are you thinking of using flex to simulate some other edge cases in attach/detach flow?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it would be nice if we could simulate different cases on this volume plugin.

@jingxu97
Copy link
Contributor

/lgtm just a small question about the test

@gnufied
Copy link
Member Author

gnufied commented Nov 20, 2018

/test pull-kubernetes-integration

@AishSundar
Copy link
Contributor

@jingxu97 @saad-ali does it looks good to go in 1.13?

@jingxu97
Copy link
Contributor

@AishSundar The PR looks good for me.

@msau42 msau42 mentioned this pull request Nov 22, 2018
Open
@AishSundar
Copy link
Contributor

Thanks @jingxu97 can either you or @saad-ali lgtm plz

@jingxu97
Copy link
Contributor

I think @saad-ali want to give a final check whether to merge it or not.

@saad-ali
Copy link
Member

/lgtm
/approve

This would be good to get in to 1.13. It's a small enough change, has e2e test to verify it, and it fixes a pretty bad race.

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit 205eaee into kubernetes:master Nov 23, 2018
@saad-ali
Copy link
Member

Let's cherry pick this back to k8s v1.12, v1.11, v1.10

@gnufied
Copy link
Member Author

gnufied commented Nov 29, 2018

@saad-ali yeah I will. with fix that fixes e2e as well.

This was referenced Nov 30, 2018
Merged
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Dec 11, 2018
@k8s-ci-robot
Merge pull request #71620 from gnufied/automated-cherry-pick-of-#71145-…
f614805 
…#71429-upstream-release-1.10

Automated cherry pick of #71145: Fix bug with volume getting marked as not in-use with pending #71429: Fix flake with e2e test that checks detach while mount in
@DaveKriegerEmerald DaveKriegerEmerald mentioned this pull request Jan 9, 2019
Closed
k8s-ci-robot added a commit that referenced this pull request Jan 17, 2019
@k8s-ci-robot
Merge pull request #71619 from gnufied/automated-cherry-pick-of-#71145-…
0fdd118 
…#71429-upstream-release-1.11

Automated cherry pick of #71145: Fix bug with volume getting marked as not in-use with pending #71429: Fix flake with e2e test that checks detach while mount in
k8s-ci-robot added a commit that referenced this pull request Jun 26, 2019
@k8s-ci-robot
Merge pull request #71618 from gnufied/automated-cherry-pick-of-#71145-…
13f7884 
…#71429-upstream-release-1.12

Automated cherry pick of #71145: Fix bug with volume getting marked as not in-use with pending #71429: Fix flake with e2e test that checks detach while mount in
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingxu97 jingxu97 jingxu97 left review comments

@andyzhangx andyzhangx Awaiting requested review from andyzhangx

@ixdy ixdy Awaiting requested review from ixdy

Assignees

@saad-ali saad-ali

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet 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/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.13
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gnufied @AishSundar @jingxu97 @saad-ali @k8s-ci-robot