✨ Use watchList in typed and unstructured client if WatchListClient is enabled #3136

sbueringer · 2025-03-02T13:57:13Z

Signed-off-by: Stefan Büringer [email protected]

Fixes #3055

k8s-ci-robot · 2025-03-02T13:57:19Z

[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

sbueringer · 2025-03-02T13:57:35Z

/hold
First commit is from #3135

Otherwise ready for review

sbueringer · 2025-03-02T14:26:58Z

pkg/client/client_test.go

@@ -2199,1464 +2205,1759 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
 		})
 	})



I had to move the List tests out, but with a bit of creative diff'ing it should be okay'ish to review

sbueringer · 2025-03-02T14:33:28Z

pkg/client/client_test.go

+		})
+
+		// TODO(seans): get label selector test working
+		It("should filter results by label selector", func() {


Added this test as it was not implemented for unstructured before

Can the TODO go then?

Yup. Copied this from another case and forgot to look up why the TODO was there. Apparently when the TODO was added the test was commented out. Dropped the TODOs

sbueringer · 2025-03-02T14:34:04Z

pkg/client/client_test.go

+			deleteNamespace(ctx, clientset, tns4)
+		})
+
+		It("should filter results using limit and continue options", func() {


Added this test as it was not implemented for unstructured before

sbueringer · 2025-03-02T14:36:05Z

pkg/client/client_test.go


-			It("should fetch unstructured collection of objects, even if scheme is empty", func() {


Moved to the unstructured section

sbueringer · 2025-03-02T14:36:23Z

pkg/client/client_test.go


-			It("should fetch unstructured collection of objects", func() {


This test was redundant with the one in the unstructured section, so I deleted it

sbueringer · 2025-03-02T14:36:43Z

/assign @alvaroaleman @vincepri

sbueringer · 2025-03-02T16:03:56Z

/hold cancel

Rebased on top of main, now that #3135 is merged

sbueringer · 2025-03-02T16:04:40Z

/cc @p0lyn0mial @sttts (just fyi)

k8s-ci-robot · 2025-03-02T16:04:43Z

@sbueringer: GitHub didn't allow me to request PR reviews from the following users: p0lyn0mial.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @p0lyn0mial @sttts

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.

alvaroaleman · 2025-03-02T16:47:24Z

pkg/client/typed_client.go

+		if err == nil {
+			return nil
+		}
+		log.FromContext(ctx).Error(err, fmt.Sprintf("Warning: The watchlist request for %s ended with an error, falling back to the standard LIST semantics", r.resource()))


If I understand it correctly, this failing is expected if the KAS is too old, right? If yes, then this i IMHO a debug log. The other log re preparing watchlist opts however seems like an actual error log because that is never expected to happen or am I missunderstanding things?

Took another look.

The other log re preparing watchlist opts however seems like an actual error log because that is never expected to happen

I agree. PrepareWatchListOptionsFromListOptions only returns an error if scheme.Convert fails.

this failing is expected if the KAS is too old, right? If yes, then this i IMHO a debug log.

This error will happen whenever the WatchList feature is disabled in KAS (either because KAS is too old to have the feature or because it's explicitly disabled).

I personally would never enable the WatchListClient client-side feature gate if I"m not 100% sure that I'm communicating with a v1.32 apiserver. The reason for that is that the fallback behavior means that every single List call will fail with a WatchList first. That's pretty disastrous if it happens in my opinion, so I thought it's good if in these cases the log message shows up so they can fix their config (basically it surfaces a client-side configuration error).

I personally would never enable the WatchListClient client-side feature gate if I"m not 100% sure that I'm communicating with a v1.32 apiserver

Right but this will be enabled by default in 1.33 client-go or not? So its not something people will be aware of.

Right but this will be enabled by default in 1.33 client-go or not? So its not something people will be aware of.

Hm no idea when it will be enabled per default, it's currently not enabled per default in k/k main: https://github.com/kubernetes/kubernetes/blob/a9aab298b4738f4ea9111131cdf193a3b1ba14e5/staging/src/k8s.io/client-go/features/known_features.go#L83

But that's actually a good point. As soon as this is enabled per default, folks will hit these failure cases if they hit an apiserver which is too old which is not great (I think the log message is the smallest problem here :))

@p0lyn0mial I couldn't find a tracking issue for WatchListClient. What are the current plans regarding enabling this per default? Or is the idea to keep it of for the forseeable future to avoid that folks are implicitly hitting these error cases with old apiservers simply by bumping client-go?

Found the tracking issue: kubernetes/enhancements#3157 Still not sure about when WatchListClient would be enabled per default though :)

The plan is to enable the feature (WatchListClient) by default on 1.34 (next release). The issue is quite outdated :)

Okay, I think that means a lot of folks will have to expliclity set the feature gate to false starting with client-go v1.34 to not run into problems.

pkg/client/client_test.go

alvaroaleman · 2025-03-02T16:50:28Z

pkg/client/client_test.go

+		})
+
+		// TODO(seans): get label selector test working
+		It("should filter results by label selector", func() {


Can the TODO go then?

sbueringer · 2025-03-07T06:08:28Z

/hold
Until we have consensus if and how we want to support watchlist in our live clients.

caozhuozi · 2025-03-08T03:24:41Z

is it possible we can have a one-shot global check for whether we have WATCH permission at the controller-runtime start?

sbueringer · 2025-03-08T05:41:58Z

is it possible we can have a one-shot global check for whether we have WATCH permission at the controller-runtime start?

In general roughly yes, although I would implement it differently. But we're going to wait to see what will be done in client-go: https://kubernetes.slack.com/archives/C0EG7JC6T/p1741283769908819?thread_ts=1741283769.908819&cid=C0EG7JC6T

…enabled

sbueringer · 2025-03-08T12:55:52Z

(just rebased onto main & added a commit to ensure all unstructured & partialobjectmeta tests use a client with an empty scheme)

k8s-ci-robot requested review from FillZpp and troy0820 March 2, 2025 13:57

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 2, 2025

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

sbueringer mentioned this pull request Mar 2, 2025

Ensure APIStreaming is used if available #3055

Open

sbueringer commented Mar 2, 2025

View reviewed changes

sbueringer force-pushed the pr-api-streaming branch from 9b8b4c1 to 44cbac6 Compare March 2, 2025 14:33

sbueringer commented Mar 2, 2025

View reviewed changes

k8s-ci-robot assigned alvaroaleman and vincepri Mar 2, 2025

sbueringer force-pushed the pr-api-streaming branch from 44cbac6 to ae53a80 Compare March 2, 2025 16:03

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

k8s-ci-robot requested a review from sttts March 2, 2025 16:04

alvaroaleman reviewed Mar 2, 2025

View reviewed changes

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 3, 2025

sbueringer mentioned this pull request Mar 3, 2025

🌱 Validate that unstructued objects don't require scheme registration #3138

Merged

alvaroaleman mentioned this pull request Mar 6, 2025

List calls in a k8s 1.34 client will make additional api requests and log a warning w/o watch perms kubernetes/kubernetes#130619

Open

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

sbueringer added 3 commits March 8, 2025 13:39

Use watchList in typed and unstructured client if WatchListClient is …

abe7bd5

…enabled

Fix review finding

48dcfc4

Use minimal scheme for all unstructured & metadata tests

ea98b9a

sbueringer force-pushed the pr-api-streaming branch from 65a10e2 to ea98b9a Compare March 8, 2025 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Use watchList in typed and unstructured client if WatchListClient is enabled #3136

✨ Use watchList in typed and unstructured client if WatchListClient is enabled #3136

sbueringer commented Mar 2, 2025 •

edited

Loading

k8s-ci-robot commented Mar 2, 2025

sbueringer commented Mar 2, 2025

sbueringer Mar 2, 2025

sbueringer Mar 2, 2025

alvaroaleman Mar 2, 2025

sbueringer Mar 3, 2025

sbueringer Mar 2, 2025

sbueringer Mar 2, 2025

sbueringer Mar 2, 2025

sbueringer commented Mar 2, 2025

sbueringer commented Mar 2, 2025

sbueringer commented Mar 2, 2025 •

edited

Loading

k8s-ci-robot commented Mar 2, 2025

alvaroaleman Mar 2, 2025

sbueringer Mar 3, 2025 •

edited

Loading

alvaroaleman Mar 3, 2025

sbueringer Mar 3, 2025

sbueringer Mar 5, 2025

p0lyn0mial Mar 6, 2025

sbueringer Mar 6, 2025

alvaroaleman Mar 2, 2025

sbueringer commented Mar 7, 2025

caozhuozi commented Mar 8, 2025

sbueringer commented Mar 8, 2025 •

edited

Loading

sbueringer commented Mar 8, 2025 •

edited

Loading

		@@ -2199,1464 +2205,1759 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
		})
		})


		It("should fetch unstructured collection of objects, even if scheme is empty", func() {

✨ Use watchList in typed and unstructured client if WatchListClient is enabled #3136

Are you sure you want to change the base?

✨ Use watchList in typed and unstructured client if WatchListClient is enabled #3136

Conversation

sbueringer commented Mar 2, 2025 • edited Loading

k8s-ci-robot commented Mar 2, 2025

sbueringer commented Mar 2, 2025

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

sbueringer commented Mar 2, 2025

sbueringer commented Mar 2, 2025

sbueringer commented Mar 2, 2025 • edited Loading

k8s-ci-robot commented Mar 2, 2025

Choose a reason for hiding this comment

sbueringer Mar 3, 2025 • edited Loading

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

sbueringer commented Mar 7, 2025

caozhuozi commented Mar 8, 2025

sbueringer commented Mar 8, 2025 • edited Loading

sbueringer commented Mar 8, 2025 • edited Loading

sbueringer commented Mar 2, 2025 •

edited

Loading

sbueringer commented Mar 2, 2025 •

edited

Loading

sbueringer Mar 3, 2025 •

edited

Loading

sbueringer commented Mar 8, 2025 •

edited

Loading

sbueringer commented Mar 8, 2025 •

edited

Loading