-
Notifications
You must be signed in to change notification settings - Fork 904
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
feat: cluster-level resource scheduling suspend and resume capabilities #5937
Conversation
Welcome @Monokaix! It looks like this is your first PR to karmada-io/karmada 🎉 |
c4d5f12
to
08407d6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5937 +/- ##
==========================================
+ Coverage 48.37% 48.39% +0.02%
==========================================
Files 665 665
Lines 54795 54811 +16
==========================================
+ Hits 26507 26527 +20
+ Misses 26569 26567 -2
+ Partials 1719 1717 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
187911e
to
208246d
Compare
// Suspension defines the policy for suspending of propagation, dispatching and scheduling suspension are independent | ||
// of each other. The propagation suspension inherited from PropagationPolicy can be transparently transmitted here, | ||
// and ResourceBinding can also set scheduling suspension separately. |
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.
// Suspension defines the policy for suspending of propagation, dispatching and scheduling suspension are independent | |
// of each other. The propagation suspension inherited from PropagationPolicy can be transparently transmitted here, | |
// and ResourceBinding can also set scheduling suspension separately. | |
// Suspension defines the policy for suspending dispatching and scheduling. |
// Scheduling controls whether scheduling should be suspended, the scheduler will pause scheduling and not | ||
// process resource binding after it's created when value is true and resume scheduling when it's false or nil. | ||
// This is usually used for admission checks by third-party such as quota check, multi-tenancy prioritization | ||
// and maybe more checks in the future before scheduler really does scheduling. |
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.
// Scheduling controls whether scheduling should be suspended, the scheduler will pause scheduling and not | |
// process resource binding after it's created when value is true and resume scheduling when it's false or nil. | |
// This is usually used for admission checks by third-party such as quota check, multi-tenancy prioritization | |
// and maybe more checks in the future before scheduler really does scheduling. | |
// Scheduling controls whether scheduling should be suspended, the scheduler will pause scheduling and not | |
// process resource binding when the value is true and resume scheduling when it's false or nil. | |
// This is designed for third-party systems to temporarily pause the scheduling of applications, which enabling | |
// manage resource allocation, prioritize critical workloads, etc. | |
// | |
// It is expected that third-party systems use an admission webhook to suspend scheduling at the time of | |
// ResourceBinding creation. Once a ResourceBinding has been scheduled, it cannot be paused afterward, as it may | |
// lead to ineffective suspension. | |
// +optional |
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.
- note that scheduling can not be paused after it has been scheduled.
- +optional
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.
Generally looks good to me except the condition part.
I suggest moving the condition out of this PR and starting another PR to handle it.
You mean seperate the webhook validation to another pr? |
I mean not introduce the condition by this PR: // SchedulingSuspended represents the condition that the ResourceBinding or ClusterResourceBinding is suspended to schedule.
SchedulingSuspended string = "SchedulingSuspended" |
Upated. |
// +listType=map | ||
// +listMapKey=type |
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.
Why add these? What does it mean?
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.
It will be useful to manage conditions as a map. but current PR doesn't require it.
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.
Yeah we can track it at #6002, which is needed by ApplyStatus
method.
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.
removed.
Signed-off-by: Monokaix <[email protected]>
Signed-off-by: Monokaix <[email protected]>
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 |
What type of PR is this?
/kind api-change
/kind feature
What this PR does / why we need it:
Add rb suspension capability, background: #5690
Which issue(s) this PR fixes:
Part of #5690
Special notes for your reviewer:
It extends the Suspension fileds and support rb susupend and resume, mostly like the previous issue #5217 and PRs implemented in it.
This pr just implemented binding suspension in
scheduler
, setting status condition incontroller
will be implemented in another pr after #6002 merged because it may lead to status update conflict between scheduler and controller.Does this PR introduce a user-facing change?: