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

feat: when the cluster resources change, reschedule the resource templates #4456

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chaunceyjiang
Copy link
Member

Signed-off-by: chaunceyjiang [email protected]
What type of PR is this?
/kind feature

What this PR does / why we need it:
when the cluster resources change, reschedule the resource templates
Which issue(s) this PR fixes:
Fixes #4434

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-scheduler`: when the cluster resources change, reschedule the resource templates

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 20, 2023
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2023

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (c6b2ada) 51.89% compared to head (af21f85) 51.75%.
Report is 29 commits behind head on master.

Files Patch % Lines
pkg/scheduler/event_handler.go 0.00% 39 Missing ⚠️
pkg/scheduler/scheduler.go 26.31% 14 Missing ⚠️
pkg/util/binding.go 0.00% 5 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4456      +/-   ##
==========================================
- Coverage   51.89%   51.75%   -0.14%     
==========================================
  Files         243      243              
  Lines       24164    24243      +79     
==========================================
+ Hits        12539    12547       +8     
- Misses      10944    11014      +70     
- Partials      681      682       +1     
Flag Coverage Δ
unittests 51.75% <7.93%> (-0.14%) ⬇️

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.

@karmada-bot karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 21, 2023
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 21, 2023
@chaunceyjiang
Copy link
Member Author

@XiShanYongYe-Chang PTAL.

@XiShanYongYe-Chang
Copy link
Member

/assign

@chaunceyjiang chaunceyjiang force-pushed the resources branch 2 times, most recently from 91526b0 to 4a4a413 Compare December 21, 2023 09:05
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Thanks a lot~

pkg/scheduler/event_handler.go Outdated Show resolved Hide resolved
pkg/scheduler/event_handler.go Outdated Show resolved Hide resolved
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Overall, LGTM, some little suggestion.

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from xishanyongye-chang after the PR has been reviewed.

The full list of commands accepted by this bot can be found 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

@XiShanYongYe-Chang
Copy link
Member

/lgtm
/cc @RainbowMango

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 25, 2023
@XiShanYongYe-Chang
Copy link
Member

/cc @chaosi-zju

@karmada-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 26, 2023
@chaunceyjiang
Copy link
Member Author

/cc @RainbowMango PTAL.

@chaosi-zju
Copy link
Member

/lgtm thanks a lot

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 +213 to +214
case !equality.Semantic.DeepEqual(oldCluster.Status.ResourceSummary, newCluster.Status.ResourceSummary):
s.clusterResourcesReconcileWorker.Enqueue(newCluster)
Copy link
Member

Choose a reason for hiding this comment

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

My concern is that this will make the scheduler very sensitive because the cluster's resources will be updated frequently, when there are multiple clusters, it will put a lot of pressure on the scheduler.

What I've been thinking:

  1. Is it reasonable for the scheduler to give up on scheduling a resource if it can not find a feasible cluster? Should the scheduler retry these tasks? such as by introducing a retry queue?
  2. Karmada needs a capability that allows users to trigger rescheduling. There are many scenarios where rescheduling is required, cluster resource change is just one small case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When the resources of the member clusters change, karmada-scheduler should trigger a rescheduling.
6 participants