-
Notifications
You must be signed in to change notification settings - Fork 296
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
🐛 Improve clustermodule existence check #2535
🐛 Improve clustermodule existence check #2535
Conversation
/hold Just to merge the logging PR first |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2535 +/- ##
==========================================
+ Coverage 63.59% 63.83% +0.24%
==========================================
Files 120 119 -1
Lines 8685 8678 -7
==========================================
+ Hits 5523 5540 +17
+ Misses 2735 2718 -17
+ Partials 427 420 -7 ☔ View full report in Codecov by Sentry. |
ok, will wait for a new govmomi version to bump here :) |
/hold cancel logging PR is merged |
60a02dd
to
2b1b1aa
Compare
/hold |
all done, I've kept the commits separated to easy the review, let me know when to squash it :) |
4a6683d
to
3299124
Compare
Last nit: https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/pull/2535/files#r1425328351 Feel free to squash directly |
3299124
to
4016141
Compare
/hold cancel |
4016141
to
94356e1
Compare
/approve /assign @chrischdi @rikatz Would be probably nice to cherry-pick into 1.8 / 1.7? |
/test pull-cluster-api-provider-vsphere-e2e-full-main |
@sbueringer yeah, on my side we are using CAPV 1.8 so would be great, but also I'm happy to cherrypick to v1.7 (if everything fails here due to rebases, I can do it manually!) |
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
/approve
LGTM label has been added. Git tree hash: c3e8ed5603204716376ec0a0895599ad6e7f6912
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrischdi, sbueringer 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 |
Sounds good, thank you! I think automatic cherry-pick will fail because of the logging changes (I think you had to rebase after the logging PR was merged) |
/retest |
@rikatz: 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. |
What this PR does / why we need it:
NodeAntiAffinity uses an api called clustermodules. This API does not have a "GET" operation, which means that checking it existence needs to do a list and iterate over this list, being an expensive operation.
But the "membership" API, part of clustermodules does have a Get members that, in case of the clustermodule not existing, will throw a 404 error.
Using this check instead may help improving the usage of clustermodules on large environments.
Note from Ricardo
I'm testing this PR locally on a place I have found this to be a problem :) Will post more information soon- This works really fine and the consumption of rest endpoint has felt a lot!