From 3016b3d8f868a3041c7cb86695de09d7ab27cf3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Tyczy=C5=84ski?= Date: Thu, 10 Feb 2022 09:58:31 +0100 Subject: [PATCH] Change storage.Interface to utilize recurisve parameter --- pkg/controlplane/reconcilers/lease.go | 3 +- .../pkg/registry/generic/registry/dryrun.go | 4 +- .../pkg/registry/generic/registry/store.go | 4 +- .../apiserver/pkg/storage/cacher/cacher.go | 7 ++- .../storage/cacher/cacher_whitebox_test.go | 14 +++-- .../apiserver/pkg/storage/etcd3/store.go | 4 +- .../apiserver/pkg/storage/etcd3/store_test.go | 57 ++++++++++++------- .../apiserver/pkg/storage/interfaces.go | 10 ++-- .../pkg/storage/tests/cacher_test.go | 13 +++-- 9 files changed, 71 insertions(+), 45 deletions(-) diff --git a/pkg/controlplane/reconcilers/lease.go b/pkg/controlplane/reconcilers/lease.go index ca3e1dee04b..891ab31c4d7 100644 --- a/pkg/controlplane/reconcilers/lease.go +++ b/pkg/controlplane/reconcilers/lease.go @@ -67,8 +67,9 @@ func (s *storageLeases) ListLeases() ([]string, error) { ResourceVersion: "0", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan, Predicate: storage.Everything, + Recursive: true, } - if err := s.storage.List(apirequest.NewDefaultContext(), s.baseKey, storageOpts, ipInfoList); err != nil { + if err := s.storage.GetList(apirequest.NewDefaultContext(), s.baseKey, storageOpts, ipInfoList); err != nil { return nil, err } 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 f746d097889..647383b5540 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 @@ -68,8 +68,8 @@ func (s *DryRunnableStorage) GetToList(ctx context.Context, key string, opts sto return s.Storage.GetToList(ctx, key, opts, listObj) } -func (s *DryRunnableStorage) List(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error { - return s.Storage.List(ctx, key, opts, listObj) +func (s *DryRunnableStorage) GetList(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error { + return s.Storage.GetList(ctx, key, opts, listObj) } func (s *DryRunnableStorage) GuaranteedUpdate( 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 5721c3c98ee..28e600175c2 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 @@ -347,16 +347,18 @@ func (e *Store) ListPredicate(ctx context.Context, p storage.SelectionPredicate, ResourceVersion: options.ResourceVersion, ResourceVersionMatch: options.ResourceVersionMatch, Predicate: p, + Recursive: true, } if name, ok := p.MatchesSingle(); ok { if key, err := e.KeyFunc(ctx, name); err == nil { + storageOpts.Recursive = false err := e.Storage.GetToList(ctx, key, storageOpts, list) return list, storeerr.InterpretListError(err, qualifiedResource) } // if we cannot extract a key based on the current context, the optimization is skipped } - err := e.Storage.List(ctx, e.KeyRootFunc(ctx), storageOpts, list) + err := e.Storage.GetList(ctx, e.KeyRootFunc(ctx), storageOpts, list) return list, storeerr.InterpretListError(err, qualifiedResource) } 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 001a96a02ba..362a4a8f38f 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go @@ -609,8 +609,8 @@ func (c *Cacher) GetToList(ctx context.Context, key string, opts storage.ListOpt return c.list(ctx, key, opts, listObj, false) } -// List implements storage.Interface. -func (c *Cacher) List(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error { +// GetList implements storage.Interface. +func (c *Cacher) GetList(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error { return c.list(ctx, key, opts, listObj, true) } @@ -1090,8 +1090,9 @@ func (lw *cacherListerWatcher) List(options metav1.ListOptions) (runtime.Object, storageOpts := storage.ListOptions{ ResourceVersionMatch: options.ResourceVersionMatch, Predicate: pred, + Recursive: true, } - if err := lw.storage.List(context.TODO(), lw.resourcePrefix, storageOpts, list); err != nil { + if err := lw.storage.GetList(context.TODO(), lw.resourcePrefix, storageOpts, list); err != nil { return nil, err } return list, nil 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 30fbe2388e0..58fcab02d61 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 @@ -314,7 +314,7 @@ func (d *dummyStorage) Get(_ context.Context, _ string, _ storage.GetOptions, _ func (d *dummyStorage) GetToList(_ context.Context, _ string, _ storage.ListOptions, _ runtime.Object) error { return d.err } -func (d *dummyStorage) List(_ context.Context, _ string, _ storage.ListOptions, listObj runtime.Object) error { +func (d *dummyStorage) GetList(_ context.Context, _ string, _ storage.ListOptions, listObj runtime.Object) error { podList := listObj.(*example.PodList) podList.ListMeta = metav1.ListMeta{ResourceVersion: "100"} return d.err @@ -326,7 +326,7 @@ func (d *dummyStorage) Count(_ string) (int64, error) { return 0, fmt.Errorf("unimplemented") } -func TestListCacheBypass(t *testing.T) { +func TestGetListCacheBypass(t *testing.T) { backingStorage := &dummyStorage{} cacher, _, err := newTestCacher(backingStorage) if err != nil { @@ -344,20 +344,22 @@ func TestListCacheBypass(t *testing.T) { // Inject error to underlying layer and check if cacher is not bypassed. backingStorage.err = errDummy - err = cacher.List(context.TODO(), "pods/ns", storage.ListOptions{ + err = cacher.GetList(context.TODO(), "pods/ns", storage.ListOptions{ ResourceVersion: "0", Predicate: pred, + Recursive: true, }, result) if err != nil { - t.Errorf("List with Limit and RV=0 should be served from cache: %v", err) + t.Errorf("GetList with Limit and RV=0 should be served from cache: %v", err) } - err = cacher.List(context.TODO(), "pods/ns", storage.ListOptions{ + err = cacher.GetList(context.TODO(), "pods/ns", storage.ListOptions{ ResourceVersion: "", Predicate: pred, + Recursive: true, }, result) if err != errDummy { - t.Errorf("List with Limit without RV=0 should bypass cacher: %v", err) + t.Errorf("GetList with Limit without RV=0 should bypass cacher: %v", err) } } 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 d263148cac8..77d6cdcce84 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -557,8 +557,8 @@ func encodeContinue(key, keyPrefix string, resourceVersion int64) (string, error return base64.RawURLEncoding.EncodeToString(out), nil } -// List implements storage.Interface.List. -func (s *store) List(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error { +// GetList implements storage.Interface. +func (s *store) GetList(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error { return s.list(ctx, key, opts, listObj, true) } 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 b00eafdc361..a47a1e84e0b 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 @@ -1080,8 +1080,9 @@ func TestTransformationFailure(t *testing.T) { var got example.PodList storageOpts := storage.ListOptions{ Predicate: storage.Everything, + Recursive: true, } - if err := store.List(ctx, "/", storageOpts, &got); !storage.IsInternalError(err) { + if err := store.GetList(ctx, "/", storageOpts, &got); !storage.IsInternalError(err) { t.Errorf("Unexpected error %v", err) } @@ -1166,7 +1167,7 @@ func TestList(t *testing.T) { // we want to figure out the resourceVersion before we create anything initialList := &example.PodList{} - if err := store.List(ctx, "/", storage.ListOptions{Predicate: storage.Everything}, initialList); err != nil { + if err := store.GetList(ctx, "/", storage.ListOptions{Predicate: storage.Everything, Recursive: true}, initialList); err != nil { t.Errorf("Unexpected List error: %v", err) } initialRV := initialList.ResourceVersion @@ -1183,9 +1184,10 @@ func TestList(t *testing.T) { storageOpts := storage.ListOptions{ ResourceVersion: "0", Predicate: storage.Everything, + Recursive: true, } - if err := store.List(ctx, "/two-level", storageOpts, list); err != nil { - t.Errorf("Unexpected List error: %v", err) + if err := store.GetList(ctx, "/two-level", storageOpts, list); err != nil { + t.Errorf("Unexpected error: %v", err) } continueRV, _ := strconv.Atoi(list.ResourceVersion) secondContinuation, err := encodeContinue("/two-level/2", "/two-level/", int64(continueRV)) @@ -1576,12 +1578,13 @@ func TestList(t *testing.T) { ResourceVersion: tt.rv, ResourceVersionMatch: tt.rvMatch, Predicate: tt.pred, + Recursive: true, } var err error if tt.disablePaging { - err = disablePagingStore.List(ctx, tt.prefix, storageOpts, out) + err = disablePagingStore.GetList(ctx, tt.prefix, storageOpts, out) } else { - err = store.List(ctx, tt.prefix, storageOpts, out) + err = store.GetList(ctx, tt.prefix, storageOpts, out) } if tt.expectRVTooLarge { if err == nil || !storage.IsTooLargeResourceVersion(err) { @@ -1592,7 +1595,7 @@ func TestList(t *testing.T) { if err != nil { if !tt.expectError { - t.Fatalf("List failed: %v", err) + t.Fatalf("GetList failed: %v", err) } return } @@ -1690,8 +1693,9 @@ func TestListContinuation(t *testing.T) { options := storage.ListOptions{ ResourceVersion: "0", Predicate: pred(1, ""), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Fatalf("Unable to get initial list: %v", err) } if len(out.Continue) == 0 { @@ -1716,8 +1720,9 @@ func TestListContinuation(t *testing.T) { options = storage.ListOptions{ ResourceVersion: "0", Predicate: pred(0, continueFromSecondItem), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Fatalf("Unable to get second page: %v", err) } if len(out.Continue) != 0 { @@ -1742,8 +1747,9 @@ func TestListContinuation(t *testing.T) { options = storage.ListOptions{ ResourceVersion: "0", Predicate: pred(1, continueFromSecondItem), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Fatalf("Unable to get second page: %v", err) } if len(out.Continue) == 0 { @@ -1767,8 +1773,9 @@ func TestListContinuation(t *testing.T) { options = storage.ListOptions{ ResourceVersion: "0", Predicate: pred(1, continueFromThirdItem), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Fatalf("Unable to get second page: %v", err) } if len(out.Continue) != 0 { @@ -1861,8 +1868,9 @@ func TestListContinuationWithFilter(t *testing.T) { options := storage.ListOptions{ ResourceVersion: "0", Predicate: pred(2, ""), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Errorf("Unable to get initial list: %v", err) } if len(out.Continue) == 0 { @@ -1894,8 +1902,9 @@ func TestListContinuationWithFilter(t *testing.T) { options = storage.ListOptions{ ResourceVersion: "0", Predicate: pred(2, cont), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Errorf("Unable to get second page: %v", err) } if len(out.Continue) != 0 { @@ -1976,8 +1985,9 @@ func TestListInconsistentContinuation(t *testing.T) { options := storage.ListOptions{ ResourceVersion: "0", Predicate: pred(1, ""), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Fatalf("Unable to get initial list: %v", err) } if len(out.Continue) == 0 { @@ -2021,8 +2031,9 @@ func TestListInconsistentContinuation(t *testing.T) { options = storage.ListOptions{ ResourceVersion: "0", Predicate: pred(0, continueFromSecondItem), + Recursive: true, } - err = store.List(ctx, "/", options, out) + err = store.GetList(ctx, "/", options, out) if err == nil { t.Fatalf("unexpected no error") } @@ -2042,8 +2053,9 @@ func TestListInconsistentContinuation(t *testing.T) { options = storage.ListOptions{ ResourceVersion: "0", Predicate: pred(1, inconsistentContinueFromSecondItem), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Fatalf("Unable to get second page: %v", err) } if len(out.Continue) == 0 { @@ -2060,8 +2072,9 @@ func TestListInconsistentContinuation(t *testing.T) { options = storage.ListOptions{ ResourceVersion: "0", Predicate: pred(1, continueFromThirdItem), + Recursive: true, } - if err := store.List(ctx, "/", options, out); err != nil { + if err := store.GetList(ctx, "/", options, out); err != nil { t.Fatalf("Unable to get second page: %v", err) } if len(out.Continue) != 0 { @@ -2316,8 +2329,9 @@ func TestConsistentList(t *testing.T) { result1 := example.PodList{} options := storage.ListOptions{ Predicate: predicate, + Recursive: true, } - if err := store.List(ctx, "/", options, &result1); err != nil { + if err := store.GetList(ctx, "/", options, &result1); err != nil { t.Fatalf("failed to list objects: %v", err) } @@ -2326,10 +2340,11 @@ func TestConsistentList(t *testing.T) { Predicate: predicate, ResourceVersion: result1.ResourceVersion, ResourceVersionMatch: metav1.ResourceVersionMatchExact, + Recursive: true, } result2 := example.PodList{} - if err := store.List(ctx, "/", options, &result2); err != nil { + if err := store.GetList(ctx, "/", options, &result2); err != nil { t.Fatalf("failed to list objects: %v", err) } @@ -2341,7 +2356,7 @@ func TestConsistentList(t *testing.T) { options.ResourceVersionMatch = metav1.ResourceVersionMatchNotOlderThan result3 := example.PodList{} - if err := store.List(ctx, "/", options, &result3); err != nil { + if err := store.GetList(ctx, "/", options, &result3); err != nil { t.Fatalf("failed to list objects: %v", err) } @@ -2349,7 +2364,7 @@ func TestConsistentList(t *testing.T) { options.ResourceVersionMatch = metav1.ResourceVersionMatchExact result4 := example.PodList{} - if err := store.List(ctx, "/", options, &result4); err != nil { + if err := store.GetList(ctx, "/", options, &result4); err != nil { t.Fatalf("failed to list objects: %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 feda90e9f03..d2bcf199740 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go @@ -183,7 +183,7 @@ type Interface interface { // and send it in an "ADDED" event, before watch starts. Watch(ctx context.Context, key string, opts ListOptions) (watch.Interface, error) - // Get unmarshals json found at key into objPtr. On a not found error, will either + // Get unmarshals object found at key into objPtr. On a not found error, will either // return a zero object of the requested type, or an error, depending on 'opts.ignoreNotFound'. // Treats empty responses and nil response nodes exactly like a not found error. // The returned contents may be delayed, but it is guaranteed that they will @@ -196,11 +196,13 @@ type Interface interface { // match 'opts.ResourceVersion' according 'opts.ResourceVersionMatch'. GetToList(ctx context.Context, key string, opts ListOptions, listObj runtime.Object) error - // List unmarshalls jsons found at directory defined by key and opaque them - // into *List api object (an object that satisfies runtime.IsList definition). + // GetList unmarshalls objects found at key into a *List api object (an object + // that satisfies runtime.IsList definition). + // If 'opts.Recursive' is false, 'key' is used as an exact match. If `opts.Recursive' + // is true, 'key' is used as a prefix. // The returned contents may be delayed, but it is guaranteed that they will // match 'opts.ResourceVersion' according 'opts.ResourceVersionMatch'. - List(ctx context.Context, key string, opts ListOptions, listObj runtime.Object) error + GetList(ctx context.Context, key string, opts ListOptions, listObj runtime.Object) error // GuaranteedUpdate keeps calling 'tryUpdate()' to update key 'key' (of type 'ptrToType') // retrying the update until success if there is index conflict. 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 791f63825f0..3d4511fe892 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 @@ -293,8 +293,9 @@ func TestList(t *testing.T) { rvResult := &example.PodList{} options := storage.ListOptions{ Predicate: storage.Everything, + Recursive: true, } - if err := cacher.List(context.TODO(), "pods/ns", options, rvResult); err != nil { + if err := cacher.GetList(context.TODO(), "pods/ns", options, rvResult); err != nil { t.Errorf("Unexpected error: %v", err) } deletedPodRV := rvResult.ListMeta.ResourceVersion @@ -305,8 +306,9 @@ func TestList(t *testing.T) { options = storage.ListOptions{ ResourceVersion: deletedPodRV, Predicate: storage.Everything, + Recursive: true, } - if err := cacher.List(context.TODO(), "pods/ns", options, result); err != nil { + if err := cacher.GetList(context.TODO(), "pods/ns", options, result); err != nil { t.Errorf("Unexpected error: %v", err) } if result.ListMeta.ResourceVersion != deletedPodRV { @@ -371,8 +373,9 @@ func TestTooLargeResourceVersionList(t *testing.T) { options := storage.ListOptions{ ResourceVersion: listRV, Predicate: storage.Everything, + Recursive: true, } - err = cacher.List(context.TODO(), "pods/ns", options, result) + err = cacher.GetList(context.TODO(), "pods/ns", options, result) if !errors.IsTimeout(err) { t.Errorf("Unexpected error: %v", err) } @@ -408,12 +411,12 @@ type injectListError struct { storage.Interface } -func (self *injectListError) List(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error { +func (self *injectListError) GetList(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error { if self.errors > 0 { self.errors-- return fmt.Errorf("injected error") } - return self.Interface.List(ctx, key, opts, listObj) + return self.Interface.GetList(ctx, key, opts, listObj) } func TestWatch(t *testing.T) {