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.

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

server: rate limit prebuilds by cloneURL #8568

Merged
merged 1 commit into from
Mar 8, 2022
Merged

server: rate limit prebuilds by cloneURL #8568

merged 1 commit into from
Mar 8, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Mar 3, 2022

Description

Introduce a rate-limit for prebuilds.

Prebuilds are rate limited by cloneURL in the last 1 minute to X. X either comes from Config, or defaults to 50.

Related Issue(s)

Fixes #5176

How to test

Same instructions as #8504

  • To have an easier time, you can modify the server-configmap.yaml on your preview and a special rate limit for your clone URL
     "https://gitlab.com/<USER>/test-gitpod-prebuilds-rate-limit.git": 10,
    

Tests

With a rate limit of 10:
Screenshot 2022-03-07 at 11 59 59

Release Notes

Rate-limit workspace prebuilds to 50 per minute (rolling-window) by default, configurable through config.

Documentation

  • /werft with-clean-slate-deployment

@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #8568 (ec57d20) into main (f4826d3) will decrease coverage by 1.13%.
The diff coverage is n/a.

❗ Current head ec57d20 differs from pull request most recent head 519dbe9. Consider uploading reports for the commit 519dbe9 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8568      +/-   ##
==========================================
- Coverage   12.31%   11.17%   -1.14%     
==========================================
  Files          20       18       -2     
  Lines        1161      993     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1014      880     -134     
+ Partials        4        2       -2     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4826d3...519dbe9. Read the comment docs.

@roboquat roboquat added size/M and removed size/S labels Mar 4, 2022
@easyCZ easyCZ changed the title Add logging around starting prebuilds server: rate limit prebuilds by cloneURL Mar 4, 2022
@easyCZ easyCZ force-pushed the af/rate-limits branch 2 times, most recently from 4e8ac63 to de567c4 Compare March 5, 2022 06:54
@easyCZ
Copy link
Member

easyCZ commented Mar 7, 2022

/werft run

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Nice! 👏

Two comments in passing. 😇

EDIT: Also, I love that this limit is configurable per cloneUrl. Thanks a lot for doing that. 💯

chart/values.yaml Outdated Show resolved Hide resolved
@easyCZ easyCZ marked this pull request as ready for review March 7, 2022 16:20
@easyCZ easyCZ requested a review from a team March 7, 2022 16:20
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 7, 2022
@jankeromnes
Copy link
Contributor

jankeromnes commented Mar 7, 2022

Reminder before this gets merged: Please try to squash your commits into one or a few logical commits with a descriptive commit message. 🙏

@easyCZ
Copy link
Member

easyCZ commented Mar 7, 2022

Squashed commits.

@easyCZ
Copy link
Member

easyCZ commented Mar 7, 2022

I suppose this PR will also need documentation - around configuration of the prebuild rate limits.

@easyCZ
Copy link
Member

easyCZ commented Mar 8, 2022

/werft run

👍 started the job as gitpod-build-af-rate-limits.40

@easyCZ easyCZ requested review from geropl and jankeromnes March 8, 2022 09:49
@geropl
Copy link
Member

geropl commented Mar 8, 2022

@andrew-farries Alright, thanks a lot for tackling this! 🙏 I expected I would manage to finish the review quicker... but anyway, I'm through now.

@easyCZ thx for the adjustments and squashing (we should really think about allowing for this/make it the default).

The only thing left I see here is the migration for an index.

Prebuilds are rate-limited to N in the last S seconds on a rolling
window basis.
By default, 50 prebuilds are allowed in a 1 minute window.
A configuration property `prebuildLimiter` is added which controls
default rate limit but allows for explicit overrides by cloneURL.
@easyCZ easyCZ requested a review from geropl March 8, 2022 11:51
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Awesome, thanks to both of you! 🙏

@geropl
Copy link
Member

geropl commented Mar 8, 2022

/unhold

@roboquat roboquat merged commit 54a765d into main Mar 8, 2022
@roboquat roboquat deleted the af/rate-limits branch March 8, 2022 14:10
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Mar 15, 2022
@jldec jldec added type: epic and removed release-note deployed Change is completely running in production deployed: webapp Meta team change is running in production labels Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L team: webapp Issue belongs to the WebApp team type: epic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rate limit prebuilds
6 participants