added scheduler plugins interfaces to initialize scheduler refactoring #721
Conversation
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nirrozenbaum The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
| } | ||
|
|
||
| hasCapacityFilter = &basicFilter{ | ||
| name: "has capacity for sheddable requests", | ||
| filter: toFilterFunc(queueThresholdPredicate(config.QueueThresholdCritical).and(kvCacheThresholdPredicate(config.KVCacheThreshold))), | ||
| } | ||
|
|
||
| dropRequestFilter = &basicFilter{ |
There was a problem hiding this comment.
I would prefer we not remove any existing code to prevent regression
There was a problem hiding this comment.
dropRequestFilter was never in real use, cause we never check error in code.
the only place is this line in Schedule function:
if err != nil || len(pods) == 0 { ...and for that we don't need the error return value, nor the dropRequestFilter (which seems very odd.. it's not a filter)
| } | ||
|
|
||
| return filtered_available, nil |
There was a problem hiding this comment.
Oh...we never returned an error? I wonder if there is a go linter that checks for this kind of thing.
There was a problem hiding this comment.
exactly. no use of error. also no use of DropRequestFilter.
the scheduler can just check if len(returned_pods)==0 (up until today scheduler did both err check and that).
that was one of my comments in the original PR:
https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/677/files#r2042685374
|
This seems reasonably thin, I would have even preferred a more cautious approach where we duplicate much of the code. But a lot of this seems to just be non-functional cleanup. |
indeed, most of the changes here are non-functional cleanup to adapt to the new filter interface and to the updated Pod interface. |
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
|
We can close this now that the other PR has been merged, sorry for the confusion |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
closing since #677 was merged |
after several maintainers reviewed @liu-cong PR #677, it was decided to break down that PR into smaller PRs to allow easier reviews and refactoring.
This PR is a first out of a series of PRs that will result in the scheduler refactoring according to the description in #677.
Current PR defines the scheduler new plugins interfaces and does the minimal changes to adapt the existing code to use the new Filter interface.
In a follow up PR, the scheduler will be updated to use these interfaces in its
Schedulefunction.