BZ 1388319 handle -1 in maxScheduledImageImportsPerMinute #13315
Conversation
|
|
||
| var limiter flowcontrol.RateLimiter = nil | ||
| if c.Options.ImagePolicyConfig.MaxScheduledImageImportsPerMinute == -1 { | ||
| limiter = flowcontrol.NewTokenBucketRateLimiter(math.MaxFloat32, math.MaxInt32) |
There was a problem hiding this comment.
THere's an unlimited rate limiter (which is a hardcoded no-op to succeed). Use that instead. Also, you should have <=0, because someone could call this code with 0.
There was a problem hiding this comment.
I was looking for something like that and didn't see it. I'll look again.
Any negative number greater that -1 is rejected in ValidateImagePolicyConfig but I'll change this anyway
There was a problem hiding this comment.
NewFakeNeverRateLimiter
There was a problem hiding this comment.
heh, I was going to use it but I ignored it because of the "fake" in the name and only saw it being used in tests so I didn't want to shoot myself in the foot. Thanks.
pweil-
force-pushed
the
image-import-rate
branch
from
b437825 to
4992e26
Compare
|
[test] |
|
Evaluated for origin test up to 4992e26 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/230/) (Base Commit: 0009bfa) |
|
LGTM [merge] |
|
Evaluated for origin merge up to 4992e26 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/104/) (Base Commit: 906adf9) (Image: devenv-rhel7_6072) |
master panics when setting -1 in maxScheduledImageImportsPerMinute though it should be handled. It is specific, validated value and was documented as unlimited.
This checks for -1 and sets the limiter to the
NewFakeAlwaysRateLimiter. This name is confusing but it looks like it is the one that won't block on accept and is used in tests to turn off limiting@mfojtik @smarterclayton WDYT? The other option, I suppose, is to remove the validation and just have users set this to a very high value themselves.