-
Notifications
You must be signed in to change notification settings - Fork 45
[Main] Move to ListAll() calls for clusters to enable scalability testing #492
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
Conversation
|
Due there being a possible regression, can we get this tested on the rancher/tests side to be sure? |
|
For the purposes of review, putting the functions here https://github.com/git-ival/shepherd/blob/92397445854882ab3b438fd2e56299b674bfefac/clients/rancher/v1/client.go#L220-L260: |
|
thanks @rancher-max so the difference really is between getting all clusters, or just a single page. Which seems like it won't be an issue in rancher/tests since they're not dealing with 100(0)s of clusters. If we can get someone from hostbusters to ok this. I'm good with approving it. |
|
Hey @susesgartner @markusewalker 👋, I got pointed in your direction for some reviews 🫡 |
markusewalker
left a comment
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.
Ran quick tests on rancher/tests and rancher/tfp-automation and it looked fine. If something ends up breaking, can address later 👍🏿
|
Do we need backports for this PR, or do we wanna have it only on main? @git-ival cc: @markusewalker |
|
Would be ideal to have backports, I'll work on those 👍 |
ac26062
ac26062 to
b078498
Compare
b078498 to
58739d8
Compare
igomez06
left a comment
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.
LGTM
Issue: N/A
Problem
Currently, we use
List()calls when making GETs for clusters. This is needed to enable higher scalability testing levels (> 1000 clusters).Solution
Changed
List()calls toListAll()calls.Testing
I have not tested on
rancher/tests, but downstream inrancher/dartboardthere were no regressions observed.Regressions Considerations
Its possible there may be regressions if downstream users expect the original
List()behavior.