Storage: Add DualWriter improvements (#85968)

* Add Create tests
* Adjust mode 2 and mode 3 Get behavior

---------

Co-authored-by: Dan Cech <dcech@grafana.com>

pkg/apiserver/rest/dualwriter_mode1.go

@@ -31,7 +31,7 @@ func (d *DualWriterMode1) Create(ctx context.Context, obj runtime.Object, create
 	return legacy.Create(ctx, obj, createValidation, options)
 }
 
-// Get overrides the default behavior of the DualWriter and reads only to LegacyStorage.
+// Get overrides the behavior of the generic DualWriter and reads only from LegacyStorage.
 func (d *DualWriterMode1) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
 	return d.Legacy.Get(ctx, name, options)
 }

pkg/apiserver/rest/dualwriter_mode1_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
 const kind = "dummy"
@@ -19,8 +20,14 @@ func TestMode1(t *testing.T) {
 
 	dw := NewDualWriterMode1(lsSpy, sSpy)
 
+	// Create: it should use the Legacy Create implementation
+	_, err := dw.Create(context.Background(), &dummyObject{}, func(context.Context, runtime.Object) error { return nil }, &metav1.CreateOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Create"))
+	assert.Equal(t, 0, sSpy.Counts("Storage.Create"))
+
 	// Get: it should use the Legacy Get implementation
-	_, err := dw.Get(context.Background(), kind, &metav1.GetOptions{})
+	_, err = dw.Get(context.Background(), kind, &metav1.GetOptions{})
 	assert.NoError(t, err)
 	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Get"))
 	assert.Equal(t, 0, sSpy.Counts("Storage.Get"))

pkg/apiserver/rest/dualwriter_mode2.go

@@ -3,6 +3,7 @@ package rest
 import (
 	"context"
 
+	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"
@@ -55,8 +56,19 @@ func (d *DualWriterMode2) Create(ctx context.Context, obj runtime.Object, create
 	return rsp, err
 }
 
-// Get overrides the behavior of the generic DualWriter and retrieves an object from LegacyStorage.
+// Get overrides the behavior of the generic DualWriter.
+// It retrieves an object from Storage if possible, and if not it falls back to LegacyStorage.
 func (d *DualWriterMode2) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
+	s, err := d.Storage.Get(ctx, name, &metav1.GetOptions{})
+	if err == nil {
+		return s, err
+	}
+	if apierrors.IsNotFound(err) {
+		klog.Info("object not found in duplicate storage", "name", name)
+	} else {
+		klog.Error("unable to fetch object from duplicate storage", "error", err, "name", name)
+	}
+
 	return d.Legacy.Get(ctx, name, &metav1.GetOptions{})
 }
 

pkg/apiserver/rest/dualwriter_mode2_test.go

@@ -8,6 +8,7 @@ import (
 	"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"
 )
 
 func TestMode2(t *testing.T) {
@@ -18,19 +19,26 @@ func TestMode2(t *testing.T) {
 
 	dw := NewDualWriterMode2(lsSpy, sSpy)
 
-	// Get: it should use the Legacy Get implementation
+	// Create: it should use the Legacy Create implementation
-	_, err := dw.Get(context.Background(), kind, &metav1.GetOptions{})
+	_, err := dw.Create(context.Background(), &dummyObject{}, func(context.Context, runtime.Object) error { return nil }, &metav1.CreateOptions{})
 	assert.NoError(t, err)
-	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Get"))
+	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Create"))
-	assert.Equal(t, 0, sSpy.Counts("Storage.Get"))
+	assert.Equal(t, 1, sSpy.Counts("Storage.Create"))
+
+	// Get: it should read from Storage with LegacyStorage as a fallback
+	// #TODO: Currently only testing the happy path. Refactor testing to more easily test other cases.
+	_, err = dw.Get(context.Background(), kind, &metav1.GetOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Get"))
+	assert.Equal(t, 1, sSpy.Counts("Storage.Get"))
 
 	// List: it should use call both Legacy and Storage List methods
-	res, err := dw.List(context.Background(), &metainternalversion.ListOptions{})
+	l, err := dw.List(context.Background(), &metainternalversion.ListOptions{})
 	assert.NoError(t, err)
 	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.List"))
 	assert.Equal(t, 1, sSpy.Counts("Storage.List"))
 
-	resList, err := meta.ExtractList(res)
+	resList, err := meta.ExtractList(l)
 	assert.NoError(t, err)
 
 	expectedItems := map[string]string{

pkg/apiserver/rest/dualwriter_mode3.go

@@ -2,9 +2,7 @@ package rest
 
 import (
 	"context"
-	"fmt"
 
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/registry/rest"
@@ -40,23 +38,7 @@ func (d *DualWriterMode3) Create(ctx context.Context, obj runtime.Object, create
 	return created, nil
 }
 
-// Get overrides the default behavior of the Storage and retrieves an object from Unified Storage
+// Get overrides the behavior of the generic DualWriter and retrieves an object from Storage.
-// the object is still fetched from Legacy Storage if it's not found in Unified Storage
 func (d *DualWriterMode3) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
-	legacy, ok := d.Legacy.(rest.Getter)
+	return d.Storage.Get(ctx, name, &metav1.GetOptions{})
-	if !ok {
-		return nil, fmt.Errorf("legacy storage rest.Getter is missing")
-	}
-
-	s, err := d.Storage.Get(ctx, name, &metav1.GetOptions{})
-	if err == nil {
-		return s, err
-	}
-	if !apierrors.IsNotFound(err) {
-		return nil, err
-	}
-
-	klog.Info("object not found in unified storage. Getting it from legacy", "name", name)
-
-	return legacy.Get(ctx, name, &metav1.GetOptions{})
 }

pkg/apiserver/rest/dualwriter_mode3_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
 func TestMode3(t *testing.T) {
@@ -17,8 +18,14 @@ func TestMode3(t *testing.T) {
 
 	dw := NewDualWriterMode3(lsSpy, sSpy)
 
+	// Create: it should use the Legacy Create implementation
+	_, err := dw.Create(context.Background(), &dummyObject{}, func(context.Context, runtime.Object) error { return nil }, &metav1.CreateOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Create"))
+	assert.Equal(t, 1, sSpy.Counts("Storage.Create"))
+
 	// Get: it should use the Storage Get implementation
-	_, err := dw.Get(context.Background(), kind, &metav1.GetOptions{})
+	_, err = dw.Get(context.Background(), kind, &metav1.GetOptions{})
 	assert.NoError(t, err)
 	assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Get"))
 	assert.Equal(t, 1, sSpy.Counts("Storage.Get"))

pkg/apiserver/rest/dualwriter_mode4.go

@@ -25,7 +25,7 @@ func (d *DualWriterMode4) Create(ctx context.Context, obj runtime.Object, create
 	return d.Storage.Create(ctx, obj, createValidation, options)
 }
 
-// Get overrides the behavior of the generic DualWriter and retrieves an object from Unified Storage.
+// Get overrides the behavior of the generic DualWriter and retrieves an object from Storage.
 func (d *DualWriterMode4) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
 	return d.Storage.Get(ctx, name, &metav1.GetOptions{})
 }

pkg/apiserver/rest/dualwriter_mode4_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
 func TestMode4(t *testing.T) {
@@ -17,8 +18,14 @@ func TestMode4(t *testing.T) {
 
 	dw := NewDualWriterMode4(lsSpy, sSpy)
 
+	// Create: it should use the Legacy Create implementation
+	_, err := dw.Create(context.Background(), &dummyObject{}, func(context.Context, runtime.Object) error { return nil }, &metav1.CreateOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Create"))
+	assert.Equal(t, 1, sSpy.Counts("Storage.Create"))
+
 	// Get: it should use the Storage Get implementation
-	_, err := dw.Get(context.Background(), kind, &metav1.GetOptions{})
+	_, err = dw.Get(context.Background(), kind, &metav1.GetOptions{})
 	assert.NoError(t, err)
 	assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Get"))
 	assert.Equal(t, 1, sSpy.Counts("Storage.Get"))

pkg/apiserver/rest/spy_client.go

@@ -59,7 +59,7 @@ type spyLegacyStorageShim struct {
 func (c *spyStorageClient) Create(ctx context.Context, obj runtime.Object, valitation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
 	c.spy.record("Storage.Create")
 	klog.Info("method: Storage.Create")
-	return nil, nil
+	return &dummyObject{}, nil
 }
 
 func (c *spyStorageClient) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
@@ -137,7 +137,7 @@ func (c *spyLegacyStorageClient) Counts(method string) int {
 func (c *spyLegacyStorageClient) Create(ctx context.Context, obj runtime.Object, valitation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
 	c.spy.record("LegacyStorage.Create")
 	klog.Info("method: LegacyStorage.Create")
-	return nil, nil
+	return &dummyObject{}, nil
 }
 
 func (c *spyLegacyStorageClient) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {