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.

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

Do not report error when deleting an attached volume #31978

Merged
merged 2 commits into from Sep 26, 2016

Conversation

jsafrane
Copy link
Member

@jsafrane jsafrane commented Sep 2, 2016

Persistent volume controller should not send warning events to a PV and mark the PV as failed when the volume is still attached.

This happens when a user quickly deletes a pod and associated PVC - PV is slowly detaching, while the PVC is already deleted and the PV enters Failed phase.

Deleter.Deleter can now return tryAgainError, which is sent as INFO to the PV to let the user know we did not forget to delete the PV, however the PV stays in Released state. The controller tries again in the next sync (15 seconds by default).

Fixes #31511


This change is Reviewable

@jsafrane jsafrane added sig/storage Categorizes an issue or PR as relevant to SIG Storage. team/cluster release-note-none Denotes a PR that doesn't merit a release note. labels Sep 2, 2016
@jsafrane
Copy link
Member Author

jsafrane commented Sep 2, 2016

cc @kubernetes/sig-storage

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 2, 2016
@thockin
Copy link
Member

thockin commented Sep 2, 2016

Do we want this in 1.4 (maybe 1.4.1) ?

On Fri, Sep 2, 2016 at 7:52 AM, Kubernetes Submit Queue <
notifications@github.com> wrote:

Labelling this PR as size/L


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#31978 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVMWZvBiy7gw_A9p236pOlTlpAQtvks5qmDgUgaJpZM4Jzxyi
.

glog.V(4).Infof("deleteVolumeOperation [%s]: failed to mark volume as failed: %v", volume.Name, err)
// Save failed, retry on the next deletion attempt
return
if vol.IsTryAgain(err) {
Copy link
Member

Choose a reason for hiding this comment

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

How frequent is the retry? Is it worth doing exponential backoff?

Copy link
Member Author

Choose a reason for hiding this comment

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

Every controller sync, which is 15 seconds by default. And it does not start new delete until an old one (on the same PV) returns.

@jsafrane jsafrane force-pushed the detach-before-delete branch 2 times, most recently from 9023ec9 to d20131d Compare September 7, 2016 10:48
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2016
@jsafrane
Copy link
Member Author

rebased

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 15, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 15, 2016
@jsafrane
Copy link
Member Author

@k8s-bot: test this issue: #32772

1 similar comment
@jsafrane
Copy link
Member Author

@k8s-bot: test this issue: #32772

@jsafrane
Copy link
Member Author

@k8s-bot: unit test this issue: #32772

@jsafrane
Copy link
Member Author

@k8s-bot test this issue: #32772

@k8s-bot
Copy link

k8s-bot commented Sep 15, 2016

GCE e2e build/test passed for commit 9903b38.

@gnufied
Copy link
Member

gnufied commented Sep 23, 2016

I am shadowing this review. lgtm!

@saad-ali
Copy link
Member

LGTM

@saad-ali saad-ali added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 9903b38. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 4785f6f into kubernetes:master Sep 26, 2016
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Automatic merge from submit-queue

Do not report error when deleting an attached volume

Persistent volume controller should not send warning events to a PV and mark the PV as failed when the volume is still attached.

This happens when a user quickly deletes a pod and associated PVC - PV is slowly detaching, while the PVC is already deleted and the PV enters Failed phase.

`Deleter.Deleter` can now return `tryAgainError`, which is sent as INFO to the PV to let the user know we did not forget to delete the PV, however the PV stays in Released state. The controller tries again in the next sync (15 seconds by default).

Fixes kubernetes#31511
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants