From 8b98305858b107369f2c9b9fd8ef1c5b0da078c0 Mon Sep 17 00:00:00 2001 From: wojtekt Date: Mon, 2 Nov 2020 15:52:51 +0100 Subject: [PATCH] Remove variadic argument from storage interface --- pkg/controlplane/reconcilers/lease.go | 2 +- pkg/registry/apps/deployment/storage/storage.go | 2 +- pkg/registry/core/namespace/storage/storage.go | 1 + pkg/registry/core/pod/storage/storage.go | 2 +- .../core/service/allocator/storage/storage.go | 2 ++ .../pkg/registry/customresourcedefinition/etcd.go | 1 + .../pkg/registry/generic/registry/dryrun.go | 4 ++-- .../pkg/registry/generic/registry/dryrun_test.go | 10 +++++----- .../pkg/registry/generic/registry/store.go | 3 ++- .../pkg/registry/generic/registry/store_test.go | 2 +- .../k8s.io/apiserver/pkg/storage/cacher/cacher.go | 4 ++-- .../pkg/storage/cacher/cacher_whitebox_test.go | 2 +- .../src/k8s.io/apiserver/pkg/storage/etcd3/store.go | 6 +++--- .../apiserver/pkg/storage/etcd3/store_test.go | 13 +++++++------ .../apiserver/pkg/storage/etcd3/watcher_test.go | 10 +++++----- .../src/k8s.io/apiserver/pkg/storage/interfaces.go | 8 ++++---- .../apiserver/pkg/storage/tests/cacher_test.go | 2 +- 17 files changed, 40 insertions(+), 34 deletions(-) diff --git a/pkg/controlplane/reconcilers/lease.go b/pkg/controlplane/reconcilers/lease.go index f7e0ba50f19..4d794ff74b7 100644 --- a/pkg/controlplane/reconcilers/lease.go +++ b/pkg/controlplane/reconcilers/lease.go @@ -108,7 +108,7 @@ func (s *storageLeases) UpdateLease(ip string) error { klog.V(6).Infof("Resetting TTL on master IP %q listed in storage to %v", ip, leaseTime) return existing, &leaseTime, nil - }) + }, nil) } // RemoveLease removes the lease on a master IP in storage diff --git a/pkg/registry/apps/deployment/storage/storage.go b/pkg/registry/apps/deployment/storage/storage.go index c310d3b6bb1..8bbe4690cea 100644 --- a/pkg/registry/apps/deployment/storage/storage.go +++ b/pkg/registry/apps/deployment/storage/storage.go @@ -231,7 +231,7 @@ func (r *RollbackREST) setDeploymentRollback(ctx context.Context, deploymentID s d.Spec.RollbackTo = config finalDeployment = d return d, nil - }), dryRun) + }), dryRun, nil) return finalDeployment, err } diff --git a/pkg/registry/core/namespace/storage/storage.go b/pkg/registry/core/namespace/storage/storage.go index 662fa664a38..91c923533a3 100644 --- a/pkg/registry/core/namespace/storage/storage.go +++ b/pkg/registry/core/namespace/storage/storage.go @@ -221,6 +221,7 @@ func (r *REST) Delete(ctx context.Context, name string, deleteValidation rest.Va return existingNamespace, nil }), dryrun.IsDryRun(options.DryRun), + nil, ) if err != nil { diff --git a/pkg/registry/core/pod/storage/storage.go b/pkg/registry/core/pod/storage/storage.go index 15ef375b458..e82bb4c5db9 100644 --- a/pkg/registry/core/pod/storage/storage.go +++ b/pkg/registry/core/pod/storage/storage.go @@ -214,7 +214,7 @@ func (r *BindingREST) setPodHostAndAnnotations(ctx context.Context, podID, oldMa }) finalPod = pod return pod, nil - }), dryRun) + }), dryRun, nil) return finalPod, err } diff --git a/pkg/registry/core/service/allocator/storage/storage.go b/pkg/registry/core/service/allocator/storage/storage.go index 08f6c188db7..a1d47927975 100644 --- a/pkg/registry/core/service/allocator/storage/storage.go +++ b/pkg/registry/core/service/allocator/storage/storage.go @@ -172,6 +172,7 @@ func (e *Etcd) tryUpdate(fn func() error) error { existing.Data = data return existing, nil }), + nil, ) return storeerr.InterpretUpdateError(err, e.resource, "") } @@ -207,6 +208,7 @@ func (e *Etcd) CreateOrUpdate(snapshot *api.RangeAllocation) error { last = snapshot.ResourceVersion return snapshot, nil }), + nil, ) if err != nil { return storeerr.InterpretUpdateError(err, e.resource, "") diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go index 06a3fac6c9a..aa1f264297f 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go @@ -145,6 +145,7 @@ func (r *REST) Delete(ctx context.Context, name string, deleteValidation rest.Va return existingCRD, nil }), dryrun.IsDryRun(options.DryRun), + nil, ) if err != nil { 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 65a533102f9..2f184c50e01 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 @@ -78,7 +78,7 @@ func (s *DryRunnableStorage) List(ctx context.Context, key string, opts storage. func (s *DryRunnableStorage) GuaranteedUpdate( ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool, - preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, dryRun bool, suggestion ...runtime.Object) error { + preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, dryRun bool, suggestion runtime.Object) error { if dryRun { err := s.Storage.Get(ctx, key, storage.GetOptions{IgnoreNotFound: ignoreNotFound}, ptrToType) if err != nil { @@ -98,7 +98,7 @@ func (s *DryRunnableStorage) GuaranteedUpdate( } return s.copyInto(out, ptrToType) } - return s.Storage.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, suggestion...) + return s.Storage.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, suggestion) } func (s *DryRunnableStorage) Count(key string) (int64, 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 25f8e96f5da..fc57d73fc27 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 @@ -121,7 +121,7 @@ func TestDryRunUpdateMissingObjectFails(t *testing.T) { return input, nil, errors.New("UpdateFunction shouldn't be called") } - err := s.GuaranteedUpdate(context.Background(), "key", obj, false, nil, updateFunc, true) + err := s.GuaranteedUpdate(context.Background(), "key", obj, false, nil, updateFunc, true, nil) if e, ok := err.(*storage.StorageError); !ok || e.Code != storage.ErrCodeKeyNotFound { t.Errorf("Expected key to be not found, error: %v", err) } @@ -148,12 +148,12 @@ func TestDryRunUpdatePreconditions(t *testing.T) { } wrongID := types.UID("wrong-uid") myID := types.UID("my-uid") - err = s.GuaranteedUpdate(context.Background(), "key", obj, false, &storage.Preconditions{UID: &wrongID}, updateFunc, true) + err = s.GuaranteedUpdate(context.Background(), "key", obj, false, &storage.Preconditions{UID: &wrongID}, updateFunc, true, nil) if e, ok := err.(*storage.StorageError); !ok || e.Code != storage.ErrCodeInvalidObj { t.Errorf("Expected invalid object, error: %v", err) } - err = s.GuaranteedUpdate(context.Background(), "key", obj, false, &storage.Preconditions{UID: &myID}, updateFunc, true) + err = s.GuaranteedUpdate(context.Background(), "key", obj, false, &storage.Preconditions{UID: &myID}, updateFunc, true, nil) if err != nil { t.Fatalf("Failed to update with valid precondition: %v", err) } @@ -180,7 +180,7 @@ func TestDryRunUpdateDoesntUpdate(t *testing.T) { return u, nil, nil } - err = s.GuaranteedUpdate(context.Background(), "key", obj, false, nil, updateFunc, true) + err = s.GuaranteedUpdate(context.Background(), "key", obj, false, nil, updateFunc, true, nil) if err != nil { t.Fatalf("Failed to dry-run update: %v", err) } @@ -212,7 +212,7 @@ func TestDryRunUpdateReturnsObject(t *testing.T) { return u, nil, nil } - err = s.GuaranteedUpdate(context.Background(), "key", obj, false, nil, updateFunc, true) + err = s.GuaranteedUpdate(context.Background(), "key", obj, false, nil, updateFunc, true, nil) if err != nil { t.Fatalf("Failed to dry-run update: %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 2e0f7d9ecd4..7c2f4c390eb 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 @@ -568,7 +568,7 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj return obj, &ttl, nil } return obj, nil, nil - }, dryrun.IsDryRun(options.DryRun)) + }, dryrun.IsDryRun(options.DryRun), nil) if err != nil { // delete the object @@ -855,6 +855,7 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx context.Context, name return existing, nil }), dryrun.IsDryRun(options.DryRun), + nil, ) switch err { case nil: diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go index d177d28972f..cf679d066a8 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go @@ -1926,7 +1926,7 @@ type staleGuaranteedUpdateStorage struct { // GuaranteedUpdate overwrites the method with one that always suggests the cachedObj. func (s *staleGuaranteedUpdateStorage) GuaranteedUpdate( ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool, - preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, _ ...runtime.Object) error { + preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, _ runtime.Object) error { return s.Interface.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, s.cachedObj) } 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 82a8f3065ba..0c3967dd76e 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go @@ -730,7 +730,7 @@ func (c *Cacher) List(ctx context.Context, key string, opts storage.ListOptions, // GuaranteedUpdate implements storage.Interface. func (c *Cacher) GuaranteedUpdate( ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool, - preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, _ ...runtime.Object) error { + preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, _ runtime.Object) error { // Ignore the suggestion and try to pass down the current version of the object // read from cache. if elem, exists, err := c.watchCache.GetByKey(key); err != nil { @@ -740,7 +740,7 @@ func (c *Cacher) GuaranteedUpdate( return c.storage.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, currObj) } // If we couldn't get the object, fallback to no-suggestion. - return c.storage.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate) + return c.storage.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, nil) } // Count 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 ec5ff109eb8..bb78ea81dc7 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 @@ -319,7 +319,7 @@ func (d *dummyStorage) List(_ context.Context, _ string, _ storage.ListOptions, podList.ListMeta = metav1.ListMeta{ResourceVersion: "100"} return d.err } -func (d *dummyStorage) GuaranteedUpdate(_ context.Context, _ string, _ runtime.Object, _ bool, _ *storage.Preconditions, _ storage.UpdateFunc, _ ...runtime.Object) error { +func (d *dummyStorage) GuaranteedUpdate(_ context.Context, _ string, _ runtime.Object, _ bool, _ *storage.Preconditions, _ storage.UpdateFunc, _ runtime.Object) error { return fmt.Errorf("unimplemented") } func (d *dummyStorage) Count(_ string) (int64, 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 a7307f46f30..0cff6b3fc9d 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -238,7 +238,7 @@ func (s *store) conditionalDelete(ctx context.Context, key string, out runtime.O // GuaranteedUpdate implements storage.Interface.GuaranteedUpdate. func (s *store) GuaranteedUpdate( ctx context.Context, key string, out runtime.Object, ignoreNotFound bool, - preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, suggestion ...runtime.Object) error { + preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, suggestion runtime.Object) error { trace := utiltrace.New("GuaranteedUpdate etcd3", utiltrace.Field{"type", getTypeName(out)}) defer trace.LogIfLong(500 * time.Millisecond) @@ -260,8 +260,8 @@ func (s *store) GuaranteedUpdate( var origState *objState var mustCheckData bool - if len(suggestion) == 1 && suggestion[0] != nil { - origState, err = s.getStateFromObject(suggestion[0]) + if suggestion != nil { + origState, err = s.getStateFromObject(suggestion) if err != nil { return err } 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 e48462a8e39..8496e03a7fd 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 @@ -204,7 +204,7 @@ func TestGet(t *testing.T) { func(_ runtime.Object, _ storage.ResponseMeta) (runtime.Object, *uint64, error) { ttl := uint64(1) return updateObj, &ttl, nil - }) + }, nil) if err != nil { t.Fatalf("Update failed: %v", err) } @@ -592,7 +592,7 @@ func TestGuaranteedUpdate(t *testing.T) { } pod.Name = name return &pod, nil - })) + }), nil) store.transformer = originalTransformer if tt.expectNotFoundErr { @@ -645,7 +645,7 @@ func TestGuaranteedUpdateWithTTL(t *testing.T) { func(_ runtime.Object, _ storage.ResponseMeta) (runtime.Object, *uint64, error) { ttl := uint64(1) return input, &ttl, nil - }) + }, nil) if err != nil { t.Fatalf("Create failed: %v", err) } @@ -742,7 +742,7 @@ func TestGuaranteedUpdateWithConflict(t *testing.T) { pod.Name = "foo-1" secondToEnter.Wait() return pod, nil - })) + }), nil) firstToFinish.Done() errChan <- err }() @@ -758,7 +758,7 @@ func TestGuaranteedUpdateWithConflict(t *testing.T) { pod := obj.(*example.Pod) pod.Name = "foo-2" return pod, nil - })) + }), nil) if err != nil { t.Fatalf("Second GuaranteedUpdate error %#v", err) } @@ -784,6 +784,7 @@ func TestGuaranteedUpdateWithSuggestionAndConflict(t *testing.T) { pod.Name = "foo-2" return pod, nil }), + nil, ) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -875,7 +876,7 @@ func TestTransformationFailure(t *testing.T) { // GuaranteedUpdate without suggestion should return an error if err := store.GuaranteedUpdate(ctx, preset[1].key, &example.Pod{}, false, nil, func(input runtime.Object, res storage.ResponseMeta) (output runtime.Object, ttl *uint64, err error) { return input, nil, nil - }); !storage.IsInternalError(err) { + }, nil); !storage.IsInternalError(err) { t.Errorf("Unexpected error: %v", err) } // GuaranteedUpdate with suggestion should return an error if we don't change the object 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 e706edf3d6a..1f614c2033f 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 @@ -108,7 +108,7 @@ func testWatch(t *testing.T, recursive bool) { err := store.GuaranteedUpdate(ctx, key, out, true, nil, storage.SimpleUpdate( func(runtime.Object) (runtime.Object, error) { return watchTest.obj, nil - })) + }), nil) if err != nil { t.Fatalf("GuaranteedUpdate failed: %v", err) } @@ -161,7 +161,7 @@ func TestWatchFromZero(t *testing.T) { err = store.GuaranteedUpdate(ctx, key, out, true, nil, storage.SimpleUpdate( func(runtime.Object) (runtime.Object, error) { return &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "ns", Annotations: map[string]string{"a": "1"}}}, nil - })) + }), nil) if err != nil { t.Fatalf("GuaranteedUpdate failed: %v", err) } @@ -179,7 +179,7 @@ func TestWatchFromZero(t *testing.T) { err = store.GuaranteedUpdate(ctx, key, out, true, nil, storage.SimpleUpdate( func(runtime.Object) (runtime.Object, error) { return &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "ns"}}, nil - })) + }), nil) if err != nil { t.Fatalf("GuaranteedUpdate failed: %v", err) } @@ -217,7 +217,7 @@ func TestWatchFromNoneZero(t *testing.T) { store.GuaranteedUpdate(ctx, key, out, true, nil, storage.SimpleUpdate( func(runtime.Object) (runtime.Object, error) { return &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "bar"}}, err - })) + }), nil) testCheckResult(t, 0, watch.Modified, w, out) } @@ -235,7 +235,7 @@ func TestWatchError(t *testing.T) { validStore.GuaranteedUpdate(ctx, "/abc", &example.Pod{}, true, nil, storage.SimpleUpdate( func(runtime.Object) (runtime.Object, error) { return &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "foo"}}, nil - })) + }), nil) testCheckEventType(t, watch.Error, w) } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go index 6249b5cc5fd..01f9132f55d 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go @@ -215,9 +215,9 @@ type Interface interface { // or zero value in 'ptrToType' parameter otherwise. // If the object to update has the same value as previous, it won't do any update // but will return the object in 'ptrToType' parameter. - // If 'suggestion' can contain zero or one element - in such case this can be used as - // a suggestion about the current version of the object to avoid read operation from - // storage to get it. + // If 'suggestion' 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. // // Example: // @@ -239,7 +239,7 @@ type Interface interface { // ) GuaranteedUpdate( ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool, - precondtions *Preconditions, tryUpdate UpdateFunc, suggestion ...runtime.Object) error + precondtions *Preconditions, tryUpdate UpdateFunc, suggestion runtime.Object) error // Count returns number of different entries under the key (generally being path prefix). Count(key string) (int64, error) 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 d412f295649..af16b97d511 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 @@ -149,7 +149,7 @@ func updatePod(t *testing.T, s storage.Interface, obj, old *example.Pod) *exampl return obj.DeepCopyObject(), nil, nil } key := "pods/" + obj.Namespace + "/" + obj.Name - if err := s.GuaranteedUpdate(context.TODO(), key, &example.Pod{}, old == nil, nil, updateFn); err != nil { + if err := s.GuaranteedUpdate(context.TODO(), key, &example.Pod{}, old == nil, nil, updateFn, nil); err != nil { t.Errorf("unexpected error: %v", err) } obj.ResourceVersion = ""