Merge pull request #3741 from whitewindmills/schedule-condition

feat: optimize scheduling condition semantics

pkg/apis/work/v1alpha2/binding_types.go

@@ -307,6 +307,24 @@ const (
 	FullyApplied string = "FullyApplied"
 )
 
+// These are reasons for a binding's transition to a Scheduled condition.
+const (
+	// BindingReasonSuccess reason in Scheduled condition means that binding has been scheduled successfully.
+	BindingReasonSuccess = "Success"
+
+	// BindingReasonSchedulerError reason in Scheduled condition means that some internal error happens
+	// during scheduling, for example due to api-server connection error.
+	BindingReasonSchedulerError = "SchedulerError"
+
+	// BindingReasonNoClusterFit reason in Scheduled condition means that scheduling has finished
+	// due to no fit cluster.
+	BindingReasonNoClusterFit = "NoClusterFit"
+
+	// BindingReasonUnschedulable reason in Scheduled condition means that the scheduler can't schedule
+	// the binding right now, for example due to insufficient resources in the clusters.
+	BindingReasonUnschedulable = "Unschedulable"
+)
+
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 
 // ResourceBindingList contains a list of ResourceBinding.

pkg/scheduler/core/division_algorithm.go

@@ -7,6 +7,7 @@ import (
 	clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
 	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
 	workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2"
+	"github.com/karmada-io/karmada/pkg/scheduler/framework"
 	"github.com/karmada-io/karmada/pkg/util"
 	"github.com/karmada-io/karmada/pkg/util/helper"
 )
@@ -48,7 +49,7 @@ func getStaticWeightInfoList(clusters []*clusterv1alpha1.Cluster, weightList []p
 // dynamicDivideReplicas assigns a total number of replicas to the selected clusters by preference according to the resource.
 func dynamicDivideReplicas(state *assignState) ([]workv1alpha2.TargetCluster, error) {
 	if state.availableReplicas < state.targetReplicas {
-		return nil, fmt.Errorf("clusters resources are not enough to schedule, max %d replicas are support", state.availableReplicas)
+		return nil, &framework.UnschedulableError{Message: fmt.Sprintf("Clusters available replicas %d are not enough to schedule.", state.availableReplicas)}
 	}
 
 	switch state.strategyType {

pkg/scheduler/core/generic_scheduler.go

@@ -59,13 +59,9 @@ func (g *genericScheduler) Schedule(
 	scheduleAlgorithmOption *ScheduleAlgorithmOption,
 ) (result ScheduleResult, err error) {
 	clusterInfoSnapshot := g.schedulerCache.Snapshot()
-	if clusterInfoSnapshot.NumOfClusters() == 0 {
-		return result, fmt.Errorf("no clusters available to schedule")
-	}
-
 	feasibleClusters, diagnosis, err := g.findClustersThatFit(ctx, spec, status, &clusterInfoSnapshot)
 	if err != nil {
-		return result, fmt.Errorf("failed to findClustersThatFit: %v", err)
+		return result, fmt.Errorf("failed to find fit clusters: %w", err)
 	}
 
 	// Short path for case no cluster fit.
@@ -79,19 +75,19 @@ func (g *genericScheduler) Schedule(
 
 	clustersScore, err := g.prioritizeClusters(ctx, g.scheduleFramework, spec, feasibleClusters)
 	if err != nil {
-		return result, fmt.Errorf("failed to prioritizeClusters: %v", err)
+		return result, fmt.Errorf("failed to prioritize clusters: %w", err)
 	}
 	klog.V(4).Infof("Feasible clusters scores: %v", clustersScore)
 
 	clusters, err := g.selectClusters(clustersScore, spec.Placement, spec)
 	if err != nil {
-		return result, fmt.Errorf("failed to select clusters: %v", err)
+		return result, fmt.Errorf("failed to select clusters: %w", err)
 	}
 	klog.V(4).Infof("Selected clusters: %v", clusters)
 
 	clustersWithReplicas, err := g.assignReplicas(clusters, spec.Placement, spec)
 	if err != nil {
-		return result, fmt.Errorf("failed to assignReplicas: %v", err)
+		return result, fmt.Errorf("failed to assign replicas: %w", err)
 	}
 	if scheduleAlgorithmOption.EnableEmptyWorkloadPropagation {
 		clustersWithReplicas = attachZeroReplicasCluster(clusters, clustersWithReplicas)

pkg/scheduler/framework/plugins/apienablement/api_enablement.go

@@ -40,7 +40,7 @@ func (p *APIEnablement) Filter(
 ) *framework.Result {
 	if !helper.IsAPIEnabled(cluster.Status.APIEnablements, bindingSpec.Resource.APIVersion, bindingSpec.Resource.Kind) {
 		klog.V(2).Infof("Cluster(%s) not fit as missing API(%s, kind=%s)", cluster.Name, bindingSpec.Resource.APIVersion, bindingSpec.Resource.Kind)
-		return framework.NewResult(framework.Unschedulable, "cluster(s) didn't have the API resource")
+		return framework.NewResult(framework.Unschedulable, "cluster(s) did not have the API resource")
 	}
 
 	return framework.NewResult(framework.Success)

pkg/scheduler/framework/plugins/clusteraffinity/cluster_affinity.go

@@ -54,7 +54,7 @@ func (p *ClusterAffinity) Filter(
 		if util.ClusterMatches(cluster, *affinity) {
 			return framework.NewResult(framework.Success)
 		}
-		return framework.NewResult(framework.Unschedulable, "cluster(s) didn't match the placement cluster affinity constraint")
+		return framework.NewResult(framework.Unschedulable, "cluster(s) did not match the placement cluster affinity constraint")
 	}
 
 	// If no clusters specified and it is not excluded, mark it matched

pkg/scheduler/framework/plugins/spreadconstraint/spread_constraint.go

@@ -38,11 +38,11 @@ func (p *SpreadConstraint) Filter(
 ) *framework.Result {
 	for _, spreadConstraint := range bindingSpec.Placement.SpreadConstraints {
 		if spreadConstraint.SpreadByField == policyv1alpha1.SpreadByFieldProvider && cluster.Spec.Provider == "" {
-			return framework.NewResult(framework.Unschedulable, "cluster(s) didn't have provider property")
+			return framework.NewResult(framework.Unschedulable, "cluster(s) did not have provider property")
 		} else if spreadConstraint.SpreadByField == policyv1alpha1.SpreadByFieldRegion && cluster.Spec.Region == "" {
-			return framework.NewResult(framework.Unschedulable, "cluster(s) didn't have region property")
+			return framework.NewResult(framework.Unschedulable, "cluster(s) did not have region property")
 		} else if spreadConstraint.SpreadByField == policyv1alpha1.SpreadByFieldZone && cluster.Spec.Zone == "" {
-			return framework.NewResult(framework.Unschedulable, "cluster(s) didn't have zone property")
+			return framework.NewResult(framework.Unschedulable, "cluster(s) did not have zone property")
 		}
 	}
 

pkg/scheduler/framework/types.go

@@ -9,8 +9,8 @@ import (
 )
 
 const (
-	// NoClusterAvailableMsg is used to format message when no clusters available.
-	NoClusterAvailableMsg = "0/%v clusters are available"
+	// noClusterAvailableMsg is used to format message when no clusters available.
+	noClusterAvailableMsg = "0/%d clusters are available: %s."
 )
 
 // ClusterToResultMap declares map from cluster name to its Result.
@@ -50,6 +50,10 @@ type FitError struct {
 
 // Error returns detailed information of why the object failed to fit on each cluster
 func (f *FitError) Error() string {
+	if f.NumAllClusters == 0 || len(f.Diagnosis.ClusterToResultMap) == 0 {
+		return fmt.Sprintf(noClusterAvailableMsg, f.NumAllClusters, "no cluster exists")
+	}
+
 	reasons := make(map[string]int)
 	for _, result := range f.Diagnosis.ClusterToResultMap {
 		for _, reason := range result.Reasons() {
@@ -65,6 +69,15 @@ func (f *FitError) Error() string {
 		sort.Strings(reasonStrings)
 		return reasonStrings
 	}
-	reasonMsg := fmt.Sprintf(NoClusterAvailableMsg+": %v.", f.NumAllClusters, strings.Join(sortReasonsHistogram(), ", "))
+	reasonMsg := fmt.Sprintf(noClusterAvailableMsg, f.NumAllClusters, strings.Join(sortReasonsHistogram(), ", "))
 	return reasonMsg
 }
+
+// UnschedulableError describes a unschedulable error, for example due to insufficient resources.
+type UnschedulableError struct {
+	Message string
+}
+
+func (u *UnschedulableError) Error() string {
+	return u.Message
+}

pkg/scheduler/helper.go

@@ -2,11 +2,17 @@ package scheduler
 
 import (
 	"encoding/json"
+	"errors"
 	"reflect"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 
 	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
+	workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2"
+	"github.com/karmada-io/karmada/pkg/scheduler/framework"
+	"github.com/karmada-io/karmada/pkg/util"
 )
 
 func placementChanged(
@@ -84,3 +90,33 @@ func getAffinityIndex(affinities []policyv1alpha1.ClusterAffinityTerm, observedN
 	}
 	return 0
 }
+
+// getConditionByError returns condition by error type, bool to indicate if ignore this error.
+func getConditionByError(err error) (metav1.Condition, bool) {
+	if err == nil {
+		return util.NewCondition(workv1alpha2.Scheduled, workv1alpha2.BindingReasonSuccess, successfulSchedulingMessage, metav1.ConditionTrue), true
+	}
+
+	var unschedulableErr *framework.UnschedulableError
+	if errors.As(err, &unschedulableErr) {
+		return util.NewCondition(workv1alpha2.Scheduled, workv1alpha2.BindingReasonUnschedulable, err.Error(), metav1.ConditionFalse), false
+	}
+
+	fitErrMatcher := func(e error) bool {
+		var fitErr *framework.FitError
+		return errors.As(e, &fitErr)
+	}
+	if fitErrMatcher(err) {
+		return util.NewCondition(workv1alpha2.Scheduled, workv1alpha2.BindingReasonNoClusterFit, err.Error(), metav1.ConditionFalse), true
+	}
+	var aggregatedErr utilerrors.Aggregate
+	if errors.As(err, &aggregatedErr) {
+		for _, ae := range aggregatedErr.Errors() {
+			if fitErrMatcher(ae) {
+				// if aggregated NoClusterFit error got, we do not ignore error but retry scheduling.
+				return util.NewCondition(workv1alpha2.Scheduled, workv1alpha2.BindingReasonNoClusterFit, err.Error(), metav1.ConditionFalse), false
+			}
+		}
+	}
+	return util.NewCondition(workv1alpha2.Scheduled, workv1alpha2.BindingReasonSchedulerError, err.Error(), metav1.ConditionFalse), false
+}

pkg/scheduler/helper_test.go

@@ -2,11 +2,16 @@ package scheduler
 
 import (
 	"encoding/json"
+	"errors"
 	"testing"
 
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
 	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
+	workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2"
+	"github.com/karmada-io/karmada/pkg/scheduler/framework"
 )
 
 func Test_needConsideredPlacementChanged(t *testing.T) {
@@ -428,3 +433,58 @@ func Test_getAffinityIndex(t *testing.T) {
 		})
 	}
 }
+
+func Test_getConditionByError(t *testing.T) {
+	tests := []struct {
+		name              string
+		err               error
+		expectedCondition metav1.Condition
+		ignoreErr         bool
+	}{
+		{
+			name:              "no error",
+			err:               nil,
+			expectedCondition: metav1.Condition{Type: workv1alpha2.Scheduled, Reason: workv1alpha2.BindingReasonSuccess, Status: metav1.ConditionTrue},
+			ignoreErr:         true,
+		},
+		{
+			name:              "failed to schedule",
+			err:               utilerrors.NewAggregate([]error{errors.New("")}),
+			expectedCondition: metav1.Condition{Type: workv1alpha2.Scheduled, Reason: workv1alpha2.BindingReasonSchedulerError, Status: metav1.ConditionFalse},
+			ignoreErr:         false,
+		},
+		{
+			name:              "no cluster fit",
+			err:               &framework.FitError{},
+			expectedCondition: metav1.Condition{Type: workv1alpha2.Scheduled, Reason: workv1alpha2.BindingReasonNoClusterFit, Status: metav1.ConditionFalse},
+			ignoreErr:         true,
+		},
+		{
+			name:              "aggregated fit error",
+			err:               utilerrors.NewAggregate([]error{&framework.FitError{}, errors.New("")}),
+			expectedCondition: metav1.Condition{Type: workv1alpha2.Scheduled, Reason: workv1alpha2.BindingReasonNoClusterFit, Status: metav1.ConditionFalse},
+			ignoreErr:         false,
+		},
+		{
+			name:              "unschedulable error",
+			err:               &framework.UnschedulableError{},
+			expectedCondition: metav1.Condition{Type: workv1alpha2.Scheduled, Reason: workv1alpha2.BindingReasonUnschedulable, Status: metav1.ConditionFalse},
+			ignoreErr:         false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			condition, ignoreErr := getConditionByError(tt.err)
+			if condition.Type != tt.expectedCondition.Type ||
+				condition.Reason != tt.expectedCondition.Reason ||
+				condition.Status != tt.expectedCondition.Status {
+				t.Errorf("expected condition: (%s, %s, %s), but got (%s, %s, %s)",
+					tt.expectedCondition.Type, tt.expectedCondition.Reason, tt.expectedCondition.Status, condition.Type, condition.Reason, condition.Status)
+			}
+
+			if ignoreErr != tt.ignoreErr {
+				t.Errorf("expect to ignore error: %v. but got: %v", tt.ignoreErr, ignoreErr)
+			}
+		})
+	}
+}