-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Use the same processors for all currently supported provisioning classes #6873
Use the same processors for all currently supported provisioning classes #6873
Conversation
@yaroslava-serdiuk please confirm if lack of validation in injector was intentional, I'll add it if it was a bug. |
Thanks for the changes! autoscaler/cluster-autoscaler/main.go Line 510 in c61e295
Otherwise /lgtm |
53c262f
to
0a0d867
Compare
/lgtm |
/cc @towca |
Sorry for late reply, I just realised that this is a bug and we actually should add validation in injector. |
/assign @x13n |
Let's merge this and add it later (preferably using the map introduced in this PR as source-of-truth). |
// -BookingExpired condition for Provisioned ProvisioningRequest if capacity reservation time is expired. | ||
// -Failed condition for ProvisioningRequest that were not provisioned during defaultExpirationTime. | ||
// TODO(yaroslava): fetch reservation and expiration time from ProvisioningRequest | ||
func (p *provReqProcessor) Process(provReqs []*provreqwrapper.ProvisioningRequest) { |
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.
@x13n just to clarify, most of this code is being moved from cluster-autoscaler/provisioningrequest/checkcapacity/processor.go
(adding comment as the diff doesn't render it clearly)
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.
Yeah, I noticed, ended up copying both files and running a diff locally :)
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aleksandra-malinowska, x13n 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 |
0a0d867
to
11de307
Compare
/lgtm |
/cherry-pick cluster-autoscaler-release-1.30 |
@yaroslava-serdiuk: #6873 failed to apply on top of branch "cluster-autoscaler-release-1.30":
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-sigs/prow repository. |
/cherry-pick cluster-autoscaler-release-1.30 |
@yaroslava-serdiuk: #6873 failed to apply on top of branch "cluster-autoscaler-release-1.30":
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-sigs/prow repository. |
/cherry-pick cluster-autoscaler-release-1.30 |
@yaroslava-serdiuk: new pull request created: #7051 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-sigs/prow repository. |
What this PR does / why we need it:
Refactor that removes CombinedProcessor (we don't need separate processors for any currently envisioned provisioning classes) and adds a source-of-truth for supported provisioning classes.
The only change in behavior is including best-effort-atomic provisioning class as supported in now generic provisioning requests processor.
Which issue(s) this PR fixes:
Issue: #6815
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @yaroslava-serdiuk