Fixed cluster affinity scheduling evaluation order when scheduling is triggered via WorkloadRebalancer

[bharathguvvala]

What type of PR is this?

/kind bug

What this PR does / why we need it:
As discussed in the community meet on 2024-07-09 , added the fix to evaluate the cluster affinities freshly when scheduling is triggered via WorkloadRebalancer.

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

Progress

• share opinion in Community Meeting
• proposal
• implementation
• UT test
• e2e test

Special notes for your reviewer:
Raising this PR to get early feedback on the implementation as it is still work in progress and tested manually by building the deploying the scheduler. There may be unit test failures which I will fix so please don't merge the PR until that's done.

Does this PR introduce a user-facing change?:
Yes. [Rest of the section TBA]

[karmada-bot]

Welcome @bharathguvvala! It looks like this is your first PR to karmada-io/karmada 🎉

[chaosi-zju]

hi @bharathguvvala, really glad to see your PR submission!

By the way, here may be some minor problems with the ci now, and can you please fix the ci and squash the commit records into one? thanks~

[bharathguvvala]

@chaosi-zju Sure, will do that.

[chaosi-zju]

hi @bharathguvvala, your this line may be submitted by mistake, ctx is required~

Suggested change

	func (c *EndpointsliceDispatchController) dispatchEndpointSlice(_ context.Context, work *workv1alpha1.Work, mcs *networkingv1alpha1.MultiClusterService) error {
	func (c *EndpointsliceDispatchController) dispatchEndpointSlice(ctx context.Context, work *workv1alpha1.Work, mcs *networkingv1alpha1.MultiClusterService) error {

[bharathguvvala]

yea this seems like a mistake. will correct it.

[chaosi-zju]

hi, adding a parameter performFreshScheduling is indeed works, but may be not the best practice~

It just occurred to me that we have similar logic in:

karmada/pkg/scheduler/core/assignment.go

Lines 109 to 115 in 673a603

	// the assignment mode is defaults to Steady to minimizes disruptions and preserves the balance across clusters.
	expectAssignmentMode := Steady
	// when spec.rescheduleTriggeredAt is updated, it represents a rescheduling is manually triggered by user, and the
	// expected behavior of this action is to do a complete recalculation without referring to last scheduling results.
	if util.RescheduleRequired(spec.RescheduleTriggeredAt, status.LastScheduledTime) {
	expectAssignmentMode = Fresh
	}

may be we can also use util.RescheduleRequired(spec.RescheduleTriggeredAt, status.LastScheduledTime) to judge whether need to reset the affinityIndex, since now a fresh reschedule is defaults to reset the index of ClusterAffinities, so we can do just like:

if util.RescheduleRequired(crb.Spec.RescheduleTriggeredAt, crb.Status.LastScheduledTime) {
  affinityIndex = 0
}

[bharathguvvala]

I did this to keep the status quo functionality. I will change it based on your suggestion.

[chaosi-zju]

thanks~
By the way, I'd appreciate it if you could rebase the latest master code and squash the commit log into a single record~

[bharathguvvala]

Done incorporating these changes. Currently there's only one scheduler worker which I think will continue to stay that way. If there are more then that could cause a race condition where the condition util.RescheduleRequired could evaluate to false while deciding scheduled cluster(s) after first evaluating to true .

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

Project coverage is 48.17%. Comparing base (cf7ac41) to head (762f992).

Files with missing lines	Patch %	Lines
pkg/scheduler/scheduler.go	0.00%	4 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5425      +/-   ##
==========================================
- Coverage   48.18%   48.17%   -0.02%     
==========================================
  Files         664      664              
  Lines       54799    54805       +6     
==========================================
- Hits        26405    26402       -3     
- Misses      26680    26686       +6     
- Partials     1714     1717       +3     

Flag	Coverage Δ
unittests	48.17% <0.00%> (-0.02%)	⬇️

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.

[chaosi-zju]

/retest

[chaosi-zju]

Hi, there are still two nits:

1. The CI of this PR failed due to it wasn't signed off, usually please use git commit -s -m 'your message ' or git commit -m ' Signed-off-by: AuthorName <[email protected]> \n <other message> ' to pass DCO (detail guideline can refer to: https://probot.github.io/apps/dco/).

2. I see this PR has 26 commit record, can you please squash these 26 commits into one commit, you can refer to: https://stackoverflow.com/questions/5189560/how-do-i-squash-my-last-n-commits-together

thanks~

[karmada-bot]

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

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.

[bharathguvvala]

@chaosi-zju I've tried to squash the commits and sort of messed it up. Those squashed commits aren't showing up in the branch. Can we do "squash and merge" while merging this PR?

[chaosi-zju]

I've tried to squash the commits and sort of messed it up. Those squashed commits aren't showing up in the branch. Can we do "squash and merge" while merging this PR?

can you try one more time by following command:

git remote add upstream https://github.com/karmada-io/karmada.git
git fetch upstream
git rebase upstream/master

then if you successfully rebased, you can directly push tou commit,

but you may also encounter some rebase conflict, in that case you should manually resolve the conflict and execute git rebase --continue

[karmada-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign garrybest for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[bharathguvvala]

@chaosi-zju can you check now?