Storage: dualwriter delete implementation (#86000)

* Add delete methods

* Remove duplicated const

* Add tests

* Lint

* Lint

* Remove duplicated test file

* Update pkg/apiserver/rest/dualwriter_mode2.go

Co-authored-by: Arati R. <33031346+suntala@users.noreply.github.com>

* Update pkg/apiserver/rest/dualwriter.go

Co-authored-by: Arati R. <33031346+suntala@users.noreply.github.com>

* Update pkg/apiserver/rest/dualwriter_mode2.go

Co-authored-by: Arati R. <33031346+suntala@users.noreply.github.com>

* Add missing dependency

* Return if object deletion goes wrong

* Add a more complete log

---------

Co-authored-by: Arati R. <33031346+suntala@users.noreply.github.com>

pkg/apiserver/rest/dualwriter.go

@@ -68,6 +68,7 @@ type DualWriter struct {
 
 var errDualWriterCreaterMissing = errors.New("legacy storage rest.Creater is missing")
 var errDualWriterListerMissing = errors.New("legacy storage rest.Lister is missing")
+var errDualWriterDeleterMissing = errors.New("legacy storage rest.GracefulDeleter is missing")
 
 type DualWriterMode int
 

pkg/apiserver/rest/dualwriter_mode1.go

@@ -45,3 +45,12 @@ func (d *DualWriterMode1) List(ctx context.Context, options *metainternalversion
 
 	return legacy.List(ctx, options)
 }
+
+func (d *DualWriterMode1) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
+	legacy, ok := d.Legacy.(rest.GracefulDeleter)
+	if !ok {
+		return nil, false, errDualWriterDeleterMissing
+	}
+
+	return legacy.Delete(ctx, name, deleteValidation, options)
+}

pkg/apiserver/rest/dualwriter_mode1_test.go

@@ -37,4 +37,11 @@ func TestMode1(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.List"))
 	assert.Equal(t, 0, sSpy.Counts("Storage.List"))
+
+	// Delete: it should use the Legacy Delete implementation
+	var deleteValidation = func(ctx context.Context, obj runtime.Object) error { return nil }
+	_, _, err = dw.Delete(context.Background(), kind, deleteValidation, &metav1.DeleteOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Delete"))
+	assert.Equal(t, 0, sSpy.Counts("Storage.Delete"))
 }

pkg/apiserver/rest/dualwriter_mode2.go

@@ -142,10 +142,29 @@ func enrichObject(orig, copy runtime.Object) (runtime.Object, error) {
 	}
 	accessorC.SetAnnotations(ac)
 
-	// #TODO set resource version and UID when required (Update for example)
-	// accessorC.SetResourceVersion(accessorO.GetResourceVersion())
-
-	// accessorC.SetUID(accessorO.GetUID())
-
 	return copy, nil
 }
+
+func (d *DualWriterMode2) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
+	legacy, ok := d.Legacy.(rest.GracefulDeleter)
+	if !ok {
+		return nil, false, errDualWriterDeleterMissing
+	}
+
+	deletedLS, async, err := legacy.Delete(ctx, name, deleteValidation, options)
+	if err != nil {
+		if !apierrors.IsNotFound(err) {
+			klog.FromContext(ctx).Error(err, "could not delete from legacy store", "mode", 2)
+			return deletedLS, async, err
+		}
+	}
+
+	_, _, errUS := d.Storage.Delete(ctx, name, deleteValidation, options)
+	if errUS != nil {
+		if !apierrors.IsNotFound(errUS) {
+			klog.FromContext(ctx).Error(errUS, "could not delete from duplicate storage", "mode", 2, "name", name)
+		}
+	}
+
+	return deletedLS, async, err
+}

pkg/apiserver/rest/dualwriter_mode2_test.go

@@ -61,4 +61,11 @@ func TestMode2(t *testing.T) {
 		assert.True(t, ok)
 		assert.Equal(t, k, v.Foo)
 	}
+
+	// Delete: it should use call both Legacy and Storage Delete methods
+	var deleteValidation = func(ctx context.Context, obj runtime.Object) error { return nil }
+	_, _, err = dw.Delete(context.Background(), kind, deleteValidation, &metav1.DeleteOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Delete"))
+	assert.Equal(t, 1, sSpy.Counts("Storage.Delete"))
 }

pkg/apiserver/rest/dualwriter_mode3.go

@@ -3,6 +3,7 @@ package rest
 import (
 	"context"
 
+	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"
@@ -42,3 +43,26 @@ func (d *DualWriterMode3) Create(ctx context.Context, obj runtime.Object, create
 func (d *DualWriterMode3) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
 	return d.Storage.Get(ctx, name, &metav1.GetOptions{})
 }
+
+func (d *DualWriterMode3) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
+	legacy, ok := d.Legacy.(rest.GracefulDeleter)
+	if !ok {
+		return nil, false, errDualWriterDeleterMissing
+	}
+
+	deleted, async, err := d.Storage.Delete(ctx, name, deleteValidation, options)
+	if err != nil {
+		if !apierrors.IsNotFound(err) {
+			klog.FromContext(ctx).Error(err, "could not delete from unified store", "mode", Mode3)
+		}
+	}
+
+	_, _, errLS := legacy.Delete(ctx, name, deleteValidation, options)
+	if errLS != nil {
+		if !apierrors.IsNotFound(errLS) {
+			klog.FromContext(ctx).Error(errLS, "could not delete from legacy store", "mode", Mode3)
+		}
+	}
+
+	return deleted, async, err
+}

pkg/apiserver/rest/dualwriter_mode3_test.go

@@ -35,4 +35,11 @@ func TestMode3(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.List"))
 	assert.Equal(t, 1, sSpy.Counts("Storage.List"))
+
+	// Delete: it should use call both Legacy and Storage Delete methods
+	var deleteValidation = func(ctx context.Context, obj runtime.Object) error { return nil }
+	_, _, err = dw.Delete(context.Background(), kind, deleteValidation, &metav1.DeleteOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Delete"))
+	assert.Equal(t, 1, sSpy.Counts("Storage.Delete"))
 }

pkg/apiserver/rest/dualwriter_mode4.go

@@ -29,3 +29,7 @@ func (d *DualWriterMode4) Create(ctx context.Context, obj runtime.Object, create
 func (d *DualWriterMode4) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
 	return d.Storage.Get(ctx, name, &metav1.GetOptions{})
 }
+
+func (d *DualWriterMode4) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
+	return d.Storage.Delete(ctx, name, deleteValidation, options)
+}

pkg/apiserver/rest/dualwriter_mode4_test.go

@@ -35,4 +35,11 @@ func TestMode4(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.List"))
 	assert.Equal(t, 1, sSpy.Counts("Storage.List"))
+
+	// Delete: it should use call Storage Delete method
+	var deleteValidation = func(ctx context.Context, obj runtime.Object) error { return nil }
+	_, _, err = dw.Delete(context.Background(), kind, deleteValidation, &metav1.DeleteOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Delete"))
+	assert.Equal(t, 1, sSpy.Counts("Storage.Delete"))
 }

pkg/apiserver/rest/spy_client.go

@@ -192,8 +192,8 @@ type dummyList struct {
 
 type dummyObject struct {
 	metav1.TypeMeta   `json:",inline"`
-	metav1.ObjectMeta `json:"metadata,omitempty"`
 	Foo               string
+	metav1.ObjectMeta `json:"metadata,omitempty"`
 }
 
 func (d *dummyList) GetObjectKind() schema.ObjectKind {