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

Disable kubernetes client side rate limiting #352

Merged
merged 3 commits into from
Jul 25, 2024
Merged

Disable kubernetes client side rate limiting #352

merged 3 commits into from
Jul 25, 2024

Conversation

seankhliao
Copy link
Contributor

Trust that server side API Priority and Fairness is enabled on the cluster, which will provide server side rate limiting. This allows a single istio-csr pod to serve larger bursts of traffic, such as when a large number of pods restart.

Fixes #144
Fixes #217

@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2024
@cert-manager-prow
Copy link
Contributor

Hi @seankhliao. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@cert-manager-prow cert-manager-prow bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 18, 2024
@wallrj
Copy link
Member

wallrj commented Jul 18, 2024

/ok-to-test

@cert-manager-prow cert-manager-prow bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2024
@SgtCoDFish
Copy link
Member

We discussed this a bit in the cert-manager standup today (2024-07-25) and we reached the conclusion that this probably wants to live behind a an opt-in flag since API Priority and Fairness is quite new (stable as of k8s v1.29)

@wallrj had a plan but I've already forgotten so I'll tag him again here!

@wallrj wallrj self-assigned this Jul 25, 2024
@wallrj
Copy link
Member

wallrj commented Jul 25, 2024

I'll push a command line flag and ping you for a review.

@cert-manager-prow cert-manager-prow bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 25, 2024
seankhliao and others added 3 commits July 25, 2024 13:40
@seankhliao @wallrj
Disable kubernetes client side rate limiting
82f4bc3 
Trust that server side API Priority and Fairness is enabled on the
cluster, which will provide server side rate limiting. This allows a
single istio-csr pod to serve larger bursts of traffic, such as when a
large number of pods restart.

Fixes cert-manager#144
Fixes cert-manager#217

Signed-off-by: Sean Liao <sean@liao.dev>
@wallrj
Add a command line flag to disable Kubernetes client rate limiter
3446843 
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj
Add Helm chart option to disable Kubernetes client rate limiter
e0d40f6 
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@cert-manager-prow cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2024
@wallrj wallrj requested a review from SgtCoDFish July 25, 2024 12:44
@wallrj
Copy link
Member

wallrj commented Jul 25, 2024

@seankhliao I added a command line option and a Helm chart option to allow this feature to be disable by default, but easily enablable by a Helm chart option. Also rebased the branch to resolve some conflicts with origin/main.

SgtCoDFish
SgtCoDFish approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Making this opt-in makes it very easy to merge. Thanks @seankhliao for this! (and thanks also to Richard for the flag ofc!) 😁

@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 25, 2024
@cert-manager-prow cert-manager-prow bot merged commit cb8edf6 into cert-manager:main Jul 25, 2024
12 checks passed
This was referenced Aug 13, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SgtCoDFish SgtCoDFish SgtCoDFish approved these changes

Assignees

@wallrj wallrj

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. ok-to-test size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
3 participants
@seankhliao @wallrj @SgtCoDFish