Deflake watchcache tests #124610

wojtek-t · 2024-04-29T12:28:32Z

Found when working on kubernetes/enhancements#4568

NONE

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

k8s-ci-robot · 2024-04-29T12:29:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 · 2024-04-29T12:30:01Z

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

-		ResourceVersion: "0",
+		// Limit is ignored when ResourceVersion is set to 0.
+		// Set it to consistent read.
+		ResourceVersion: "",


The failures from before this PR could be easily triggered by adding time.Sleep(time.Second) in appropriate tests (cacher_test.go) after creating watchcache but before starting the test.

Why does adding a sleep cause the test to fail?
By changing the RV to consistent read, the list call will be delegated to the underlying storage. I assume this was what this test intended to do, right?

Without sleep, the list is happening before watchcache is initialized and was delegated to underlying storage.
If watchcache is initialized, the this test is failing, because watchcache is ignoring Limit for RV=0.

We know that for RV=0, limit is ignored when used by watchcache - so yes, we want to test if for other RVs, it doesn't matter if its delegated or not, the result is the important stuff (but yes, it's delegated).

Do we have a test for RV=0 to check if the limit is ignored for the watchcache ?

I'm sure we had one but I can't find any now. We should add that as a follow-up (though I wouldn't block this PR on it as it's not regressing our coverage).

okay, I can add the new test if you don't have time. Just let me know.

So I found what I was looking for:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go#L862-L866
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go#L881-L885

We are now ignoring it, it might be better to slightly update them to check that we return everything in those cases.
If you have time to take it, it would be great.

sure, i will have a look.

wojtek-t · 2024-04-29T13:17:28Z

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

@@ -2407,7 +2415,7 @@ func RunTestGuaranteedUpdateWithSuggestionAndConflict(ctx context.Context, t *te
 	err := store.GuaranteedUpdate(ctx, key, updatedPod, false, nil,
 		storage.SimpleUpdate(func(obj runtime.Object) (runtime.Object, error) {
 			pod := obj.(*example.Pod)
-			pod.Name = "foo-2"
+			pod.Generation = 2


This tests were somewhat broken for two reasons:

changing name is unrealistic (forbidden) and watchcache is lost - switched to use Generation instead

suggestion can be ignored by implementation - relaxed the validation check below

was RunTestGuaranteedUpdateWithSuggestionAndConflict flaky or did you change it to match reality ?

both:

due to changing names, watchcache wasn't behaving as it is in reality (it's caching by objects namespace/name, not by the given key, so the caching was changing in the meantime

the last check was incorrect and you can trigger it by putting sleep

since we are changing the test to match reality would it make sense to add a new annotation instead of messing with the Generation field which is set by the system ?

actually, the Generation will be updated during update before we call this function so it is okay to mess with that field.

generation isn't handled automatically by the system - we're handing it manually at strategy level for individual resources. So no - I think this is good.

wojtek-t · 2024-04-29T17:52:48Z

/assign @p0lyn0mial

p0lyn0mial · 2024-04-30T06:52:21Z

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

-		ResourceVersion: "0",
+		// Limit is ignored when ResourceVersion is set to 0.
+		// Set it to consistent read.
+		ResourceVersion: "",


Why does adding a sleep cause the test to fail?
By changing the RV to consistent read, the list call will be delegated to the underlying storage. I assume this was what this test intended to do, right?

p0lyn0mial · 2024-04-30T06:56:34Z

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

@@ -1654,7 +1656,8 @@ func RunTestListContinuation(ctx context.Context, t *testing.T, store storage.In
 	// no limit, should get two items
 	out = &example.PodList{}
 	options = storage.ListOptions{
-		ResourceVersion: "0",


interesting, setting the continuation token and an RV doesn't yield an error, is that correct ?

we're checking for non-0:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go#L593-L595

p0lyn0mial · 2024-04-30T07:13:17Z

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

@@ -2407,7 +2415,7 @@ func RunTestGuaranteedUpdateWithSuggestionAndConflict(ctx context.Context, t *te
 	err := store.GuaranteedUpdate(ctx, key, updatedPod, false, nil,
 		storage.SimpleUpdate(func(obj runtime.Object) (runtime.Object, error) {
 			pod := obj.(*example.Pod)
-			pod.Name = "foo-2"
+			pod.Generation = 2


was RunTestGuaranteedUpdateWithSuggestionAndConflict flaky or did you change it to match reality ?

p0lyn0mial · 2024-04-30T10:39:58Z

/lgtm

k8s-ci-robot · 2024-04-30T10:40:04Z

LGTM label has been added.

Git tree hash: 40c0154dc0f651e815ea8da46471bac6ad77c859

cici37 · 2024-04-30T20:16:36Z

/triage accepted

wojtek-t assigned jpbetz Apr 29, 2024

k8s-ci-robot requested review from enj and smarterclayton April 29, 2024 12:28

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

wojtek-t commented Apr 29, 2024

View reviewed changes

Deflake watchcache tests

6d9edcc

wojtek-t force-pushed the deflake_cacher_tests branch from ac3e675 to 6d9edcc Compare April 29, 2024 13:15

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 29, 2024

wojtek-t commented Apr 29, 2024

View reviewed changes

k8s-ci-robot assigned p0lyn0mial Apr 29, 2024

p0lyn0mial reviewed Apr 30, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2024

k8s-ci-robot merged commit 02365ec into kubernetes:master Apr 30, 2024
14 checks passed

k8s-ci-robot added this to the v1.31 milestone Apr 30, 2024

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 Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deflake watchcache tests #124610

Deflake watchcache tests #124610

wojtek-t commented Apr 29, 2024

k8s-ci-robot commented Apr 29, 2024

wojtek-t Apr 29, 2024

p0lyn0mial Apr 30, 2024

wojtek-t Apr 30, 2024

p0lyn0mial Apr 30, 2024

wojtek-t Apr 30, 2024

p0lyn0mial Apr 30, 2024

wojtek-t Apr 30, 2024

p0lyn0mial Apr 30, 2024

wojtek-t Apr 29, 2024

p0lyn0mial Apr 30, 2024

wojtek-t Apr 30, 2024

p0lyn0mial Apr 30, 2024

p0lyn0mial Apr 30, 2024

wojtek-t Apr 30, 2024

wojtek-t commented Apr 29, 2024

p0lyn0mial Apr 30, 2024

p0lyn0mial Apr 30, 2024

wojtek-t Apr 30, 2024

p0lyn0mial Apr 30, 2024

p0lyn0mial Apr 30, 2024

p0lyn0mial commented Apr 30, 2024

k8s-ci-robot commented Apr 30, 2024

cici37 commented Apr 30, 2024

Deflake watchcache tests #124610

Deflake watchcache tests #124610

Conversation

wojtek-t commented Apr 29, 2024

k8s-ci-robot commented Apr 29, 2024

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 commented Apr 29, 2024

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

p0lyn0mial commented Apr 30, 2024

k8s-ci-robot commented Apr 30, 2024

cici37 commented Apr 30, 2024