diff --git a/pkg/storage/cacher/cacher_whitebox_test.go b/pkg/storage/cacher/cacher_whitebox_test.go index 1d1007145..f35cea231 100644 --- a/pkg/storage/cacher/cacher_whitebox_test.go +++ b/pkg/storage/cacher/cacher_whitebox_test.go @@ -218,53 +218,87 @@ func (d *dummyCacher) Ready() bool { func TestGetListCacheBypass(t *testing.T) { type opts struct { + Recursive bool ResourceVersion string ResourceVersionMatch metav1.ResourceVersionMatch Limit int64 Continue string } + toInternalOpts := func(opt opts) *internalversion.ListOptions { + return &internalversion.ListOptions{ + ResourceVersion: opt.ResourceVersion, + ResourceVersionMatch: opt.ResourceVersionMatch, + Limit: opt.Limit, + Continue: opt.Continue, + } + } + + toStorageOpts := func(opt opts) storage.ListOptions { + return storage.ListOptions{ + Recursive: opt.Recursive, + ResourceVersion: opt.ResourceVersion, + ResourceVersionMatch: opt.ResourceVersionMatch, + Predicate: storage.SelectionPredicate{ + Continue: opt.Continue, + Limit: opt.Limit, + }, + } + } + + keyPrefix := "/pods/" + continueOnRev1, err := storage.EncodeContinue(keyPrefix+"foo", keyPrefix, 1) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } testCases := map[opts]bool{} - testCases[opts{}] = false - testCases[opts{Limit: 100}] = false - testCases[opts{Continue: "continue"}] = true - testCases[opts{Limit: 100, Continue: "continue"}] = true + testCases[opts{}] = true + testCases[opts{Limit: 100}] = true + testCases[opts{Continue: continueOnRev1}] = true + testCases[opts{Limit: 100, Continue: continueOnRev1}] = true testCases[opts{ResourceVersion: "0"}] = false testCases[opts{ResourceVersion: "0", Limit: 100}] = false - testCases[opts{ResourceVersion: "0", Continue: "continue"}] = true - testCases[opts{ResourceVersion: "0", Limit: 100, Continue: "continue"}] = true + testCases[opts{ResourceVersion: "0", Continue: continueOnRev1}] = true + testCases[opts{ResourceVersion: "0", Limit: 100, Continue: continueOnRev1}] = true testCases[opts{ResourceVersion: "0", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan}] = false testCases[opts{ResourceVersion: "0", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan, Limit: 100}] = false testCases[opts{ResourceVersion: "1"}] = false testCases[opts{ResourceVersion: "1", Limit: 100}] = true - testCases[opts{ResourceVersion: "1", Continue: "continue"}] = true - testCases[opts{ResourceVersion: "1", Limit: 100, Continue: "continue"}] = true testCases[opts{ResourceVersion: "1", ResourceVersionMatch: metav1.ResourceVersionMatchExact}] = true testCases[opts{ResourceVersion: "1", ResourceVersionMatch: metav1.ResourceVersionMatchExact, Limit: 100}] = true testCases[opts{ResourceVersion: "1", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan}] = false testCases[opts{ResourceVersion: "1", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan, Limit: 100}] = false + // Bypass for most requests doesn't depend on Recursive + for opts, expectBypass := range testCases { + opts.Recursive = true + testCases[opts] = expectBypass + } + // Continue is ignored on non recursive LIST + testCases[opts{ResourceVersion: "1", Continue: continueOnRev1}] = true + testCases[opts{ResourceVersion: "1", Continue: continueOnRev1, Limit: 100}] = true + for _, rv := range []string{"", "0", "1"} { for _, match := range []metav1.ResourceVersionMatch{"", metav1.ResourceVersionMatchExact, metav1.ResourceVersionMatchNotOlderThan} { - for _, c := range []string{"", "continue"} { + for _, continueKey := range []string{"", continueOnRev1} { for _, limit := range []int64{0, 100} { - errs := validation.ValidateListOptions(&internalversion.ListOptions{ - ResourceVersion: rv, - ResourceVersionMatch: match, - Limit: limit, - Continue: c, - }, false) - if len(errs) != 0 { - continue - } - opt := opts{ - ResourceVersion: rv, - ResourceVersionMatch: match, - Limit: limit, - Continue: c, - } - _, found := testCases[opt] - if !found { - t.Errorf("Test case not covered, but passes validation: %+v", opt) + for _, recursive := range []bool{true, false} { + opt := opts{ + Recursive: recursive, + ResourceVersion: rv, + ResourceVersionMatch: match, + Limit: limit, + Continue: continueKey, + } + if errs := validation.ValidateListOptions(toInternalOpts(opt), false); len(errs) != 0 { + continue + } + if _, _, err = storage.ValidateListOptions(keyPrefix, storage.APIObjectVersioner{}, toStorageOpts(opt)); err != nil { + continue + } + _, found := testCases[opt] + if !found { + t.Errorf("Test case not covered, but passes validation: %+v", opt) + } } } @@ -273,35 +307,37 @@ func TestGetListCacheBypass(t *testing.T) { } for opt := range testCases { - errs := validation.ValidateListOptions(&internalversion.ListOptions{ - ResourceVersion: opt.ResourceVersion, - ResourceVersionMatch: opt.ResourceVersionMatch, - Limit: opt.Limit, - Continue: opt.Continue, - }, false) - if len(errs) != 0 { + if errs := validation.ValidateListOptions(toInternalOpts(opt), false); len(errs) != 0 { + t.Errorf("Invalid LIST request that should not be tested %+v", opt) + continue + } + if _, _, err = storage.ValidateListOptions(keyPrefix, storage.APIObjectVersioner{}, toStorageOpts(opt)); err != nil { t.Errorf("Invalid LIST request that should not be tested %+v", opt) continue } } - t.Run("ConsistentListFromStorage", func(t *testing.T) { - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConsistentListFromCache, false) - testCases[opts{}] = true - testCases[opts{Limit: 100}] = true + runTestCases := func(t *testing.T, testcases map[opts]bool, overrides ...map[opts]bool) { for opt, expectBypass := range testCases { - testGetListCacheBypass(t, storage.ListOptions{ - ResourceVersion: opt.ResourceVersion, - ResourceVersionMatch: opt.ResourceVersionMatch, - Predicate: storage.SelectionPredicate{ - Continue: opt.Continue, - Limit: opt.Limit, - }, - }, expectBypass) + for _, override := range overrides { + if bypass, ok := override[opt]; ok { + expectBypass = bypass + } + } + testGetListCacheBypass(t, toStorageOpts(opt), expectBypass) } + } + consistentListFromCacheOverrides := map[opts]bool{} + for _, recursive := range []bool{true, false} { + consistentListFromCacheOverrides[opts{Recursive: recursive}] = false + consistentListFromCacheOverrides[opts{Limit: 100, Recursive: recursive}] = false + } + t.Run("ConsistentListFromCache=false", func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConsistentListFromCache, false) + runTestCases(t, testCases) }) - t.Run("ConsistentListFromCache", func(t *testing.T) { + t.Run("ConsistentListFromCache=true", func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConsistentListFromCache, true) // TODO(p0lyn0mial): the following tests assume that etcdfeature.DefaultFeatureSupportChecker.Supports(storage.RequestWatchProgress) @@ -311,19 +347,7 @@ func TestGetListCacheBypass(t *testing.T) { // However in CI all test are run and there must be a test(s) that properly // initialize the storage layer so that the mentioned method evaluates to true forceRequestWatchProgressSupport(t) - - testCases[opts{}] = false - testCases[opts{Limit: 100}] = false - for opt, expectBypass := range testCases { - testGetListCacheBypass(t, storage.ListOptions{ - ResourceVersion: opt.ResourceVersion, - ResourceVersionMatch: opt.ResourceVersionMatch, - Predicate: storage.SelectionPredicate{ - Continue: opt.Continue, - Limit: opt.Limit, - }, - }, expectBypass) - } + runTestCases(t, testCases, consistentListFromCacheOverrides) }) }