From c2d61896f45967cdea15072b7a12faeb635b5dc5 Mon Sep 17 00:00:00 2001 From: wojtekt Date: Tue, 3 Nov 2020 13:21:00 +0100 Subject: [PATCH] Add suggestion to storage interface Delete method --- pkg/controlplane/reconcilers/lease.go | 2 +- pkg/registry/core/pod/storage/storage_test.go | 2 +- .../apiserver/pkg/registry/generic/registry/dryrun.go | 4 ++-- .../pkg/registry/generic/registry/dryrun_test.go | 10 +++++----- .../apiserver/pkg/registry/generic/registry/store.go | 4 ++-- .../src/k8s.io/apiserver/pkg/storage/cacher/cacher.go | 6 ++++-- .../pkg/storage/cacher/cacher_whitebox_test.go | 2 +- .../src/k8s.io/apiserver/pkg/storage/etcd3/store.go | 10 +++++++--- .../k8s.io/apiserver/pkg/storage/etcd3/store_test.go | 8 ++++---- .../k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go | 4 ++-- staging/src/k8s.io/apiserver/pkg/storage/interfaces.go | 7 ++++++- .../k8s.io/apiserver/pkg/storage/tests/cacher_test.go | 4 ++-- 12 files changed, 37 insertions(+), 26 deletions(-) diff --git a/pkg/controlplane/reconcilers/lease.go b/pkg/controlplane/reconcilers/lease.go index 4d794ff74b7..ca3e1dee04b 100644 --- a/pkg/controlplane/reconcilers/lease.go +++ b/pkg/controlplane/reconcilers/lease.go @@ -113,7 +113,7 @@ func (s *storageLeases) UpdateLease(ip string) error { // RemoveLease removes the lease on a master IP in storage func (s *storageLeases) RemoveLease(ip string) error { - return s.storage.Delete(apirequest.NewDefaultContext(), s.baseKey+"/"+ip, &corev1.Endpoints{}, nil, rest.ValidateAllObjectFunc) + return s.storage.Delete(apirequest.NewDefaultContext(), s.baseKey+"/"+ip, &corev1.Endpoints{}, nil, rest.ValidateAllObjectFunc, nil) } // NewLeases creates a new etcd-based Leases implementation. diff --git a/pkg/registry/core/pod/storage/storage_test.go b/pkg/registry/core/pod/storage/storage_test.go index 978b467d794..0ef8198c89c 100644 --- a/pkg/registry/core/pod/storage/storage_test.go +++ b/pkg/registry/core/pod/storage/storage_test.go @@ -159,7 +159,7 @@ type FailDeletionStorage struct { Called *bool } -func (f FailDeletionStorage) Delete(ctx context.Context, key string, out runtime.Object, precondition *apiserverstorage.Preconditions, _ apiserverstorage.ValidateObjectFunc) error { +func (f FailDeletionStorage) Delete(_ context.Context, key string, _ runtime.Object, _ *apiserverstorage.Preconditions, _ apiserverstorage.ValidateObjectFunc, _ runtime.Object) error { *f.Called = true return apiserverstorage.NewKeyNotFoundError(key, 0) } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go index 2f184c50e01..469893cd1ef 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go @@ -43,7 +43,7 @@ func (s *DryRunnableStorage) Create(ctx context.Context, key string, obj, out ru return s.Storage.Create(ctx, key, obj, out, ttl) } -func (s *DryRunnableStorage) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, deleteValidation storage.ValidateObjectFunc, dryRun bool) error { +func (s *DryRunnableStorage) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, deleteValidation storage.ValidateObjectFunc, dryRun bool, cachedExistingObject runtime.Object) error { if dryRun { if err := s.Storage.Get(ctx, key, storage.GetOptions{}, out); err != nil { return err @@ -53,7 +53,7 @@ func (s *DryRunnableStorage) Delete(ctx context.Context, key string, out runtime } return deleteValidation(ctx, out) } - return s.Storage.Delete(ctx, key, out, preconditions, deleteValidation) + return s.Storage.Delete(ctx, key, out, preconditions, deleteValidation, cachedExistingObject) } func (s *DryRunnableStorage) Watch(ctx context.Context, key string, opts storage.ListOptions) (watch.Interface, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go index fc57d73fc27..ccf97f64ac1 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go @@ -234,7 +234,7 @@ func TestDryRunDeleteDoesntDelete(t *testing.T) { t.Fatalf("Failed to create new object: %v", err) } - err = s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true) + err = s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true, nil) if err != nil { t.Fatalf("Failed to dry-run delete the object: %v", err) } @@ -250,7 +250,7 @@ func TestDryRunDeleteMissingObjectFails(t *testing.T) { defer destroy() out := UnstructuredOrDie(`{}`) - err := s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true) + err := s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true, nil) if e, ok := err.(*storage.StorageError); !ok || e.Code != storage.ErrCodeKeyNotFound { t.Errorf("Expected key to be not found, error: %v", err) } @@ -270,7 +270,7 @@ func TestDryRunDeleteReturnsObject(t *testing.T) { out = UnstructuredOrDie(`{}`) expected := UnstructuredOrDie(`{"kind": "Pod", "metadata": {"resourceVersion": "2"}}`) - err = s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true) + err = s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true, nil) if err != nil { t.Fatalf("Failed to delete with valid precondition: %v", err) } @@ -293,12 +293,12 @@ func TestDryRunDeletePreconditions(t *testing.T) { wrongID := types.UID("wrong-uid") myID := types.UID("my-uid") - err = s.Delete(context.Background(), "key", out, &storage.Preconditions{UID: &wrongID}, rest.ValidateAllObjectFunc, true) + err = s.Delete(context.Background(), "key", out, &storage.Preconditions{UID: &wrongID}, rest.ValidateAllObjectFunc, true, nil) if e, ok := err.(*storage.StorageError); !ok || e.Code != storage.ErrCodeInvalidObj { t.Errorf("Expected invalid object, error: %v", err) } - err = s.Delete(context.Background(), "key", out, &storage.Preconditions{UID: &myID}, rest.ValidateAllObjectFunc, true) + err = s.Delete(context.Background(), "key", out, &storage.Preconditions{UID: &myID}, rest.ValidateAllObjectFunc, true, nil) if err != nil { t.Fatalf("Failed to delete with valid precondition: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index 7c2f4c390eb..bbc167d3996 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -428,7 +428,7 @@ func (e *Store) deleteWithoutFinalizers(ctx context.Context, name, key string, o out := e.NewFunc() klog.V(6).Infof("going to delete %s from registry, triggered by update", name) // Using the rest.ValidateAllObjectFunc because the request is an UPDATE request and has already passed the admission for the UPDATE verb. - if err := e.Storage.Delete(ctx, key, out, preconditions, rest.ValidateAllObjectFunc, dryRun); err != nil { + if err := e.Storage.Delete(ctx, key, out, preconditions, rest.ValidateAllObjectFunc, dryRun, nil); err != nil { // Deletion is racy, i.e., there could be multiple update // requests to remove all finalizers from the object, so we // ignore the NotFound error. @@ -963,7 +963,7 @@ func (e *Store) Delete(ctx context.Context, name string, deleteValidation rest.V // delete immediately, or no graceful deletion supported klog.V(6).Infof("going to delete %s from registry: ", name) out = e.NewFunc() - if err := e.Storage.Delete(ctx, key, out, &preconditions, storage.ValidateObjectFunc(deleteValidation), dryrun.IsDryRun(options.DryRun)); err != nil { + if err := e.Storage.Delete(ctx, key, out, &preconditions, storage.ValidateObjectFunc(deleteValidation), dryrun.IsDryRun(options.DryRun), nil); err != nil { // Please refer to the place where we set ignoreNotFound for the reason // why we ignore the NotFound error . if storage.IsNotFound(err) && ignoreNotFound && lastExisting != nil { diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go index 45951776754..8609faef028 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go @@ -431,8 +431,10 @@ func (c *Cacher) Create(ctx context.Context, key string, obj, out runtime.Object } // Delete implements storage.Interface. -func (c *Cacher) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, validateDeletion storage.ValidateObjectFunc) error { - return c.storage.Delete(ctx, key, out, preconditions, validateDeletion) +func (c *Cacher) Delete( + ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, + validateDeletion storage.ValidateObjectFunc, cachedExistingObject runtime.Object) error { + return c.storage.Delete(ctx, key, out, preconditions, validateDeletion, cachedExistingObject) } // Watch implements storage.Interface. diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go index e42c93b5b57..3eacd186f85 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go @@ -299,7 +299,7 @@ func (d *dummyStorage) Versioner() storage.Versioner { return nil } func (d *dummyStorage) Create(_ context.Context, _ string, _, _ runtime.Object, _ uint64) error { return fmt.Errorf("unimplemented") } -func (d *dummyStorage) Delete(_ context.Context, _ string, _ runtime.Object, _ *storage.Preconditions, _ storage.ValidateObjectFunc) error { +func (d *dummyStorage) Delete(_ context.Context, _ string, _ runtime.Object, _ *storage.Preconditions, _ storage.ValidateObjectFunc, _ runtime.Object) error { return fmt.Errorf("unimplemented") } func (d *dummyStorage) Watch(_ context.Context, _ string, _ storage.ListOptions) (watch.Interface, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go index 0cff6b3fc9d..34b1b5c21d0 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -185,16 +185,20 @@ func (s *store) Create(ctx context.Context, key string, obj, out runtime.Object, } // Delete implements storage.Interface.Delete. -func (s *store) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, validateDeletion storage.ValidateObjectFunc) error { +func (s *store) Delete( + ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, + validateDeletion storage.ValidateObjectFunc, cachedExistingObject runtime.Object) error { v, err := conversion.EnforcePtr(out) if err != nil { return fmt.Errorf("unable to convert output object to pointer: %v", err) } key = path.Join(s.pathPrefix, key) - return s.conditionalDelete(ctx, key, out, v, preconditions, validateDeletion) + return s.conditionalDelete(ctx, key, out, v, preconditions, validateDeletion, cachedExistingObject) } -func (s *store) conditionalDelete(ctx context.Context, key string, out runtime.Object, v reflect.Value, preconditions *storage.Preconditions, validateDeletion storage.ValidateObjectFunc) error { +func (s *store) conditionalDelete( + ctx context.Context, key string, out runtime.Object, v reflect.Value, preconditions *storage.Preconditions, + validateDeletion storage.ValidateObjectFunc, cachedExistingObject runtime.Object) error { startTime := time.Now() getResp, err := s.client.KV.Get(ctx, key) metrics.RecordEtcdRequestLatency("get", getTypeName(out), startTime) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go index 8496e03a7fd..00bf218e008 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go @@ -323,7 +323,7 @@ func TestUnconditionalDelete(t *testing.T) { for i, tt := range tests { out := &example.Pod{} // reset - err := store.Delete(ctx, tt.key, out, nil, storage.ValidateAllObjectFunc) + err := store.Delete(ctx, tt.key, out, nil, storage.ValidateAllObjectFunc, nil) if tt.expectNotFoundErr { if err == nil || !storage.IsNotFound(err) { t.Errorf("#%d: expecting not found error, but get: %s", i, err) @@ -357,7 +357,7 @@ func TestConditionalDelete(t *testing.T) { for i, tt := range tests { out := &example.Pod{} - err := store.Delete(ctx, key, out, tt.precondition, storage.ValidateAllObjectFunc) + err := store.Delete(ctx, key, out, tt.precondition, storage.ValidateAllObjectFunc, nil) if tt.expectInvalidObjErr { if err == nil || !storage.IsInvalidObj(err) { t.Errorf("#%d: expecting invalid UID error, but get: %s", i, err) @@ -887,7 +887,7 @@ func TestTransformationFailure(t *testing.T) { } // Delete fails with internal error. - if err := store.Delete(ctx, preset[1].key, &example.Pod{}, nil, storage.ValidateAllObjectFunc); !storage.IsInternalError(err) { + if err := store.Delete(ctx, preset[1].key, &example.Pod{}, nil, storage.ValidateAllObjectFunc, nil); !storage.IsInternalError(err) { t.Errorf("Unexpected error: %v", err) } if err := store.Get(ctx, preset[1].key, storage.GetOptions{}, &example.Pod{}); !storage.IsInternalError(err) { @@ -1833,7 +1833,7 @@ func testPropogateStoreWithKey(ctx context.Context, t *testing.T, store *store, if err != nil { panic("unable to convert output object to pointer") } - err = store.conditionalDelete(ctx, key, &example.Pod{}, v, nil, storage.ValidateAllObjectFunc) + err = store.conditionalDelete(ctx, key, &example.Pod{}, v, nil, storage.ValidateAllObjectFunc, nil) if err != nil && !storage.IsNotFound(err) { t.Fatalf("Cleanup failed: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go index 1f614c2033f..993ecec7215 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go @@ -135,7 +135,7 @@ func TestDeleteTriggerWatch(t *testing.T) { if err != nil { t.Fatalf("Watch failed: %v", err) } - if err := store.Delete(ctx, key, &example.Pod{}, nil, storage.ValidateAllObjectFunc); err != nil { + if err := store.Delete(ctx, key, &example.Pod{}, nil, storage.ValidateAllObjectFunc, nil); err != nil { t.Fatalf("Delete failed: %v", err) } testCheckEventType(t, watch.Deleted, w) @@ -295,7 +295,7 @@ func TestWatchDeleteEventObjectHaveLatestRV(t *testing.T) { } etcdW := cluster.RandClient().Watch(ctx, "/", clientv3.WithPrefix()) - if err := store.Delete(ctx, key, &example.Pod{}, &storage.Preconditions{}, storage.ValidateAllObjectFunc); err != nil { + if err := store.Delete(ctx, key, &example.Pod{}, &storage.Preconditions{}, storage.ValidateAllObjectFunc, nil); err != nil { t.Fatalf("Delete failed: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go index 01f9132f55d..7de3bf9363a 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go @@ -167,7 +167,12 @@ type Interface interface { // Delete removes the specified key and returns the value that existed at that spot. // If key didn't exist, it will return NotFound storage error. - Delete(ctx context.Context, key string, out runtime.Object, preconditions *Preconditions, validateDeletion ValidateObjectFunc) error + // If 'cachedExistingObject' is non-nil, it can be used as a suggestion about the + // current version of the object to avoid read operation from storage to get it. + // However, the implementations have to retry in case suggestion is stale. + Delete( + ctx context.Context, key string, out runtime.Object, preconditions *Preconditions, + validateDeletion ValidateObjectFunc, cachedExistingObject runtime.Object) error // Watch begins watching the specified key. Events are decoded into API objects, // and any items selected by 'p' are sent down to returned watch.Interface. diff --git a/staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go b/staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go index fbc1d27455e..3e4fa3ec3ae 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go @@ -282,7 +282,7 @@ func TestList(t *testing.T) { updatePod(t, etcdStorage, podFooNS2, nil) deleted := example.Pod{} - if err := etcdStorage.Delete(context.TODO(), "pods/ns/bar", &deleted, nil, storage.ValidateAllObjectFunc); err != nil { + if err := etcdStorage.Delete(context.TODO(), "pods/ns/bar", &deleted, nil, storage.ValidateAllObjectFunc, nil); err != nil { t.Errorf("Unexpected error: %v", err) } @@ -575,7 +575,7 @@ func TestFiltering(t *testing.T) { _ = updatePod(t, etcdStorage, podFooPrime, fooUnfiltered) deleted := example.Pod{} - if err := etcdStorage.Delete(context.TODO(), "pods/ns/foo", &deleted, nil, storage.ValidateAllObjectFunc); err != nil { + if err := etcdStorage.Delete(context.TODO(), "pods/ns/foo", &deleted, nil, storage.ValidateAllObjectFunc, nil); err != nil { t.Errorf("Unexpected error: %v", err) }