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

[Proposal] Support for resource scheduling suspend and resume capabilities #5690

Merged

Conversation

Vacant2333
Copy link
Contributor

@Vacant2333 Vacant2333 commented Oct 13, 2024

What type of PR is this?

/kind design
/kind documentation

Which issue(s) this PR fixes:
Fixes #4937

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added kind/design Categorizes issue or PR as related to design. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/documentation Categorizes issue or PR as related to documentation. labels Oct 13, 2024
@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.40%. Comparing base (ba1e68d) to head (2fdd66a).
Report is 3 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5690      +/-   ##
==========================================
+ Coverage   48.37%   48.40%   +0.03%     
==========================================
  Files         665      665              
  Lines       54795    54850      +55     
==========================================
+ Hits        26507    26552      +45     
- Misses      26569    26584      +15     
+ Partials     1719     1714       -5     
Flag Coverage Δ
unittests 48.40% <ø> (+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.

@Vacant2333 Vacant2333 marked this pull request as ready for review October 15, 2024 01:33
@karmada-bot karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 15, 2024
@karmada-bot karmada-bot requested a review from Poor12 October 15, 2024 01:33
@Monokaix
Copy link
Contributor

Should also add rb status condition just like #5317.

@Monokaix Monokaix force-pushed the proposal-for-scheduling-suspension branch 5 times, most recently from 4e3260e to e0ca35f Compare December 3, 2024 01:53
@Monokaix Monokaix force-pushed the proposal-for-scheduling-suspension branch from e0ca35f to ba71093 Compare December 9, 2024 03:12
@Monokaix
Copy link
Contributor

Monokaix commented Dec 9, 2024

/assign

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/assign

Comment on lines 25 to 35
<!--
Karmada 目前已经允许用户通过 Suspension 来暂停和恢复资源的 propagation 行为,这提高了 Karmada 的灵活性。

但是目前 Suspension 支持的不够彻底,在 Karmada 中资源的分发可以简单理解为两个阶段,资源的调度和资源的传播。

目前传播阶段可以较为灵活的由用户暂停和恢复,但是没有在调度阶段,没有提供暂停和恢复的能力。

我们希望在 Karmada 的基础之上,通过暂停调度的方式,实现队列和配额管理等高阶能力,从而将集群有限的资源优先调度给高优先级的工作负载,增强 Karmada 在多集群调度的能力,从而满足用户在复杂场景下的需求。

本文基于 [#5118](https://github.com/karmada-io/karmada/pull/5118) 在其中的 Suspension 基础之上,提供了暂停资源调度的能力。
-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all Chinese content from the proposal. We should only use English to describe and review the proposal to avoid issues like being out of sync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

-->
Karmada scheduler listens the `ResourceBinding` resource , ignores the `ResourceBinding` resource when `ResourceBinding.Suspension.Scheduling`=true, pauses scheduling, and resumes scheduling when `ResourceBinding.Suspension.Scheduling`=false.

And when the `ResourceBinding` has been successfully scheduled, it is no longer allowed to modify the `ResourceBinding` from non-suspended to suspended, and when `ResourceBinding.Suspension.Scheduling` is set to true or false, the corresponding condition is refreshed in the status of the `ResourceBinding`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And when the ResourceBinding has been successfully scheduled, it is no longer allowed to modify the ResourceBinding from non-suspended to suspended,

It sounds like we need to introduce a validating webhook to force this, right? Can you explain why we need to do that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I have considered that, becaue the scheduling suspension happened before scheduling success, and after rb is successfully scheduled it's no necessary to suspend rb again, which will also confuse users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the work suspension has the same problem.

Copy link
Member

@RainbowMango RainbowMango Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and after rb is successfully scheduled it's no necessary to suspend rb again, which will also confuse users.

Yeah, I agree, it brings ambiguousness if we don't force that.
I will track this on #5977.

@Monokaix Monokaix force-pushed the proposal-for-scheduling-suspension branch from ba71093 to 971f7d1 Compare January 2, 2025 01:44
Copy link
Member

@RainbowMango RainbowMango left a 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.
But, would you like to squash the commits?

Signed-off-by: Vacant2333 <[email protected]>

Signed-off-by: Monokaix <[email protected]>
@Monokaix Monokaix force-pushed the proposal-for-scheduling-suspension branch from 971f7d1 to 2fdd66a Compare January 2, 2025 02:13
@Monokaix
Copy link
Contributor

Monokaix commented Jan 2, 2025

Generally looks good to me. But, would you like to squash the commits?

done

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 2, 2025
@karmada-bot
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2025
@Monokaix
Copy link
Contributor

Monokaix commented Jan 2, 2025

/reopen

@karmada-bot
Copy link
Collaborator

@Monokaix: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

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.

@RainbowMango
Copy link
Member

/reopen

@karmada-bot karmada-bot reopened this Jan 2, 2025
@karmada-bot
Copy link
Collaborator

@RainbowMango: Reopened this PR.

In response to this:

/reopen

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.

@karmada-bot karmada-bot merged commit b3b1aa6 into karmada-io:master Jan 2, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/design Categorizes issue or PR as related to design. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Support suspend ResourceBinding when create
7 participants