-
Notifications
You must be signed in to change notification settings - Fork 182
remove the PreCycle plugin from scheduler #876
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
Conversation
Signed-off-by: Nir Rozenbaum <[email protected]>
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
/assign |
I am fine with removing the preCycle as there is no use case for it anyway. Is the plan to replace PostCycle with PostResponse? IMO we need something like PreRequest. Consider this scenario, two requests |
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.
/lgtm
pkg/epp/scheduling/framework/plugins/multi/prefix/plugin_test.go
Outdated
Show resolved
Hide resolved
pkg/epp/scheduling/framework/plugins/multi/prefix/plugin_test.go
Outdated
Show resolved
Hide resolved
pkg/epp/scheduling/framework/plugins/multi/prefix/plugin_test.go
Outdated
Show resolved
Hide resolved
pkg/epp/scheduling/framework/plugins/multi/prefix/plugin_test.go
Outdated
Show resolved
Hide resolved
pkg/epp/scheduling/framework/plugins/multi/prefix/plugin_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Cong Liu <[email protected]>
@liu-cong applied the suggested changes, need lgtm again :) |
/lgtm |
@liu-cong the discussion on scheduler extension points and interfaces is still ongoing (in PR #845). |
/approve |
[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 |
* remove the PreCycle plugin from scheduler Signed-off-by: Nir Rozenbaum <[email protected]> * Apply suggestions from code review Co-authored-by: Cong Liu <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]> Co-authored-by: Cong Liu <[email protected]>
* remove the PreCycle plugin from scheduler Signed-off-by: Nir Rozenbaum <[email protected]> * Apply suggestions from code review Co-authored-by: Cong Liu <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]> Co-authored-by: Cong Liu <[email protected]>
* remove the PreCycle plugin from scheduler Signed-off-by: Nir Rozenbaum <[email protected]> * Apply suggestions from code review Co-authored-by: Cong Liu <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]> Co-authored-by: Cong Liu <[email protected]>
* remove the PreCycle plugin from scheduler Signed-off-by: Nir Rozenbaum <[email protected]> * Apply suggestions from code review Co-authored-by: Cong Liu <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]> Co-authored-by: Cong Liu <[email protected]>
as discussed in scheduler design latest discussions, the plan is to completely remove the Pre/Post Cycle (previously called Pre/Post Schedule) from Scheduler. atm the only plugin that uses both is Prefix.
This PR completely removes
PreCycle
plugin from the scheduler.As a side effect, this PR also fixes the following bug:
before this PR, the logic that calculated hashes in Prefix plugin
PreCycle
could have been buggy, since pods that have the longest prefix may be filtered out in the filter phase.This PR is fixing this problem by calculating longest prefix only for the pods that passed the filtering phase.
cc: @ahg-g @kfswain @liu-cong