Skip to content
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

scheduler: revise ReservationFilterPlugin and fix preempting pods resources #2315

Conversation

saintube
Copy link
Member

@saintube saintube commented Jan 7, 2025

Ⅰ. Describe what this PR does

Koord-Scheduler:

  1. Revise the ReservationFilterPlugin interface to make it extensible and provide more accurate filtering results. Please see the section "Special notes" for the details.
  2. Support node reservation API in a Reservation object.
  3. Fix a bug about counting reservation's "pods" resources during the preemption.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Rename the ReservationFilterPlugin method FilterReservation to FilterNominateReservation since only the NominateReservation calls it during the PreScore or the Reserve phase.

Re-define the method FilterReservation that is invoked in the Filter phase:

type ReservationFilterPlugin interface {
	framework.Plugin
	FilterReservation(ctx context.Context, cycleState *framework.CycleState, pod *corev1.Pod, reservationInfo *ReservationInfo, nodeInfo *framework.NodeInfo) *framework.Status
	FilterNominateReservation(ctx context.Context, cycleState *framework.CycleState, pod *corev1.Pod, reservationInfo *ReservationInfo, nodeName string) *framework.Status
}

Compared with the current FilterReservation which runs in the PreScore or Reserve, the new method is invoked during the Filter phase of the reservation plugin. It should show more concise reasons when the scheduling fails, and it is helpful for internal extensions.

In this version, we keep the FilterNominateReservation and will move the current implementation to the Filter phase in the future.

V. Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in make test

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 74.07407% with 14 lines in your changes missing coverage. Please review.

Project coverage is 66.02%. Comparing base (1e0be0b) to head (7275544).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/scheduler/plugins/reservation/plugin.go 80.64% 4 Missing and 2 partials ⚠️
pkg/scheduler/frameworkext/framework_extender.go 72.72% 2 Missing and 1 partial ⚠️
pkg/scheduler/plugins/deviceshare/plugin.go 33.33% 2 Missing ⚠️
pkg/scheduler/plugins/nodenumaresource/plugin.go 33.33% 2 Missing ⚠️
pkg/scheduler/frameworkext/reservation_info.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2315      +/-   ##
==========================================
- Coverage   66.04%   66.02%   -0.03%     
==========================================
  Files         458      458              
  Lines       53947    53984      +37     
==========================================
+ Hits        35631    35644      +13     
- Misses      15757    15776      +19     
- Partials     2559     2564       +5     
Flag Coverage Δ
unittests 66.02% <74.07%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@saintube saintube force-pushed the scheduler-enhance-reservation-filter branch from bc4666d to c96c408 Compare January 8, 2025 01:46
@zwzhang0107
Copy link
Contributor

/lgtm

@saintube saintube force-pushed the scheduler-enhance-reservation-filter branch from c96c408 to df11698 Compare January 8, 2025 02:53
@ZiMengSheng
Copy link
Contributor

/lgtm

@saintube saintube force-pushed the scheduler-enhance-reservation-filter branch from df11698 to 570f709 Compare January 8, 2025 06:53
@saintube saintube added the lgtm label Jan 8, 2025
saintube and others added 2 commits January 9, 2025 10:18
Co-authored-by: shenxin <rougang.hrg@alibaba-inc.com>
Signed-off-by: saintube <saintube@foxmail.com>
Co-authored-by: shenxin <rougang.hrg@alibaba-inc.com>
Signed-off-by: saintube <saintube@foxmail.com>
@saintube saintube force-pushed the scheduler-enhance-reservation-filter branch from 570f709 to 7275544 Compare January 9, 2025 03:11
@saintube saintube removed the lgtm label Jan 9, 2025
@saintube saintube changed the title scheduler: revise ReservationFilterPlugin scheduler: revise ReservationFilterPlugin and fix preempting pods resources in reservation Jan 9, 2025
@saintube saintube changed the title scheduler: revise ReservationFilterPlugin and fix preempting pods resources in reservation scheduler: revise ReservationFilterPlugin and fix preempting pods resources Jan 9, 2025
@zwzhang0107
Copy link
Contributor

/lgtm
/approve

@koordinator-bot koordinator-bot bot merged commit 236bcc9 into koordinator-sh:main Jan 10, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants