Reuse generic TestGet in cache tests #113427

wojtek-t · 2022-10-28T08:42:52Z

This is the first step just to prove that reusing tests across implementations is feasible.

NONE

/kind cleanup
/priority important-longterm
/sig api-machinery

wojtek-t · 2022-10-28T09:29:39Z

/hold

I need to debug test failures.

stevekuznetsov · 2022-10-28T14:05:32Z

staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go

@@ -129,33 +130,40 @@ func RunTestGet(ctx context.Context, t *testing.T, store storage.Interface) {
 		ignoreNotFound    bool
 		expectNotFoundErr bool
 		expectRVTooLarge  bool
-		expectedOut       *example.Pod


What was forcing this change?

The change to a slice is described below - the "NotOlderThan" semantic is making some tests to have multiple potentially correct results.

The change to interface{} was to allow passing a generic thing to the newly created ExpectNoDiffWithOne function.
I reverted that and added a helper function for conversion instead.

wojtek-t

@stevekuznetsov - thanks for the review, PTAL

fedebongio · 2022-11-01T21:20:15Z

/assign @stevekuznetsov
/triage accepted

stevekuznetsov

Couple small comments, but mostly LGTM. Love that by using the generic TestGet we gain so many more test cases for the cache layer.

stevekuznetsov · 2022-11-02T12:58:28Z

staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go

@@ -1663,7 +1683,7 @@ func RunTestConsistentList(ctx context.Context, t *testing.T, store InterfaceWit
 }

 func RunTestGuaranteedUpdate(ctx context.Context, t *testing.T, store InterfaceWithPrefixTransformer, validation KeyValidation) {
-	key := "/testkey"


What did this change do, and why did we need it?

Good question. So basically what happened is what:

the default watchcache behavior assumes a specific naming function for object (it's possible to configure it via the CacherConfig, but I think the closer to the actual code we're in tests the better)

for these tests it's set to:

https://github.com/kubernetes/kubernetes/pull/113427/files#diff-0ef268eda54e793fece7779731777cfc7cb0fb1f0d8b3954e18e8ba74f94cc07R956
[which is the same thing we're using for namespaced resources in production code]

the function is basically used to cache objects that are read via watch from etcd (so how the cache is propagated)

So if we end-up using "key" (that is used for create/update/delete operations) that is different than what the KeyFunc in watchcache is computing for objects, it will result in get/list/watch operations to not work correctly.

So I basically switched the key (here and in some other places) to be consistent with what we're actually really computing in production code.

[I don't think it actually decreases the usefullness of the test, so it seems fine.]

stevekuznetsov · 2022-11-02T12:58:40Z

staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go

@@ -1801,7 +1821,7 @@ func RunTestGuaranteedUpdate(ctx context.Context, t *testing.T, store InterfaceW
 			}

 			// verify that kv pair is not empty after set and that the underlying data matches expectations
-			validation(ctx, t, key)


What did this change do, and why did we need it?

yeah...

So basically it's the same problem as described above.
For etcd3 implementation the implementation is doing it itself:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go#L127

However, after I changed the key generation in TestPropagateStore, it started returning "//foo".
And the difference is the path.Join("/", "/testkey") is idempotent, but path.Join("/", "//testkey") is not (it returns "/test/key")

But now looking into it, I think much cleaner way of doing that is to fix the etcd3-specific implementation to take that into account. Fixed there instead.

stevekuznetsov · 2022-11-02T12:58:45Z

staging/src/k8s.io/apiserver/pkg/storage/testing/utils.go

@@ -86,7 +86,7 @@ func DeepEqualSafePodSpec() example.PodSpec {
 // keys and stored objects.
 func TestPropagateStore(ctx context.Context, t *testing.T, store storage.Interface, obj *example.Pod) (string, *example.Pod) {
 	// Setup store with a key and grab the output for returning.
-	key := "/testkey"


What did this change do, and why did we need it?

(at this interface level the key just needs to be self-consistent, right? but I agree having it follow k8s semantics makes working with this code way nicer/more readable)

See my answer to your other comment above - it should explain it well.

FWIW - the failure of the integration test is expected now - it should self-resolve once: #113369 is merged

[I don't want to change it in this PR to potentially allow clean-cherrypick of the PR mentioned above.]

stevekuznetsov · 2022-11-02T12:59:17Z

staging/src/k8s.io/apiserver/pkg/storage/testing/utils.go

@@ -115,6 +115,16 @@ func ExpectNoDiff(t *testing.T, msg string, expected, got interface{}) {
 	}
 }

+func ExpectNoDiffWithOne(t *testing.T, msg string, expectedList []interface{}, got interface{}) {


nit: would prefer a name like ExpectContains or something more familiar to users of e.g. sets API

I like it - changed.

stevekuznetsov · 2022-11-02T13:01:00Z

staging/src/k8s.io/apiserver/pkg/storage/testing/utils.go

+			return
+		}
+	}
+	t.Errorf("%s: differs from all items, first: %s", msg, cmp.Diff(expectedList[0], got))


nit: when I moved the rest of the tests to use cmp.Diff, it was asked to check that cmp.Diff() actually produced a diff when reflect.DeepEqual() == false, and if not, to fall back to the previous impl, see ExpectNoDiff - probably want the same here

changed - PTAL

stevekuznetsov · 2022-11-02T13:07:53Z

staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go

-	if err := cacher.Get(context.TODO(), "pods/ns/bar", storage.GetOptions{ResourceVersion: fooCreated.ResourceVersion, IgnoreNotFound: true}, result); err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	emptyPod := example.Pod{}


Having a hard time following this test. Is the use of resourceVersion in GET options simply a red herring? We're asking for a different key ...

no - we're passing IgnoreFound=true and checking if really the empty object is returned.

Isn't that independent of passing RV here though? We are not asking for the wrong revision of an object, there is no bar pod at all ...

Yeah - it is independent from RV - the only case is that we're setting RV=0 to force getting from cache (as opposed to listing from RV="" which is passed down to etcd).

wojtek-t

@stevekuznetsov - thanks for comments; PTAL

wojtek-t · 2022-11-02T13:19:28Z

staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go

@@ -1663,7 +1683,7 @@ func RunTestConsistentList(ctx context.Context, t *testing.T, store InterfaceWit
 }

 func RunTestGuaranteedUpdate(ctx context.Context, t *testing.T, store InterfaceWithPrefixTransformer, validation KeyValidation) {
-	key := "/testkey"


Good question. So basically what happened is what:

the default watchcache behavior assumes a specific naming function for object (it's possible to configure it via the CacherConfig, but I think the closer to the actual code we're in tests the better)

for these tests it's set to:

https://github.com/kubernetes/kubernetes/pull/113427/files#diff-0ef268eda54e793fece7779731777cfc7cb0fb1f0d8b3954e18e8ba74f94cc07R956
[which is the same thing we're using for namespaced resources in production code]

the function is basically used to cache objects that are read via watch from etcd (so how the cache is propagated)

So if we end-up using "key" (that is used for create/update/delete operations) that is different than what the KeyFunc in watchcache is computing for objects, it will result in get/list/watch operations to not work correctly.

So I basically switched the key (here and in some other places) to be consistent with what we're actually really computing in production code.

[I don't think it actually decreases the usefullness of the test, so it seems fine.]

wojtek-t · 2022-11-02T13:20:10Z

staging/src/k8s.io/apiserver/pkg/storage/testing/utils.go

@@ -86,7 +86,7 @@ func DeepEqualSafePodSpec() example.PodSpec {
 // keys and stored objects.
 func TestPropagateStore(ctx context.Context, t *testing.T, store storage.Interface, obj *example.Pod) (string, *example.Pod) {
 	// Setup store with a key and grab the output for returning.
-	key := "/testkey"


See my answer to your other comment above - it should explain it well.

wojtek-t · 2022-11-02T13:23:57Z

staging/src/k8s.io/apiserver/pkg/storage/testing/utils.go

@@ -115,6 +115,16 @@ func ExpectNoDiff(t *testing.T, msg string, expected, got interface{}) {
 	}
 }

+func ExpectNoDiffWithOne(t *testing.T, msg string, expectedList []interface{}, got interface{}) {


I like it - changed.

wojtek-t · 2022-11-02T13:24:04Z

staging/src/k8s.io/apiserver/pkg/storage/testing/utils.go

+			return
+		}
+	}
+	t.Errorf("%s: differs from all items, first: %s", msg, cmp.Diff(expectedList[0], got))


changed - PTAL

wojtek-t · 2022-11-02T13:25:03Z

staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go

-	if err := cacher.Get(context.TODO(), "pods/ns/bar", storage.GetOptions{ResourceVersion: fooCreated.ResourceVersion, IgnoreNotFound: true}, result); err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	emptyPod := example.Pod{}


no - we're passing IgnoreFound=true and checking if really the empty object is returned.

wojtek-t · 2022-11-02T13:31:25Z

staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go

@@ -1801,7 +1821,7 @@ func RunTestGuaranteedUpdate(ctx context.Context, t *testing.T, store InterfaceW
 			}

 			// verify that kv pair is not empty after set and that the underlying data matches expectations
-			validation(ctx, t, key)


yeah...

So basically it's the same problem as described above.
For etcd3 implementation the implementation is doing it itself:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go#L127

However, after I changed the key generation in TestPropagateStore, it started returning "//foo".
And the difference is the path.Join("/", "/testkey") is idempotent, but path.Join("/", "//testkey") is not (it returns "/test/key")

But now looking into it, I think much cleaner way of doing that is to fix the etcd3-specific implementation to take that into account. Fixed there instead.

stevekuznetsov

Cool! This is a great proof for the concept here.

stevekuznetsov · 2022-11-02T17:43:59Z

I assume we'll pass integration now that the other PR has merged.

k8s-ci-robot · 2022-11-02T17:44:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/pkg/storage/OWNERS~~ [wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wojtek-t · 2022-11-02T18:18:00Z

/retest

wojtek-t · 2022-11-02T18:18:22Z

/hold cancel

wojtek-t · 2022-11-02T19:40:42Z

@stevekuznetsov - tests are passing now - can you tag it?

stevekuznetsov · 2022-11-02T20:04:16Z

/lgtm

wojtek-t assigned stevekuznetsov Oct 28, 2022

k8s-ci-robot requested review from deads2k and soltysh October 28, 2022 08:43

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver labels Oct 28, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2022

wojtek-t unassigned stevekuznetsov Oct 28, 2022

wojtek-t force-pushed the reuse_generic_tests_for_cacher branch from 8efd0af to 3528755 Compare October 28, 2022 13:46

stevekuznetsov reviewed Oct 28, 2022

View reviewed changes

wojtek-t force-pushed the reuse_generic_tests_for_cacher branch from 3528755 to 1fa799e Compare October 31, 2022 16:42

wojtek-t commented Oct 31, 2022

View reviewed changes

k8s-ci-robot assigned stevekuznetsov Nov 1, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 1, 2022

stevekuznetsov reviewed Nov 2, 2022

View reviewed changes

wojtek-t commented Nov 2, 2022

View reviewed changes

wojtek-t force-pushed the reuse_generic_tests_for_cacher branch 2 times, most recently from c3158e0 to d775bea Compare November 2, 2022 13:48

Reuse generic TestGet in cache tests.

75a1ef8

wojtek-t force-pushed the reuse_generic_tests_for_cacher branch from d775bea to 75a1ef8 Compare November 2, 2022 15:37

stevekuznetsov approved these changes Nov 2, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2022

k8s-ci-robot merged commit de95671 into kubernetes:master Nov 2, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 2, 2022

This was referenced Nov 3, 2022

Reuse generic GetListNonRecursive test for watchcache #113588

Merged

apiserver/storage: test caching and indexing layers at the unit level #109831

Open

Reuse generic TestGet in cache tests #113427

Reuse generic TestGet in cache tests #113427

Conversation

wojtek-t commented Oct 28, 2022

wojtek-t commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

fedebongio commented Nov 1, 2022

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov left a comment

Choose a reason for hiding this comment

stevekuznetsov commented Nov 2, 2022

k8s-ci-robot commented Nov 2, 2022

wojtek-t commented Nov 2, 2022

wojtek-t commented Nov 2, 2022

wojtek-t commented Nov 2, 2022

stevekuznetsov commented Nov 2, 2022