Controllers should rate limit themselves and space retries out across a longer period of time

[bparees]

No description provided.

[bparees]

the throttle.go change will be removed from this PR since it's being made upstream.

@smarterclayton @ironcladlou ptal.

with this change we get a burst of up to 10 retries as fast as possible, otherwise we're limited to 1 retry per second (for each controller's retry handler). call it a poor man's backoff.

[smarterclayton]

So in the presence of retries the throughput of the controller drops to 10/second? Seems reasonable.

[bparees]

@smarterclayton it's a little harder to quantify than that...... if you have nothing but retries in your queue, the rate will drop to 1 per second (not 10 per second).

If you have a mix of retries and new items in your queue, we'll process the new items as fast as possible, but every time we come to a retry item, we'll get held up for a second while we wait for the retry item to get enqueued. (assuming we've drained our burst allocation already).

in otherwords, we are throttling the rate at which you can enqueue a retry, not the rate at which you can process an event.

[smarterclayton]

This blocks the controller loop so the controller won't make progress.

On Apr 1, 2015, at 6:34 PM, Ben Parees [email protected] wrote:

@smarterclayton it's a little harder to quantify than that...... if you have nothing but retries in your queue, the rate will drop to 1 per second (not 10 per second).

If you have a mix of retries and new items in your queue, we'll process the new items as fast as possible, but every time we come to a retry item, we'll get held up for a second while we wait for the retry item to get enqueued. (assuming we've drained our burst allocation already).

in otherwords, we are throttling the rate at which you can enqueue a retry, not the rate at which you can process an event.

—
Reply to this email directly or view it on GitHub.

[bparees]

it'll make progress more slowly in the presence of retries, but again the delay only affects it when a retry is procesed, so if you have 1 retry and 100 new events, it'll delay 1s, requeue the retry, then process the 100 new events as fast as possible, then delay another 1s on the retry event (assuming it continues to fail).

I don't think there's a better approach short of the available-time-aware-FIFO we discussed that would actually bypass dequeuing events until their next retry time was reached.

[bparees]

And note that if processing the other 100 events takes longer than 1s, then the retry event will be requeueable immediately because there will be a token available again.

[smarterclayton]

On Apr 1, 2015, at 6:40 PM, Ben Parees [email protected] wrote:

it'll make progress more slowly in the presence of retries, but again the delay only affects it when a retry is procesed, so if you have 1 retry and 100 new events, it'll delay 1s, requeue the retry, then process the 100 new events as fast as possible, then delay another 1s on the retry event (assuming it continues to fail).

How? Retry handler is invoked from handleOne, and is single threaded.
I don't think there's a better approach short of the available-time-aware-FIFO we discussed that would actually bypass dequeuing events until their next retry time was reached.

—
Reply to this email directly or view it on GitHub.

[bparees]

not following your point. but per our other discussion i'll ensure all our controllers configure a max number of retries of 60 or something (effectively ensuring at least 50 seconds of retry in the "worst" case)

[smarterclayton]

And double check the sync period (it should probably be nonzero but several minutes apart).

On Apr 1, 2015, at 7:29 PM, Ben Parees [email protected] wrote:

not following your point. but per our other discussion i'll ensure all our controllers configure a max number of retries of 60 or something (effectively ensuring at least 50 seconds of retry in the "worst" case)

—
Reply to this email directly or view it on GitHub.

[bparees]

Updated build controllers to retry up to 60 times instead of forever. deploy controllers already retry only once.

sync period is 2 minutes for deploy+build controller watches.

[bparees]

this will fix #1555

[bparees]

[test]

[smarterclayton]

Should deployment config retry more times?

On Apr 1, 2015, at 11:23 PM, Ben Parees [email protected] wrote:

Updated build controllers to retry up to 60 times instead of forever. deploy controllers already retry only once.

sync period is 2 minutes for deploy+build controller watches.

—
Reply to this email directly or view it on GitHub.

[bparees]

that's a question for @ironcladlou, i haven't studied deployment logic enough to know what's appropriate there.

[bparees]

@ironcladlou added a unit test for the rate limiting behavior.

[openshift-bot]

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1707/)

[bparees]

Note that the behavior of this is such that every time we re-sync the image repo changes, we attempt to trigger the build again and ultimately fail (after 60 retries) because the To is not valid.

We could fix this by updating the BuildConfig so we recognize we've already attempted to trigger a build for this imagechange (successfully or otherwise). We could also go ahead and create the build in a failed state when there is something invalid about it (like a bad To).

Both of those things go against my original design thought which was that you should be able to correct your errors (ie create the missing imagerepo), but the latter approach (creating the build but immediately marking it failed) is starting to have more appeal to me. not sure it needs to hold up this merge though.

[bparees]

@smarterclayton i think this is good to go, assuming I did the copy from upstream commit correctly.

[bparees]

[test]

[smarterclayton]

On Apr 3, 2015, at 12:09 AM, Ben Parees [email protected] wrote:

Note that the behavior of this is such that every time we re-sync the image repo changes, we attempt to trigger the build again and ultimately fail (after 60 retries) because the To is not valid.

We could fix this by updating the BuildConfig so we recognize we've already attempted to trigger a build for this imagechange (successfully or otherwise). We could also go ahead and create the build in a failed state when there is something invalid about it (like a bad To).

Both of those things go against my original design thought which was that you should be able to correct your errors (ie create the missing imagerepo), but the latter approach (creating the build but immediately marking it failed) is starting to have more appeal to me. not sure it needs to hold up this merge though.

The thing is I really do want to be able to create things out of order and have it "just work".
—
Reply to this email directly or view it on GitHub.

[smarterclayton]

the comment should really be on the func, and then you can simplify this to return count < 60. I would rename the method to be limitedLogAndRetry.

[bparees]

good idea, will do.

[smarterclayton]

I think it would be better to take kutil.RateLimiter as the argument, not the Args. That would enable you to properly test this.

[bparees]

it also means any consumers of this now have to import, understand, and instantiate, another type instead of two primitives. i'm not convinced that's an improvement from a usability+readability perspective. I like that the RateLimiter interface is currently encapsulated so users of QueueRetryManager don't need to be aware of it, the potential benefits w/ respect to test code (that you mention below) not withstanding.

[smarterclayton]

On Apr 3, 2015, at 10:18 PM, Ben Parees [email protected] wrote:

In pkg/controller/controller.go:

return &QueueRetryManager{
    queue:     queue,
    keyFunc:   keyFn,
    retryFunc: retryFn,
    retries:   make(map[string]int),

•   limiter:   kutil.NewTokenBucketRateLimiter(rate, burst),
  

  it also means any consumers of this now have to import, understand, and instantiate, another type instead of two primitives. i'm not convinced that's an improvement from a usability+readability perspective.

Eh... Two arguments that you have to explain are not any worse to me than a copy and paste constructor (which is going to be pretty name clear). Taking the interface is straight up better - it means if you want a complex rate limiter you don't have to change the class. Also, Factory is kind of a "configure a bunch of things" including other lower level bits. I don't like encapsulating function that isn't specific to the class (the "how" of limiting is orthogonal to the need to limit).
I like that the RateLimiter interface is currently encapsulated so users of QueueRetryManager don't need to be aware of it, the potential benefits w/ respect to test code (that you mention below) not withstanding.

—
Reply to this email directly or view it on GitHub.

[smarterclayton]

An example here - if we want to add a new rate limited token behavior, we shouldn't have to take more arguments to QueueRetryManager.

[bparees]

The thing is I really do want to be able to create things out of order and have it "just work".

@smarterclayton is that an argument for, or against, failing builds like this? it sounds like an argument against. Certainly it's an argument against immediatley failing them. Failing them after some period of time is something we could consider doing too (we know when the Build object was created... if we can't validly resolve it after X time, fail). But again, outside the scope of this PR.

[smarterclayton]

On Apr 3, 2015, at 10:08 PM, Ben Parees [email protected] wrote:

The thing is I really do want to be able to create things out of order and have it "just work".

@smarterclayton is that an argument for, or against, failing builds like this? it sounds like an argument against. Certainly it's an argument against immediatley failing them. Failing them after some period of time is something we could consider doing too (we know when the Build object was created... if we can't validly resolve it after X time, fail). But again, outside the scope of this PR.

Post 3.0 we should come back to it.
—
Reply to this email directly or view it on GitHub.

[bparees]

@smarterclayton reworked with RateLimiter interface..ptal.

[smarterclayton]

LGTM [merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1472/) (Image: devenv-fedora_1209)

[bparees]

router test failures. @pweil- any known flakiness in router that's being looked into?

[merge]

[openshift-bot]

Evaluated for origin up to e218a54

[pweil-]

@bparees #1603 is what I know about (issues with the probes happening too quickly). There have been some int test failures that folks have mentioned but we've been unable to reproduce them.