From e2672021bc828c3ef914ca2bcd0413754d5b686d Mon Sep 17 00:00:00 2001 From: Prem Saraswat Date: Fri, 11 Oct 2024 17:15:43 +0530 Subject: [PATCH] [unified-storage/apistore] Fix GuranteedUpdate skipping updates when `tryUpdate` is passed (#94557) `GuranteedUpdate` method of `apistore.Storage` had a bug, where it would errorneously conclude that the object is unchanged, in case a `tryUpdate` function is passed that modifies the existing object itself (as it is the case in many core types in K8s upstream). The modified `existingObj` was compared with `updatedObj`, which would essentially be same and this lead to the update being skipped. This patch fixes this by always passing a copy of the `existingObj`. Signed-off-by: Prem Kumar --- pkg/storage/unified/apistore/store.go | 2 +- pkg/storage/unified/apistore/store_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/storage/unified/apistore/store.go b/pkg/storage/unified/apistore/store.go index d4911da167c..11a5cc3e5ea 100644 --- a/pkg/storage/unified/apistore/store.go +++ b/pkg/storage/unified/apistore/store.go @@ -465,7 +465,7 @@ func (s *Storage) GuaranteedUpdate( return apierrors.NewNotFound(s.gr, req.Key.Name) } - updatedObj, _, err = tryUpdate(existingObj, res) + updatedObj, _, err = tryUpdate(existingObj.DeepCopyObject(), res) if err != nil { if attempt >= MaxUpdateAttempts { return err diff --git a/pkg/storage/unified/apistore/store_test.go b/pkg/storage/unified/apistore/store_test.go index 287aeea5c41..f0d648b9b5e 100644 --- a/pkg/storage/unified/apistore/store_test.go +++ b/pkg/storage/unified/apistore/store_test.go @@ -135,12 +135,12 @@ func TestDeleteWithSuggestion(t *testing.T) { storagetesting.RunTestDeleteWithSuggestion(ctx, t, store) } -//func TestDeleteWithSuggestionAndConflict(t *testing.T) { -// ctx, store, destroyFunc, err := testSetup(t) -// defer destroyFunc() -// assert.NoError(t, err) -// storagetesting.RunTestDeleteWithSuggestionAndConflict(ctx, t, store) -//} +func TestDeleteWithSuggestionAndConflict(t *testing.T) { + ctx, store, destroyFunc, err := testSetup(t) + defer destroyFunc() + assert.NoError(t, err) + storagetesting.RunTestDeleteWithSuggestionAndConflict(ctx, t, store) +} // TODO: this test relies on update //func TestDeleteWithSuggestionOfDeletedObject(t *testing.T) {