-
Notifications
You must be signed in to change notification settings - Fork 52
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
add configurable throughput for clients #91
Conversation
Welcome @sonasingh46! |
Hi @sonasingh46. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
/ok-to-test |
Please add a release note. |
|
||
retryIntervalStart = flag.Duration("retry-interval-start", time.Second, "Initial retry interval of failed pv monitoring. It doubles with each failure, up to retry-interval-max. Default is 1 second.") | ||
retryIntervalMax = flag.Duration("retry-interval-max", 5*time.Minute, "Maximum retry interval of failed pv monitoring. Default is 5 minutes.") | ||
kubeAPIQPS = flag.Float32("kube-api-qps", 5, "QPS to use while communicating with the kubernetes apiserver. Defaults to 5.0.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. You would have to run a scalability experiment to determine which default works better. Also note that client-side throttling is being replaced by API priority and fairness.
IMHO more important than tweaks like this is to look at why health monitor causes API traffic. Wasn't it because it establishes watches for objects that change a lot (nodes, pods)? Then throttling won't help at all because it is applied to outbound requests, not incoming watch updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API priority and fairness is now beta. Have you run any experiments that can demonstrate that this change is still effective?
Can you make the following workqueue ratelimited as well? Also check if there's anything else in the NodeWatcher that should be configurable. |
Sure, raised a commit for this. I do not see anything more configurable in the node watcher. Though there is |
I have my doubts whether it actually fixes any of that... please remove that and manually close the issue after it has been confirmed that the issue is gone. |
Done |
a0f195c Merge pull request kubernetes-csi#106 from msau42/fix-canary 7100c12 Only set staging registry when running canary job b3c65f9 Merge pull request kubernetes-csi#99 from msau42/add-release-process e53f3e8 Merge pull request kubernetes-csi#103 from msau42/fix-canary d129462 Document new method for adding CI jobs are new K8s versions e73c2ce Use staging registry for canary tests 2c09846 Add cleanup instructions to release-notes generation 60e1cd3 Merge pull request kubernetes-csi#98 from pohly/kubernetes-1-19-fixes 0979c09 prow.sh: fix E2E suite for Kubernetes >= 1.18 3b4a2f1 prow.sh: fix installing Go for Kubernetes 1.19.0 1fbb636 Merge pull request kubernetes-csi#97 from pohly/go-1.15 82d108a switch to Go 1.15 d8a2530 Merge pull request kubernetes-csi#95 from msau42/add-release-process 843bddc Add steps on promoting release images 0345a83 Merge pull request kubernetes-csi#94 from linux-on-ibm-z/bump-timeout 1fdf2d5 cloud build: bump timeout in Prow job 41ec6d1 Merge pull request kubernetes-csi#93 from animeshk08/patch-1 5a54e67 filter-junit: Fix gofmt error 0676fcb Merge pull request kubernetes-csi#92 from animeshk08/patch-1 36ea4ff filter-junit: Fix golint error f5a4203 Merge pull request kubernetes-csi#91 from cyb70289/arm64 43e50d6 prow.sh: enable building arm64 image 0d5bd84 Merge pull request kubernetes-csi#90 from pohly/k8s-staging-sig-storage 3df86b7 cloud build: k8s-staging-sig-storage c5fd961 Merge pull request kubernetes-csi#89 from pohly/cloud-build-binfmt db0c2a7 cloud build: initialize support for running commands in Dockerfile be902f4 Merge pull request kubernetes-csi#88 from pohly/multiarch-windows-fix 340e082 build.make: optional inclusion of Windows in multiarch images 5231f05 build.make: properly declare push-multiarch 4569f27 build.make: fix push-multiarch ambiguity 17dde9e Merge pull request kubernetes-csi#87 from pohly/cloud-build bd41690 cloud build: initial set of shared files 9084fec Merge pull request kubernetes-csi#81 from msau42/add-release-process 6f2322e Update patch release notes generation command 0fcc3b1 Merge pull request kubernetes-csi#78 from ggriffiths/fix_csi_snapshotter_rbac_version_set d8c76fe Support local snapshot RBAC for pull jobs c1bdf5b Merge pull request kubernetes-csi#80 from msau42/add-release-process ea1f94a update release tools instructions 152396e Merge pull request kubernetes-csi#77 from ggriffiths/snapshotter201_update 7edc146 Update snapshotter to version 2.0.1 git-subtree-dir: release-tools git-subtree-split: a0f195cc2ddc2a1f07d4d3e46fc08187db358f94
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
Although this may not have fixed #76, it is still useful to make QPS and Burst settings configurable. While API server priority and fairness focuses on providing a granular rate limit mechanism on in-flight requests on the server side, QPS and Burst settings serve as a rate limiting mechanism on Kube API client side. |
Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>
11d561e
to
d2eaf25
Compare
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
@sonasingh46: PR needs rebase. 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/test-infra repository. |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
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/test-infra repository. |
/reopen |
@xing-yang: Reopened this PR. In response to this:
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/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sonasingh46 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@sonasingh46: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
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/test-infra repository. |
Signed-off-by: Ashutosh Kumar sonasingh46@gmail.com
What type of PR is this?
What this PR does / why we need it:
This change adds two new flags "kube-api-qps" and "kube-api-burst" to configure the QPS and Burst of clients to the API server as the default value is not effective at larger scale.
This change also uses a separate client for leader election go routine.
This change also replace the native go
flag
library withgithub.com/spf13/pflag
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?: