fix comments

Signed-off-by: Ryan Zhang <[email protected]>

Author: Ryan Zhang

pkg/scheduler/framework/uniquename/uniquename.go

@@ -62,7 +62,7 @@ func NewBindingName(placementName string, clusterName string) (string, error) {
 
 	if errs := validation.IsDNS1123Label(uniqueName); len(errs) != 0 {
 		// Do a sanity check here; normally this would not occur.
-		return "", fmt.Errorf("failed to format a unique RFC 1123 label name with CRP name %s, cluster name %s: %v", placementName, clusterName, errs)
+		return "", fmt.Errorf("failed to format a unique RFC 1123 label name with placement name %s, cluster name %s: %v", placementName, clusterName, errs)
 	}
 	return uniqueName, nil
 }

pkg/scheduler/scheduler.go

@@ -19,12 +19,13 @@ package scheduler
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strconv"
 	"sync"
 	"time"
 
-	"k8s.io/apimachinery/pkg/api/errors"
+	apiErrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/labels"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/tools/record"
@@ -135,7 +136,7 @@ func (s *Scheduler) scheduleOnce(ctx context.Context, worker int) {
 	// Retrieve the placement object (either ClusterResourcePlacement or ResourcePlacement).
 	placement, err := controller.FetchPlacementFromKey(ctx, s.client, placementName)
 	if err != nil {
-		if errors.IsNotFound(err) {
+		if apiErrors.IsNotFound(err) {
 			// The placement has been gone before the scheduler gets a chance to
 			// process it; normally this would not happen as sources would not enqueue any placement that
 			// has been marked for deletion but does not have the scheduler cleanup finalizer to
@@ -144,6 +145,13 @@ func (s *Scheduler) scheduleOnce(ctx context.Context, worker int) {
 			klog.ErrorS(err, "placement is already deleted", "placement", placementRef)
 			return
 		}
+		if errors.Is(err, controller.ErrUnexpectedBehavior) {
+			// The placement is in an unexpected state; this is a scheduler-side error, and
+			// Note that this is a scheduler-side error, so it does not return an error to the caller.
+			// Raise an alert for it.
+			klog.ErrorS(err, "Placement is in an unexpected state", "placement", placementRef)
+			return
+		}
 		// Wrap the error for metrics; this method does not return an error.
 		klog.ErrorS(controller.NewAPIServerError(true, err), "Failed to get placement", "placement", placementRef)
 
@@ -158,6 +166,10 @@ func (s *Scheduler) scheduleOnce(ctx context.Context, worker int) {
 		if controllerutil.ContainsFinalizer(placement, fleetv1beta1.SchedulerCleanupFinalizer) {
 			if err := s.cleanUpAllBindingsFor(ctx, placement); err != nil {
 				klog.ErrorS(err, "Failed to clean up all bindings for placement", "placement", placementRef)
+				if errors.Is(err, controller.ErrUnexpectedBehavior) {
+					// The placement is in an unexpected state; this is a scheduler-side error, and
+					return
+				}
 				// Requeue for later processing.
 				s.queue.AddRateLimited(placementName)
 				return
@@ -200,10 +212,18 @@ func (s *Scheduler) scheduleOnce(ctx context.Context, worker int) {
 	cycleStartTime := time.Now()
 	res, err := s.framework.RunSchedulingCycleFor(ctx, controller.GetPlacementKeyFromObj(placement), latestPolicySnapshot)
 	if err != nil {
-		klog.ErrorS(err, "Failed to run scheduling cycle", "placement", placementRef)
+		if errors.Is(err, controller.ErrUnexpectedBehavior) {
+			// The placement is in an unexpected state; this is a scheduler-side error, and
+			// Note that this is a scheduler-side error, so it does not return an error to the caller.
+			// Raise an alert for it.
+			klog.ErrorS(err, "Placement is in an unexpected state", "placement", placementRef)
+			observeSchedulingCycleMetrics(cycleStartTime, true, false)
+			return
+		}
 		// Requeue for later processing.
+		klog.ErrorS(err, "Failed to run scheduling cycle", "placement", placementRef)
 		s.queue.AddRateLimited(placementName)
-		observeSchedulingCycleMetrics(cycleStartTime, true, false)
+		observeSchedulingCycleMetrics(cycleStartTime, true, true)
 		return
 	}
 
@@ -289,7 +309,7 @@ func (s *Scheduler) cleanUpAllBindingsFor(ctx context.Context, placement fleetv1
 	bindings, err := controller.ListBindingsFromKey(ctx, s.uncachedReader, placementKey)
 	if err != nil {
 		klog.ErrorS(err, "Failed to list all bindings", "placement", placementRef)
-		return controller.NewAPIServerError(false, err)
+		return err
 	}
 
 	// Remove scheduler CRB cleanup finalizer from deleting bindings.
@@ -304,12 +324,12 @@ func (s *Scheduler) cleanUpAllBindingsFor(ctx context.Context, placement fleetv1
 		binding := bindings[idx]
 		controllerutil.RemoveFinalizer(binding, fleetv1beta1.SchedulerCRBCleanupFinalizer)
 		if err := s.client.Update(ctx, binding); err != nil {
-			klog.ErrorS(err, "Failed to remove scheduler reconcile finalizer from cluster resource binding", "binding", klog.KObj(binding))
+			klog.ErrorS(err, "Failed to remove scheduler reconcile finalizer from resource binding", "binding", klog.KObj(binding))
 			return controller.NewUpdateIgnoreConflictError(err)
 		}
 		// Delete the binding if it has not been marked for deletion yet.
 		if binding.GetDeletionTimestamp() == nil {
-			if err := s.client.Delete(ctx, binding); err != nil && !errors.IsNotFound(err) {
+			if err := s.client.Delete(ctx, binding); err != nil && !apiErrors.IsNotFound(err) {
 				klog.ErrorS(err, "Failed to delete binding", "binding", klog.KObj(binding))
 				return controller.NewAPIServerError(false, err)
 			}

pkg/utils/controller/binding_resolver.go

@@ -19,45 +19,12 @@ package controller
 import (
 	"context"
 
-	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
 	"github.com/kubefleet-dev/kubefleet/pkg/scheduler/queue"
 )
 
-// FetchBindingFromKey resolves a specific binding by name using the placement key to determine the type.
-// The placement key format determines whether to fetch from ClusterResourceBinding (cluster-scoped)
-// or ResourceBinding (namespaced). For namespaced resources, the key format is "namespace/name".
-func FetchBindingFromKey(ctx context.Context, c client.Reader, placementKey queue.PlacementKey) (placementv1beta1.BindingObj, error) {
-	// Extract namespace and name from the placement key
-	namespace, name, err := ExtractNamespaceNameFromKey(placementKey)
-	if err != nil {
-		return nil, err
-	}
-	// Check if the placement key contains a namespace separator
-	if namespace != "" {
-		// This is a namespaced ResourceBinding
-		rb := &placementv1beta1.ResourceBinding{}
-		key := types.NamespacedName{
-			Namespace: namespace,
-			Name:      name,
-		}
-		if err := c.Get(ctx, key, rb); err != nil {
-			return nil, err
-		}
-		return rb, nil
-	} else {
-		// This is a cluster-scoped ClusterResourceBinding
-		crb := &placementv1beta1.ClusterResourceBinding{}
-		key := types.NamespacedName{Name: name} // name from placement key
-		if err := c.Get(ctx, key, crb); err != nil {
-			return nil, err
-		}
-		return crb, nil
-	}
-}
-
 // ListBindingsFromKey returns all bindings (either ClusterResourceBinding and ResourceBinding)
 // that belong to the specified placement key.
 // The placement key format determines whether to list ClusterResourceBindings (cluster-scoped)

pkg/utils/controller/binding_resolver_test.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -341,198 +340,3 @@ type failingListClient struct {
 func (c *failingListClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
 	return fmt.Errorf("simulated client error")
 }
-
-func TestFetchBindingFromKey(t *testing.T) {
-	ctx := context.Background()
-
-	tests := []struct {
-		name         string
-		placementKey queue.PlacementKey
-		objects      []client.Object
-		wantBinding  placementv1beta1.BindingObj
-		wantErr      bool
-		expectedErr  error
-	}{
-		{
-			name:         "cluster-scoped binding - found",
-			placementKey: queue.PlacementKey("test-binding"),
-			objects: []client.Object{
-				&placementv1beta1.ClusterResourceBinding{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "test-binding",
-						Labels: map[string]string{
-							placementv1beta1.CRPTrackingLabel: "test-placement",
-						},
-					},
-					Spec: placementv1beta1.ResourceBindingSpec{
-						TargetCluster: "cluster-1",
-					},
-				},
-			},
-			wantBinding: &placementv1beta1.ClusterResourceBinding{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test-binding",
-					Labels: map[string]string{
-						placementv1beta1.CRPTrackingLabel: "test-placement",
-					},
-				},
-				Spec: placementv1beta1.ResourceBindingSpec{
-					TargetCluster: "cluster-1",
-				},
-			},
-			wantErr: false,
-		},
-		{
-			name:         "cluster-scoped binding - not found",
-			placementKey: queue.PlacementKey("nonexistent-binding"),
-			objects:      []client.Object{},
-			wantErr:      true,
-		},
-		{
-			name:         "namespaced binding - found",
-			placementKey: queue.PlacementKey("test-namespace/test-binding"),
-			objects: []client.Object{
-				&placementv1beta1.ResourceBinding{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test-binding",
-						Namespace: "test-namespace",
-						Labels: map[string]string{
-							placementv1beta1.CRPTrackingLabel: "test-placement",
-						},
-					},
-					Spec: placementv1beta1.ResourceBindingSpec{
-						TargetCluster: "cluster-1",
-					},
-				},
-			},
-			wantBinding: &placementv1beta1.ResourceBinding{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-binding",
-					Namespace: "test-namespace",
-					Labels: map[string]string{
-						placementv1beta1.CRPTrackingLabel: "test-placement",
-					},
-				},
-				Spec: placementv1beta1.ResourceBindingSpec{
-					TargetCluster: "cluster-1",
-				},
-			},
-			wantErr: false,
-		},
-		{
-			name:         "namespaced binding - not found",
-			placementKey: queue.PlacementKey("test-namespace/nonexistent-binding"),
-			objects:      []client.Object{},
-			wantErr:      true,
-		},
-		{
-			name:         "namespaced binding - wrong namespace",
-			placementKey: queue.PlacementKey("wrong-namespace/test-binding"),
-			objects: []client.Object{
-				&placementv1beta1.ResourceBinding{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test-binding",
-						Namespace: "test-namespace", // different namespace
-						Labels: map[string]string{
-							placementv1beta1.CRPTrackingLabel: "test-placement",
-						},
-					},
-					Spec: placementv1beta1.ResourceBindingSpec{
-						TargetCluster: "cluster-1",
-					},
-				},
-			},
-			wantErr: true,
-		},
-		{
-			name:         "invalid placement key format - too many separators",
-			placementKey: queue.PlacementKey("namespace/binding/extra"),
-			objects:      []client.Object{},
-			wantErr:      true,
-			expectedErr:  ErrUnexpectedBehavior,
-		},
-		{
-			name:         "invalid placement key format - empty parts",
-			placementKey: queue.PlacementKey("namespace/"),
-			objects:      []client.Object{},
-			wantErr:      true,
-			expectedErr:  ErrUnexpectedBehavior,
-		},
-		{
-			name:         "invalid placement key format - empty namespace",
-			placementKey: queue.PlacementKey("/binding"),
-			objects:      []client.Object{},
-			wantErr:      true,
-			expectedErr:  ErrUnexpectedBehavior,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			scheme := runtime.NewScheme()
-			_ = placementv1beta1.AddToScheme(scheme)
-
-			fakeClient := fake.NewClientBuilder().
-				WithScheme(scheme).
-				WithObjects(tt.objects...).
-				Build()
-
-			got, err := FetchBindingFromKey(ctx, fakeClient, tt.placementKey)
-
-			if tt.wantErr {
-				if err == nil {
-					t.Fatalf("Expected error but got nil")
-				}
-				if tt.expectedErr != nil && !errors.Is(err, tt.expectedErr) {
-					t.Fatalf("Expected error: %v, but got: %v", tt.expectedErr, err)
-				}
-				return
-			}
-
-			if err != nil {
-				t.Fatalf("Expected no error but got: %v", err)
-			}
-
-			// Use cmp.Diff to compare the actual result with expected binding
-			// Ignore resource version and other metadata fields that may differ
-			if diff := cmp.Diff(got, tt.wantBinding,
-				cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion")); diff != "" {
-				t.Errorf("FetchBindingFromKey() diff (-got +want):\n%s", diff)
-			}
-		})
-	}
-}
-
-func TestFetchBindingFromKey_ClientError(t *testing.T) {
-	ctx := context.Background()
-
-	// Create a client that will return an error
-	scheme := runtime.NewScheme()
-	_ = placementv1beta1.AddToScheme(scheme)
-
-	// Use a failing client for Get operations
-	fakeClient := &failingGetClient{
-		Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
-	}
-
-	_, err := FetchBindingFromKey(ctx, fakeClient, queue.PlacementKey("test-binding"))
-
-	if err == nil {
-		t.Fatalf("Expected error but got nil")
-	}
-
-	// The error should be the simulated client error, not wrapped as APIServerError
-	// since FetchBindingFromKey doesn't wrap the Get errors
-	if !strings.Contains(err.Error(), "simulated get error") {
-		t.Errorf("Expected error to contain 'simulated get error' but got: %s", err.Error())
-	}
-}
-
-// failingGetClient is a test helper that wraps a client and makes Get calls fail
-type failingGetClient struct {
-	client.Client
-}
-
-func (c *failingGetClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
-	return fmt.Errorf("simulated get error")
-}