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 local volume directory can't be deleted issue #65310

Merged
merged 1 commit into from
Jun 30, 2018
Merged

Fix local volume directory can't be deleted issue #65310

merged 1 commit into from
Jun 30, 2018

Conversation

wenlxie
Copy link
Contributor

@wenlxie wenlxie commented Jun 21, 2018

What this PR does / why we need it:
Need to add volume mode field to constructed pv spec.

Special notes for your reviewer:
I get an issue:

  1. kubelet has lots of logs with errors related with volume mode
Jun 21 10:31:18  kubelet[19333]: E0621 10:31:18.422321   19333 reconciler.go:179] operationExecutor.NewVolumeHandler for UnmountVolume failed for volume "lv-e57cf589-4658-4881-b125-7b9f35c2c8eb" (UniqueName: "kubernetes.io/local-volume/4103e613-656c-11e8-8c20-74dbd180ddb4-lv-e57cf589-4658-4881-b125-7b9f35c2c8eb") pod "4103e613-656c-11e8-8c20-74dbd180ddb4" (UID: "4103e613-656c-11e8-8c20-74dbd180ddb4") : cannot get volumeMode for volume: lv-e57cf589-4658-4881-b125-7b9f35c2c8eb
Jun 21 10:31:18  kubelet[19333]: E0621 10:31:18.422351   19333 reconciler.go:179] operationExecutor.NewVolumeHandler for UnmountVolume failed for volume "lv-b1e788ac-78eb-4d26-819a-263cef5337ea" (UniqueName: "kubernetes.io/local-volume/4082c1da-656c-11e8-8c20-74dbd180ddb4-lv-b1e788ac-78eb-4d26-819a-263cef5337ea") pod "4082c1da-656c-11e8-8c20-74dbd180ddb4" (UID: "4082c1da-656c-11e8-8c20-74dbd180ddb4") : cannot get volumeMode for volume: lv-b1e788ac-78eb-4d26-819a-263cef5337ea

  1. The pod is an orphan pod and have the volume directory left at the node

  2. Because of the errors, the volume directory will never be deleted

Release note:

Fix local volume directory can't be deleted because of volumeMode error

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 21, 2018
@k8s-ci-robot k8s-ci-robot requested review from saad-ali and vishh June 21, 2018 12:18
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 21, 2018
@wenlxie
Copy link
Contributor Author

wenlxie commented Jun 21, 2018

/sig storage

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jun 21, 2018
@dixudx
Copy link
Member

dixudx commented Jun 21, 2018

/ok-to-test

@k8s-ci-robot 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 Jun 21, 2018
@wenlxie
Copy link
Contributor Author

wenlxie commented Jun 23, 2018

@kubernetes/sig-storage-pr-reviews

@k8s-ci-robot
Copy link
Contributor

@wenlxie: Reiterating the mentions to trigger a notification:
@kubernetes/sig-storage-pr-reviews

In response to this:

@kubernetes/sig-storage-pr-reviews

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.

msau42
msau42 reviewed Jun 25, 2018
View reviewed changes
pkg/volume/local/local.go
@@ -169,6 +169,7 @@ func (plugin *localVolumePlugin) NewBlockVolumeUnmapper(volName string,

// TODO: check if no path and no topology constraints are ok
func (plugin *localVolumePlugin) ConstructVolumeSpec(volumeName, mountPath string) (*volume.Spec, error) {
fs := v1.PersistentVolumeFilesystem
Copy link
Member

Choose a reason for hiding this comment

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

I think CSI volume driver also has the same issue. Could you update the csi plugin too?

@saad-ali this is another instance of #52820

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msau42 Thanks for review this.
CSI plugin also updated: #65456

Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msau42 unit test added.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 29, 2018
@msau42
Copy link
Member

msau42 commented Jun 29, 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 Jun 29, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, wenlxie

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 Jun 29, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 65582, 65480, 65310, 65644, 65645). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 55c5aaa into kubernetes:master Jun 30, 2018
@wenlxie wenlxie deleted the upstream.master.fixlocalvolumevmnotfound branch October 24, 2019 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@saad-ali saad-ali Awaiting requested review from saad-ali

@vishh vishh Awaiting requested review from vishh

Assignees

@msau42 msau42

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.
5 participants
@wenlxie @dixudx @k8s-ci-robot @msau42 @k8s-github-robot