fix: ensure status updates are not skipped due to stale cache

Signed-off-by: 浩韵 <[email protected]>

Author: zhy76

pkg/controllers/multiclusterservice/endpointslice_dispatch_controller_test.go

@@ -77,13 +77,7 @@ func TestUpdateEndpointSliceDispatched(t *testing.T) {
 
 			// Expectations Setup
 			mockClient.On("Status").Return(mockStatusWriter)
-			mockClient.On("Get", mock.Anything, mock.AnythingOfType("types.NamespacedName"), mock.AnythingOfType("*v1alpha1.MultiClusterService"), mock.Anything).
-				Run(func(args mock.Arguments) {
-					arg := args.Get(2).(*networkingv1alpha1.MultiClusterService)
-					*arg = *tt.mcs // Copy the input MCS to the output
-				}).Return(nil)
-
-			mockStatusWriter.On("Update", mock.Anything, mock.AnythingOfType("*v1alpha1.MultiClusterService"), mock.Anything).
+			mockStatusWriter.On("Patch", mock.Anything, mock.AnythingOfType("*v1alpha1.MultiClusterService"), mock.Anything, mock.Anything).
 				Run(func(args mock.Arguments) {
 					mcs := args.Get(1).(*networkingv1alpha1.MultiClusterService)
 					mcs.Status.Conditions = []metav1.Condition{tt.expectedCondition}

pkg/controllers/remediation/remedy_controller_test.go

@@ -192,7 +192,11 @@ func setupScheme() *runtime.Scheme {
 
 // Helper function to create a fake client
 func createFakeClient(scheme *runtime.Scheme, objs ...runtime.Object) client.Client {
-	return fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objs...).Build()
+	return fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithRuntimeObjects(objs...).
+		WithStatusSubresource(&clusterv1alpha1.Cluster{}).
+		Build()
 }
 
 // Helper function to set up the manager

pkg/util/helper/status.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"fmt"
 
-	"k8s.io/apimachinery/pkg/api/equality"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
@@ -38,20 +37,17 @@ import (
 // already exist.
 func UpdateStatus(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) {
 	key := client.ObjectKeyFromObject(obj)
-	if err := c.Get(ctx, key, obj); err != nil {
-		return controllerutil.OperationResultNone, err
-	}
 
-	existing := obj.DeepCopyObject()
+	oldObj := obj.DeepCopyObject().(client.Object)
 	if err := mutate(f, key, obj); err != nil {
 		return controllerutil.OperationResultNone, err
 	}
 
-	if equality.Semantic.DeepEqual(existing, obj) {
-		return controllerutil.OperationResultNone, nil
-	}
-
-	if err := c.Status().Update(ctx, obj); err != nil {
+	// Patch status directly without checking for equality to ensure the status is always updated,
+	// even if the content appears unchanged. This avoids issues where stale cache data might cause
+	// updates to be skipped incorrectly.
+	// FYI: https://github.com/karmada-io/karmada/issues/6858
+	if err := c.Status().Patch(ctx, obj, client.MergeFrom(oldObj)); err != nil {
 		return controllerutil.OperationResultNone, err
 	}
 	return controllerutil.OperationResultUpdatedStatusOnly, nil

pkg/util/helper/status_test.go

@@ -24,25 +24,30 @@ import (
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+
+	workv1alpha1 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha1"
 )
 
 func TestUpdateStatus(t *testing.T) {
 	scheme := runtime.NewScheme()
 	_ = corev1.AddToScheme(scheme)
+	_ = workv1alpha1.Install(scheme)
 
 	tests := []struct {
 		name          string
-		setupObj      *corev1.Pod
-		mutateStatus  func(*corev1.Pod)
+		setupObj      client.Object
+		mutateStatus  func(client.Object)
 		statusError   bool
 		expectedOp    controllerutil.OperationResult
 		expectedError string
+		verify        func(*testing.T, client.Client)
 	}{
 		{
 			name: "successful status update",
@@ -55,12 +60,18 @@ func TestUpdateStatus(t *testing.T) {
 					Phase: corev1.PodPending,
 				},
 			},
-			mutateStatus: func(pod *corev1.Pod) {
+			mutateStatus: func(obj client.Object) {
+				pod := obj.(*corev1.Pod)
 				pod.Status.Phase = corev1.PodRunning
 			},
 			statusError:   false,
 			expectedOp:    controllerutil.OperationResultUpdatedStatusOnly,
 			expectedError: "",
+			verify: func(t *testing.T, c client.Client) {
+				pod := &corev1.Pod{}
+				assert.NoError(t, c.Get(context.TODO(), types.NamespacedName{Name: "test-pod", Namespace: "default"}, pod))
+				assert.Equal(t, corev1.PodRunning, pod.Status.Phase)
+			},
 		},
 		{
 			name: "status update error",
@@ -73,15 +84,16 @@ func TestUpdateStatus(t *testing.T) {
 					Phase: corev1.PodPending,
 				},
 			},
-			mutateStatus: func(pod *corev1.Pod) {
+			mutateStatus: func(obj client.Object) {
+				pod := obj.(*corev1.Pod)
 				pod.Status.Phase = corev1.PodRunning
 			},
 			statusError:   true,
 			expectedOp:    controllerutil.OperationResultNone,
 			expectedError: "Internal error occurred: status update failed",
 		},
 		{
-			name: "no changes needed",
+			name: "no changes, should still update",
 			setupObj: &corev1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test-pod",
@@ -91,22 +103,54 @@ func TestUpdateStatus(t *testing.T) {
 					Phase: corev1.PodRunning,
 				},
 			},
-			mutateStatus: func(_ *corev1.Pod) {
-				// No changes to status
+			mutateStatus: func(_ client.Object) {
+				// No changes
 			},
 			statusError:   false,
-			expectedOp:    controllerutil.OperationResultNone,
+			expectedOp:    controllerutil.OperationResultUpdatedStatusOnly,
 			expectedError: "",
+			verify: func(t *testing.T, c client.Client) {
+				pod := &corev1.Pod{}
+				assert.NoError(t, c.Get(context.TODO(), types.NamespacedName{Name: "test-pod", Namespace: "default"}, pod))
+				assert.Equal(t, corev1.PodRunning, pod.Status.Phase)
+			},
 		},
 		{
-			name:     "object not found",
-			setupObj: nil, // Not create the object
-			mutateStatus: func(pod *corev1.Pod) {
-				pod.Status.Phase = corev1.PodRunning
+			name: "update with same WorkApplied condition, should still update",
+			setupObj: &workv1alpha1.Work{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-work",
+					Namespace: "default",
+				},
+				Status: workv1alpha1.WorkStatus{
+					Conditions: []metav1.Condition{
+						{
+							Type:    workv1alpha1.WorkApplied,
+							Status:  metav1.ConditionTrue,
+							Reason:  "AppliedSuccessful",
+							Message: "Manifest has been successfully applied",
+						},
+					},
+				},
+			},
+			mutateStatus: func(obj client.Object) {
+				meta.SetStatusCondition(&obj.(*workv1alpha1.Work).Status.Conditions, metav1.Condition{
+					Type:    workv1alpha1.WorkApplied,
+					Status:  metav1.ConditionTrue,
+					Reason:  "AppliedSuccessful",
+					Message: "Manifest has been successfully applied",
+				})
+			},
+			expectedOp: controllerutil.OperationResultUpdatedStatusOnly,
+			verify: func(t *testing.T, c client.Client) {
+				work := &workv1alpha1.Work{}
+				assert.NoError(t, c.Get(context.TODO(), types.NamespacedName{Name: "test-work", Namespace: "default"}, work))
+
+				cond := meta.FindStatusCondition(work.Status.Conditions, workv1alpha1.WorkApplied)
+				assert.NotNil(t, cond)
+				assert.Equal(t, metav1.ConditionTrue, cond.Status)
+				assert.Equal(t, "AppliedSuccessful", cond.Reason)
 			},
-			statusError:   false,
-			expectedOp:    controllerutil.OperationResultNone,
-			expectedError: "not found",
 		},
 		{
 			name: "mutation error",
@@ -116,9 +160,9 @@ func TestUpdateStatus(t *testing.T) {
 					Namespace: "default",
 				},
 			},
-			mutateStatus: func(pod *corev1.Pod) {
+			mutateStatus: func(obj client.Object) {
 				// Simulate mutation error by changing name
-				pod.Name = "different-name"
+				obj.SetName("different-name")
 			},
 			statusError:   false,
 			expectedOp:    controllerutil.OperationResultNone,
@@ -131,31 +175,19 @@ func TestUpdateStatus(t *testing.T) {
 			clientBuilder := fake.NewClientBuilder().WithScheme(scheme)
 			if tt.setupObj != nil {
 				clientBuilder = clientBuilder.WithObjects(tt.setupObj)
-			}
-			fakeClient := clientBuilder.Build()
-
-			var client client.Client
-			if tt.statusError {
-				client = &mockClient{
-					Client:      fakeClient,
-					shouldError: true,
+				if _, ok := tt.setupObj.(*workv1alpha1.Work); ok {
+					clientBuilder = clientBuilder.WithStatusSubresource(tt.setupObj)
 				}
-			} else {
-				client = fakeClient
 			}
 
-			// Create a new object for update
-			obj := &corev1.Pod{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-pod",
-					Namespace: "default",
-				},
+			c := client.Client(clientBuilder.Build())
+			if tt.statusError {
+				c = &mockClient{Client: c, shouldError: true}
 			}
 
-			// Run the update
-			op, err := UpdateStatus(context.TODO(), client, obj, func() error {
+			op, err := UpdateStatus(context.TODO(), c, tt.setupObj, func() error {
 				if tt.mutateStatus != nil {
-					tt.mutateStatus(obj)
+					tt.mutateStatus(tt.setupObj)
 				}
 				return nil
 			})
@@ -171,12 +203,8 @@ func TestUpdateStatus(t *testing.T) {
 			// Check operation result
 			assert.Equal(t, tt.expectedOp, op)
 
-			// If successful update, verify the status was actually changed
-			if tt.expectedOp == controllerutil.OperationResultUpdatedStatusOnly {
-				updatedPod := &corev1.Pod{}
-				err := client.Get(context.TODO(), types.NamespacedName{Name: "test-pod", Namespace: "default"}, updatedPod)
-				assert.NoError(t, err)
-				assert.Equal(t, corev1.PodRunning, updatedPod.Status.Phase)
+			if tt.verify != nil {
+				tt.verify(t, c)
 			}
 		})
 	}
@@ -359,6 +387,13 @@ func (m *mockStatusWriter) Update(_ context.Context, _ client.Object, _ ...clien
 	return nil
 }
 
+func (m *mockStatusWriter) Patch(_ context.Context, _ client.Object, _ client.Patch, _ ...client.SubResourcePatchOption) error {
+	if m.shouldError {
+		return apierrors.NewInternalError(fmt.Errorf("status update failed"))
+	}
+	return nil
+}
+
 // mockClient wraps the fake client and returns our mock status writer
 type mockClient struct {
 	client.Client