🌱 inmemory: fix watch to continue serving based on resourceVersion parameter #11695

chrischdi · 2025-01-17T08:56:08Z

What this PR does / why we need it:

Watches on remote cluster expires every 10s #8893 leads to watches getting recreated every 10s
When using the inmemory provider, changes during recreation of a watch got lost
- e.g. Node got created in the simulated cluster
- e.g. Pd got created in the simulated cluster
This caused the controllers to require reaching the sync period (or e.g. in the Node missing case, MHC kicked in at 10 mins and the machine got recreated)
This lead to e.g. longer taking scale tests due to e.g. machines not getting ready because an event got missed

This PR has the following changes:

Change the way in-memory provider sets resourceVersions, to have one increasing resourceVersion per simulated cluster (similar to what k8s is doing)
use the resourceVersion parameter on watches (if given) to enqueue events for all objects which have a higher resourceVersion

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

/area provider/infrastructure-in-memory

chrischdi · 2025-01-17T08:57:49Z

Kudos to @sbueringer for helping analysing and brainstorming to find the root cause.

chrischdi · 2025-01-17T09:03:10Z

/test help

k8s-ci-robot · 2025-01-17T09:03:13Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main

/test pull-cluster-api-e2e-blocking-main

/test pull-cluster-api-e2e-conformance-ci-latest-main

/test pull-cluster-api-e2e-conformance-main

/test pull-cluster-api-e2e-latestk8s-main

/test pull-cluster-api-e2e-main

/test pull-cluster-api-e2e-mink8s-main

/test pull-cluster-api-e2e-upgrade-1-32-1-33-main

/test pull-cluster-api-test-main

/test pull-cluster-api-test-mink8s-main

/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main

pull-cluster-api-build-main

pull-cluster-api-e2e-blocking-main

pull-cluster-api-test-main

pull-cluster-api-verify-main

In response to this:

/test help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

chrischdi · 2025-01-17T09:03:23Z

/test pull-cluster-api-e2e-main

test/infrastructure/inmemory/pkg/server/api/watch.go

chrischdi · 2025-01-17T13:39:53Z

/test pull-cluster-api-e2e-main

test/infrastructure/inmemory/pkg/server/api/watch.go

fabriziopandini

Great job in investigating and fixing this issue, kudos!

fabriziopandini · 2025-01-17T19:39:26Z

test/infrastructure/inmemory/pkg/runtime/cache/client.go

@@ -145,6 +145,9 @@ func (c *cache) List(resourceGroup string, list client.ObjectList, opts ...clien
 	if err := meta.SetList(list, items); err != nil {
 		return apierrors.NewInternalError(err)
 	}
+
+	list.SetResourceVersion(fmt.Sprintf("%d", tracker.lastResourceVersion))


Q: is it correct that list ResourceVersion in the system and not the last resource version in the list of items?
Q: is is correct set set this value non matter of it is a "plain" list or a lis watch?
(from a quick check with kubectl yes to both, but I like a confirmation)

Q: is it correct that list ResourceVersion in the system and not the last resource version in the list of items?

In case of kube-apiserver it sets the resourceVersion of the system, which makes sense because new events will only have a higher resourceVersion.
Any new event (create/update/delete) will have a higher resourceVersion, no matter which type of object is going to change.

For the in-memory provider it does not make a difference. For kube-apiserver its a performance thing: its simply unnecessary to go through all events between highest resourceversion of objects in list and last used resourceversion of the system.

Also: It would not make sense to set the list's resourceVersion to the highest resourceVersion of all items, because it would be a duplication of information.

Q: is is correct set set this value non matter of it is a "plain" list or a lis watch?
(from a quick check with kubectl yes to both, but I like a confirmation)

kubectl get just hides/removes the resourceVersion of a list:

❯ k get no -v=8 -o yaml ... I0120 09:19:14.542768 77874 request.go:1212] Response Body: {"kind":"NodeList","apiVersion":"v1","metadata":{"resourceVersion":"195803"},"items":[{" ... ... kind: List metadata: resourceVersion: "" ...

I think the second question was about "list watch". @fabriziopandini Are you referring to the new ListWatch beta feature in Kubernetes 1.32?

I don't think we have implemented ListWatch yet, though
(also it's disabled per default in client-go atm so we're not forced to implement it yet)

I was not thinking about ListWatch :D

But overall looks fine:

List: we have to set it

Watch: the "list" is not send to the client

ListWatch: not implemented

test/infrastructure/inmemory/pkg/server/api/handler.go

test/infrastructure/inmemory/pkg/server/api/watch.go

chrischdi · 2025-01-20T09:16:22Z

/test pull-cluster-api-e2e-main

fabriziopandini · 2025-01-20T09:47:22Z

/lgtm
Great work!

k8s-ci-robot · 2025-01-20T09:47:28Z

LGTM label has been added.

Git tree hash: 914bd9fbece01b130980a84832feef4bcf5e2f79

sbueringer · 2025-01-20T09:41:42Z

test/infrastructure/inmemory/pkg/runtime/cache/client.go

@@ -145,6 +145,9 @@ func (c *cache) List(resourceGroup string, list client.ObjectList, opts ...clien
 	if err := meta.SetList(list, items); err != nil {
 		return apierrors.NewInternalError(err)
 	}
+
+	list.SetResourceVersion(fmt.Sprintf("%d", tracker.lastResourceVersion))


I think the second question was about "list watch". @fabriziopandini Are you referring to the new ListWatch beta feature in Kubernetes 1.32?

I don't think we have implemented ListWatch yet, though
(also it's disabled per default in client-go atm so we're not forced to implement it yet)

test/infrastructure/inmemory/pkg/server/api/watch.go

sbueringer · 2025-01-20T10:02:47Z

/cherry-pick release-1.9

k8s-infra-cherrypick-robot · 2025-01-20T10:02:50Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.9 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sbueringer · 2025-01-20T11:11:01Z

Thank you!

/lgtm
/approve

k8s-ci-robot · 2025-01-20T11:11:08Z

LGTM label has been added.

Git tree hash: 85a402458616ed3456dad109453fa94ffbc67c47

k8s-ci-robot · 2025-01-20T11:11:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

~~OWNERS~~ [sbueringer]

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

k8s-infra-cherrypick-robot · 2025-01-20T11:57:22Z

@sbueringer: new pull request created: #11710

In response to this:

/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

fix resourceVersion to increment across a tracker

e08e606

chrischdi added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 17, 2025

k8s-ci-robot added area/provider/infrastructure-in-memory Issues or PRs related to the in-memory infrastructure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 17, 2025

k8s-ci-robot requested review from jackfrancis and richardcase January 17, 2025 08:56

chrischdi force-pushed the pr-inmemory-fix-watch-resourceversion branch from 54e7e43 to 1b644a2 Compare January 17, 2025 08:56

chrischdi commented Jan 17, 2025

View reviewed changes

test/infrastructure/inmemory/pkg/server/api/watch.go Show resolved Hide resolved

sbueringer reviewed Jan 17, 2025

View reviewed changes

test/infrastructure/inmemory/pkg/server/api/watch.go Show resolved Hide resolved

test/infrastructure/inmemory/pkg/server/api/watch.go Outdated Show resolved Hide resolved

inmemory: implement continuing watch based on resourceVersion

3bac618

chrischdi force-pushed the pr-inmemory-fix-watch-resourceversion branch from 1b644a2 to 3bac618 Compare January 17, 2025 13:18

test fixes

cce5bde

chrischdi added 3 commits January 17, 2025 15:18

inmemory: add some debug log lines for watches

b4ed0fc

stream all events if resourceVersion is not given

42309df

fixes

3054216

sbueringer reviewed Jan 17, 2025

View reviewed changes

test/infrastructure/inmemory/pkg/server/api/watch.go Show resolved Hide resolved

test/infrastructure/inmemory/pkg/server/api/watch.go Outdated Show resolved Hide resolved

test/infrastructure/inmemory/pkg/server/api/watch.go Outdated Show resolved Hide resolved

chrischdi added 3 commits January 17, 2025 17:31

set proper resourceVersion on list calls

caf66a6

fixup tests

e2d871c

another fixes

36fa992

fabriziopandini reviewed Jan 17, 2025

View reviewed changes

review fixes

a30463b

k8s-ci-robot assigned fabriziopandini Jan 20, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2025

sbueringer reviewed Jan 20, 2025

View reviewed changes

fixup

80fb9db

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2025

k8s-ci-robot requested a review from fabriziopandini January 20, 2025 11:10

k8s-ci-robot assigned sbueringer Jan 20, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2025

k8s-ci-robot merged commit d01f2a8 into kubernetes-sigs:main Jan 20, 2025
18 checks passed

k8s-ci-robot added this to the v1.10 milestone Jan 20, 2025

k8s-infra-cherrypick-robot mentioned this pull request Jan 20, 2025

[release-1.9] 🌱 inmemory: fix watch to continue serving based on resourceVersion parameter #11710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 inmemory: fix watch to continue serving based on resourceVersion parameter #11695

🌱 inmemory: fix watch to continue serving based on resourceVersion parameter #11695

chrischdi commented Jan 17, 2025

chrischdi commented Jan 17, 2025

chrischdi commented Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

chrischdi commented Jan 17, 2025

chrischdi commented Jan 17, 2025

fabriziopandini left a comment

fabriziopandini Jan 17, 2025

chrischdi Jan 20, 2025

sbueringer Jan 20, 2025

chrischdi Jan 20, 2025

sbueringer Jan 20, 2025

chrischdi commented Jan 20, 2025

fabriziopandini commented Jan 20, 2025

k8s-ci-robot commented Jan 20, 2025

sbueringer Jan 20, 2025

sbueringer commented Jan 20, 2025

k8s-infra-cherrypick-robot commented Jan 20, 2025

sbueringer commented Jan 20, 2025

k8s-ci-robot commented Jan 20, 2025

k8s-ci-robot commented Jan 20, 2025

k8s-infra-cherrypick-robot commented Jan 20, 2025

🌱 inmemory: fix watch to continue serving based on resourceVersion parameter #11695

🌱 inmemory: fix watch to continue serving based on resourceVersion parameter #11695

Conversation

chrischdi commented Jan 17, 2025

chrischdi commented Jan 17, 2025

chrischdi commented Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

chrischdi commented Jan 17, 2025

chrischdi commented Jan 17, 2025

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Jan 17, 2025

Choose a reason for hiding this comment

chrischdi Jan 20, 2025

Choose a reason for hiding this comment

sbueringer Jan 20, 2025

Choose a reason for hiding this comment

chrischdi Jan 20, 2025

Choose a reason for hiding this comment

sbueringer Jan 20, 2025

Choose a reason for hiding this comment

chrischdi commented Jan 20, 2025

fabriziopandini commented Jan 20, 2025

k8s-ci-robot commented Jan 20, 2025

sbueringer Jan 20, 2025

Choose a reason for hiding this comment

sbueringer commented Jan 20, 2025

k8s-infra-cherrypick-robot commented Jan 20, 2025

sbueringer commented Jan 20, 2025

k8s-ci-robot commented Jan 20, 2025

k8s-ci-robot commented Jan 20, 2025

k8s-infra-cherrypick-robot commented Jan 20, 2025