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 cluster-level resource propagation pause and resume capabilities #5118

Merged

Conversation

XiShanYongYe-Chang
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang commented Jul 2, 2024

What type of PR is this?

/kind design
/kind documentation

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1567, #4421, #4688

Special notes for your reviewer:

Other related issues: #4937

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/design Categorizes issue or PR as related to design. label Jul 2, 2024
@karmada-bot karmada-bot added kind/documentation Categorizes issue or PR as related to documentation. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 2, 2024
@XiShanYongYe-Chang
Copy link
Member Author

/cc @CharlesQQ @a7i @chaunceyjiang @whitewindmills
Hi guys, can you help take a look?

@karmada-bot karmada-bot requested review from a7i and chaunceyjiang July 2, 2024 03:23
@karmada-bot
Copy link
Collaborator

@XiShanYongYe-Chang: GitHub didn't allow me to request PR reviews from the following users: CharlesQQ.

Note that only karmada-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @CharlesQQ @a7i @chaunceyjiang @whitewindmills
Hi guys, can you help take a look?

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.

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 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 28.25%. Comparing base (a87ec2a) to head (24e56c7).
Report is 4 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    #5118      +/-   ##
==========================================
+ Coverage   28.23%   28.25%   +0.01%     
==========================================
  Files         632      632              
  Lines       43723    43739      +16     
==========================================
+ Hits        12345    12358      +13     
- Misses      30473    30479       +6     
+ Partials      905      902       -3     
Flag Coverage Δ
unittests 28.25% <ø> (+0.01%) ⬆️

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.

@whitewindmills
Copy link
Member

@XiShanYongYe-Chang
it's also releated issue

@whitewindmills
Copy link
Member

/assign

@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the add-work-pause-proposal branch 3 times, most recently from 81b39a1 to 2dd94a0 Compare July 5, 2024 07:57
@XiShanYongYe-Chang
Copy link
Member Author

Hi @CharlesQQ @chaunceyjiang @whitewindmills @RainbowMango , it's ready to review again.

@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the add-work-pause-proposal branch from 2dd94a0 to f5a3454 Compare July 6, 2024 09:47
Copy link
Contributor

@Vacant2333 Vacant2333 left a comment

Choose a reason for hiding this comment

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

Hi!
Thanks for your proposal!

In Solution One, the SchedulePause of ResourceBinding is propagated through PropagationPolicy. If I create a PropagationPolicy with a PauseStrategy set to nil by default, is it feasible to only update the SchedulePause of ResourceBinding to true via a webhook when the karmada detector creates the ResourceBinding? If this is feasible, does "propagation" mean that the associated ResourceBinding/Work is only updated when the PropagationPolicy updates the PauseStrategy field?

I also have a suggestion: can this issue be considered as a user story? In brief, it involves pausing the ResourceBinding at the time of creation through a webhook, and then using a custom controller to determine when certain ResourceBindings can be scheduled.

#4919

@Monokaix
Copy link
Contributor

Monokaix commented Jul 8, 2024

Hi! Thanks for your proposal!

In Solution One, the SchedulePause of ResourceBinding is propagated through PropagationPolicy. If I create a PropagationPolicy with a PauseStrategy set to nil by default, is it feasible to only update the SchedulePause of ResourceBinding to true via a webhook when the karmada detector creates the ResourceBinding? If this is feasible, does "propagation" mean that the associated ResourceBinding/Work is only updated when the PropagationPolicy updates the PauseStrategy field?

I also have a suggestion: can this issue be considered as a user story? In brief, it involves pausing the ResourceBinding at the time of creation through a webhook, and then using a custom controller to determine when certain ResourceBindings can be scheduled.

#4919

+1
What if set rb's schedulePause to true directly by webhook? this case will not get propagation involved, seems it's an independent case and is not like the work pause which is inherited from pp.

@XiShanYongYe-Chang
Copy link
Member Author

is it feasible to only update the SchedulePause of ResourceBinding to true via a webhook when the karmada detector creates the ResourceBinding?

For solution 1, the source of user control is in the PropagationPolicy. If it is modified through webhook, it will be inconsistent with the declaration of PropagationPolicy.

I think solution 2 might be better for your scenario.

@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the add-work-pause-proposal branch from f5a3454 to 30c5f03 Compare July 15, 2024 08:21
@XiShanYongYe-Chang
Copy link
Member Author

/retest

@XiShanYongYe-Chang
Copy link
Member Author

Hi guys, at the moment, I'm inclined towards Plan One, as it is more straightforward and convenient for users, without the need for learning new APIs. If we introduce a new API to describe the pause strategy for resources, it might seem a bit limited. Later on, if we provide a Rollout API, we can then focus on thinking about adding new APIs.

How do you think? @CharlesQQ @chaunceyjiang @whitewindmills @RainbowMango @a7i @Monokaix @Vacant2333

@RainbowMango
Copy link
Member

I vote for solution one and sent the draft API on #5118 (comment).

@a7i
Copy link
Contributor

a7i commented Jul 16, 2024

That sounds good!

Once approved/finalized, would you be open to contributions or did you want to lead this change?

@XiShanYongYe-Chang
Copy link
Member Author

Once approved/finalized, would you be open to contributions or did you want to lead this change?

Of course, I will create a new issue to track the tasks that need to be completed, and I look forward to your participation in some of these tasks.

@RainbowMango
Copy link
Member

Hi Amir, After we make the decision here, we can get started from #4838(the one you sent 3 months ago, sorry again for the delay).
As @XiShanYongYe-Chang said above, he will help to open an umbrella issue to track all the tasks there, including code, testing, etc, feel free to take any tasks you are interested in, even all of them, just leave a comment on that umbrella issue would be fine. Thank you in advance!

@CharlesQQ
Copy link
Member

I vote for solution one and sent the draft API on #5118 (comment).

+1

docs/proposals/work-pause/README.md Outdated Show resolved Hide resolved
docs/proposals/work-pause/README.md Outdated Show resolved Hide resolved
docs/proposals/work-pause/README.md Outdated Show resolved Hide resolved
docs/proposals/work-pause/README.md Outdated Show resolved Hide resolved
docs/proposals/work-pause/README.md Outdated Show resolved Hide resolved
@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the add-work-pause-proposal branch from 30c5f03 to 9ad7037 Compare July 18, 2024 02:11
@karmada-bot karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 18, 2024
@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the add-work-pause-proposal branch from 9ad7037 to 24e56c7 Compare July 18, 2024 04:02
@RainbowMango
Copy link
Member

/retest

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 Jul 19, 2024
@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 Jul 19, 2024
@karmada-bot karmada-bot merged commit bf2a604 into karmada-io:master Jul 19, 2024
12 checks passed
@XiShanYongYe-Chang XiShanYongYe-Chang deleted the add-work-pause-proposal branch August 17, 2024 09:08
@RainbowMango RainbowMango added this to the v1.11 milestone Aug 31, 2024
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.

How to Support dynamically adding overriders
10 participants