Refactor schedule type

[dddddai]

Signed-off-by: dddddai [email protected]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
As discussed with @RainbowMango at last meeting, it's not a good idea to branch by schedule types because it's tricky for us to extend scheduler behavior

Get ready for #829

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[RainbowMango]

Many thanks to @dddddai , I'll look into it ASAP.
After a quick look, I see you removed AvoidSchedule and Unknown in this PR, is that right?

[dddddai]

After a quick look, I see you removed AvoidSchedule and Unknown in this PR, is that right?

These types were used to choose branches, I removed them since they are no longer needed
Other schedule types still remain for metrics, for exmaple:

karmada/pkg/scheduler/scheduler.go

Line 406 in de55101

metrics.BindingSchedule(string(FirstSchedule), metrics.SinceInSeconds(start), err)

[RainbowMango]

cc @Garrybest @mrlihanbo

[Garrybest]

I'll check it later.

[Garrybest]

/assign

[mrlihanbo]

hi, the Scheduled condition is just to indicate that the ResourceBinding or ClusterResourceBinding has been scheduled once, not to indicate that the status of every scheduling process, right?

[dddddai]

Yes, please refer to #821 (comment), and I'm not sure if #988 meets what @RainbowMango said

[mrlihanbo]

It is out of my expectation, I used to think that this condition is to indicate the status of every scheduling process. If first schedule, rescedule or scale schedule failed, it should be set to False. For now, we will keep the schedule result of last success scheduling if reschedule failed. But for users, they don't know why schedule failed.

[mrlihanbo]

/cc @RainbowMango Is it the expected behavior of this condition?

[RainbowMango]

We do need a condition to reflect the schedule failed state. I thought it might be too complicated, so we didn't handle the failed case in #821.

[mrlihanbo]

uh-huh, it seems like the "First" schedule is a kind of special "Reschedule"

[dddddai]

Oh there is a problem: for example, if FailoverSchedule failed, then Scheduled condition would be false, then scheduler would regard it as FirstSchedule whose behavior is different from FailoverSchedule

[mrlihanbo]

How about to do it like:

func (s *Scheduler) doScheduleBinding(namespace, name string) error {
	rb, err := s.bindingLister.ResourceBindings(namespace).Get(name)
	if err != nil {
		if apierrors.IsNotFound(err) {
			// the binding does not exist, do nothing
			return nil
		}
		return err
	}

	start := time.Now()

	defer func() {
		if err != nil {
			// set "Scheduled" condition false
		} else {
			// set "Scheduled" condition true
		}
	}()

	policyPlacement, policyPlacementStr, err := s.getPlacement(rb)
	if err != nil {
		return err
	}
	appliedPlacement := util.GetLabelValue(rb.Annotations, util.PolicyPlacementAnnotation)
	if policyPlacementStr != appliedPlacement {
		// policy placement changed, need reschedule
		klog.Infof("Reschedule ResourceBinding(%s/%s) as placement changed", namespace, name)
		err = s.scheduleResourceBinding(rb)
		metrics.BindingSchedule(string(ReconcileSchedule), metrics.SinceInSeconds(start), err)
		return err
	}

Scheduled condition is to indicate the schedule result of every schedule process. If schedule failed, we won't update the applied-placement annation, but will set the Scheduled condition as false. The scheduler behavior will be consistent. The schedule will scheduleResourceBinding once applied policy is not the same as current policy. Otherwise, perform failover schedule or scale schedule.

[dddddai]

Yeah, it sounds reasonable, i think we can get this one merged then do this job in #988

[mrlihanbo]

Yeah, I agree

[mrlihanbo]

/lgtm
/cc @Garrybest

[Garrybest]

Generally LGTM.

[Garrybest]

Thanks!
/lgtm

[RainbowMango]

/approve

We can get benefits from this path, like:

• Complete schedule condition
• get ready for Reschedule bindings on cluster change #829

Thanks @dddddai.

[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