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.

Sign up for GitHub

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

Jump to bottom

Scale ratelimit with ingress pods #5113

Merged
merged 7 commits into from
May 23, 2024
Merged

Scale ratelimit with ingress pods #5113

merged 7 commits into from
May 23, 2024

Conversation

dbaumgarten
Copy link
Contributor

Proposed changes

Implements #4894

Add a flag (in VS-Policy and in Ingress-Annotations) to enable ratelimit-scaling. If enabled, the configured ratelimit is divided by the number of currently active ingress-pods. This way the total sum of the ratelimit stays constant, even when autoscaling changes the number of nginx-ingress pods.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@dbaumgarten dbaumgarten requested review from a team as code owners February 16, 2024 12:42
@netlify Netlify
Copy link

netlify bot commented Feb 16, 2024 •
edited

👷 Deploy request for nginx-kubernetes-ingress pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 5323b23

@github-actions github-actions bot added documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements labels Feb 16, 2024
ADubhlaoich
ADubhlaoich reviewed Feb 20, 2024
View reviewed changes
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM docs-wise, but most of the PR is code-based, so I am not approving.

@shaun-nx shaun-nx self-assigned this Feb 26, 2024
@brianehlert brianehlert linked an issue Mar 6, 2024 that may be closed by this pull request
Scale rate-limit with ingress-pods #4894
Closed
@dbaumgarten
Copy link
Contributor Author

@shaun-nx What do you think? Does this look fine or are any changes required?

@brianehlert
Copy link
Collaborator

Just an FYI as this has been lingering for review.
The NGINX Plus version of the implementation should get accurate rate limiting across all pods with state sync.
That comes with the shared memory zone being configured for the rate limit and then state sharing being enabled.

@github-actions github-actions bot added the go Pull requests that update Go code label Apr 30, 2024
@dbaumgarten
Copy link
Contributor Author

Rebased on current master and added a commit clarifying the docs about the limitations of this feature.

@j1m-ryan
Copy link
Member

Hi @dbaumgarten, thanks for following through, just an FYI that github actions are not working properly at the moment, that is why the tests are not running. https://www.githubstatus.com/

j1m-ryan
j1m-ryan approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@j1m-ryan j1m-ryan left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

@j1m-ryan j1m-ryan enabled auto-merge (squash) May 23, 2024 15:25
@j1m-ryan j1m-ryan merged commit e654db5 into nginxinc:main May 23, 2024
76 of 93 checks passed
@vepatel vepatel mentioned this pull request Jun 11, 2024
Closed
vepatel
vepatel reviewed Jun 13, 2024
View reviewed changes
internal/k8s/controller.go
}
}

// handle virtualservers
Copy link
Contributor

Choose a reason for hiding this comment

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

@dbaumgarten something we observed in regression, the code below was causing panic when CRD flag was not set so we've wrapped it in

if lbc.areCustomResourcesEnabled {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. Nice find.
Yes, wrapping it in that if sounds like a viable solution 👍

ssrahul96 pushed a commit to ssrahul96/kubernetes-ingress that referenced this pull request Jun 20, 2024
@dbaumgarten @j1m-ryan @ssrahul96
Scale ratelimit with ingress pods (nginxinc#5113)
5e05cde 
* Ratelimit Scaling for Ingresses

* Added ratelimit scaling for VS

* Testcases for ratelimit scaling

* Fixed linter errors

* Apply suggestions from code review

Co-authored-by: Jim Ryan <j.ryan@f5.com>
Signed-off-by: dbaumgarten <mail@dbaumgarten.net>

* Clarify documentation about ratelimit-scaling feature

---------

Signed-off-by: dbaumgarten <mail@dbaumgarten.net>
Co-authored-by: Jim Ryan <j.ryan@f5.com>
This was referenced Jul 1, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ADubhlaoich ADubhlaoich ADubhlaoich left review comments

@vepatel vepatel vepatel left review comments

@j1m-ryan j1m-ryan j1m-ryan approved these changes

@pdabelf5 pdabelf5 pdabelf5 approved these changes

Assignees

@shaun-nx shaun-nx

Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements go Pull requests that update Go code
Projects
NGINX Ingress Controller
Status: Done 🚀
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Scale rate-limit with ingress-pods
7 participants
@dbaumgarten @brianehlert @j1m-ryan @ADubhlaoich @vepatel @pdabelf5 @shaun-nx