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

Retry recycle or delete operation on failure. #19365

Merged
merged 1 commit into from Feb 5, 2016
Merged

Retry recycle or delete operation on failure. #19365

merged 1 commit into from Feb 5, 2016
+328 −42

Conversation

jsafrane
Copy link
Member

@jsafrane jsafrane commented Jan 7, 2016

Recycle controller tries to recycle or delete a PV several times.
It stores count of failed attempts and timestamp of the last attempt in
annotations of the PV internal map.

By default, the controller tries to recycle/delete a PV 3 times in
10 minutes interval. These values are configurable by
kube-controller-manager --pvrecycler-retry-count=X --pvclaimbinder-sync-period=Y
arguments.

cc: @kubernetes/rh-storage

@k8s-github-robot
Copy link

Labelling this PR as size/L

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

@markturansky - can you pick this up?

@markturansky
Copy link
Contributor

race condition. i assigned myself just as you posted.

yes, i will review this.

@k8s-bot
Copy link

k8s-bot commented Jan 7, 2016

GCE e2e test build/test passed for commit 723d7114ea54ebb907d57cd29072c16196f79597.

@k8s-bot
Copy link

k8s-bot commented Jan 7, 2016

GCE e2e test build/test passed for commit 3268503e52431c56691293679ac17d6e10f83681.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

markturansky
markturansky reviewed Jan 7, 2016
View reviewed changes
cmd/kube-controller-manager/app/controllermanager.go
@@ -81,6 +81,7 @@ type CMServer struct {
ResourceQuotaSyncPeriod time.Duration
NamespaceSyncPeriod time.Duration
PVClaimBinderSyncPeriod time.Duration
PVRecycleRetryCount int
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is better in the VolumeConfigFlags struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, moved.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2016
@jsafrane jsafrane force-pushed the devel/retry-delete branch 2 times, most recently from 79461bb to a9422aa Compare January 8, 2016 16:04
@k8s-bot
Copy link

k8s-bot commented Jan 8, 2016

GCE e2e test build/test passed for commit 79461bb278aa46dd16c746f073dd591e6d10de2c.

@k8s-bot
Copy link

k8s-bot commented Jan 8, 2016

GCE e2e test build/test passed for commit a9422aac6b2f5b7af5df50fa6828a2ceb665fbb0.

@k8s-bot
Copy link

k8s-bot commented Jan 12, 2016

GCE e2e build/test failed for commit a1a770ef6455bc12b5567a6009976d7d9b97d959.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2016
@k8s-bot
Copy link

k8s-bot commented Jan 12, 2016

GCE e2e test build/test passed for commit b2dfa48f06a5740529dc0569cd82bb4b14e55583.

markturansky
markturansky reviewed Jan 12, 2016
View reviewed changes
cmd/kube-controller-manager/app/options/options.go
@@ -85,6 +85,7 @@ type VolumeConfigFlags struct {
PersistentVolumeRecyclerMinimumTimeoutHostPath int
PersistentVolumeRecyclerIncrementTimeoutHostPath int
EnableHostPathProvisioning bool
PVRecycleRetryCount int
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, I think this should be PersistentVolumeRecyclerRetryCount, which would match the other variables in this struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Feb 3, 2016

GCE e2e build/test failed for commit 7f241c842454d0f03bb1ce50e06d6f04dfbe5981.

@k8s-github-robot
Copy link

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2016
@k8s-github-robot
Copy link

PR needs rebase

@jsafrane
Copy link
Member Author

jsafrane commented Feb 5, 2016

Rebased (loosing LGTM again).

@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 5, 2016
@markturansky
Copy link
Contributor

Re-adding LGTM after rebase.

@markturansky markturansky added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e test build/test passed for commit 3f94a15168488edfa6b41867fb4142aafb9a82e0.

@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e test build/test passed for commit 3bdd16a3517868d2073b13b7190a3d48ea4f285d.

@jsafrane
Retry recycle or delete operation on failure.
76b6449 
Recycle controller tries to recycle or delete a PV several times.
It stores count of failed attempts and timestamp of the last attempt in
annotations of the PV.

By default, the controller tries to recycle/delete a PV 3 times in
10 minutes interval. These values are configurable by
kube-controller-manager --pv-recycler-maximum-retry=X --pvclaimbinder-sync-period=Y
arguments.
@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e test build/test passed for commit 76b6449.

@jsafrane
Copy link
Member Author

jsafrane commented Feb 5, 2016

@markturansky, I had to regenerate docs, please review again

markturansky
markturansky reviewed Feb 5, 2016
View reviewed changes
pkg/controller/persistentvolume/persistentvolume_recycler_controller.go
// How many recycle/delete operations failed.
retryCount int
// Timestamp of the last attempt.
lastAttempt time.Time
Copy link
Contributor

Choose a reason for hiding this comment

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

It's interesting now that we're thinking about Conditions. These are the kinds of fileds on a Condition (type, timestamp, etc).

@markturansky markturansky added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2016
@markturansky
Copy link
Contributor

re-added LGTM after doc regen.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e test build/test passed for commit 76b6449.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Feb 5, 2016
Merge pull request #19365 from jsafrane/devel/retry-delete
14d74a1 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 14d74a1 into kubernetes:master Feb 5, 2016
@jsafrane jsafrane deleted the devel/retry-delete branch August 19, 2016 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@markturansky markturansky

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jsafrane @k8s-github-robot @derekwaynecarr @markturansky @k8s-bot @k8s-teamcity-mesosphere @googlebot @eparis