refactor scheduler filters package #797
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XL
|
/assign |
…y and maintainability Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
nirrozenbaum
force-pushed
the
filter-refactor
branch
from
3eaeeef to
eb5da45
Compare
| var defaultConfig = &SchedulerConfig{ | ||
| preSchedulePlugins: []plugins.PreSchedule{}, | ||
| filters: []plugins.Filter{&filter.SheddableRequestFilter{}, filter.LowLatencyFilter}, | ||
| scorers: map[plugins.Scorer]int{}, | ||
| picker: &picker.RandomPicker{}, | ||
| postSchedulePlugins: []plugins.PostSchedule{}, | ||
| } |
There was a problem hiding this comment.
after having some experience with setting a custom plugins, the recommended way would be to set in main.go the scheduler using NewSchedulerWithConfig.
no need for global var. default configuration is defined inside NewScheduler instead.
| // Filter filters out pods that doesn't meet the filter criteria. | ||
| func (f *HasCapacityFilter) Filter(ctx *types.SchedulingContext, pods []types.Pod) []types.Pod { | ||
| filteredPods := []types.Pod{} | ||
|
|
There was a problem hiding this comment.
I'm reading this now again (reviewing myself lol).
@liu-cong does it make sense to get rid of Sheddable filter and put the following condition here at the beginning of the filter func?
if ctx.Req.Critical {
return pods
}sheddable filter doesn't do anything other than that..
anyway, if you agree with me, please let's defer to next PR since this is large enough and I don't want to include more changes unless it's a must.
There was a problem hiding this comment.
Yeah it makes sense to combine the two, perhaps call it HasCapacityForSheddableRequestFilter. Feel free to refactor or defer it.
| // Filter filters out pods that doesn't meet the filter criteria. | ||
| func (f *HasCapacityFilter) Filter(ctx *types.SchedulingContext, pods []types.Pod) []types.Pod { | ||
| filteredPods := []types.Pod{} | ||
|
|
There was a problem hiding this comment.
Yeah it makes sense to combine the two, perhaps call it HasCapacityForSheddableRequestFilter. Feel free to refactor or defer it.
k8s-ci-robot
added
the
lgtm
|
/approve in case you want to address fusing the two filters into one |
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, nirrozenbaum 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 |
k8s-ci-robot
added
the
approved
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm Great stuff! |
* refactor schdeuler filters package to simplify and improve readability and maintainability Signed-off-by: Nir Rozenbaum <[email protected]> * filter refactor finalizing Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
* refactor schdeuler filters package to simplify and improve readability and maintainability Signed-off-by: Nir Rozenbaum <[email protected]> * filter refactor finalizing Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
* refactor schdeuler filters package to simplify and improve readability and maintainability Signed-off-by: Nir Rozenbaum <[email protected]> * filter refactor finalizing Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
5 participants
This PR simplifies scheduler filters structuring in order to simplify, improve readability and maintainability, and align with best practices.
NO LOGIC CHANGE, only moving existing filters/logic to separate files.
the plugins that live in this repo should serve as an example for newcomers and adopters and as such, it's required to have the most simplified and aligned with best practices code.
not in scope of this PR:
handling of any scheduler package other than filter. any additional change will come in a separate PR.