Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure APIStreaming is used if available #3055

Open
alvaroaleman opened this issue Jan 2, 2025 · 10 comments
Open

Ensure APIStreaming is used if available #3055

alvaroaleman opened this issue Jan 2, 2025 · 10 comments

Comments

@alvaroaleman
Copy link
Member

We should be using https://kubernetes.io/blog/2024/12/17/kube-apiserver-api-streaming/ if the server supports it. It could be that there are no changes required for that, but we should validate that assumption before the next release.

@nathanperkins
Copy link

Thanks! Looking forward to this feature and was wondering how we would enable it in controller-runtime. Automatic enablement would be really cool :)

@sbueringer
Copy link
Member

sbueringer commented Jan 7, 2025

I think CR should have the same default behavior as the underlying client-go version (it's not enabled per default in client-go v0.32)

@sttts
Copy link

sttts commented Jan 9, 2025

xref @p0lyn0mial's https://github.com/kubernetes-sigs/controller-runtime/pull/2934/files where he proved that controller-runtime can adopt watchlist.

@p0lyn0mial
Copy link

xref @p0lyn0mial's https://github.com/kubernetes-sigs/controller-runtime/pull/2934/files where he proved that controller-runtime can adopt watchlist.

the test results from the pr can be found in this comment.

@alvaroaleman
Copy link
Member Author

One thing @sbueringer noticed is that it doesn't seem the live clients LIST will use watchlist under the hood by default

And generally speaking, this doesn't necessarily mean that watchlist was actually used, just that nothing broke or am I missing something?

All tests from controller-runtime passed, except for ClientWithCache Get when getting unstructured objects should call client reader when not cached which doesn't use the watchlist feature.

@alvaroaleman
Copy link
Member Author

For the cache, it seems our usage of cache.NewSharedIndexInformer will give us WatchList support if available without anything further needed (It gets set from the client-go featuregate in NewReflectorWithOptions which in turn is called from *controller.Run which in turn is called from *sharedIndexInformer.Run), is that correct @p0lyn0mial ?

I think to use WatchList in the live client, a change like this is required for typed/unstructured/metadata, similiar to what the generated clients do:

$ g diff
diff --git a/pkg/client/typed_client.go b/pkg/client/typed_client.go
index 92afd9a9..63014d9b 100644
--- a/pkg/client/typed_client.go
+++ b/pkg/client/typed_client.go
@@ -20,6 +20,7 @@ import (
        "context"

        "k8s.io/apimachinery/pkg/runtime"
+       "k8s.io/client-go/util/watchlist"
 )

 var _ Reader = &typedClient{}
@@ -157,6 +158,17 @@ func (c *typedClient) List(ctx context.Context, obj ObjectList, opts ...ListOpti
        listOpts := ListOptions{}
        listOpts.ApplyOptions(opts)

+       watchListOptions, hasWatchListOptionsPrepared, err := watchlist.PrepareWatchListOptionsFromListOptions(*listOpts.AsListOptions())
+       if err == nil && hasWatchListOptionsPrepared {
+               if err := r.Get().
+                       NamespaceIfScoped(listOpts.Namespace, r.isNamespaced()).
+                       Resource(r.resource()).
+                       VersionedParams(&watchListOptions, c.paramCodec).
+                       WatchList(ctx).
+                       Into(obj); err == nil {
+                       return nil
+               }
+       }
        return r.Get().
                NamespaceIfScoped(listOpts.Namespace, r.isNamespaced()).
                Resource(r.resource()).

I can not off-hand think of a good way to write tests for this though

@p0lyn0mial
Copy link

There are two FGs. The WatchList used by the kube-apiserver and enabled by default as of 1.32 and the WatchListClient used by the client (client-go/informers) disabled by default.

In order to use the new feature on a 1.32 cluster you need to explicitly enable WatchListClient for client-go library. I had to force it in my PR to test the new feature.
After enabling the FG it will work for both the informers and the "live" client/calls.

@sbueringer
Copy link
Member

The live client Alvaro is referring to is implemented here:

func (c *typedClient) List(ctx context.Context, obj ObjectList, opts ...ListOption) error {
(or at least this is the part for typed objects). We are not using a List method from client-go.

	r, err := c.resources.getResource(obj)
	if err != nil {
		return err
	}

	listOpts := ListOptions{}
	listOpts.ApplyOptions(opts)

	return r.Get().
		NamespaceIfScoped(listOpts.Namespace, r.isNamespaced()).
		Resource(r.resource()).
		VersionedParams(listOpts.AsListOptions(), c.paramCodec).
		Do(ctx).
		Into(obj)

Our assumption at the moment is that this doesn't automatically use Watchlist when the WatchListClient feature flag is enabled.

@p0lyn0mial
Copy link

Our assumption at the moment is that this doesn't automatically use Watchlist when the WatchListClient feature flag is enabled.

ah, yes, this is correct, the feature was added to the generated client not to the rest client int the client-go lib.

@alvaroaleman
Copy link
Member Author

Note: Due to lack of time from the maintainers, we will not update the live client to use apistreaming for the next c-r release. It already works in the cache which is the majority use-case and is default disabled in client-go anyways, so we do not want to block the release on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants