Convert service account token controller to use a work queue #23858
Conversation
|
cc @deads2k |
k8s-github-robot
added
kind/api-change
| // Unknown type. If we missed a ServiceAccount deletion, the | ||
| // corresponding secrets will be cleaned up during the Secret re-list | ||
| return | ||
| func (e *TokensController) requeue(queue *workqueue.Type, key interface{}) { |
There was a problem hiding this comment.
@deads2k is there a cleaner way to do this? this also doesn't protect against a hotloop if you get more than N persistent failures (other controllers may have similar issues with requeue). Without a place to record backoff (which presumes an object and the ability to update it successfully), I'm not sure how to make this better
There was a problem hiding this comment.
That in combination with a ratelimit.NewBucketWithRate() and then bucket.Take() is what I planned to do downstream. I think that should give the duration to add to the requeue. MaxFailures should be tracked both on your object (which may fail) and a list of those failures in your controller (which should not fail).
liggitt
added
release-note
|
cc @lavalamp for controller review (or nominate alternate) |
| @@ -332,7 +397,8 @@ func (e *TokensController) createSecret(serviceAccount *api.ServiceAccount) erro | |||
|
|
|||
| // Save the secret | |||
| if createdToken, err := e.client.Core().Secrets(serviceAccount.Namespace).Create(secret); err != nil { | |||
| return err | |||
| // retriable error | |||
| return true, err | |||
| } else { | |||
There was a problem hiding this comment.
if you mean remove the else, I'd rather keep createdToken and err scoped to this block
liggitt
force-pushed
the
satoken-queue
branch
3 times, most recently
from
4e5c9c7 to
b8f98d9
Compare
| @@ -53,6 +53,7 @@ func NewCMServer() *CMServer { | |||
| ConcurrentResourceQuotaSyncs: 5, | |||
| ConcurrentDeploymentSyncs: 5, | |||
| ConcurrentNamespaceSyncs: 2, | |||
| ConcurrentSATokenSyncs: 5, | |||
There was a problem hiding this comment.
I guess that would be inconsistent and I won't ask you to change them all. Sigh. "Syncs" is confusing.
|
We may need a strategy for testing this sort of code. There's a lot of possible places for error returns, are they all exercised? |
|
fixed mesos controllermanager int32 cast |
liggitt
added
the
lgtm
| } | ||
| if cl != nil && cl.Core().GetRESTClient().GetRateLimiter() != nil { | ||
| metrics.RegisterMetricAndTrackRateLimiterUsage("serviceaccount_controller", cl.Core().GetRESTClient().GetRateLimiter()) | ||
| } | ||
| e.serviceAccounts, e.serviceAccountController = framework.NewIndexerInformer( | ||
|
|
||
| e.serviceAccounts, e.serviceAccountController = framework.NewInformer( |
There was a problem hiding this comment.
we don't need to look up service accounts by namespace, so no need to maintain that index
k8s-github-robot
added
the
needs-rebase
|
rebased |
liggitt
added
lgtm
k8s-github-robot
removed
the
needs-rebase
|
GCE e2e build/test passed for commit 9d6d8c475495430f24e62aeb0532b154d6b4fe61. |
k8s-github-robot
added
the
needs-rebase
liggitt
added
lgtm
|
rebased |
|
GCE e2e build/test passed for commit f45d9dc. |
|
GCE e2e build/test passed for commit f45d9dc. |
|
GCE e2e build/test passed for commit f45d9dc. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit f45d9dc. |
|
Automatic merge from submit-queue |
Converts the service account token controller to use a work queue. This allows parallelization of token generation (useful when there are several simultaneous namespaces or service accounts being created). It also lets us requeue failures to be retried sooned than the next sync period (which can be very long).
Fixes an issue seen when a namespace is created with secrets quotaed, and the token controller tries to create a token secret prior to the quota status having been initialized. In that case, the secret is rejected at admission, and the token controller wasn't retrying until the resync period.