[feature]support rescheduling when deleting a cluster #1383

huone1 · 2022-02-20T16:29:34Z

Signed-off-by: huone1 huwanxing@huawei.com

What type of PR is this?

/kind feature

What this PR does / why we need it:
support rescheduling when deleting a cluster
Which issue(s) this PR fixes:
Fixes #829
Fixes #1411

Special notes for your reviewer:
This PR does not consider the spreadconstraints and it will be considered in the scheduler refactoring .
Does this PR introduce a user-facing change?:

`karmada-scheduler`: Workload is now able to reschedule after the cluster is unregistered.

huone1 · 2022-02-20T16:29:57Z

/cc @dddddai

huone1 · 2022-02-21T02:21:38Z

/cc @mrlihanbo

mrlihanbo · 2022-02-22T11:37:32Z

notes: This PR does not consider the spreadconstraints.
/lgtm

RainbowMango · 2022-03-01T01:54:20Z

Please solve the conflict. @huone1

huone1 · 2022-03-01T01:56:09Z

issue #1411 is also fixed in the PR

merryzhou · 2022-03-01T04:06:56Z

pkg/scheduler/core/division_algorithm.go

@@ -48,7 +51,7 @@ func divideReplicasByResource(
 	} else if assignedReplicas < spec.Replicas {
 		// We need to enlarge the replicas in terms of the previous result (if exists).
 		// First scheduling is considered as a special kind of scaling up.
-		newTargetClusters, err := scaleUpScheduleByReplicaDivisionPreference(clusters, spec, preference)
+		newTargetClusters, err := scaleUpScheduleByReplicaDivisionPreference(clusters, spec, preference, scheduledClusters, assignedReplicas)
 		if err != nil {
 			return nil, fmt.Errorf("failed to scaleUp: %v", err)
 		}


what if assignedReplicas == spec.Replicas, but len(scheduledClusters) < len(spec.clusters)

what if assignedReplicas == spec.Replicas, but len(scheduledClusters) < len(spec.clusters)

I didn't think of a similar rescheduling scenario

Could you please give an example? It seems not possible to happen "in one event"

Let's say we have 2 clusters [A、B] , both with allocatable resources: 4C8G
a deployment requests: 2C4G, 3 replicas
and the propagationPolicy is:

placement: clusterAffinity: clusterNames: - A - B replicaScheduling: replicaDivisionPreference: Weighted replicaSchedulingType: Divided weightPreference: dynamicWeight: AvailableReplic

so the deployment was placed in both A and B, let's assume A has 2 replicas and B has 1。

update propagationPolicy placement.clusterAffinity.clusterNames=[A], now no enough resources for this deployment to reschedule, so nothing happened;

update deployment.spec.replicas=2, with this pr, nothing happened again..

you are right that it should return the scheduledClusters as the result

Signed-off-by: huone1 <huwanxing@huawei.com>

huone1 · 2022-03-03T02:34:33Z

Please solve the conflict. @huone1

ok, it is done

RainbowMango · 2022-03-03T07:41:28Z

cc @dddddai @Garrybest
Can you take a look?

RainbowMango

/approve

Leave LGTM to @Garrybest or @dddddai

karmada-bot · 2022-03-03T08:21:16Z

[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

dddddai

Just one concern, would leave lgtm to @Garrybest

dddddai · 2022-03-03T09:00:23Z

pkg/scheduler/core/generic_scheduler.go

-	deltaLen := len(spec.Clusters) - len(reservedClusters)
-	if len(candidateClusters) < deltaLen {
-		// for ReplicaSchedulingTypeDivided, we will try to migrate replicas to the other health clusters
-		if placement.ReplicaScheduling == nil || placement.ReplicaScheduling.ReplicaSchedulingType == policyv1alpha1.ReplicaSchedulingTypeDuplicated {
-			klog.Warningf("ignore reschedule binding as insufficient available cluster")
-			return ScheduleResult{}, nil


Let's say we have 2 clusters [A、B] (with Failover enabled)

a rb was scheduled to [A、B] (duplicated)

cluster A becomes not ready

the orignal behavior: scheduled to [A、B] while the current behavior: scheduled to [B]

I'm not sure if we want to keep the original behavior, if not then lgtm

@mrlihanbo Please see if this change is reasonable.

Garrybest · 2022-03-03T11:55:06Z

/assign

Garrybest · 2022-03-03T12:13:03Z

pkg/scheduler/core/division_algorithm.go

-	// Step 1: Get previous total sum of replicas.
-	assignedReplicas := util.GetSumOfReplicas(spec.Clusters)
+	// Step 1: Find the ready clusters that have old replicas
+	scheduledClusters := findOutScheduledCluster(spec.Clusters, clusters)


What if we just delete the unjoined clusters from spec.clusters?

Then the replicas in unjoined clusters will be considered as a special kind of scaling up.

Seems we don't need more changes, am I right?

it is a special kind of scaling up

Garrybest · 2022-03-04T07:12:06Z

/lgtm

Garrybest · 2022-04-21T12:15:53Z

I think there is a bug here. Imagine:

Failover is disabled.
A cluster is not ready.

We should not erase the scheduling replicas of the not-ready cluster in RB because Failover is disabled. However, if it happens to scale up, e.g. the user scale up desired replicas. This PR will delete all replicas in this cluster which is dangerous. PTAL, @huone1

huone1 · 2022-04-22T01:34:40Z

when Failover is disabled, the rescheduling for not-ready cluster is not triggered;
if A cluster is not ready and unhealthy, i think it is normal to delete all replicas in this cluster

Garrybest · 2022-04-22T01:53:55Z

I'm afraid not. When scaling up, rescheduling will be triggered.

huone1 · 2022-04-22T01:55:31Z

I'm afraid not. When scaling up, rescheduling will be triggered.

what is wrong with it

RainbowMango · 2022-04-22T01:56:58Z

However, if it happens to scale up, e.g. the user scale up desired replicas. This PR will delete all replicas in this cluster which is dangerous.

Why is it dangerous?

What I'm thinking about is how to postpone the deletion operation until the desired replicas are all in the available state. That guarantees there are always sufficient replicas running at any time.

Garrybest · 2022-04-22T01:57:25Z

Failover is disabled, but under this circumstance all replicas will be removed. It does not match the expectation.

Garrybest · 2022-04-22T02:01:01Z

Failover is disabled because the user does not want to remove all replicas when a cluster is not ready. If the api-server of a member cluster is temporarily down and Failover is disabled, we potentially do not want the replicas in this member cluster to be evicted when the user triggers scaling up. However, now it does not match the expectation.

RainbowMango · 2022-04-22T02:20:20Z

Agree with @Garrybest.
We should consider the false positive cluster failure seriously. Can we hold the replicas(for the un-healthy cluster) unchanged even in the case of scaling up and decreasing the replicas in the case of scaling down.?

huone1 · 2022-04-22T02:24:15Z

it is different between deleting a cluster and the change from healthy to unhealthy。it is reasonable to migrate the replicas to Other Clusters when deleting a cluster

RainbowMango · 2022-04-22T02:27:49Z

Yeah, deleting a cluster is another story.

Garrybest · 2022-04-22T02:40:42Z

Let me describe an example.

We have a deployment with a 5 desired replicas which are propagated to member cluster A.
The cluster is a little bit busy so the connection between karmada-agent and kube-apiserber of member A is lost for about 1 minute. So the cluster status is unhealthy.
It happens that the deployment has been scaled up to 10 replicas at this time.
Even if we disable Failover, the scaling up triggers the scheduling procedure, now the karmada-scheduler will remove replicas in cluster A which may be dangerous. However, it is possible that there is nothing wrong with the 5 replicas in member cluster A.

RainbowMango · 2022-04-22T02:51:24Z

Thanks @Garrybest for the details. Can you help file an issue to track this?

Garrybest · 2022-04-22T02:54:53Z

Sure.

karmada-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 20, 2022

karmada-bot requested review from Garrybest and XiShanYongYe-Chang February 20, 2022 16:29

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 20, 2022

karmada-bot requested a review from dddddai February 20, 2022 16:29

karmada-bot requested a review from mrlihanbo February 21, 2022 02:21

karmada-bot assigned mrlihanbo Feb 22, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2022

huone1 mentioned this pull request Mar 1, 2022

cannot reschedule when propagationpolicy changed #1411

Closed

RainbowMango linked an issue Mar 1, 2022 that may be closed by this pull request

cannot reschedule when propagationpolicy changed #1411

Closed

merryzhou reviewed Mar 1, 2022

View reviewed changes

[feature]support rescheduling when deleting a cluster

f7e6ecd

Signed-off-by: huone1 <huwanxing@huawei.com>

huone1 force-pushed the DeleteCluster branch from 1693fdd to f7e6ecd Compare March 3, 2022 01:50

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2022

RainbowMango approved these changes Mar 3, 2022

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2022

dddddai reviewed Mar 3, 2022

View reviewed changes

karmada-bot assigned Garrybest Mar 3, 2022

Garrybest reviewed Mar 3, 2022

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2022

karmada-bot merged commit b33bda9 into karmada-io:master Mar 4, 2022

huone1 deleted the DeleteCluster branch March 7, 2022 02:31

huone1 mentioned this pull request Mar 8, 2022

fix rb spec.Clusters is incorrent in some scenarios #1448

Merged

huone1 mentioned this pull request Mar 26, 2022

Request to be a Member of Karmada community #1551

Closed

[feature]support rescheduling when deleting a cluster #1383

[feature]support rescheduling when deleting a cluster #1383

Conversation

huone1 commented Feb 20, 2022 • edited by RainbowMango

huone1 commented Feb 20, 2022

huone1 commented Feb 21, 2022

mrlihanbo commented Feb 22, 2022

RainbowMango commented Mar 1, 2022

huone1 commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huone1 commented Mar 3, 2022

RainbowMango commented Mar 3, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Mar 3, 2022

dddddai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garrybest commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garrybest commented Mar 4, 2022

Garrybest commented Apr 21, 2022

huone1 commented Apr 22, 2022 • edited

Garrybest commented Apr 22, 2022

huone1 commented Apr 22, 2022

RainbowMango commented Apr 22, 2022

Garrybest commented Apr 22, 2022

Garrybest commented Apr 22, 2022

RainbowMango commented Apr 22, 2022 • edited

huone1 commented Apr 22, 2022

RainbowMango commented Apr 22, 2022

Garrybest commented Apr 22, 2022 • edited

RainbowMango commented Apr 22, 2022

Garrybest commented Apr 22, 2022

huone1 commented Feb 20, 2022 •

edited by RainbowMango

huone1 commented Apr 22, 2022 •

edited

RainbowMango commented Apr 22, 2022 •

edited

Garrybest commented Apr 22, 2022 •

edited