-
Notifications
You must be signed in to change notification settings - Fork 293
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
Introduce the ClusterAutoscaler APIs module #1872
Introduce the ClusterAutoscaler APIs module #1872
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tenzen-y 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 |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
Lines 400 to 404 in a751ffd
needs an update |
Yes, that's right.
Or, putting https://github.com/kubernetes/autoscaler/tree/09954b6741cbb910971916c079f45f6e8878d192/cluster-autoscaler/config/crd to https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/apis/provisioningrequest would be better. @alculquicondor @x13n WDYT? |
Putting the CRD in CA apis module makes sense to me, it has to be in sync with the code there anyway. |
I'm also ok upgrading to golang 1.22 But all the APIs should be in the separate module |
@x13n @alculquicondor Thank you for responding. |
Raised at kubernetes/autoscaler#6651 |
kubernetes/autoscaler#6651 is merged. |
Thank you! |
305f2f9
to
db5acf4
Compare
db5acf4
to
2fe331a
Compare
Signed-off-by: Yuki Iwai <[email protected]>
2fe331a
to
cf651cf
Compare
@alculquicondor This PR is ready for review. |
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.
Thank you so much @tenzen-y!!!!
This was a long effort that simplifies the maintainability of our deps greatly!
I appreciate to collaboration with SIG-Autoscaling (@x13n and @mwielgus)! @alculquicondor btw maybe you forgot to add |
cc: @B1F030 @village-way |
I did indeed forget :) /lgtm |
LGTM label has been added. Git tree hash: 2111d2abc5d15cc89d44ac302fa4c42e8b44d122
|
@alculquicondor How about cherry-picking this into the release-0.6 branch? |
I'm not too sure about that. Why do you suggest it? Also, I'm not sure what's the origin of the Job failure. |
Cherry-picking would allow cluster-admins to avoid #1345 error when cluster-admins implement a separate controller for the in-house job. [MODIFIED COMMENT] |
I'll try to investigate CI failure. |
Maybe this is the reason 🧐 |
I guess that this error occurred by throttling CPU performance because integration tests often use larger CPUs. |
It seems that we need to set 8 Cores in the limit. |
One of my teammates is working in splitting the Multikueue E2E test into its own job. Let's decide if the limits are ok after that. |
That makes sense. |
@alculquicondor How about this? |
/cherry-pick release-0.6 |
@alculquicondor: #1872 failed to apply on top of branch "release-0.6":
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. |
ACK |
Signed-off-by: Yuki Iwai <[email protected]>
Signed-off-by: Yuki Iwai <[email protected]>
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
I replaced the
k8s.io/autoscaler/cluster-autoscaler
with thek8s.io/autoscaler/cluster-autoscaler/apis
because kubernetes/autoscaler#6315 is finally merged.Which issue(s) this PR fixes:
Fixes #1345
Special notes for your reviewer:
Does this PR introduce a user-facing change?