fix spreadconstraints[i].MaxGroups Invalidation when scaleup replicas

[huone1]

Signed-off-by: huone1 [email protected]

What type of PR is this?
/kind bug
What this PR does / why we need it:
because the spreadconstraints[i].maxgroup define the max cluster number, So the target cluster number could not be greater than it in any scenario.

The difference between the current cluster and the scheduled cluster is not considered when calculating the scheduled replicas
Which issue(s) this PR fixes:
Fixes #1323

let's test it According to the issue #1323 operation

before scaleup

spec:
    clusters:
    - name: member1
      replicas: 55
    - name: member2
      replicas: 55
    replicas: 110

after scaleup

spec:
    clusters:
    - name: member2
      replicas: 95
    - name: member3
      replicas: 85
    replicas: 180

Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?:

fix spreadconstraints[i].MaxGroups is invalidated in some scenarios

[huone1]

/cc @Garrybest

[Garrybest]

Thanks @huone1, I will check it later.

[Garrybest]

/assign

[Garrybest]

This PR generally LGTM. But I'm a little curious about how does the spreadConstraint choose the available clusters?

Taken #1323 as an example. We divided 56 replicas into member1 and 54 replicas member3. When scaling up additional 70 replicas, it seems like we can't accommodate more available replicas in member1. If resource is enough, we should scheduled all additional replicas in to member1 but the final replica is 56.

How does the spreadConstraint choose the right clusters? Why not still choose member1 and member3 again?

/cc @mrlihanbo

[huone1]

This PR generally LGTM. But I'm a little curious about how does the spreadConstraint choose the available clusters?

Taken #1323 as an example. We divided 56 replicas into member1 and 54 replicas member3. When scaling up additional 70 replicas, it seems like we can't accommodate more available replicas in member1. If resource is enough, we should scheduled all additional replicas in to member1 but the final replica is 56.

How does the spreadConstraint choose the right clusters? Why not still choose member1 and member3 again?

/cc @mrlihanbo

the PR #1334 add a new score plugin clusterLocality to favors cluster that already have requested and supply a preferred policy to choose the available clusters

[Garrybest]

Got it, nice work👍
/lgtm

[RainbowMango]

The res could be calculated by validTarget, I suggest remove it.

diff --git a/pkg/scheduler/core/division_algorithm.go b/pkg/scheduler/core/division_algorithm.go
index e0b2c082..689b8231 100644
--- a/pkg/scheduler/core/division_algorithm.go
+++ b/pkg/scheduler/core/division_algorithm.go
@@ -210,7 +210,7 @@ func scaleUpScheduleByReplicaDivisionPreference(
        preference policyv1alpha1.ReplicaDivisionPreference,
 ) ([]workv1alpha2.TargetCluster, error) {
        // Step 1: Find the clusters that have old replicas, so we can prefer to assign new replicas towards them.
-       scheduledClusterNames, scheduledClusters := findOutScheduledCluster(spec.Clusters, clusters)
+       scheduledClusters := findOutScheduledCluster(spec.Clusters, clusters)

        // Step 2: calculate the assigned Replicas in scheduledClusters
        assignedReplicas := util.GetSumOfReplicas(scheduledClusters)
@@ -229,7 +229,7 @@ func scaleUpScheduleByReplicaDivisionPreference(
        // If not, the old replicas may be recreated which is not expected during scaling up.
        // The parameter `scheduledClusterNames` is used to make sure that we assign new replicas to them preferentially
        // so that all the replicas are aggregated.
-       result, err := divideReplicasByPreference(clusterAvailableReplicas, newSpec.Replicas, preference, scheduledClusterNames)
+       result, err := divideReplicasByPreference(clusterAvailableReplicas, newSpec.Replicas, preference, util.ConvertToClusterNames(scheduledClusters))
        if err != nil {
                return result, err
        }
diff --git a/pkg/scheduler/core/util.go b/pkg/scheduler/core/util.go
index 1ceab827..93e2b8e4 100644
--- a/pkg/scheduler/core/util.go
+++ b/pkg/scheduler/core/util.go
@@ -77,11 +77,10 @@ func calAvailableReplicas(clusters []*clusterv1alpha1.Cluster, spec *workv1alpha

 // findOutScheduledCluster will return a name set of clusters
 // which are a part of `feasibleClusters` and have non-zero replicas.
-func findOutScheduledCluster(tcs []workv1alpha2.TargetCluster, candidates []*clusterv1alpha1.Cluster) (sets.String, []workv1alpha2.TargetCluster) {
+func findOutScheduledCluster(tcs []workv1alpha2.TargetCluster, candidates []*clusterv1alpha1.Cluster) []workv1alpha2.TargetCluster {
        validTarget := make([]workv1alpha2.TargetCluster, 0)
-       res := sets.NewString()
        if len(tcs) == 0 {
-               return res, validTarget
+               return validTarget
        }

        for _, targetCluster := range tcs {
@@ -92,14 +91,13 @@ func findOutScheduledCluster(tcs []workv1alpha2.TargetCluster, candidates []*clu
                // must in `candidates`
                for _, cluster := range candidates {
                        if targetCluster.Name == cluster.Name {
-                               res.Insert(targetCluster.Name)
                                validTarget = append(validTarget, targetCluster)
                                break
                        }
                }
        }

-       return res, validTarget
+       return validTarget
 }

 // resortClusterList is used to make sure scheduledClusterNames are in front of the other clusters in the list of

[huone1]

it is better, and I have fixed it

[RainbowMango]

The comments should be updated as well. Sorry for missing it from my last comment.

[RainbowMango]

/lgtm
/approve

[karmada-bot]

[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:

• OWNERS [RainbowMango]

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

[RainbowMango]

/lgtm