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.

Sign up for GitHub

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

check the scalability of one kind prior to get its scale subresource from apiserver #6431

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

check the scalability of one kind prior to get its scale subresource from apiserver #6431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 29 additions & 15 deletions vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ type ControllerFetcher interface {
type controllerFetcher struct {
scaleNamespacer scale.ScalesGetter
mapper apimeta.RESTMapper
cachedDiscoveryClient discovery.DiscoveryInterface
informersMap map[wellKnownController]cache.SharedIndexInformer
scaleSubresourceCacheStorage controllerCacheStorage
}
Expand Down Expand Up @@ -146,6 +147,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),
}
Expand Down Expand Up @@ -206,6 +208,12 @@ func (f *controllerFetcher) getParentOfController(ctx context.Context, controlle
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
Expand Down Expand Up @@ -258,27 +266,33 @@ func (f *controllerFetcher) isWellKnownOrScalable(ctx context.Context, key *Cont
return true
}

//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 "<key>/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.ErrorS(err, "Could not find groupKind", "object", klog.KRef(key.Namespace, key.Name))
klog.ErrorS(err, "Could not find groupKind", "kind", kind)
return false
}

if wellKnownController(groupKind.Kind) == node {
return false
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 r.Kind == kind {
plural = r.Name
break
}
}

mappings, err := f.mapper.RESTMappings(groupKind)
if err != nil {
klog.ErrorS(err, "Could not find mappings", "groupKind", groupKind, "object", klog.KRef(key.Namespace, key.Name))
if plural == "" {
klog.ErrorS(fmt.Errorf("could not find plural name"), "Could not find plural name", "kind", kind)
return false
}

for _, mapping := range mappings {
groupResource := mapping.Resource.GroupResource()
scale, err := f.getScaleForResource(ctx, key.Namespace, groupResource, key.Name)
if err == nil && scale != nil {
expectedScaleResource := fmt.Sprintf("%s/%s", plural, "scale")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Assuming the RBAC issues mentioned earlier are OK)

Could we use the ScaleKindResolver?

If you look at the implementation, it is effectively doing what you are doing here by looping over the DiscoveryClient resources. Using that directly would cut down the amount of code needed here :)

Copy link
Author

@yunwang0911 yunwang0911 May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I'll replace it by ScaleKindResolver. Thanks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Author

@yunwang0911 yunwang0911 May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstand the function of ScaleKindResolver. Actually, it's not work as I expected. Let me elaborate the difference between current implement, ScaleKindResolver and the implement in this PR.

for _, r := range resourceList.APIResources {
if r.Name == expectedScaleResource {
return true
}
}
Expand Down
60 changes: 57 additions & 3 deletions vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,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"
Expand Down Expand Up @@ -61,6 +63,57 @@ func simpleControllerFetcher() *controllerFetcher {
versioned := map[string][]metav1.APIResource{
"Foo": {{Kind: "Foo", Name: "bah", Group: "foo"}, {Kind: "Scale", Name: "iCanScale", Group: "foo"}},
}
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",
},
{
Name: "scales",
Namespaced: true,
Kind: "Scale",
Group: "Foo",
Version: "Foo",
},
{
Name: "scales/scale",
Namespaced: true,
Kind: "Scale",
Group: "Foo",
Version: "Foo",
},
},
},
{
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{
Expand All @@ -76,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")
Expand Down Expand Up @@ -389,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,
},
{
name: "custom resource with no scale subresource",
Expand Down
Loading