-
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
Add ProvisioningRequestPodsFilter processor #6386
Conversation
b821e53
to
1e1be97
Compare
cluster-autoscaler/processors/provreq/provisioning_request_pods_filter.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/provreq/provisioning_request_pods_filter.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/provreq/provisioning_request_pods_filter.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/provreq/provisioning_request_pods_filter.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/provreq/provisioning_request_pods_filter.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/provreq/provisioning_request_pods_filter.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/provreq/provisioning_request_pods_filter.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/provreq/provisioning_request_pods_filter.go
Outdated
Show resolved
Hide resolved
a8e12e9
to
c92c5f1
Compare
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.
Couple more comments
cluster-autoscaler/processors/provreq/provisioning_request_processors_test.go
Show resolved
Hide resolved
e1afc78
to
4bfc6f7
Compare
cluster-autoscaler/processors/provreq/provisioning_request_processors_test.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/provreq/provisioning_request_processors_test.go
Show resolved
Hide resolved
febb424
to
dea3f7a
Compare
/lgtm |
@kisieland: changing LGTM is restricted to collaborators 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. |
Assigning, since you're already looking into this. /assign @kisieland Please assign to me (or another code owner) after LGTMing. |
@yaroslava-serdiuk please resolve the merge conflicts on the PR and it should be ready for review by @x13n |
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.
/assign
Took a look, just a few comments.
cluster-autoscaler/processors/provreq/provisioning_request_processors.go
Outdated
Show resolved
Hide resolved
dea3f7a
to
772f753
Compare
b7672fc
to
164e53f
Compare
0633fea
to
f4c0407
Compare
f4c0407
to
1c4e6c9
Compare
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.
Thanks! Just one minor comment, feel free to ignore. Putting PR on hold in case you want to address it.
/lgtm
/approve
/hold
@@ -250,6 +251,7 @@ var ( | |||
"--max-graceful-termination-sec flag should not be set when this flag is set. Not setting this flag will use unordered evictor by default."+ | |||
"Priority evictor reuses the concepts of drain logic in kubelet(https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2712-pod-priority-based-graceful-node-shutdown#migration-from-the-node-graceful-shutdown-feature)."+ | |||
"Eg. flag usage: '10000:20,1000:100,0:60'") | |||
provisioningRequestsEnabled = flag.Bool("enable-provisioning-requests", false, "Whether the clusterautoscaler will be handling the ProvisioningRequest CRs.") |
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.
nit: I'd avoid mentioning clusterautoscaler in the description. It is a flag to clusterautoscaler binary, so saying the flag will affect clusterautoscaler is a bit redundant.
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, the alternative is to put it in passive voice, but I don't think it's necessary. We already mention clusterautoscaler in many flags.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: x13n, yaroslava-serdiuk 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 |
/label tide/merge-method-squash |
What type of PR is this?
/kind feature
Implementation of https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/proposals/provisioning-request.md