[server, installer] Make PrebuildRateLimiter period configurable #14975
Conversation
|
started the job as gitpod-build-gpl-14791-leaky.2 because the annotations in the pull request description changed |
geropl
force-pushed
the
gpl/14791-leaky
branch
3 times, most recently
from
89479f0 to
89f6ab9
Compare
| cloneURL: string, | ||
| ): PrebuildRateLimiterConfig { | ||
| // First we use any explicit overrides for a given cloneURL | ||
| let config = rateLimiters[cloneURL]; |
There was a problem hiding this comment.
What would be the scenario where we would want to change the rate limiting for one cloneURL but not for all?
There was a problem hiding this comment.
Note that this is the same code as before, and I'd argue out of scope for this PR.
The only change here is to pull up the time period to be configurable with changing the installer code, which is justified by the argument made above.
Ultimately, I think the whole approach is ill-suited for what we want to achieve. And I feel we have alignment on that we want to pursue a controller-pased/async approach for handling prebuild triggers instead.
But as long as we are not there yet (and it's still unclear how to prioritize that work) it's important to me that we have the ability to quickly and confidently react to incidents like the one we had the other week.
There was a problem hiding this comment.
I think having a default, which can be overriden for specific clone URLs makes perfect sense. It exists largely for the case where you get hit hard, system pages you and you want to adjust the config only to mitigate the issue. But adjusting for all is outside of the scope of that incident and would require deeper investigation.
| PrebuildLimiter: PrebuildRateLimiters{ | ||
| // default limit for all cloneURLs | ||
| "*": 50, | ||
| "*": PrebuildRateLimiterConfig{ | ||
| Limit: 50, | ||
| Period: 300, | ||
| }, |
There was a problem hiding this comment.
Wanna extend the configmap_test.go to check for this? Given there was no change to the test file with this change suggests there's no test coverage for this.
|
/hold for discussion |
geropl
force-pushed
the
gpl/14791-leaky
branch
from
89f6ab9 to
654f772
Compare
geropl
force-pushed
the
gpl/14791-leaky
branch
from
654f772 to
49b9f68
Compare
|
/unhold |
|
started the job as gitpod-build-gpl-14791-leaky.8 because the annotations in the pull request description changed |
|
/werft run 👍 started the job as gitpod-build-gpl-14791-leaky.9 |
roboquat
added
deployed: webapp
|
FYI @geropl removed the release notes as this does not impact the end user of gitpod.io or dedicated. |
Description
Effectively changes the rate limiter from being
to be
Related Issue(s)
Fixes #14791
How to test
limit: 2, andkubectl rollout restart deployment/server...project/eventsendpoint for that)Release Notes
Documentation
Werft options:
If enabled this will build
install/previewValid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh