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

GroupClusters should sort by score and availableReplica count #5144

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

mszacillo
Copy link
Contributor

@mszacillo mszacillo commented Jul 5, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:
With the PR, Karmada will sort clusterInfo by score, and fallback to sorting by availableReplicas. This will help cases in which some clusters have the same score, but we should prefer scheduling on clusters with higher availableReplicas.

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

Does this PR introduce a user-facing change?:

`karmada-scheduler`: GroupClusters will sort clusters by score and availableReplica count.

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

codecov-commenter commented Jul 5, 2024

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

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 28.22%. Comparing base (c8acebc) to head (01be91b).
Report is 14 commits behind head on master.

Files Patch % Lines
.../scheduler/core/spreadconstraint/group_clusters.go 0.00% 3 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5144      +/-   ##
==========================================
+ Coverage   28.21%   28.22%   +0.01%     
==========================================
  Files         632      632              
  Lines       43568    43548      -20     
==========================================
+ Hits        12291    12292       +1     
+ Misses      30381    30359      -22     
- Partials      896      897       +1     
Flag Coverage Δ
unittests 28.22% <0.00%> (+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.

@@ -17,6 +17,8 @@ limitations under the License.
package spreadconstraint

import (
"k8s.io/utils/ptr"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth noting - the linter was failing because the original import (k8s.io/utils/pointer) is deprecated. This new import will work for newer karmada versions, but is not currently compatible with the version we use (v1.9.0) because it uses an older k8s-util version.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the information.
I wonder if you maintain some private commits based on v1.9.0?
I'm asking because I want to know if this going to be a problem for you. I mean if this patch not present on release 1.9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a couple private commits on our v1.9.0 branch, but most of them we are planning to contribute via our published proposals. I think we can try updating to the v1.10.0 release branch which will have the updated k8s-util version and can pick up this commit. I'll test v1.10 with out setup and confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed changes work with v1.10 - I think we can move forward with upgrading our local Karmada version.

@RainbowMango
Copy link
Member

/assign @whitewindmills

Copy link
Member

@whitewindmills whitewindmills left a comment

Choose a reason for hiding this comment

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

/lgtm
note that you need to squash your commits

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2024
Signed-off-by: mszacillo <mszacillo@bloomberg.net>
@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2024
@mszacillo
Copy link
Contributor Author

Thanks for the review - squashed my commits.

@whitewindmills
Copy link
Member

/approve

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: whitewindmills

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 10, 2024
@whitewindmills
Copy link
Member

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2024
@karmada-bot karmada-bot merged commit 15df251 into karmada-io:master Jul 10, 2024
12 checks passed
@mszacillo
Copy link
Contributor Author

@whitewindmills Will this commit be cherrypicked into the v1.10 release branch?

@RainbowMango
Copy link
Member

Normally we don't backport this kind of PR, but this seems no harm/risk in doing that, so I'm happy to backport it to meet users' expectations.

If no objection from @whitewindmills, we can do that. (PS: there is a doc explains how to cherry-pick PRs)

@whitewindmills
Copy link
Member

but this seems no harm/risk in doing that, so I'm happy to backport it to meet users' expectations.

I'm ok with this.

karmada-bot added a commit that referenced this pull request Jul 12, 2024
…4-upstream-release-1.10

Automated cherry pick of #5144: GroupClusters should sort by score and availableReplica count
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/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. 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.

GenerateClustersInfo should take AvailableReplicas into account when sorting
5 participants