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

Add a rate limiter to the GCE cloudprovider #23019

Merged
merged 1 commit into from
Mar 19, 2016
Merged

Add a rate limiter to the GCE cloudprovider #23019

merged 1 commit into from
Mar 19, 2016
+34 −22

Conversation

alex-mohr
Copy link
Contributor

It will poll for operation completion with at most 10 qps to avoid
triggering GCE's rate limits.

Without this PR, creating large 1000 node clusters can result in the routecontroller being too spammy.

@alex-mohr
Add a rate limiter to the GCE cloudprovider
6dc63f8 
It will poll for operation completion with at most 10 qps to avoid
triggering GCE's rate limits.
@alex-mohr alex-mohr added this to the v1.2 milestone Mar 15, 2016
@alex-mohr alex-mohr added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. area/platform/gce labels Mar 15, 2016
@alex-mohr
Copy link
Contributor Author

Presumably too late for 1.2? Would be really nice for 1.2.1 though.

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 15, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 15, 2016

GCE e2e build/test failed for commit 6dc63f8.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@bgrant0607
Copy link
Member

@alex-mohr Is there an issue for this?

@bgrant0607
Copy link
Member

Tests failed

@bgrant0607
Copy link
Member

This is maybe debatable, but only P0s should be cherrypicked into the 1.2 branch beyond this point

@bgrant0607 bgrant0607 modified the milestones: next-candidate, v1.2 Mar 16, 2016
@bgrant0607
Copy link
Member

And TJ told me the issue is #21563 (please xref)

@alex-mohr
Copy link
Contributor Author

Test failure appears to be a known issue. For posterity, logs: https://00e9e64bac3cffdfd09b0d1a42b711f1928bee10e75c38ed37-apidata.googleusercontent.com/download/storage/v1_internal/b/kubernetes-jenkins/o/pr-logs%2Fpull%2F23019%2Fkubernetes-pull-build-test-e2e-gce%2F32730%2Fbuild-log.txt?qk=AD5uMEswHLmJ2ebOJReASWQFyLv80wd3X7_zvnrfegH5eI7p_MvRCnP_KFb0FuzkMM7H_4kMqh9nMhKTyWyY7Iz015QehnD6KFwxUrzI4AgPCiMxta4DNFi3-DG-IGfzmX902SvMlxeD-4hIhLI-QH_9Z1Rc19cAIi83VBno7T6mYLHWCvhIOyaoAjh6HXjK-jbrwIyQlK-0VIQg2cFnMBUeZK-cJyU0n_1R3hkdF95YgQy4TMB54flW6Awc-P9CywR7UIOYzYYcjG4tHCkae7rwnc3jzSsm3Pi1R6lhSw4eTkGkcfsn2WxhJOLOZQc7ppoV5Dp5zGHNAUpT0KjyHUcf4wvLu-26nUKtS74FbqUi3lbJSnYHIe68p8W2i5e8EJV4x4CC78wDuM82mpWYJPIS3MmGuDQp6WRgY6FYHFRI4Z_mOwliPGd6JvlTN3VoC9ot0aeyjmKKfY8N2Be2nXMApbzuB1_-VGarJfJhF75VVu9tbLa_NleuxZ8R6FDGQvvwtELFeESegzvCWy9WcUn6j8aXy-hmu00dxzFkoH_ykN81InnVWtCgL635Li8xWSx4tKIjvbjLcd-lidbV7YU9JdXJRUaPMiK0c-pHw1uatFAXD2f8_sfA-ycdnwap5IootMppqDjMlH-2wzmJMScQxO0Twc1H7z7SOrR-AgfTn1BhX2LUL0-VCVKA3qsb77ncY_6MNEzkQowcmaWY2XtxGiWWJTaDNPyWoVmI4JgSQPGee694nvouIG9uaASxBn4ccvd-7tiswXnyjv57j4DpB6e57zir54biriWgAr864SHOowP0BeyJ8ZAkhwwxo9GbNbSjGa3m2CK2Nq275pIbBt_9iwDZo0F9yUZDBJjK5YCG7dzANnTajkZOO-JpOP0MD6nOo7yHFXGCLM8FKP-ntNhcYh9RkvzDpDZOX_XHdHAYlBh15s0

Of course, it's possible there's an unanticipated interaction -- I'll check the retest results.

Re: issue, apologies for not including the initial xref.

Re: impact, it makes creating 1000 node clusters on GCE and GKE worse enough that I support it as an important enough reliability fix that it should go into 1.2, given the relatively contained nature of the PR. Willing to discuss further if you think it worth the time to do so.

@k8s-github-robot
Copy link

@alex-mohr
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@alex-mohr
Copy link
Contributor Author

@k8s-bot test this issue: #22671

@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@k8s-bot
Copy link

k8s-bot commented Mar 16, 2016

GCE e2e build/test passed for commit 6dc63f8.

@k8s-bot
Copy link

k8s-bot commented Mar 16, 2016

GCE e2e build/test passed for commit 6dc63f8.

@goltermann
Copy link
Contributor

Moving back to v1.2 and adding cherrypick label, as this is a candidate for 1.2.1.

@goltermann goltermann modified the milestones: v1.2, next-candidate Mar 18, 2016
@alex-mohr
Copy link
Contributor Author

@davidopp builds are passing and polling rates dropped from > 100 qps to 10 qps as expected via manual testing. I couldn't think of a reasonable way to reliably unittest that the rate limiter is working as expected, but I'm not too concerned given code complexity -- let me know if you have ideas there or also think it's fine w/o. And if someone else can/should review, please do delegate to them!

@davidopp
Copy link
Member

LGTM

Very sorry for not seeing this sooner.

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2016
@davidopp
Copy link
Member

And thanks for the fix.

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Mar 19, 2016

GCE e2e build/test passed for commit 6dc63f8.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Mar 19, 2016

GCE e2e build/test passed for commit 6dc63f8.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 19, 2016
Merge pull request #23019 from alex-mohr/oplimit
0fe049f 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 0fe049f into kubernetes:master Mar 19, 2016
@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 23, 2016
eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 24, 2016
@eparis
Merge pull request kubernetes#23019 from alex-mohr/oplimit
bfd84d4 
Auto commit by PR queue bot
(cherry picked from commit 0fe049f)
@eparis eparis mentioned this pull request Mar 24, 2016
Merged
@bgrant0607 bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 24, 2016
@k8s-cherrypick-bot
Copy link

Commit bfd84d4 found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this s an error find help to get your PR picked.

@liggitt liggitt mentioned this pull request Mar 28, 2016
Closed
85 tasks
AlainRoy pushed a commit to vmware-archive/kubernetes-archived that referenced this pull request Mar 29, 2016
Merge pull request kubernetes#23019 from alex-mohr/oplimit
2035b13 
Auto commit by PR queue bot
(cherry picked from commit 0fe049f)
alena1108 pushed a commit to rancher/kubernetes that referenced this pull request May 20, 2016
Merge pull request kubernetes#23019 from alex-mohr/oplimit
6785e97 
Auto commit by PR queue bot
(cherry picked from commit 0fe049f)
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@eparis
Merge pull request kubernetes#23019 from alex-mohr/oplimit
d654948 
Auto commit by PR queue bot
(cherry picked from commit 0fe049f)
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@eparis
Merge pull request kubernetes#23019 from alex-mohr/oplimit
dc560e8 
Auto commit by PR queue bot
(cherry picked from commit 0fe049f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davidopp davidopp

Labels
area/platform/gce cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
9 participants
@alex-mohr @k8s-github-robot @k8s-bot @bgrant0607 @k8s-cherrypick-bot @goltermann @davidopp @k8s-teamcity-mesosphere @googlebot