Skip to content

Commit 137ea86

Browse files
zhy76zhy76
committed
fix: ensure status updates are not skipped due to stale cache
Fixes #6858
1 parent 310dcff commit 137ea86

File tree

2 files changed

+128
-47
lines changed

2 files changed

+128
-47
lines changed

‎pkg/util/helper/status.go‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"context"
2121
"fmt"
2222

23-
"k8s.io/apimachinery/pkg/api/equality"
2423
"sigs.k8s.io/controller-runtime/pkg/client"
2524
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
2625
)
@@ -42,16 +41,15 @@ func UpdateStatus(ctx context.Context, c client.Client, obj client.Object, f con
4241
return controllerutil.OperationResultNone, err
4342
}
4443

45-
existing := obj.DeepCopyObject()
4644
if err := mutate(f, key, obj); err != nil {
4745
return controllerutil.OperationResultNone, err
4846
}
4947

50-
if equality.Semantic.DeepEqual(existing, obj) {
51-
return controllerutil.OperationResultNone, nil
52-
}
53-
54-
if err := c.Status().Update(ctx, obj); err != nil {
48+
// Patch status directly without checking for equality to ensure the status is always updated,
49+
// even if the content appears unchanged. This avoids issues where stale cache data might cause
50+
// updates to be skipped incorrectly.
51+
// FYI: https://github.com/karmada-io/karmada/issues/6858
52+
if err := c.Status().Patch(ctx, obj, client.Merge); err != nil {
5553
return controllerutil.OperationResultNone, err
5654
}
5755
return controllerutil.OperationResultUpdatedStatusOnly, nil

‎pkg/util/helper/status_test.go‎

Lines changed: 123 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -20,29 +20,36 @@ import (
2020
"context"
2121
"fmt"
2222
"testing"
23+
"time"
2324

2425
"github.com/stretchr/testify/assert"
2526
corev1 "k8s.io/api/core/v1"
2627
apierrors "k8s.io/apimachinery/pkg/api/errors"
28+
"k8s.io/apimachinery/pkg/api/meta"
2729
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2830
"k8s.io/apimachinery/pkg/runtime"
2931
"k8s.io/apimachinery/pkg/types"
3032
"sigs.k8s.io/controller-runtime/pkg/client"
3133
"sigs.k8s.io/controller-runtime/pkg/client/fake"
3234
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
35+
36+
workv1alpha1 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha1"
3337
)
3438

3539
func TestUpdateStatus(t *testing.T) {
3640
scheme := runtime.NewScheme()
3741
_ = corev1.AddToScheme(scheme)
42+
_ = workv1alpha1.AddToScheme(scheme)
3843

3944
tests := []struct {
4045
name string
41-
setupObj *corev1.Pod
42-
mutateStatus func(*corev1.Pod)
46+
setupObj client.Object
47+
updateObj client.Object
48+
mutateStatus func(client.Object)
4349
statusError bool
4450
expectedOp controllerutil.OperationResult
4551
expectedError string
52+
verify func(*testing.T, client.Client)
4653
}{
4754
{
4855
name: "successful status update",
@@ -55,12 +62,24 @@ func TestUpdateStatus(t *testing.T) {
5562
Phase: corev1.PodPending,
5663
},
5764
},
58-
mutateStatus: func(pod *corev1.Pod) {
65+
updateObj: &corev1.Pod{
66+
ObjectMeta: metav1.ObjectMeta{
67+
Name: "test-pod",
68+
Namespace: "default",
69+
},
70+
},
71+
mutateStatus: func(obj client.Object) {
72+
pod := obj.(*corev1.Pod)
5973
pod.Status.Phase = corev1.PodRunning
6074
},
6175
statusError: false,
6276
expectedOp: controllerutil.OperationResultUpdatedStatusOnly,
6377
expectedError: "",
78+
verify: func(t *testing.T, c client.Client) {
79+
pod := &corev1.Pod{}
80+
assert.NoError(t, c.Get(context.TODO(), types.NamespacedName{Name: "test-pod", Namespace: "default"}, pod))
81+
assert.Equal(t, corev1.PodRunning, pod.Status.Phase)
82+
},
6483
},
6584
{
6685
name: "status update error",
@@ -73,15 +92,22 @@ func TestUpdateStatus(t *testing.T) {
7392
Phase: corev1.PodPending,
7493
},
7594
},
76-
mutateStatus: func(pod *corev1.Pod) {
95+
updateObj: &corev1.Pod{
96+
ObjectMeta: metav1.ObjectMeta{
97+
Name: "test-pod",
98+
Namespace: "default",
99+
},
100+
},
101+
mutateStatus: func(obj client.Object) {
102+
pod := obj.(*corev1.Pod)
77103
pod.Status.Phase = corev1.PodRunning
78104
},
79105
statusError: true,
80106
expectedOp: controllerutil.OperationResultNone,
81107
expectedError: "Internal error occurred: status update failed",
82108
},
83109
{
84-
name: "no changes needed",
110+
name: "no changes, should still update",
85111
setupObj: &corev1.Pod{
86112
ObjectMeta: metav1.ObjectMeta{
87113
Name: "test-pod",
@@ -91,20 +117,80 @@ func TestUpdateStatus(t *testing.T) {
91117
Phase: corev1.PodRunning,
92118
},
93119
},
94-
mutateStatus: func(_ *corev1.Pod) {
95-
// No changes to status
120+
updateObj: &corev1.Pod{
121+
ObjectMeta: metav1.ObjectMeta{
122+
Name: "test-pod",
123+
Namespace: "default",
124+
},
125+
},
126+
mutateStatus: func(_ client.Object) {
127+
// No changes
96128
},
97129
statusError: false,
98-
expectedOp: controllerutil.OperationResultNone,
130+
expectedOp: controllerutil.OperationResultUpdatedStatusOnly,
99131
expectedError: "",
132+
verify: func(t *testing.T, c client.Client) {
133+
pod := &corev1.Pod{}
134+
assert.NoError(t, c.Get(context.TODO(), types.NamespacedName{Name: "test-pod", Namespace: "default"}, pod))
135+
assert.Equal(t, corev1.PodRunning, pod.Status.Phase)
136+
},
100137
},
101138
{
102-
name: "object not found",
103-
setupObj: nil, // Not create the object
104-
mutateStatus: func(pod *corev1.Pod) {
105-
pod.Status.Phase = corev1.PodRunning
139+
name: "update with same WorkApplied condition, should still update (issue #6858)",
140+
setupObj: &workv1alpha1.Work{
141+
ObjectMeta: metav1.ObjectMeta{
142+
Name: "test-work",
143+
Namespace: "default",
144+
},
145+
Status: workv1alpha1.WorkStatus{
146+
Conditions: []metav1.Condition{
147+
{
148+
Type: workv1alpha1.WorkApplied,
149+
Status: metav1.ConditionTrue,
150+
Reason: "AppliedSuccessful",
151+
Message: "Manifest has been successfully applied",
152+
LastTransitionTime: metav1.Time{Time: metav1.Now().Add(-1 * time.Hour)},
153+
},
154+
},
155+
},
156+
},
157+
updateObj: &workv1alpha1.Work{
158+
ObjectMeta: metav1.ObjectMeta{
159+
Name: "test-work",
160+
Namespace: "default",
161+
},
162+
},
163+
mutateStatus: func(obj client.Object) {
164+
meta.SetStatusCondition(&obj.(*workv1alpha1.Work).Status.Conditions, metav1.Condition{
165+
Type: workv1alpha1.WorkApplied,
166+
Status: metav1.ConditionTrue,
167+
Reason: "AppliedSuccessful",
168+
Message: "Manifest has been successfully applied",
169+
LastTransitionTime: metav1.Now(),
170+
})
171+
},
172+
expectedOp: controllerutil.OperationResultUpdatedStatusOnly,
173+
verify: func(t *testing.T, c client.Client) {
174+
work := &workv1alpha1.Work{}
175+
assert.NoError(t, c.Get(context.TODO(), types.NamespacedName{Name: "test-work", Namespace: "default"}, work))
176+
177+
cond := meta.FindStatusCondition(work.Status.Conditions, workv1alpha1.WorkApplied)
178+
assert.NotNil(t, cond)
179+
assert.Equal(t, metav1.ConditionTrue, cond.Status)
180+
assert.Equal(t, "AppliedSuccessful", cond.Reason)
181+
},
182+
},
183+
{
184+
name: "object not found",
185+
updateObj: &corev1.Pod{
186+
ObjectMeta: metav1.ObjectMeta{
187+
Name: "test-pod",
188+
Namespace: "default",
189+
},
190+
},
191+
mutateStatus: func(obj client.Object) {
192+
obj.(*corev1.Pod).Status.Phase = corev1.PodRunning
106193
},
107-
statusError: false,
108194
expectedOp: controllerutil.OperationResultNone,
109195
expectedError: "not found",
110196
},
@@ -116,9 +202,15 @@ func TestUpdateStatus(t *testing.T) {
116202
Namespace: "default",
117203
},
118204
},
119-
mutateStatus: func(pod *corev1.Pod) {
205+
updateObj: &corev1.Pod{
206+
ObjectMeta: metav1.ObjectMeta{
207+
Name: "test-pod",
208+
Namespace: "default",
209+
},
210+
},
211+
mutateStatus: func(obj client.Object) {
120212
// Simulate mutation error by changing name
121-
pod.Name = "different-name"
213+
obj.SetName("different-name")
122214
},
123215
statusError: false,
124216
expectedOp: controllerutil.OperationResultNone,
@@ -131,31 +223,19 @@ func TestUpdateStatus(t *testing.T) {
131223
clientBuilder := fake.NewClientBuilder().WithScheme(scheme)
132224
if tt.setupObj != nil {
133225
clientBuilder = clientBuilder.WithObjects(tt.setupObj)
134-
}
135-
fakeClient := clientBuilder.Build()
136-
137-
var client client.Client
138-
if tt.statusError {
139-
client = &mockClient{
140-
Client: fakeClient,
141-
shouldError: true,
226+
if _, ok := tt.setupObj.(*workv1alpha1.Work); ok {
227+
clientBuilder = clientBuilder.WithStatusSubresource(tt.setupObj)
142228
}
143-
} else {
144-
client = fakeClient
145229
}
146230

147-
// Create a new object for update
148-
obj := &corev1.Pod{
149-
ObjectMeta: metav1.ObjectMeta{
150-
Name: "test-pod",
151-
Namespace: "default",
152-
},
231+
c := client.Client(clientBuilder.Build())
232+
if tt.statusError {
233+
c = &mockClient{Client: c, shouldError: true}
153234
}
154235

155-
// Run the update
156-
op, err := UpdateStatus(context.TODO(), client, obj, func() error {
236+
op, err := UpdateStatus(context.TODO(), c, tt.updateObj, func() error {
157237
if tt.mutateStatus != nil {
158-
tt.mutateStatus(obj)
238+
tt.mutateStatus(tt.updateObj)
159239
}
160240
return nil
161241
})
@@ -171,12 +251,8 @@ func TestUpdateStatus(t *testing.T) {
171251
// Check operation result
172252
assert.Equal(t, tt.expectedOp, op)
173253

174-
// If successful update, verify the status was actually changed
175-
if tt.expectedOp == controllerutil.OperationResultUpdatedStatusOnly {
176-
updatedPod := &corev1.Pod{}
177-
err := client.Get(context.TODO(), types.NamespacedName{Name: "test-pod", Namespace: "default"}, updatedPod)
178-
assert.NoError(t, err)
179-
assert.Equal(t, corev1.PodRunning, updatedPod.Status.Phase)
254+
if tt.verify != nil {
255+
tt.verify(t, c)
180256
}
181257
})
182258
}
@@ -359,6 +435,13 @@ func (m *mockStatusWriter) Update(_ context.Context, _ client.Object, _ ...clien
359435
return nil
360436
}
361437

438+
func (m *mockStatusWriter) Patch(_ context.Context, _ client.Object, _ client.Patch, _ ...client.SubResourcePatchOption) error {
439+
if m.shouldError {
440+
return apierrors.NewInternalError(fmt.Errorf("status update failed"))
441+
}
442+
return nil
443+
}
444+
362445
// mockClient wraps the fake client and returns our mock status writer
363446
type mockClient struct {
364447
client.Client

0 commit comments

Comments
 (0)