Allow evicting leader scheduling when lightning importing

[rleungx]

Enhancement Task

Previously, lightning used the global pause policy to avoid scheduling when importing data, which allowed evicting the leader in the process. After #4649, we support pausing scheduling a specified key range through a label rule. But it will pause all scheduling including the evict leader.
Recently, we encountered an issue: pingcap/tidb#51143. The import process will block the pod from being rescheduled to another node. In this case, pausing evicting the leader is not suitable. At least, we need to make it compatible with the previous behavior.

[okJiang]

Hi, after I understand the context, I think that if it was just to solve this problem, it would be enough to filter out EvictLeaderScheduler when checking ScheduleDisable. Just here

pd/pkg/schedule/schedulers/scheduler_controller.go

Lines 474 to 486 in 4820bc5

	for _, op := range ops {
	if labelMgr := s.cluster.GetRegionLabeler(); labelMgr != nil {
	region := s.cluster.GetRegion(op.RegionID())
	if region == nil {
	continue
	}
	if labelMgr.ScheduleDisabled(region) {
	denySchedulersByLabelerCounter.Inc()
	foundDisabled = true
	break
	}
	}
	}

But that would make the interface proprietary and weaken its generalization.

So would you consider providing more keys like schedule.evict-leader into the Rule? If the client specifies deny schedule, it suspends all schedulers, and if the client specifies deny schedule and allow schedule.evict-leader, it disables all schedules except the evict leader. In this way, we can more easily extend other behaviors. This latter approach requires more detailed spec and design documentation, more development and discussion work.

[JmPotato]

Hi, after I understand the context, I think that if it was just to solve this problem, it would be enough to filter out EvictLeaderScheduler when checking ScheduleDisable. Just here

pd/pkg/schedule/schedulers/scheduler_controller.go

Lines 474 to 486 in 4820bc5

	for _, op := range ops {
	if labelMgr := s.cluster.GetRegionLabeler(); labelMgr != nil {
	region := s.cluster.GetRegion(op.RegionID())
	if region == nil {
	continue
	}
	if labelMgr.ScheduleDisabled(region) {
	denySchedulersByLabelerCounter.Inc()
	foundDisabled = true
	break
	}
	}
	}

But that would make the interface proprietary and weaken its generalization.
So would you consider providing more keys like schedule.evict-leader into the Rule? If the client specifies deny schedule, it suspends all schedulers, and if the client specifies deny schedule and allow schedule.evict-leader, it disables all schedules except the evict leader. In this way, we can more easily extend other behaviors. This latter approach requires more detailed spec and design documentation, more development and discussion work.

We could do both step by step. First providing the same behavior as before to skip the evict leader scheduler, then extend the ability to stop any other scheduler.

[okJiang]

Do you have any suggestions? @rleungx

[rleungx]

I think we need to clarify the semantics of deny schedule first. #4649 creates this label for denying all scheduling including the evict leader scheduler. But apparently it doesn't meet the needs of the caller. If we are going to support such as schedule.evict-leader or the other labels, we should define a priority first to avoid conflicts among each label.

[okJiang]

@rleungx You seem to prefer the second option. I completely agree with your thoughts. Let me briefly describe the prioritization issue. I try to use dot to express hierarchical relationship. A.B means that the rank of A is greater than that of B, i.e., A contains B. In this case, we prioritize A before B. For example, if a user sets deny A and allow A.B, then allow B on top of deny A, instead of denying A directly to override allow B. The case of allow A and deny A.B is similar.

The details can't be described in a few sentences, so I'd like to know how the team would like to handle this issue, and I'll write a more detailed proposal if team are inclined to implement the second one.

[okJiang]

Which solution would you like? @lance6716

[lance6716]

and if the client specifies deny schedule and allow schedule.evict-leader

I think this one is better. Client can configure what rules it wants to be excluded from "deny all".

[okJiang]

After discuss with @rleungx @easonn7 @lance6716 , we ultimately opted for the first method.

This is because after searching for all possible block situations related to the scheduler, I found that the only situation where TiKV can be taken offline using ti-operator when lightning sets the rule label.

So, the first solution completely meets our needs.