From c37839a6054236eedabf4ffd02416db38516cf31 Mon Sep 17 00:00:00 2001 From: yunwang Date: Tue, 9 Jan 2024 15:08:33 +0800 Subject: [PATCH] use cached discovery client to check scalable --- .../controller_fetcher/controller_fetcher.go | 42 ++++++++---- .../controller_fetcher_test.go | 65 +++++++++++++++++-- 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go index dcbd11ee210d..8d139989017d 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go @@ -19,6 +19,7 @@ package controllerfetcher import ( "context" "fmt" + "strings" "time" appsv1 "k8s.io/api/apps/v1" @@ -81,6 +82,7 @@ type ControllerFetcher interface { type controllerFetcher struct { scaleNamespacer scale.ScalesGetter mapper apimeta.RESTMapper + cachedDiscoveryClient discovery.DiscoveryInterface informersMap map[wellKnownController]cache.SharedIndexInformer scaleSubresourceCacheStorage controllerCacheStorage } @@ -146,6 +148,7 @@ func NewControllerFetcher(config *rest.Config, kubeClient kube_client.Interface, return &controllerFetcher{ scaleNamespacer: scaleNamespacer, mapper: mapper, + cachedDiscoveryClient: cachedDiscoveryClient, informersMap: informersMap, scaleSubresourceCacheStorage: newControllerCacheStorage(betweenRefreshes, lifeTime, jitterFactor), } @@ -206,6 +209,12 @@ func (f *controllerFetcher) getParentOfController(controllerKey ControllerKeyWit return getParentOfWellKnownController(informer, controllerKey) } + // check if it's scalable + scalable := f.isScalable(controllerKey.ApiVersion, controllerKey.Kind) + if !scalable { + return nil, nil + } + groupKind, err := controllerKey.groupKind() if err != nil { return nil, err @@ -261,23 +270,32 @@ func (f *controllerFetcher) isWellKnownOrScalable(key *ControllerKeyWithAPIVersi return false } - //if not well known check if it supports scaling - groupKind, err := key.groupKind() + return f.isScalable(key.ApiVersion, key.Kind) +} + +// isScalable returns true if the given controller is scalable. +// isScalable checks if the controller is scalable by checking if the resource "/scale" exists in the cached discovery client. +func (f *controllerFetcher) isScalable(apiVersion string, kind string) bool { + resourceList, err := f.cachedDiscoveryClient.ServerResourcesForGroupVersion(apiVersion) if err != nil { - klog.Errorf("Could not find groupKind for %s/%s: %v", key.Namespace, key.Name, err) + klog.Errorf("Could not find resources for %s: %v", apiVersion, err) return false } - - mappings, err := f.mapper.RESTMappings(groupKind) - if err != nil { - klog.Errorf("Could not find mappings for %s: %v", groupKind, err) + var plural string + for _, r := range resourceList.APIResources { + // Ref https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#types-kinds + // the kind is the singular name of the resource + if strings.EqualFold(r.SingularName, kind) { + plural = r.Name + break + } + } + if plural == "" { return false } - - for _, mapping := range mappings { - groupResource := mapping.Resource.GroupResource() - scale, err := f.getScaleForResource(key.Namespace, groupResource, key.Name) - if err == nil && scale != nil { + expectedScaleResource := fmt.Sprintf("%s/%s", plural, "scale") + for _, r := range resourceList.APIResources { + if r.Name == expectedScaleResource { return true } } diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go index 2ab3a26188f0..7e3fd6e7aaee 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go @@ -31,6 +31,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + cacheddiscovery "k8s.io/client-go/discovery/cached" + "k8s.io/client-go/discovery/fake" "k8s.io/client-go/restmapper" scalefake "k8s.io/client-go/scale/fake" core "k8s.io/client-go/testing" @@ -56,8 +58,62 @@ func simpleControllerFetcher() *controllerFetcher { f.informersMap = make(map[wellKnownController]cache.SharedIndexInformer) f.scaleSubresourceCacheStorage = newControllerCacheStorage(time.Second, time.Minute, 0.1) versioned := map[string][]metav1.APIResource{ - "Foo": {{Kind: "Foo", Name: "bah", Group: "foo"}, {Kind: "Scale", Name: "iCanScale", Group: "foo"}}, + "Foo": {{Kind: "Foo", Name: "bah", Group: "foo", SingularName: "foo"}, {Kind: "Scale", Name: "iCanScale", Group: "foo", SingularName: "scale"}}, } + fakeDiscoveryClient := &fake.FakeDiscovery{ + Fake: &core.Fake{Resources: []*metav1.APIResourceList{ + { + TypeMeta: metav1.TypeMeta{ + Kind: "APIResourceList", + APIVersion: "v1", + }, + GroupVersion: "Foo/Foo", + APIResources: []metav1.APIResource{ + { + Name: "foos", + Namespaced: true, + Kind: "Foo", + Group: "Foo", + Version: "Foo", + SingularName: "foo", + }, + { + Name: "scales", + Namespaced: true, + Kind: "Scale", + Group: "Foo", + Version: "Foo", + SingularName: "scale", + }, + { + Name: "scales/scale", + Namespaced: true, + Kind: "Scale", + Group: "Foo", + Version: "Foo", + SingularName: "scale", + }, + }, + }, + { + TypeMeta: metav1.TypeMeta{ + Kind: "APIResourceList", + APIVersion: "v1", + }, + GroupVersion: "v1", + APIResources: []metav1.APIResource{ + { + Name: "deployments", + SingularName: "deployment", + Namespaced: true, + Kind: "Deployment", + Group: "", + Version: "v1", + }, + }, + }}}, + } + f.cachedDiscoveryClient = cacheddiscovery.NewMemCacheClient(fakeDiscoveryClient) fakeMapper := []*restmapper.APIGroupResources{ { Group: metav1.APIGroup{ @@ -73,7 +129,7 @@ func simpleControllerFetcher() *controllerFetcher { scaleNamespacer := &scalefake.FakeScaleClient{} f.scaleNamespacer = scaleNamespacer - // return not found if if tries to find the scale subresource on bah + // return not found if it tries to find the scale subresource on bah scaleNamespacer.AddReactor("get", "bah", func(action core.Action) (handled bool, ret runtime.Object, err error) { groupResource := schema.GroupResource{} error := apierrors.NewNotFound(groupResource, "Foo") @@ -386,8 +442,9 @@ func TestControllerFetcher(t *testing.T) { }, }, }}, - expectedKey: nil, - expectedError: fmt.Errorf("Unhandled targetRef v1 / Node / node, last error node is not a valid owner"), + expectedKey: &ControllerKeyWithAPIVersion{ControllerKey: ControllerKey{ + Name: testDeployment, Kind: "Deployment", Namespace: testNamespace}}, // Node does not support scale subresource so should return itself" + expectedError: nil, }, } { t.Run(tc.name, func(t *testing.T) {