-
Notifications
You must be signed in to change notification settings - Fork 39k
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
Support extended resource in NodeResourcesBalancedAllocation plugin #101946
Conversation
@chendave: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
69222d8
to
c0bc925
Compare
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
c0bc925
to
3b73c18
Compare
This is ready for review. cc more maintainers in case any of you have some bandwidth to review this change, thanks in the advance! |
The release note is not accurate. It kind of implies that, by default, kube-scheduler will start considering new resources. It should say that it can be configured to consider new resources. |
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/resource_allocation.go
Outdated
Show resolved
Hide resolved
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.
Can you retitle to "Support extended resource in NodeResourcesBalancedAllocation
plugin"?
/retitle: Support extended resource in NodeResourcesBalancedAllocation plugin |
NodeResourcesBalancedAllocation
plugin to cover more resources
Thanks guys for your great suggestion! Couple of them are pending on discussion, will update soon when it is get clear. |
staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
/hold TODO
|
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation_test.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation_test.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation_test.go
Outdated
Show resolved
Hide resolved
@alculquicondor it's compatible and UTed properly. |
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 guys for the review and those great suggestion, I hope all those comments have been addressed.
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/noderesources/balanced_allocation_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go
Outdated
Show resolved
Hide resolved
137d21d
to
2bfa67f
Compare
@@ -381,6 +381,82 @@ profiles: | |||
}, | |||
}, | |||
}, | |||
{ | |||
name: "v1beta1 NodeResourcesBalancedAllocation resource weights are set properly", |
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.
This is kind of too much, I would just reuse one of the existing "custom configs" test cases.
As it is, it will add toil for the maintenance of this file.
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.
squeezed this test into "...all plugin args in default profile".
…ources Signed-off-by: Dave Chen <dave.chen@arm.com>
2bfa67f
to
1fa673c
Compare
squashed the commits since it will easier for me to rebase on the head of master. |
@alculquicondor any more comments, or can we get this in 1.22? thanks! |
/lgtm Leaving /approve to @alculquicondor . |
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.
/approve
Sorry, I was out on vacation.
/label api-review |
thanks @alculquicondor , hope you've had a good vacation! I guess this needs the approval from api review, @liggitt would you pls take the final review? |
/cc @liggitt for api review, thanks! |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, chendave, liggitt 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 |
Per the design https://docs.google.com/document/d/1MT0KVnyeAc4QcKqUsmk-4OvmBhOy6PvBQOGnjugrJ9k/edit?ts=60942305# and the discussion in the scheduler meeting.
Signed-off-by: Dave Chen dave.chen@arm.com
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: