diff --git a/pkg/apiserver/rest/dualwriter_mode1.go b/pkg/apiserver/rest/dualwriter_mode1.go index 87d8cce81d6..15f79f5b711 100644 --- a/pkg/apiserver/rest/dualwriter_mode1.go +++ b/pkg/apiserver/rest/dualwriter_mode1.go @@ -5,6 +5,7 @@ import ( "errors" "time" + 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" @@ -49,20 +50,23 @@ func (d *DualWriterMode1) Create(ctx context.Context, original runtime.Object, c } d.recordLegacyDuration(false, mode1Str, options.Kind, method, startLegacy) + createdCopy := created.DeepCopyObject() + go func() { ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("storage create timeout")) - createdLegacy, err := enrichLegacyObject(original, created, true) - if err != nil { + defer cancel() + + objStorage, errEnrichObj := enrichLegacyObject(original, createdCopy, true) + if errEnrichObj != nil { cancel() } startStorage := time.Now() - defer cancel() - _, errObjectSt := d.Storage.Create(ctx, createdLegacy, createValidation, options) + _, errObjectSt := d.Storage.Create(ctx, objStorage, createValidation, options) d.recordStorageDuration(errObjectSt != nil, mode1Str, options.Kind, method, startStorage) }() - return created, nil + return created, err } // Get overrides the behavior of the generic DualWriter and reads only from LegacyStorage. @@ -135,7 +139,7 @@ func (d *DualWriterMode1) Delete(ctx context.Context, name string, deleteValidat d.recordStorageDuration(err != nil, mode1Str, options.Kind, method, startStorage) }() - return res, async, nil + return res, async, err } // DeleteCollection overrides the behavior of the generic DualWriter and deletes only from LegacyStorage. @@ -161,7 +165,7 @@ func (d *DualWriterMode1) DeleteCollection(ctx context.Context, deleteValidation d.recordStorageDuration(err != nil, mode1Str, options.Kind, method, startStorage) }() - return res, nil + return res, err } func (d *DualWriterMode1) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) { @@ -180,27 +184,34 @@ func (d *DualWriterMode1) Update(ctx context.Context, name string, objInfo rest. go func() { ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("storage update timeout")) - updated, err := objInfo.UpdatedObject(ctx, res) - if err != nil { - log.WithValues("object", updated).Error(err, "could not update or create object") - } + resCopy := res.DeepCopyObject() // get the object to be updated foundObj, err := d.Storage.Get(ctx, name, &metav1.GetOptions{}) if err != nil { - log.WithValues("object", foundObj).Error(err, "could not get object to update") + if !apierrors.IsNotFound(err) { + log.WithValues("object", foundObj).Error(err, "could not get object to update") + cancel() + } + log.Info("object not found for update, creating one") + } + + updated, err := objInfo.UpdatedObject(ctx, resCopy) + if err != nil { + log.WithValues("object", updated).Error(err, "could not update or create object") + cancel() } // if the object is found, create a new updateWrapper with the object found if foundObj != nil { - res, err := enrichLegacyObject(foundObj, res, false) + resCopy, err := enrichLegacyObject(foundObj, resCopy, false) if err != nil { log.Error(err, "could not enrich object") cancel() } objInfo = &updateWrapper{ upstream: objInfo, - updated: res, + updated: resCopy, } } startStorage := time.Now() @@ -209,7 +220,7 @@ func (d *DualWriterMode1) Update(ctx context.Context, name string, objInfo rest. d.recordStorageDuration(errObjectSt != nil, mode1Str, options.Kind, method, startStorage) }() - return res, async, nil + return res, async, err } func (d *DualWriterMode1) Destroy() { diff --git a/pkg/apiserver/rest/dualwriter_mode1_test.go b/pkg/apiserver/rest/dualwriter_mode1_test.go index 93d4e4a039c..d248e631373 100644 --- a/pkg/apiserver/rest/dualwriter_mode1_test.go +++ b/pkg/apiserver/rest/dualwriter_mode1_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "k8s.io/apimachinery/pkg/api/meta" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -14,6 +15,7 @@ import ( ) var exampleObj = &example.Pod{TypeMeta: metav1.TypeMeta{Kind: "foo"}, ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, Spec: example.PodSpec{}, Status: example.PodStatus{}} +var exampleObjNoRV = &example.Pod{TypeMeta: metav1.TypeMeta{Kind: "foo"}, ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: ""}, Spec: example.PodSpec{}, Status: example.PodStatus{}} var exampleObjDifferentRV = &example.Pod{TypeMeta: metav1.TypeMeta{Kind: "foo"}, ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "3"}, Spec: example.PodSpec{}, Status: example.PodStatus{}} var anotherObj = &example.Pod{TypeMeta: metav1.TypeMeta{Kind: "foo"}, ObjectMeta: metav1.ObjectMeta{Name: "bar", ResourceVersion: "2"}, Spec: example.PodSpec{}, Status: example.PodStatus{}} var failingObj = &example.Pod{TypeMeta: metav1.TypeMeta{Kind: "foo"}, ObjectMeta: metav1.ObjectMeta{Name: "object-fail", ResourceVersion: "2"}, Spec: example.PodSpec{}, Status: example.PodStatus{}} @@ -24,7 +26,7 @@ func TestMode1_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) + setupStorageFn func(m *mock.Mock) name string wantErr bool } @@ -36,8 +38,8 @@ func TestMode1_Create(t *testing.T) { setupLegacyFn: func(m *mock.Mock, input runtime.Object) { m.On("Create", mock.Anything, input, mock.Anything, mock.Anything).Return(exampleObj, nil) }, - setupStorageFn: func(m *mock.Mock, input runtime.Object) { - m.On("Create", mock.Anything, anotherObj, mock.Anything, mock.Anything).Return(anotherObj, nil) + setupStorageFn: func(m *mock.Mock) { + m.On("Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(exampleObjNoRV, nil) }, }, { @@ -63,7 +65,7 @@ func TestMode1_Create(t *testing.T) { tt.setupLegacyFn(m, tt.input) } if tt.setupStorageFn != nil { - tt.setupStorageFn(m, tt.input) + tt.setupStorageFn(m) } dw := NewDualWriter(Mode1, ls, us) @@ -75,9 +77,9 @@ func TestMode1_Create(t *testing.T) { return } - us.AssertNotCalled(t, "Create", context.Background(), tt.input, func(context.Context, runtime.Object) error { return nil }, &metav1.CreateOptions{}) - - assert.Equal(t, obj, exampleObj) + acc, err := meta.Accessor(obj) + assert.NoError(t, err) + assert.Equal(t, acc.GetResourceVersion(), "1") assert.NotEqual(t, obj, anotherObj) }) } diff --git a/pkg/tests/apis/playlist/playlist_test.go b/pkg/tests/apis/playlist/playlist_test.go index ca6b4e2fdf6..a8c359959df 100644 --- a/pkg/tests/apis/playlist/playlist_test.go +++ b/pkg/tests/apis/playlist/playlist_test.go @@ -101,21 +101,19 @@ func TestIntegrationPlaylist(t *testing.T) { })) }) - // #TODO Enable this test once we have fixed dual writing mode 1 behavior - // Do_CRUD_via_k8s_(and_check_that_legacy_api_still_works) breaks - // t.Run("with dual write (file, mode 1)", func(t *testing.T) { - // doPlaylistTests(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ - // AppModeProduction: true, - // DisableAnonymous: true, - // APIServerStorageType: "file", // write the files to disk - // EnableFeatureToggles: []string{ - // featuremgmt.FlagKubernetesPlaylists, // Required so that legacy calls are also written - // }, - // DualWriterDesiredModes: map[string]grafanarest.DualWriterMode{ - // playlistv0alpha1.GROUPRESOURCE: grafanarest.Mode1, - // }, - // })) - // }) + t.Run("with dual write (file, mode 1)", func(t *testing.T) { + doPlaylistTests(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: true, + DisableAnonymous: true, + APIServerStorageType: "file", // write the files to disk + EnableFeatureToggles: []string{ + featuremgmt.FlagKubernetesPlaylists, // Required so that legacy calls are also written + }, + DualWriterDesiredModes: map[string]grafanarest.DualWriterMode{ + playlistv0alpha1.GROUPRESOURCE: grafanarest.Mode1, + }, + })) + }) t.Run("with dual write (file, mode 2)", func(t *testing.T) { doPlaylistTests(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ @@ -146,22 +144,20 @@ func TestIntegrationPlaylist(t *testing.T) { })) }) - // #TODO Enable this test once we have fixed dual writing mode 1 behavior - // Do_CRUD_via_k8s_(and_check_that_legacy_api_still_works) breaks - // t.Run("with dual write (unified storage, mode 1)", func(t *testing.T) { - // doPlaylistTests(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ - // AppModeProduction: false, // required for unified storage - // DisableAnonymous: true, - // APIServerStorageType: "unified", // use the entity api tables - // EnableFeatureToggles: []string{ - // featuremgmt.FlagUnifiedStorage, - // featuremgmt.FlagKubernetesPlaylists, // Required so that legacy calls are also written - // }, - // DualWriterDesiredModes: map[string]grafanarest.DualWriterMode{ - // playlistv0alpha1.GROUPRESOURCE: grafanarest.Mode1, - // }, - // })) - // }) + t.Run("with dual write (unified storage, mode 1)", func(t *testing.T) { + doPlaylistTests(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: false, // required for unified storage + DisableAnonymous: true, + APIServerStorageType: "unified", // use the entity api tables + EnableFeatureToggles: []string{ + featuremgmt.FlagUnifiedStorage, + featuremgmt.FlagKubernetesPlaylists, // Required so that legacy calls are also written + }, + DualWriterDesiredModes: map[string]grafanarest.DualWriterMode{ + playlistv0alpha1.GROUPRESOURCE: grafanarest.Mode1, + }, + })) + }) t.Run("with dual write (unified storage, mode 2)", func(t *testing.T) { doPlaylistTests(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{