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

Implement calc group score for duplicate to assign replicas evenly #5622

Merged

Conversation

ipsum-0320
Copy link
Contributor

@ipsum-0320 ipsum-0320 commented Sep 28, 2024

What type of PR is this?
/kind feature

What this PR does / why we need it:
Optimize the score calculation method to better measure the pros and cons of the group. This PR implements calc group score for duplicate to assign replicas evenly.

Which issue(s) this PR fixes:
Part of #5159

Special notes for your reviewer:
@whitewindmills

Does this PR introduce a user-facing change?:

`karmada-scheduler`: implement group score calculation instead of take the highest score of clusters

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 28, 2024
@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 28, 2024
@ipsum-0320 ipsum-0320 force-pushed the Implement-group-score-calc-bonus branch from 5d9742c to 59be12b Compare September 28, 2024 07:22
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2024

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

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 41.64%. Comparing base (ff6075c) to head (9e349e6).

Files with missing lines Patch % Lines
.../scheduler/core/spreadconstraint/group_clusters.go 94.11% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5622      +/-   ##
==========================================
- Coverage   41.66%   41.64%   -0.03%     
==========================================
  Files         653      653              
  Lines       55518    55533      +15     
==========================================
- Hits        23132    23127       -5     
- Misses      30885    30908      +23     
+ Partials     1501     1498       -3     
Flag Coverage Δ
unittests 41.64% <94.11%> (-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.

@ipsum-0320 ipsum-0320 force-pushed the Implement-group-score-calc-bonus branch from 59be12b to e3aafcc Compare October 15, 2024 11:22
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 15, 2024
@ipsum-0320 ipsum-0320 force-pushed the Implement-group-score-calc-bonus branch from e3aafcc to 59be12b Compare October 15, 2024 11:23
@karmada-bot karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 2024
@ipsum-0320 ipsum-0320 force-pushed the Implement-group-score-calc-bonus branch from 59be12b to 36746e4 Compare October 15, 2024 11:24
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 15, 2024
@karmada-bot karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 22, 2024
@ipsum-0320 ipsum-0320 changed the title Implement bonus group score calc to assign replicas evenly Implement calc group score for duplicate to assign replicas evenly Oct 22, 2024
@whitewindmills
Copy link
Member

wait for #5621
/hold

@karmada-bot karmada-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2024
@RainbowMango
Copy link
Member

wait for #5621 /hold

Let's get back on this as #5621 has been merged.

@ipsum-0320
Copy link
Contributor Author

i will resolve the conflicts.

@ipsum-0320 ipsum-0320 force-pushed the Implement-group-score-calc-bonus branch from 07f6f44 to df53e4d Compare October 27, 2024 04:34
@ipsum-0320
Copy link
Contributor Author

now, i have do something for this PR. i need you guys on resolving the merge conflict. @whitewindmills @RainbowMango

@whitewindmills
Copy link
Member

/unhold

@karmada-bot karmada-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2024
@RainbowMango
Copy link
Member

now, i have do something for this PR. i need you guys on resolving the merge conflict. @whitewindmills @RainbowMango

I can't figure out the intention of this PR.
And, do you mean you don't know how to resolve the conflicts?
Basically you can do that as follows:

1. checkout to your feature branch by `git checkout Implement-group-score-calc-bonus`
2. reset the latest commit on you feature branch by `git reset HEAD~1`
3. stash your commit to cache: `git stash`
4. fetch the upstream and rebase
  * `git fetch upstream`
  * `git rebase upstream/master`
5. restore your commit: `git stash pop` 
  * Note: you can skip this step too if you prefer to write the code from the start. But if you do, you will see the conflicts by checking the status `git status`
 ...

@ipsum-0320 ipsum-0320 force-pushed the Implement-group-score-calc-bonus branch from df53e4d to 7723a18 Compare October 28, 2024 09:54
@karmada-bot karmada-bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 28, 2024
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 28, 2024
@ipsum-0320
Copy link
Contributor Author

now, i have do something for this PR. i need you guys on resolving the merge conflict. @whitewindmills @RainbowMango

I can't figure out the intention of this PR. And, do you mean you don't know how to resolve the conflicts? Basically you can do that as follows:

1. checkout to your feature branch by `git checkout Implement-group-score-calc-bonus`
2. reset the latest commit on you feature branch by `git reset HEAD~1`
3. stash your commit to cache: `git stash`
4. fetch the upstream and rebase
  * `git fetch upstream`
  * `git rebase upstream/master`
5. restore your commit: `git stash pop` 
  * Note: you can skip this step too if you prefer to write the code from the start. But if you do, you will see the conflicts by checking the status `git status`
 ...

I've resolved the conflicts, so we can consider merging now. Additionally, I'll clarify the purpose of this PR:

As shown in the code, PR #5621 actually represents the GroupScore algorithm when the replica allocation strategy is set to Divided. In contrast, this PR (#5622) provides the GroupScore algorithm for the Duplicate replica allocation strategy. It stands to reason that with Duplicate, the GroupScore algorithm should differ due to the distinct behavior of replica allocation. The GroupScore algorithm I designed works as follows: in a Group, the more clusters that meet the Replica requirement for available copies, the higher the Group Score. If the number of clusters meeting the Replica requirement is the same, then the Group with a higher average cluster score will have a higher Group Score. @whitewindmills @RainbowMango

func (info *GroupClustersInfo) calcGroupScore(
clusters []ClusterDetailInfo,
rbSpec *workv1alpha2.ResourceBindingSpec,
minGroups int) int64 {
if checkIfDuplicate(rbSpec) {
Copy link
Member

Choose a reason for hiding this comment

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

we don't need such a redundant function checkIfDuplicate, use rbSpec.Placement.ReplicaSchedulingType() == policyv1alpha1.ReplicaSchedulingTypeDuplicated instead.


// Here is an example, the rbSpec.Replicas == 50.

// There is the Group 1, it has three clusters as follows.
Copy link
Member

Choose a reason for hiding this comment

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

five clusters ...


// There is another example, the rbSpec.Replicas == 50.

// There is the Group 1, it has three clusters as follows.
Copy link
Member

Choose a reason for hiding this comment

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

ditto

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.

some nits, LGTM otherwise.

@ipsum-0320 ipsum-0320 force-pushed the Implement-group-score-calc-bonus branch 2 times, most recently from a7099b3 to 9ab1a33 Compare October 28, 2024 11:01
@ipsum-0320
Copy link
Contributor Author

I've solved these problems. @whitewindmills

@whitewindmills
Copy link
Member

/approve
/assign @RainbowMango

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2024
Signed-off-by: ipsum-0320 <trueman.0320@zju.edu.cn>

feat: implement calc group score for duplicate to assign replicas evenly

Signed-off-by: ipsum-0320 <trueman.0320@zju.edu.cn>

feat: calc group score for duplicate to assign replicas evenly

Signed-off-by: ipsum-0320 <trueman.0320@zju.edu.cn>
@ipsum-0320 ipsum-0320 force-pushed the Implement-group-score-calc-bonus branch from 9ab1a33 to 9e349e6 Compare October 28, 2024 11:21
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.

/approve

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango, 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

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

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2024
@karmada-bot karmada-bot merged commit 5432d32 into karmada-io:master Oct 29, 2024
15 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/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.

5 participants