client-go: data consistency checker for list requests #124963

p0lyn0mial · 2024-05-20T10:14:17Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds data consistency checker to client-go for checking data retrieved via list requests from the cache and directly from etcd. The detector can be enabled by setting KUBE_LIST_FROM_CACHE_INCONSISTENCY_DETECTOR env var.

// CheckListFromCacheDataConsistencyIfRequested performs a data consistency check only when
// the KUBE_LIST_FROM_CACHE_INCONSISTENCY_DETECTOR environment variable was set during a binary startup
// for requests that have a high chance of being served from the watch-cache.
//
// The consistency check is meant to be enforced only in the CI, not in production.
// The check ensures that data retrieved by a list api call from the watch-cache
// is exactly the same as data received by the list api call from etcd.
//
// Note that this function will panic when data inconsistency is detected.
// This is intentional because we want to catch it in the CI.
//
// Note that this function doesn't examine the ListOptions to determine
// if the original request has hit the cache because it would be challenging
// to maintain consistency with the server-side implementation.
// For simplicity, we assume that the first request retrieved data from
// the cache (even though this might not be true for some requests)
// and issue the second call to get data from etcd for comparison.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

p0lyn0mial · 2024-05-20T10:14:38Z

/assign @wojtek-t
/cc @serathius

p0lyn0mial · 2024-05-20T10:16:53Z

staging/src/k8s.io/client-go/tools/cache/reflector_data_consistency_detector.go

+//
+// if ResourceVersion = "" and ConsistendListFromCache is disabled or RequestWatchProgress isn't supported,
+// then the request will be served from the storage.
+func wasListRequestServedFromStorage(opts metav1.ListOptions) bool {


copied from the cacher.go - it looks like it does what we want expect the case mentioned in the comment and after clarifying rows 1 and 3 from the KEP.

p0lyn0mial · 2024-05-20T10:17:25Z

staging/src/k8s.io/client-go/tools/cache/reflector_data_consistency_detector.go

+//
+// Note that this function will panic when data inconsistency is detected.
+// This is intentional because we want to catch it in the CI.
+func CheckListAgainstCacheDataConsistencyIfRequested[T runtime.Object](ctx context.Context, identity string, listItemsFn listItemsFunc[T], optionsUsedToReceiveList metav1.ListOptions, receivedList runtime.Object) {


We need to find a better home for this function. Depending on where it is defined and whether it will be public, we could consider adding more validation for opts (e.g., checking if it was actually a list request).

p0lyn0mial · 2024-05-20T10:20:54Z

staging/src/k8s.io/client-go/tools/cache/reflector_data_consistency_detector.go

@@ -64,7 +115,7 @@ func checkWatchListDataConsistencyIfRequested(ctx context.Context, identity stri
 // it is guarded by an environmental variable.
 // we cannot manipulate the environmental variable because
 // it will affect other tests in this package.
-func checkDataConsistency(ctx context.Context, identity string, lastSyncedResourceVersion string, listItemsFn listItemsFunc, retrieveCollectedItemsFn retrieveCollectedItemsFunc) {
+func checkDataConsistency[T runtime.Object, U any](ctx context.Context, identity string, lastSyncedResourceVersion string, listItemsFn listItemsFunc[T], retrieveCollectedItemsFn retrieveCollectedItemsFunc[U]) {


We need to find a better home for this function. This function is common for checkWatchListDataConsistencyIfRequested and CheckListAgainstCacheDataConsistencyIfRequested.

wojtek-t · 2024-05-20T12:38:19Z

staging/src/k8s.io/code-generator/cmd/client-gen/generators/generator_for_type.go

@@ -461,6 +462,11 @@ func new$.type|publicPlural$(c *$.GroupGoName$$.Version$Client) *$.type|privateP
 var listTemplate = `
 // List takes label and field selectors, and returns the list of $.resultType|publicPlural$ that match those selectors.
 func (c *$.type|privatePlural$) List(ctx context.Context, opts $.ListOptions|raw$) (result *$.resultType|raw$List, err error) {
+    defer func() {
+        if err != nil {


Is it really what you want? We rather want to call it if error was non-nil, right?

ups, yeah, it should have been if err == nil - thanks.

wojtek-t · 2024-05-20T12:46:49Z

staging/src/k8s.io/client-go/tools/cache/reflector.go

@@ -697,7 +697,7 @@ func (r *Reflector) watchList(stopCh <-chan struct{}) (watch.Interface, error) {
 	// we utilize the temporaryStore to ensure independence from the current store implementation.
 	// as of today, the store is implemented as a queue and will be drained by the higher-level
 	// component as soon as it finishes replacing the content.
-	checkWatchListConsistencyIfRequested(stopCh, r.name, resourceVersion, r.listerWatcher, temporaryStore)
+	checkWatchListDataConsistencyIfRequested(wait.ContextForChannel(stopCh), fmt.Sprintf("watch-list reflector with name: %q", r.name), resourceVersion, wrapListFuncWithContext(r.listerWatcher.List), temporaryStore.List)


Actually, seeing how you want to use it now and where are the problems, let's first merge this commit as a #124446

staging/src/k8s.io/code-generator/cmd/client-gen/generators/generator_for_type.go

wojtek-t · 2024-05-20T12:52:58Z

staging/src/k8s.io/client-go/tools/cache/reflector_data_consistency_detector.go

+	checkDataConsistency(ctx, identity, lastSyncedResourceVersion, listItemsFn, func() []runtime.Object { return rawListItems })
+}
+
+// wasListRequestServedFromStorage based on the passed ListOptions determines


We should rather evolve it to something like:

"if continuation wasn't set, the call LIST with RV= and ResourceVersionMatch=Exact and rest parameters the same"

Seems simpler, potentially makes this check unnecessarily, but it's not a big deal.

p0lyn0mial · 2024-05-27T14:25:52Z

/retest

p0lyn0mial · 2024-05-29T10:11:28Z

/test pull-kubernetes-node-e2e-containerd

deads2k · 2024-06-04T20:17:51Z

I approve of the approach, though I suspect further refactoring will be necessary for streaming list.

p0lyn0mial · 2024-06-05T07:32:27Z

I approve of the approach, though I suspect further refactoring will be necessary for streaming list.

thanks, yes, support for streaming list will require more changes to the code generation.

p0lyn0mial · 2024-06-06T07:57:57Z

OK, so this PR has a commit that always enables the detector for list requests. Here is the summary of test failures.

I think that we could initially enable the detector for an e2e job like pull-kubernetes-e2e-gce. If that's okay then I could create an issue to capture the following failures and fix them at a later point in time.

Unit Tests (3 failures)

TestEventIsSorted
The test produces fake data via the getFakeEvents() method, which doesn’t return a stable output (the time changes between invocations). For example:

EventTime: v1.MicroTime{Time: s"2024-06-05 11:01:04.54382 +0000 UTC"}
EventTime: v1.MicroTime{Time: s"2024-06-05 11:01:04.538752 +0000 UTC}

Test_buildClientCertificateManager
Sets up a fake/custom server which cannot handle the additional request made by the detector.
TestDrain
Sets up a fake/custom rest-client which is then used by a real client. The additional call made by the detector is intercepted by the test and, since it is unexpected, the test fails.

Integration Tests (2 failures)

TestExecPluginViaClient
This test is failing because it has a custom round tripper for collecting some data. The detector makes an additional call, causing the data to not match expectations.
TestLegacyServiceAccountTokenTracking
This test is failing because it has a custom round tripper (actually a warning handler) for collecting warnings. The detector makes an additional call, resulting in more warnings than expected.

…quested

fedebongio · 2024-06-06T16:36:11Z

/triage accepted

wojtek-t · 2024-06-07T06:00:59Z

Yes - the most important thing is that we understand the failures in unit/integration tests and those are test issues, not real issues. So that's fine.
I'm +1 on enabling that for e2e tests only (at least for now).

/lgtm
/approve

k8s-ci-robot · 2024-06-07T06:01:05Z

LGTM label has been added.

Git tree hash: dbdb31c5b4c899b9617fe73668262c8f5c915e79

wojtek-t · 2024-06-07T06:01:12Z

/kind feature
/retest

k8s-ci-robot · 2024-06-07T06:01:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, 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/apiextensions-apiserver/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/client-go/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/code-generator/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/metrics/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/sample-controller/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

k8s-ci-robot requested a review from caesarxuchao May 20, 2024 10:14

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2024

k8s-ci-robot requested a review from deads2k May 20, 2024 10:14

k8s-ci-robot assigned wojtek-t May 20, 2024

k8s-ci-robot requested a review from serathius May 20, 2024 10:14

p0lyn0mial commented May 20, 2024

View reviewed changes

p0lyn0mial force-pushed the upstream-data-consistency-checker-for-list-requests branch from 6fe1b36 to 6583cc6 Compare May 20, 2024 10:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2024

wojtek-t reviewed May 20, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2024

p0lyn0mial force-pushed the upstream-data-consistency-checker-for-list-requests branch from 6583cc6 to f7457cb Compare May 27, 2024 13:41

p0lyn0mial changed the title ~~[POC][WIP] data consistency checker for list requests~~ [WIP] data consistency checker for list requests May 27, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2024

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2024

p0lyn0mial force-pushed the upstream-data-consistency-checker-for-list-requests branch from 57c4f77 to 472043e Compare May 29, 2024 10:33

k8s-ci-robot added the area/apiserver label May 29, 2024

p0lyn0mial force-pushed the upstream-data-consistency-checker-for-list-requests branch from 472043e to 42da3cf Compare May 29, 2024 11:04

p0lyn0mial force-pushed the upstream-data-consistency-checker-for-list-requests branch 2 times, most recently from c7d63a2 to 06b7d97 Compare June 6, 2024 06:04

p0lyn0mial added 2 commits June 6, 2024 10:31

client-gen: rework list and use CheckListFromCacheDataConsistencyIfRe…

7c03e37

…quested

make update

448180d

p0lyn0mial force-pushed the upstream-data-consistency-checker-for-list-requests branch from 06b7d97 to 448180d Compare June 6, 2024 08:32

p0lyn0mial changed the title ~~[WIP] data consistency checker for list requests~~ client-go: data consistency checker for list requests Jun 6, 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 Jun 6, 2024

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

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jun 7, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2024

k8s-ci-robot merged commit 3532601 into kubernetes:master Jun 7, 2024
14 checks passed

k8s-ci-robot added this to the v1.31 milestone Jun 7, 2024

p0lyn0mial mentioned this pull request Jun 7, 2024

dynamic-client: data consistency checker for list requests #125304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client-go: data consistency checker for list requests #124963

client-go: data consistency checker for list requests #124963

p0lyn0mial commented May 20, 2024 •

edited

p0lyn0mial commented May 20, 2024

p0lyn0mial May 20, 2024

p0lyn0mial May 20, 2024 •

edited

p0lyn0mial May 20, 2024

wojtek-t May 20, 2024

p0lyn0mial May 20, 2024

wojtek-t May 20, 2024

wojtek-t May 20, 2024

p0lyn0mial commented May 27, 2024

p0lyn0mial commented May 29, 2024

deads2k commented Jun 4, 2024

p0lyn0mial commented Jun 5, 2024

p0lyn0mial commented Jun 6, 2024

fedebongio commented Jun 6, 2024

wojtek-t commented Jun 7, 2024

k8s-ci-robot commented Jun 7, 2024

wojtek-t commented Jun 7, 2024

k8s-ci-robot commented Jun 7, 2024

client-go: data consistency checker for list requests #124963

client-go: data consistency checker for list requests #124963

Conversation

p0lyn0mial commented May 20, 2024 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

p0lyn0mial commented May 20, 2024

p0lyn0mial May 20, 2024

Choose a reason for hiding this comment

p0lyn0mial May 20, 2024 • edited

Choose a reason for hiding this comment

p0lyn0mial May 20, 2024

Choose a reason for hiding this comment

wojtek-t May 20, 2024

Choose a reason for hiding this comment

p0lyn0mial May 20, 2024

Choose a reason for hiding this comment

wojtek-t May 20, 2024

Choose a reason for hiding this comment

wojtek-t May 20, 2024

Choose a reason for hiding this comment

p0lyn0mial commented May 27, 2024

p0lyn0mial commented May 29, 2024

deads2k commented Jun 4, 2024

p0lyn0mial commented Jun 5, 2024

p0lyn0mial commented Jun 6, 2024

Unit Tests (3 failures)

Integration Tests (2 failures)

fedebongio commented Jun 6, 2024

wojtek-t commented Jun 7, 2024

k8s-ci-robot commented Jun 7, 2024

wojtek-t commented Jun 7, 2024

k8s-ci-robot commented Jun 7, 2024

p0lyn0mial commented May 20, 2024 •

edited

p0lyn0mial May 20, 2024 •

edited