Add and fix tests for playlists in mode1 (#88543)

* Add and fix tests for playlists in mode1

* Make etcd tests pass mode1 for now

* Fix mode1 and add more tests for playlists in mode 1

* Remove repeated test

* Fix test setup

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() {

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)
 		})
 	}

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{