From d17af78e799ee3f672e90a77436214d9769e5f23 Mon Sep 17 00:00:00 2001 From: Leonor Oliveira <9090754+leonorfmartins@users.noreply.github.com> Date: Mon, 15 Apr 2024 09:48:31 +0100 Subject: [PATCH] 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 | 1 + pkg/apiserver/rest/dualwriter_mode1.go | 9 +++++++ pkg/apiserver/rest/dualwriter_mode1_test.go | 7 +++++ pkg/apiserver/rest/dualwriter_mode2.go | 29 +++++++++++++++++---- pkg/apiserver/rest/dualwriter_mode2_test.go | 7 +++++ pkg/apiserver/rest/dualwriter_mode3.go | 24 +++++++++++++++++ pkg/apiserver/rest/dualwriter_mode3_test.go | 7 +++++ pkg/apiserver/rest/dualwriter_mode4.go | 4 +++ pkg/apiserver/rest/dualwriter_mode4_test.go | 7 +++++ pkg/apiserver/rest/spy_client.go | 2 +- 10 files changed, 91 insertions(+), 6 deletions(-) diff --git a/pkg/apiserver/rest/dualwriter.go b/pkg/apiserver/rest/dualwriter.go index 269fc60847b..4d1dff0c35a 100644 --- a/pkg/apiserver/rest/dualwriter.go +++ b/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 diff --git a/pkg/apiserver/rest/dualwriter_mode1.go b/pkg/apiserver/rest/dualwriter_mode1.go index 06f7edb823e..a36a01c8139 100644 --- a/pkg/apiserver/rest/dualwriter_mode1.go +++ b/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) +} diff --git a/pkg/apiserver/rest/dualwriter_mode1_test.go b/pkg/apiserver/rest/dualwriter_mode1_test.go index 9177e37caaa..cca3db15f36 100644 --- a/pkg/apiserver/rest/dualwriter_mode1_test.go +++ b/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")) } diff --git a/pkg/apiserver/rest/dualwriter_mode2.go b/pkg/apiserver/rest/dualwriter_mode2.go index 0073c3c8b32..85e6ac551ea 100644 --- a/pkg/apiserver/rest/dualwriter_mode2.go +++ b/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 +} diff --git a/pkg/apiserver/rest/dualwriter_mode2_test.go b/pkg/apiserver/rest/dualwriter_mode2_test.go index ce4cdc7f54e..eb3e640528d 100644 --- a/pkg/apiserver/rest/dualwriter_mode2_test.go +++ b/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")) } diff --git a/pkg/apiserver/rest/dualwriter_mode3.go b/pkg/apiserver/rest/dualwriter_mode3.go index a0132013c13..e4caf32347b 100644 --- a/pkg/apiserver/rest/dualwriter_mode3.go +++ b/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 +} diff --git a/pkg/apiserver/rest/dualwriter_mode3_test.go b/pkg/apiserver/rest/dualwriter_mode3_test.go index dbb481727f8..5e1ee9f09d0 100644 --- a/pkg/apiserver/rest/dualwriter_mode3_test.go +++ b/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")) } diff --git a/pkg/apiserver/rest/dualwriter_mode4.go b/pkg/apiserver/rest/dualwriter_mode4.go index 7e56970acb4..fd8472cf400 100644 --- a/pkg/apiserver/rest/dualwriter_mode4.go +++ b/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) +} diff --git a/pkg/apiserver/rest/dualwriter_mode4_test.go b/pkg/apiserver/rest/dualwriter_mode4_test.go index 2815741fb77..a051f80f688 100644 --- a/pkg/apiserver/rest/dualwriter_mode4_test.go +++ b/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")) } diff --git a/pkg/apiserver/rest/spy_client.go b/pkg/apiserver/rest/spy_client.go index af9cb808b43..3ba4f92b4f0 100644 --- a/pkg/apiserver/rest/spy_client.go +++ b/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 {