fix one case of from cache is not set correctly

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

Author: ryanzhang-oss

pkg/controllers/placement/placement_status.go

@@ -265,7 +265,7 @@ func setPlacementConditions(
 func (r *Reconciler) buildClusterToBindingMap(ctx context.Context, placementObj fleetv1beta1.PlacementObj, latestSchedulingPolicySnapshot fleetv1beta1.PolicySnapshotObj) (map[string]fleetv1beta1.BindingObj, error) {
 	placementKObj := klog.KObj(placementObj)
 	// List all bindings for the placement object.
-	bindings, err := controller.ListBindingsFromKey(ctx, r.Client, types.NamespacedName{Namespace: placementObj.GetNamespace(), Name: placementObj.GetName()})
+	bindings, err := controller.ListBindingsFromKey(ctx, r.Client, types.NamespacedName{Namespace: placementObj.GetNamespace(), Name: placementObj.GetName()}, true)
 	if err != nil {
 		klog.ErrorS(err, "Failed to list bindings for placement", "placement", placementKObj)
 		return nil, controller.NewAPIServerError(true, err)

pkg/controllers/rollout/controller.go

@@ -101,7 +101,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim
 
 	// list all the bindings associated with the placement
 	// we read from the API server directly to avoid the repeated reconcile loop due to cache inconsistency
-	allBindings, err := controller.ListBindingsFromKey(ctx, r.UncachedReader, placementKey)
+	allBindings, err := controller.ListBindingsFromKey(ctx, r.UncachedReader, placementKey, false)
 	if err != nil {
 		klog.ErrorS(err, "Failed to list all the bindings associated with the placement",
 			"placement", placementObjRef)

pkg/controllers/updaterun/initialization.go

@@ -201,7 +201,7 @@ func (r *Reconciler) collectScheduledClusters(
 	updateRunRef := klog.KObj(updateRun)
 	policySnapshotRef := klog.KObj(latestPolicySnapshot)
 
-	bindingObjs, err := controller.ListBindingsFromKey(ctx, r.Client, placementKey)
+	bindingObjs, err := controller.ListBindingsFromKey(ctx, r.Client, placementKey, true)
 	if err != nil {
 		klog.ErrorS(err, "Failed to list bindings", "placement", placementKey, "policySnapshot", policySnapshotRef, "updateRun", updateRunRef)
 		// list err can be retried.

pkg/scheduler/framework/framework.go

@@ -289,7 +289,7 @@ func (f *framework) RunSchedulingCycleFor(ctx context.Context, placementKey queu
 	// overloading). In the long run we might still want to resort to a cached situation.
 	//
 	// TO-DO (chenyu1): explore the possibilities of using a mutation cache for better performance.
-	bindings, err := controller.ListBindingsFromKey(ctx, f.uncachedReader, types.NamespacedName{Namespace: namespace, Name: name})
+	bindings, err := controller.ListBindingsFromKey(ctx, f.uncachedReader, types.NamespacedName{Namespace: namespace, Name: name}, false)
 	if err != nil {
 		klog.ErrorS(err, "Failed to collect bindings", "policySnapshot", policyRef)
 		return ctrl.Result{}, err

pkg/scheduler/scheduler.go

@@ -305,7 +305,7 @@ func (s *Scheduler) cleanUpAllBindingsFor(ctx context.Context, placement fleetv1
 	// Note that the listing is performed using the uncached client; this is to ensure that all related
 	// bindings can be found, even if they have not been synced to the cache yet.
 	// TO-DO (chenyu1): this is a very expensive op; explore options for optimization.
-	bindings, err := controller.ListBindingsFromKey(ctx, s.uncachedReader, types.NamespacedName{Namespace: placement.GetNamespace(), Name: placement.GetName()})
+	bindings, err := controller.ListBindingsFromKey(ctx, s.uncachedReader, types.NamespacedName{Namespace: placement.GetNamespace(), Name: placement.GetName()}, false)
 	if err != nil {
 		klog.ErrorS(err, "Failed to list all bindings", "placement", placementRef)
 		return err

pkg/utils/controller/binding_resolver.go

@@ -47,7 +47,8 @@ func FetchBindingFromKey(ctx context.Context, c client.Reader, bindingKey types.
 // that belong to the specified binding key.
 // The binding key format determines whether to list ClusterResourceBindings (cluster-scoped)
 // or ResourceBindings (namespaced). For namespaced resources, the key format has a namespace.
-func ListBindingsFromKey(ctx context.Context, c client.Reader, placementKey types.NamespacedName) ([]placementv1beta1.BindingObj, error) {
+// The fromCache parameter indicates whether the client is a cached client (true) or an uncached client (false).
+func ListBindingsFromKey(ctx context.Context, c client.Reader, placementKey types.NamespacedName, fromCache bool) ([]placementv1beta1.BindingObj, error) {
 	// Extract namespace and name from the binding key
 	namespace := placementKey.Namespace
 	name := placementKey.Name
@@ -65,7 +66,7 @@ func ListBindingsFromKey(ctx context.Context, c client.Reader, placementKey type
 		bindingList = &placementv1beta1.ClusterResourceBindingList{}
 	}
 	if err := c.List(ctx, bindingList, listOptions...); err != nil {
-		return nil, NewAPIServerError(false, err)
+		return nil, NewAPIServerError(fromCache, err)
 	}
 
 	bindingObjs := bindingList.GetBindingObjs()

pkg/utils/controller/binding_resolver_test.go

@@ -408,7 +408,7 @@ func TestListBindingsFromKey(t *testing.T) {
 				WithObjects(tt.objects...).
 				Build()
 
-			got, err := ListBindingsFromKey(ctx, fakeClient, tt.placementKey)
+			got, err := ListBindingsFromKey(ctx, fakeClient, tt.placementKey, true)
 
 			if tt.wantErr {
 				if err == nil {
@@ -498,13 +498,13 @@ func TestListBindingsFromKey_Sorted(t *testing.T) {
 		},
 	}
 
-	bindingsInMode0, err := ListBindingsFromKey(ctx, &mockClient, types.NamespacedName{Name: "placeholder"})
+	bindingsInMode0, err := ListBindingsFromKey(ctx, &mockClient, types.NamespacedName{Name: "placeholder"}, true)
 	if err != nil {
 		t.Fatalf("ListBindingsFromKey() in mode 0 returned error: %v", err)
 	}
 
 	mockMode.Store(1)
-	bindingsInMode1, err := ListBindingsFromKey(ctx, &mockClient, types.NamespacedName{Name: "placeholder"})
+	bindingsInMode1, err := ListBindingsFromKey(ctx, &mockClient, types.NamespacedName{Name: "placeholder"}, true)
 	if err != nil {
 		t.Fatalf("ListBindingsFromKey() in mode 1 returned error: %v", err)
 	}
@@ -517,23 +517,42 @@ func TestListBindingsFromKey_Sorted(t *testing.T) {
 func TestListBindingsFromKey_ClientError(t *testing.T) {
 	ctx := context.Background()
 
-	// Create a client that will return an error
-	scheme := runtime.NewScheme()
-	_ = placementv1beta1.AddToScheme(scheme)
-
-	// Use a fake client but override List to return error
-	fakeClient := &failingListClient{
-		Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
+	tests := []struct {
+		name      string
+		fromCache bool
+		wantErr   error
+	}{
+		{
+			name:      "uncached client returns ErrAPIServerError",
+			fromCache: false,
+			wantErr:   ErrAPIServerError,
+		},
+		{
+			name:      "cached client returns ErrUnexpectedBehavior as cached List should not fail",
+			fromCache: true,
+			wantErr:   ErrUnexpectedBehavior,
+		},
 	}
 
-	_, err := ListBindingsFromKey(ctx, fakeClient, types.NamespacedName{Name: "test-placement"})
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create a client that will return an error
+			scheme := runtime.NewScheme()
+			_ = placementv1beta1.AddToScheme(scheme)
 
-	if err == nil {
-		t.Fatalf("Expected error but got nil")
-	}
+			// Use a fake client but override List to return error
+			fakeClient := &failingListClient{
+				Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
+			}
 
-	if !errors.Is(err, ErrAPIServerError) {
-		t.Errorf("Expected ErrAPIServerError but got: %v", err)
+			_, err := ListBindingsFromKey(ctx, fakeClient, types.NamespacedName{Name: "test-placement"}, tt.fromCache)
+			if err == nil {
+				t.Fatalf("Expected error but got nil")
+			}
+			if !errors.Is(err, tt.wantErr) {
+				t.Errorf("Expected %v but got: %v", tt.wantErr, err)
+			}
+		})
 	}
 }