From 124440ccee2248cef2513d3ba344a2e8df077f7c Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Thu, 19 Dec 2024 07:47:48 -0700 Subject: [PATCH] Unistore: Block on legacy writes in mode3 (#98147) --- pkg/apiserver/rest/dualwriter_mode3.go | 230 ++++++------ pkg/apiserver/rest/dualwriter_mode3_test.go | 390 +++++++------------- 2 files changed, 240 insertions(+), 380 deletions(-) diff --git a/pkg/apiserver/rest/dualwriter_mode3.go b/pkg/apiserver/rest/dualwriter_mode3.go index 04af5a00713..1e7358c62e2 100644 --- a/pkg/apiserver/rest/dualwriter_mode3.go +++ b/pkg/apiserver/rest/dualwriter_mode3.go @@ -6,6 +6,7 @@ import ( "fmt" "time" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -63,51 +64,47 @@ func (d *DualWriterMode3) Create(ctx context.Context, in runtime.Object, createV return nil, fmt.Errorf("name or generatename have to be set") } + // create in legacy first, and then unistore. if unistore fails, but legacy succeeds, + // will try to cleanup the object in legacy. + + startLegacy := time.Now() + createdFromLegacy, err := d.Legacy.Create(ctx, in, createValidation, options) + if err != nil { + log.Error(err, "unable to create object in legacy storage") + d.recordLegacyDuration(true, mode2Str, d.resource, method, startLegacy) + return createdFromLegacy, err + } + d.recordLegacyDuration(false, mode2Str, d.resource, method, startLegacy) + + createdCopy := createdFromLegacy.DeepCopyObject() + accCreated, err := meta.Accessor(createdCopy) + if err != nil { + return createdFromLegacy, err + } + accCreated.SetResourceVersion("") + startStorage := time.Now() - storageObj, errObjectSt := d.Storage.Create(ctx, in, createValidation, options) + storageObj, errObjectSt := d.Storage.Create(ctx, createdCopy, createValidation, options) d.recordStorageDuration(errObjectSt != nil, mode3Str, d.resource, method, startStorage) if errObjectSt != nil { log.Error(err, "unable to create object in storage") + + // if we cannot create in unistore, attempt to clean up legacy + _, _, err = d.Legacy.Delete(ctx, accCreated.GetName(), nil, &metav1.DeleteOptions{}) + if err != nil { + log.Error(err, "unable to cleanup object in legacy storage") + } + return storageObj, errObjectSt } - createdCopy := storageObj.DeepCopyObject() - - //nolint:errcheck - go d.createOnLegacyStorage(ctx, in, createdCopy, createValidation, options) - - return storageObj, errObjectSt -} - -func (d *DualWriterMode3) createOnLegacyStorage(ctx context.Context, in, storageObj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) error { - var method = "create" - log := d.Log.WithValues("method", method) - ctx, cancel := context.WithTimeoutCause(context.WithoutCancel(ctx), time.Second*10, errors.New("legacy create timeout")) - defer cancel() - - accessor, err := meta.Accessor(storageObj) - if err != nil { - return err - } - - // clear the UID and ResourceVersion from the object before sending it to the legacy storage - accessor.SetUID("") - accessor.SetResourceVersion("") - - startLegacy := time.Now() - legacyObj, err := d.Legacy.Create(ctx, storageObj, createValidation, options) - d.recordLegacyDuration(err != nil, mode3Str, d.resource, method, startLegacy) - if err != nil { - log.Error(err, "unable to create object in legacy storage") - cancel() - } - - areEqual := Compare(legacyObj, storageObj) + areEqual := Compare(createdFromLegacy, storageObj) d.recordOutcome(mode3Str, getName(storageObj), areEqual, method) if !areEqual { log.Info("object from legacy and storage are not equal") } - return err + + return storageObj, errObjectSt } // Get overrides the behavior of the generic DualWriter and retrieves an object from Storage. @@ -201,41 +198,42 @@ func (d *DualWriterMode3) Delete(ctx context.Context, name string, deleteValidat log := d.Log.WithValues("name", name, "method", method) ctx = klog.NewContext(ctx, d.Log) - startStorage := time.Now() - objFromStorage, async, err := d.Storage.Delete(ctx, name, deleteValidation, options) - d.recordStorageDuration(err != nil, mode3Str, name, method, startStorage) - if err != nil { - log.Error(err, "unable to delete object in storage") - return objFromStorage, async, err - } + // delete from legacy first, and then unistore. Will return a failure if either fails, + // unless its a 404. + // + // we want to delete from legacy first, otherwise if the delete from unistore was successful, + // but legacy failed, the user would get a failure, but not be able to retry the delete + // as they would not be able to see the object in unistore anymore. - //nolint:errcheck - go d.deleteFromLegacyStorage(ctx, objFromStorage, name, deleteValidation, options) - - return objFromStorage, async, err -} - -func (d *DualWriterMode3) deleteFromLegacyStorage(ctx context.Context, objFromStorage runtime.Object, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) error { - var method = "delete" - log := d.Log.WithValues("name", name, "method", method, "name", name) startLegacy := time.Now() - - ctx, cancel := context.WithTimeoutCause(context.WithoutCancel(ctx), time.Second*10, errors.New("legacy delete timeout")) - defer cancel() - - objFromLegacy, _, err := d.Legacy.Delete(ctx, name, deleteValidation, options) - d.recordLegacyDuration(err != nil, mode3Str, d.resource, method, startLegacy) + objFromLegacy, asyncLegacy, err := d.Legacy.Delete(ctx, name, deleteValidation, options) if err != nil { - log.Error(err, "unable to delete object in legacy storage") - cancel() + if !apierrors.IsNotFound(err) { + log.WithValues("object", objFromLegacy).Error(err, "could not delete from legacy store") + d.recordLegacyDuration(true, mode3Str, d.resource, method, startLegacy) + return objFromLegacy, asyncLegacy, err + } } + d.recordLegacyDuration(false, mode3Str, d.resource, method, startLegacy) + + startStorage := time.Now() + objFromStorage, asyncStorage, err := d.Storage.Delete(ctx, name, deleteValidation, options) + if err != nil { + if !apierrors.IsNotFound(err) { + log.WithValues("object", objFromStorage).Error(err, "could not delete from storage") + d.recordStorageDuration(true, mode3Str, d.resource, method, startStorage) + } + return objFromStorage, asyncStorage, err + } + d.recordStorageDuration(false, mode3Str, d.resource, method, startStorage) + areEqual := Compare(objFromStorage, objFromLegacy) d.recordOutcome(mode3Str, name, areEqual, method) if !areEqual { - log.Info("object from legacy and storage are not equal") + log.WithValues("name", name).Info("object from legacy and storage are not equal") } - return err + return objFromStorage, asyncStorage, err } // Update overrides the behavior of the generic DualWriter and writes first to Storage and then to LegacyStorage. @@ -243,45 +241,40 @@ func (d *DualWriterMode3) Update(ctx context.Context, name string, objInfo rest. var method = "update" log := d.Log.WithValues("name", name, "method", method) ctx = klog.NewContext(ctx, log) - - startStorage := time.Now() - objFromStorage, async, err := d.Storage.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) - d.recordStorageDuration(err != nil, mode3Str, d.resource, method, startStorage) - if err != nil { - log.Error(err, "unable to update in storage") - return objFromStorage, async, err - } - - //nolint:errcheck - go d.updateOnLegacyStorageMode3(ctx, objFromStorage, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) - - return objFromStorage, async, err -} - -func (d *DualWriterMode3) updateOnLegacyStorageMode3(ctx context.Context, storageObj runtime.Object, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) error { - // The incoming RV is from unified storage, so legacy can ignore it + // The incoming RV is not stable -- it may be from legacy or storage! + // This sets a flag in the context and our apistore is more lenient when it exists ctx = context.WithValue(ctx, dualWriteContextKey{}, true) - var method = "update" - log := d.Log.WithValues("name", name, "method", method, "name", name) + // update in legacy first, and then unistore. Will return a failure if either fails. + // + // we want to update in legacy first, otherwise if the update from unistore was successful, + // but legacy failed, the user would get a failure, but see the update did apply to the source + // of truth, and be less likely to retry to save (and get the stores in sync again) - ctx, cancel := context.WithTimeoutCause(context.WithoutCancel(ctx), time.Second*10, errors.New("legacy update timeout")) startLegacy := time.Now() - defer cancel() - - objLegacy, _, err := d.Legacy.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) - d.recordLegacyDuration(err != nil, mode3Str, d.resource, method, startLegacy) + objFromLegacy, createdLegacy, err := d.Legacy.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) if err != nil { - log.Error(err, "unable to update object in legacy storage") - cancel() + log.WithValues("object", objFromLegacy).Error(err, "could not update in legacy storage") + d.recordLegacyDuration(true, mode2Str, d.resource, "update", startLegacy) + return objFromLegacy, createdLegacy, err + } + d.recordLegacyDuration(false, mode2Str, d.resource, "update", startLegacy) + + startStorage := time.Now() + objFromStorage, created, err := d.Storage.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) + if err != nil { + log.WithValues("object", objFromStorage).Error(err, "could not update in storage") + d.recordStorageDuration(true, mode2Str, d.resource, "update", startStorage) + return objFromStorage, created, err } - areEqual := Compare(storageObj, objLegacy) + areEqual := Compare(objFromStorage, objFromLegacy) d.recordOutcome(mode3Str, name, areEqual, method) if !areEqual { log.WithValues("name", name).Info("object from legacy and storage are not equal") } - return err + + return objFromStorage, created, err } // DeleteCollection overrides the behavior of the generic DualWriter and deletes from both LegacyStorage and Storage. @@ -290,42 +283,49 @@ func (d *DualWriterMode3) DeleteCollection(ctx context.Context, deleteValidation log := d.Log.WithValues("resourceVersion", listOptions.ResourceVersion, "method", method) ctx = klog.NewContext(ctx, log) - startStorage := time.Now() - storageObj, err := d.Storage.DeleteCollection(ctx, deleteValidation, options, listOptions) - d.recordStorageDuration(err != nil, mode3Str, d.resource, method, startStorage) - if err != nil { - log.Error(err, "unable to delete collection in storage") - return storageObj, err - } + // delete from legacy first, and anything that is successful can be deleted in unistore too. + // + // we want to delete from legacy first, otherwise if the delete from unistore was successful, + // but legacy failed, the user would get a failure, but not be able to retry the delete + // as they would not be able to see the object in unistore anymore. - //nolint:errcheck - go d.deleteCollectionFromLegacyStorage(ctx, storageObj, deleteValidation, options, listOptions) - - return storageObj, err -} - -func (d *DualWriterMode3) deleteCollectionFromLegacyStorage(ctx context.Context, storageObj runtime.Object, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions, listOptions *metainternalversion.ListOptions) error { - var method = "delete-collection" - log := d.Log.WithValues("resourceVersion", listOptions.ResourceVersion, "method", method) startLegacy := time.Now() - - ctx, cancel := context.WithTimeoutCause(context.WithoutCancel(ctx), time.Second*10, errors.New("legacy deletecollection timeout")) - defer cancel() - - legacyObj, err := d.Legacy.DeleteCollection(ctx, deleteValidation, options, listOptions) - d.recordLegacyDuration(err != nil, mode3Str, d.resource, method, startLegacy) + deletedLegacy, err := d.Legacy.DeleteCollection(ctx, deleteValidation, options, listOptions) if err != nil { - log.Error(err, "unable to delete collection in legacy storage") - cancel() + log.WithValues("deleted", deletedLegacy).Error(err, "failed to delete collection successfully from legacy storage") + d.recordLegacyDuration(true, mode3Str, d.resource, method, startLegacy) + return deletedLegacy, err + } + d.recordLegacyDuration(false, mode3Str, d.resource, method, startLegacy) + + legacyList, err := meta.ExtractList(deletedLegacy) + if err != nil { + log.Error(err, "unable to extract list from legacy storage") + return nil, err } - areEqual := Compare(storageObj, legacyObj) - d.recordOutcome(mode3Str, getName(legacyObj), areEqual, method) + // Only the items deleted by the legacy DeleteCollection call are selected for deletion by Storage. + _, err = parseList(legacyList) + if err != nil { + return nil, err + } + + startStorage := time.Now() + deletedStorage, err := d.Storage.DeleteCollection(ctx, deleteValidation, options, listOptions) + if err != nil { + log.WithValues("deleted", deletedStorage).Error(err, "failed to delete collection successfully from Storage") + d.recordStorageDuration(true, mode3Str, d.resource, method, startStorage) + return deletedStorage, err + } + d.recordStorageDuration(false, mode3Str, d.resource, method, startStorage) + + areEqual := Compare(deletedStorage, deletedLegacy) + d.recordOutcome(mode3Str, getName(deletedLegacy), areEqual, method) if !areEqual { log.Info("object from legacy and storage are not equal") } - return err + return deletedStorage, err } func (d *DualWriterMode3) Watch(ctx context.Context, options *metainternalversion.ListOptions) (watch.Interface, error) { diff --git a/pkg/apiserver/rest/dualwriter_mode3_test.go b/pkg/apiserver/rest/dualwriter_mode3_test.go index 7aa1d0b75c3..ee1bcb888dd 100644 --- a/pkg/apiserver/rest/dualwriter_mode3_test.go +++ b/pkg/apiserver/rest/dualwriter_mode3_test.go @@ -8,14 +8,17 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + apierrors "k8s.io/apimachinery/pkg/api/errors" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" ) func TestMode3_Create(t *testing.T) { type testCase struct { input runtime.Object + setupLegacyFn func(m *mock.Mock, input runtime.Object) setupStorageFn func(m *mock.Mock, input runtime.Object) name string wantErr bool @@ -23,17 +26,32 @@ func TestMode3_Create(t *testing.T) { tests := []testCase{ { - name: "creating an object only in the unified store", + name: "creating an object in both the LegacyStorage and Storage", input: exampleObj, + setupLegacyFn: func(m *mock.Mock, input runtime.Object) { + m.On("Create", mock.Anything, exampleObjNoRV, mock.Anything, mock.Anything).Return(exampleObj, nil).Once() + }, setupStorageFn: func(m *mock.Mock, input runtime.Object) { - m.On("Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, nil) + m.On("Create", mock.Anything, exampleObj, mock.Anything, mock.Anything).Return(exampleObj, nil).Once() }, }, { - name: "error when creating object in the unified store fails", + name: "error when creating object in the legacy store fails", input: failingObj, + setupLegacyFn: func(m *mock.Mock, input runtime.Object) { + m.On("Create", mock.Anything, input, mock.Anything, mock.Anything).Return(nil, errors.New("error")).Once() + }, + wantErr: true, + }, + { + name: "error when creating object in the unistore fails - legacy delete should be called", + input: exampleObj, + setupLegacyFn: func(m *mock.Mock, input runtime.Object) { + m.On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, true, nil).Once() + m.On("Create", mock.Anything, exampleObjNoRV, mock.Anything, mock.Anything).Return(exampleObj, nil).Once() + }, setupStorageFn: func(m *mock.Mock, input runtime.Object) { - m.On("Create", mock.Anything, failingObj, mock.Anything, mock.Anything).Return(nil, errors.New("error")) + m.On("Create", mock.Anything, exampleObj, mock.Anything, mock.Anything).Return(exampleObj, errors.New("error")).Once() }, wantErr: true, }, @@ -48,75 +66,23 @@ func TestMode3_Create(t *testing.T) { ls := legacyStoreMock{m, l} us := storageMock{m, s} + if tt.setupLegacyFn != nil { + tt.setupLegacyFn(m, tt.input) + } if tt.setupStorageFn != nil { tt.setupStorageFn(m, tt.input) } dw := NewDualWriter(Mode3, ls, us, p, kind) - obj, err := dw.Create(context.Background(), tt.input, func(context.Context, runtime.Object) error { return nil }, &metav1.CreateOptions{}) + obj, err := dw.Create(context.Background(), tt.input, createFn, &metav1.CreateOptions{}) if tt.wantErr { assert.Error(t, err) return } - assert.NoError(t, err) - assert.NotEqual(t, obj, anotherObj) - }) - } -} - -func TestMode1_CreateOnLegacyStorage(t *testing.T) { - ctxCanceled, cancel := context.WithCancel(context.TODO()) - cancel() - - type testCase struct { - name string - input runtime.Object - ctx *context.Context - setupLegacyFn func(m *mock.Mock) - } - tests := - []testCase{ - { - name: "Create on legacy storage", - input: exampleObj, - setupLegacyFn: func(m *mock.Mock) { - m.On("Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObjNoRV, nil) - }, - }, - { - name: "Create on legacy storage works even if parent context is canceled", - input: exampleObj, - ctx: &ctxCanceled, - setupLegacyFn: func(m *mock.Mock) { - m.On("Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObjNoRV, nil) - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - l := (LegacyStorage)(nil) - s := (Storage)(nil) - m := &mock.Mock{} - - ls := legacyStoreMock{m, l} - us := storageMock{m, s} - - if tt.setupLegacyFn != nil { - tt.setupLegacyFn(m) - } - - ctx := context.TODO() - if tt.ctx != nil { - ctx = *tt.ctx - } - - dw := NewDualWriter(Mode3, ls, us, p, kind) - err := dw.(*DualWriterMode3).createOnLegacyStorage(ctx, tt.input, exampleObj, func(context.Context, runtime.Object) error { return nil }, &metav1.CreateOptions{}) - assert.NoError(t, err) + assert.Equal(t, exampleObj, obj) }) } } @@ -338,7 +304,8 @@ func TestMode1_ListFromLegacyStorage(t *testing.T) { func TestMode3_Delete(t *testing.T) { type testCase struct { - setupStorageFn func(m *mock.Mock, name string) + setupLegacyFn func(m *mock.Mock, input string) + setupStorageFn func(m *mock.Mock, input string) name string input string wantErr bool @@ -346,74 +313,56 @@ func TestMode3_Delete(t *testing.T) { tests := []testCase{ { - name: "deleting an object in the unified store", + name: "delete in legacy and storage", input: "foo", - setupStorageFn: func(m *mock.Mock, name string) { - m.On("Delete", mock.Anything, name, mock.Anything, mock.Anything).Return(exampleObj, false, nil) + setupLegacyFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(exampleObj, false, nil) + }, + setupStorageFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(exampleObj, false, nil) }, }, { - name: "error when deleting an object in the unified store", + name: "object delete in legacy not found, but found in storage", + input: "foo", + setupLegacyFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, "not-found-legacy", mock.Anything, mock.Anything).Return(nil, false, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "pods"}, "not-found")) + }, + setupStorageFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(exampleObj, false, nil) + }, + }, + { + name: " object delete in storage not found, but found in legacy", + input: "foo", + setupLegacyFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(exampleObj, false, nil) + }, + setupStorageFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, "not-found-storage", mock.Anything, mock.Anything).Return(nil, false, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "pods"}, "not-found")) + }, + }, + { + name: " object not found in both", input: "object-fail", - setupStorageFn: func(m *mock.Mock, name string) { - m.On("Delete", mock.Anything, name, mock.Anything, mock.Anything).Return(nil, false, errors.New("error")) + setupLegacyFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(nil, false, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "pods"}, "not-found")) + }, + setupStorageFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(nil, false, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "pods"}, "not-found")) }, wantErr: true, }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - l := (LegacyStorage)(nil) - s := (Storage)(nil) - m := &mock.Mock{} - - ls := legacyStoreMock{m, l} - us := storageMock{m, s} - - if tt.setupStorageFn != nil { - tt.setupStorageFn(m, tt.input) - } - - dw := NewDualWriter(Mode3, ls, us, p, kind) - - obj, _, err := dw.Delete(context.Background(), tt.input, func(ctx context.Context, obj runtime.Object) error { return nil }, &metav1.DeleteOptions{}) - - if tt.wantErr { - assert.Error(t, err) - return - } - - assert.Equal(t, obj, exampleObj) - assert.NotEqual(t, obj, anotherObj) - }) - } -} - -func TestMode1_DeleteFromLegacyStorage(t *testing.T) { - ctxCanceled, cancel := context.WithCancel(context.TODO()) - cancel() - - type testCase struct { - ctx *context.Context - setupLegacyFn func(m *mock.Mock, name string) - name string - input string - } - tests := - []testCase{ { - name: "Delete from legacy storage", + name: " object delete error", + input: "object-fail", setupLegacyFn: func(m *mock.Mock, input string) { - m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(exampleObj, false, nil) + m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(nil, false, errors.New("error")) }, - }, - { - name: "Delete from unified legacy works even if parent context is canceled", - ctx: &ctxCanceled, - setupLegacyFn: func(m *mock.Mock, input string) { - m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(exampleObj, false, nil) + setupStorageFn: func(m *mock.Mock, input string) { + m.On("Delete", mock.Anything, input, mock.Anything, mock.Anything).Return(nil, false, errors.New("error")) }, + wantErr: true, }, } @@ -429,62 +378,13 @@ func TestMode1_DeleteFromLegacyStorage(t *testing.T) { if tt.setupLegacyFn != nil { tt.setupLegacyFn(m, tt.input) } - - ctx := context.TODO() - if tt.ctx != nil { - ctx = *tt.ctx - } - - dw := NewDualWriter(Mode3, ls, us, p, kind) - - err := dw.(*DualWriterMode3).deleteFromLegacyStorage(ctx, exampleObj, tt.input, func(ctx context.Context, obj runtime.Object) error { return nil }, &metav1.DeleteOptions{}) - assert.NoError(t, err) - }) - } -} - -func TestMode3_DeleteCollection(t *testing.T) { - type testCase struct { - input *metav1.DeleteOptions - setupStorageFn func(m *mock.Mock, input *metav1.DeleteOptions) - name string - wantErr bool - } - tests := - []testCase{ - { - name: "deleting a collection in the unified store", - input: &metav1.DeleteOptions{TypeMeta: metav1.TypeMeta{Kind: "foo"}}, - setupStorageFn: func(m *mock.Mock, input *metav1.DeleteOptions) { - m.On("DeleteCollection", mock.Anything, mock.Anything, input, mock.Anything).Return(exampleObj, nil) - }, - }, - { - name: "error deleting a collection in the unified store", - input: &metav1.DeleteOptions{TypeMeta: metav1.TypeMeta{Kind: "fail"}}, - setupStorageFn: func(m *mock.Mock, input *metav1.DeleteOptions) { - m.On("DeleteCollection", mock.Anything, mock.Anything, input, mock.Anything).Return(nil, errors.New("error")) - }, - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - l := (LegacyStorage)(nil) - s := (Storage)(nil) - m := &mock.Mock{} - - ls := legacyStoreMock{m, l} - us := storageMock{m, s} - if tt.setupStorageFn != nil { tt.setupStorageFn(m, tt.input) } dw := NewDualWriter(Mode3, ls, us, p, kind) - obj, err := dw.DeleteCollection(context.Background(), func(ctx context.Context, obj runtime.Object) error { return nil }, tt.input, &metainternalversion.ListOptions{}) + obj, _, err := dw.Delete(context.Background(), tt.input, func(context.Context, runtime.Object) error { return nil }, &metav1.DeleteOptions{}) if tt.wantErr { assert.Error(t, err) @@ -497,32 +397,44 @@ func TestMode3_DeleteCollection(t *testing.T) { } } -func TestMode1_DeleteCollectionFromLegacyStorage(t *testing.T) { - ctxCanceled, cancel := context.WithCancel(context.TODO()) - cancel() - +func TestMode3_DeleteCollection(t *testing.T) { type testCase struct { - ctx *context.Context - setupLegacyFn func(m *mock.Mock) - name string - input *metav1.DeleteOptions + setupLegacyFn func(m *mock.Mock) + setupStorageFn func(m *mock.Mock) + name string + input string + wantErr bool } tests := []testCase{ { - name: "Delete Collection from legacy storage", - input: &metav1.DeleteOptions{TypeMeta: metav1.TypeMeta{Kind: "foo"}}, + name: "deleting a collection in both stores", + input: "foo", setupLegacyFn: func(m *mock.Mock) { - m.On("DeleteCollection", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, nil) + m.On("DeleteCollection", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleList, nil) + }, + setupStorageFn: func(m *mock.Mock) { + m.On("DeleteCollection", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleList, nil) }, }, { - name: "Delete Collection from legacy storage works even if parent context is canceled", - input: &metav1.DeleteOptions{TypeMeta: metav1.TypeMeta{Kind: "foo"}}, - ctx: &ctxCanceled, + name: "error deleting a collection in the storage when legacy store is successful", + input: "foo", setupLegacyFn: func(m *mock.Mock) { m.On("DeleteCollection", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, nil) }, + setupStorageFn: func(m *mock.Mock) { + m.On("DeleteCollection", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("error")) + }, + wantErr: true, + }, + { + name: "error deleting a collection legacy store", + input: "fail", + setupLegacyFn: func(m *mock.Mock) { + m.On("DeleteCollection", mock.Anything, mock.Anything, &metav1.DeleteOptions{TypeMeta: metav1.TypeMeta{Kind: "fail"}}, mock.Anything).Return(nil, errors.New("error")) + }, + wantErr: true, }, } @@ -538,22 +450,26 @@ func TestMode1_DeleteCollectionFromLegacyStorage(t *testing.T) { if tt.setupLegacyFn != nil { tt.setupLegacyFn(m) } - - ctx := context.TODO() - if tt.ctx != nil { - ctx = *tt.ctx + if tt.setupStorageFn != nil { + tt.setupStorageFn(m) } dw := NewDualWriter(Mode3, ls, us, p, kind) - err := dw.(*DualWriterMode3).deleteCollectionFromLegacyStorage(ctx, exampleObj, func(ctx context.Context, obj runtime.Object) error { return nil }, tt.input, &metainternalversion.ListOptions{}) - assert.NoError(t, err) + obj, err := dw.DeleteCollection(context.Background(), func(ctx context.Context, obj runtime.Object) error { return nil }, &metav1.DeleteOptions{TypeMeta: metav1.TypeMeta{Kind: tt.input}}, &metainternalversion.ListOptions{}) + + if tt.wantErr { + assert.Error(t, err) + return + } + assert.Equal(t, exampleList, obj) }) } } func TestMode3_Update(t *testing.T) { type testCase struct { + expectedObj runtime.Object setupLegacyFn func(m *mock.Mock, input string) setupStorageFn func(m *mock.Mock, input string) name string @@ -563,20 +479,32 @@ func TestMode3_Update(t *testing.T) { tests := []testCase{ { - name: "update an object in unified store", + name: "update an object in both stores", input: "foo", - setupStorageFn: func(m *mock.Mock, input string) { - m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, false, nil) - }, setupLegacyFn: func(m *mock.Mock, input string) { - m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, false, nil) + m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, false, nil).Once() }, + setupStorageFn: func(m *mock.Mock, input string) { + m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, false, nil).Once() + }, + expectedObj: exampleObj, }, { - name: "error updating an object in unified store", + name: "error updating legacy store", input: "object-fail", + setupLegacyFn: func(m *mock.Mock, input string) { + m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, false, errors.New("error")).Once() + }, + wantErr: true, + }, + { + name: "error updating unistore", + input: "object-fail", + setupLegacyFn: func(m *mock.Mock, input string) { + m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObj, false, nil).Once() + }, setupStorageFn: func(m *mock.Mock, input string) { - m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, false, errors.New("error")) + m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, false, errors.New("error")).Once() }, wantErr: true, }, @@ -607,76 +535,8 @@ func TestMode3_Update(t *testing.T) { return } - assert.NoError(t, err) - - assert.Equal(t, obj, exampleObj) - assert.NotEqual(t, obj, anotherObj) - }) - } -} - -func TestMode1_UpdateOnLegacyStorage(t *testing.T) { - ctxCanceled, cancel := context.WithCancel(context.TODO()) - cancel() - - type testCase struct { - ctx *context.Context - setupLegacyFn func(m *mock.Mock, input string) - setupGetFn func(m *mock.Mock, input string) - name string - input string - } - tests := - []testCase{ - { - name: "Update on legacy storage", - input: "foo", - setupLegacyFn: func(m *mock.Mock, input string) { - m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(anotherObj, false, nil) - }, - setupGetFn: func(m *mock.Mock, input string) { - m.On("Get", mock.Anything, input, mock.Anything).Return(exampleObj, nil) - }, - }, - { - name: "Update on legacy storage works even if parent context is canceled", - ctx: &ctxCanceled, - input: "foo", - setupLegacyFn: func(m *mock.Mock, input string) { - m.On("Update", mock.Anything, input, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(anotherObj, false, nil) - }, - setupGetFn: func(m *mock.Mock, input string) { - m.On("Get", mock.Anything, input, mock.Anything).Return(exampleObj, nil) - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - l := (LegacyStorage)(nil) - s := (Storage)(nil) - m := &mock.Mock{} - - ls := legacyStoreMock{m, l} - us := storageMock{m, s} - - if tt.setupLegacyFn != nil { - tt.setupLegacyFn(m, tt.input) - } - - if tt.setupGetFn != nil { - tt.setupGetFn(m, tt.input) - } - - ctx := context.TODO() - if tt.ctx != nil { - ctx = *tt.ctx - } - - dw := NewDualWriter(Mode3, ls, us, p, kind) - - err := dw.(*DualWriterMode3).updateOnLegacyStorageMode3(ctx, exampleObj, tt.input, updatedObjInfoObj{}, func(ctx context.Context, obj runtime.Object) error { return nil }, func(ctx context.Context, obj, old runtime.Object) error { return nil }, false, &metav1.UpdateOptions{}) - assert.NoError(t, err) + assert.Equal(t, tt.expectedObj, obj) + assert.NotEqual(t, anotherObj, obj) }) } }