diff --git a/vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go b/vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go index b8dfa3a4cd31..3ab275486d84 100644 --- a/vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go +++ b/vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go @@ -458,15 +458,15 @@ func (feeder *clusterStateFeeder) validateTargetRef(vpa *vpa_types.VerticalPodAu }, ApiVersion: vpa.Spec.TargetRef.APIVersion, } - top, err := feeder.controllerFetcher.FindTopLevel(&k) + top, err := feeder.controllerFetcher.FindTopMostWellKnownOrScalable(&k) if err != nil { - return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Error checking if target is a top level controller: %s", err)} + return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Error checking if target is a topmost well-known or scalable controller: %s", err)} } if top == nil { - return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Unknown error during checking if target is a top level controller: %s", err)} + return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Unknown error during checking if target is a topmost well-known or scalable controller: %s", err)} } if *top != k { - return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: "The targetRef controller has a parent but it should point to a top-level controller"} + return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: "The targetRef controller has a parent but it should point to a topmost well-known or scalable controller"} } return true, condition{} } diff --git a/vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go b/vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go index cfce8add2909..6465fd9f633d 100644 --- a/vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go +++ b/vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go @@ -39,7 +39,7 @@ type fakeControllerFetcher struct { err error } -func (f *fakeControllerFetcher) FindTopLevel(controller *controllerfetcher.ControllerKeyWithAPIVersion) (*controllerfetcher.ControllerKeyWithAPIVersion, error) { +func (f *fakeControllerFetcher) FindTopMostWellKnownOrScalable(controller *controllerfetcher.ControllerKeyWithAPIVersion) (*controllerfetcher.ControllerKeyWithAPIVersion, error) { return f.key, f.err } @@ -55,8 +55,8 @@ var ( unsupportedConditionNoExtraText = "Cannot read targetRef" unsupportedConditionBothDefined = "Both targetRef and label selector defined. Please remove label selector" unsupportedConditionNoTargetRef = "Cannot read targetRef" - unsupportedConditionMudaMudaMuda = "Error checking if target is a top level controller: muda muda muda" - unsupportedTargetRefHasParent = "The targetRef controller has a parent but it should point to a top-level controller" + unsupportedConditionMudaMudaMuda = "Error checking if target is a topmost well-known or scalable controller: muda muda muda" + unsupportedTargetRefHasParent = "The targetRef controller has a parent but it should point to a topmost well-known or scalable controller" ) const ( @@ -70,15 +70,15 @@ const ( func TestLoadPods(t *testing.T) { type testCase struct { - name string - selector labels.Selector - fetchSelectorError error - targetRef *autoscalingv1.CrossVersionObjectReference - topLevelKey *controllerfetcher.ControllerKeyWithAPIVersion - findTopLevelError error - expectedSelector labels.Selector - expectedConfigUnsupported *string - expectedConfigDeprecated *string + name string + selector labels.Selector + fetchSelectorError error + targetRef *autoscalingv1.CrossVersionObjectReference + topMostWellKnownOrScalableKey *controllerfetcher.ControllerKeyWithAPIVersion + findTopMostWellKnownOrScalableError error + expectedSelector labels.Selector + expectedConfigUnsupported *string + expectedConfigDeprecated *string } testCases := []testCase{ @@ -107,7 +107,7 @@ func TestLoadPods(t *testing.T) { Name: name1, APIVersion: apiVersion, }, - topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{ + topMostWellKnownOrScalableKey: &controllerfetcher.ControllerKeyWithAPIVersion{ ControllerKey: controllerfetcher.ControllerKey{ Kind: kind, Name: name1, @@ -149,7 +149,7 @@ func TestLoadPods(t *testing.T) { Name: name1, APIVersion: apiVersion, }, - topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{ + topMostWellKnownOrScalableKey: &controllerfetcher.ControllerKeyWithAPIVersion{ ControllerKey: controllerfetcher.ControllerKey{ Kind: kind, Name: name2, @@ -169,8 +169,8 @@ func TestLoadPods(t *testing.T) { Name: "doseph-doestar", APIVersion: "taxonomy", }, - expectedConfigUnsupported: &unsupportedConditionMudaMudaMuda, - findTopLevelError: fmt.Errorf("muda muda muda"), + expectedConfigUnsupported: &unsupportedConditionMudaMudaMuda, + findTopMostWellKnownOrScalableError: fmt.Errorf("muda muda muda"), }, { name: "top-level target ref", @@ -182,7 +182,7 @@ func TestLoadPods(t *testing.T) { Name: name1, APIVersion: apiVersion, }, - topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{ + topMostWellKnownOrScalableKey: &controllerfetcher.ControllerKeyWithAPIVersion{ ControllerKey: controllerfetcher.ControllerKey{ Kind: kind, Name: name1, @@ -213,8 +213,8 @@ func TestLoadPods(t *testing.T) { clusterState: clusterState, selectorFetcher: targetSelectorFetcher, controllerFetcher: &fakeControllerFetcher{ - key: tc.topLevelKey, - err: tc.findTopLevelError, + key: tc.topMostWellKnownOrScalableKey, + err: tc.findTopMostWellKnownOrScalableError, }, } diff --git a/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher.go b/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher.go index fc248df7af01..2d7f5b8d4686 100644 --- a/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher.go +++ b/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher.go @@ -24,6 +24,7 @@ import ( batchv1 "k8s.io/api/batch/v1" batchv1beta1 "k8s.io/api/batch/v1beta1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -69,10 +70,10 @@ type ControllerKeyWithAPIVersion struct { ApiVersion string } -// ControllerFetcher is responsible for finding the top level controller +// ControllerFetcher is responsible for finding the topmost well-known or scalable controller type ControllerFetcher interface { - // FindTopLevel returns top level controller. Error is returned if top level controller cannot be found. - FindTopLevel(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) + // FindTopMostWellKnownOrScalable returns topmost well-known or scalable controller. Error is returned if controller cannot be found. + FindTopMostWellKnownOrScalable(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) } type controllerFetcher struct { @@ -126,7 +127,7 @@ func NewControllerFetcher(config *rest.Config, kubeClient kube_client.Interface, func getOwnerController(owners []metav1.OwnerReference, namespace string) *ControllerKeyWithAPIVersion { for _, owner := range owners { - if owner.Controller != nil && *owner.Controller == true { + if owner.Controller != nil && *owner.Controller { return &ControllerKeyWithAPIVersion{ ControllerKey: ControllerKey{ Namespace: namespace, @@ -207,17 +208,15 @@ func (f *controllerFetcher) getParentOfController(controllerKey ControllerKeyWit return getParentOfWellKnownController(informer, controllerKey) } - // TODO: cache response - groupVersion, err := schema.ParseGroupVersion(controllerKey.ApiVersion) + groupKind, err := controllerKey.groupKind() if err != nil { return nil, err } - groupKind := schema.GroupKind{ - Group: groupVersion.Group, - Kind: controllerKey.Kind, - } owner, err := f.getOwnerForScaleResource(groupKind, controllerKey.Namespace, controllerKey.Name) + if apierrors.IsNotFound(err) { + return nil, nil + } if err != nil { return nil, fmt.Errorf("Unhandled targetRef %s / %s / %s, last error %v", controllerKey.ApiVersion, controllerKey.Kind, controllerKey.Name, err) @@ -226,6 +225,55 @@ func (f *controllerFetcher) getParentOfController(controllerKey ControllerKeyWit return owner, nil } +func (c *ControllerKeyWithAPIVersion) groupKind() (schema.GroupKind, error) { + // TODO: cache response + groupVersion, err := schema.ParseGroupVersion(c.ApiVersion) + if err != nil { + return schema.GroupKind{}, err + } + + groupKind := schema.GroupKind{ + Group: groupVersion.Group, + Kind: c.ControllerKey.Kind, + } + + return groupKind, nil +} + +func (f *controllerFetcher) isWellKnown(key *ControllerKeyWithAPIVersion) bool { + kind := wellKnownController(key.ControllerKey.Kind) + _, exists := f.informersMap[kind] + return exists +} + +func (f *controllerFetcher) isWellKnownOrScalable(key *ControllerKeyWithAPIVersion) bool { + if f.isWellKnown(key) { + return true + } + + //if not well known check if it supports scaling + groupKind, err := key.groupKind() + if err != nil { + klog.Errorf("Could not find groupKind for %s/%s: %v", key.Namespace, key.Name, err) + return false + } + + mappings, err := f.mapper.RESTMappings(groupKind) + if err != nil { + klog.Errorf("Could not find mappings for %s: %v", groupKind, err) + return false + } + + for _, mapping := range mappings { + groupResource := mapping.Resource.GroupResource() + scale, err := f.scaleNamespacer.Scales(key.Namespace).Get(groupResource, key.Name) + if err == nil && scale != nil { + return true + } + } + return false +} + func (f *controllerFetcher) getOwnerForScaleResource(groupKind schema.GroupKind, namespace, name string) (*ControllerKeyWithAPIVersion, error) { mappings, err := f.mapper.RESTMappings(groupKind) if err != nil { @@ -246,10 +294,18 @@ func (f *controllerFetcher) getOwnerForScaleResource(groupKind schema.GroupKind, return nil, lastError } -func (f *controllerFetcher) FindTopLevel(key *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) { +func (f *controllerFetcher) FindTopMostWellKnownOrScalable(key *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) { if key == nil { return nil, nil } + + var topMostWellKnownOrScalable *ControllerKeyWithAPIVersion + + wellKnownOrScalable := f.isWellKnownOrScalable(key) + if wellKnownOrScalable { + topMostWellKnownOrScalable = key + } + visited := make(map[ControllerKeyWithAPIVersion]bool) visited[*key] = true for { @@ -257,14 +313,22 @@ func (f *controllerFetcher) FindTopLevel(key *ControllerKeyWithAPIVersion) (*Con if err != nil { return nil, err } + if owner == nil { - return key, nil + return topMostWellKnownOrScalable, nil + } + + wellKnownOrScalable = f.isWellKnownOrScalable(owner) + if wellKnownOrScalable { + topMostWellKnownOrScalable = owner } + _, alreadyVisited := visited[*owner] if alreadyVisited { return nil, fmt.Errorf("Cycle detected in ownership chain") } visited[*key] = true + key = owner } } @@ -272,7 +336,7 @@ func (f *controllerFetcher) FindTopLevel(key *ControllerKeyWithAPIVersion) (*Con type identityControllerFetcher struct { } -func (f *identityControllerFetcher) FindTopLevel(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) { +func (f *identityControllerFetcher) FindTopMostWellKnownOrScalable(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) { return controller, nil } @@ -280,7 +344,7 @@ type constControllerFetcher struct { ControllerKeyWithAPIVersion *ControllerKeyWithAPIVersion } -func (f *constControllerFetcher) FindTopLevel(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) { +func (f *constControllerFetcher) FindTopMostWellKnownOrScalable(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) { return f.ControllerKeyWithAPIVersion, nil } @@ -289,7 +353,7 @@ type mockControllerFetcher struct { result *ControllerKeyWithAPIVersion } -func (f *mockControllerFetcher) FindTopLevel(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) { +func (f *mockControllerFetcher) FindTopMostWellKnownOrScalable(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) { if controller == nil && f.expected == nil { return f.result, nil } diff --git a/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher_test.go b/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher_test.go index b52d58a5f68e..d9ca9ee6c657 100644 --- a/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher_test.go +++ b/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher_test.go @@ -24,11 +24,17 @@ import ( "github.com/stretchr/testify/assert" appsv1 "k8s.io/api/apps/v1" + autoscalingv1 "k8s.io/api/autoscaling/v1" batchv1 "k8s.io/api/batch/v1" batchv1beta1 "k8s.io/api/batch/v1beta1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/restmapper" + scalefake "k8s.io/client-go/scale/fake" + core "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" ) @@ -38,6 +44,48 @@ var trueVar = true func simpleControllerFetcher() *controllerFetcher { f := controllerFetcher{} f.informersMap = make(map[wellKnownController]cache.SharedIndexInformer) + versioned := map[string][]metav1.APIResource{ + "Foo": {{Kind: "Foo", Name: "bah", Group: "foo"}, {Kind: "Scale", Name: "iCanScale", Group: "foo"}}, + } + fakeMapper := []*restmapper.APIGroupResources{ + { + Group: metav1.APIGroup{ + Name: "Foo", + Versions: []metav1.GroupVersionForDiscovery{{GroupVersion: "Foo", Version: "Foo"}}, + }, + VersionedResources: versioned, + }, + } + mapper := restmapper.NewDiscoveryRESTMapper(fakeMapper) + f.mapper = mapper + + scaleNamespacer := &scalefake.FakeScaleClient{} + f.scaleNamespacer = scaleNamespacer + + //return not found if if tries to find the scale subresouce 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") + return true, nil, error + }) + + //resource that can scale + scaleNamespacer.AddReactor("get", "iCanScale", func(action core.Action) (handled bool, ret runtime.Object, err error) { + + ret = &autoscalingv1.Scale{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Scaler", + Namespace: "foo", + }, + Spec: autoscalingv1.ScaleSpec{ + Replicas: 5, + }, + Status: autoscalingv1.ScaleStatus{ + Replicas: 5, + }, + } + return true, ret, nil + }) for _, kind := range wellKnownControllers { f.informersMap[kind] = cache.NewSharedIndexInformer( @@ -51,7 +99,10 @@ func simpleControllerFetcher() *controllerFetcher { func addController(controller *controllerFetcher, obj runtime.Object) { kind := wellKnownController(obj.GetObjectKind().GroupVersionKind().Kind) - controller.informersMap[kind].GetStore().Add(obj) + _, ok := controller.informersMap[kind] + if ok { + controller.informersMap[kind].GetStore().Add(obj) + } } func TestControllerFetcher(t *testing.T) { @@ -239,17 +290,67 @@ func TestControllerFetcher(t *testing.T) { expectedKey: nil, expectedError: fmt.Errorf("Cycle detected in ownership chain"), }, + { + key: &ControllerKeyWithAPIVersion{ControllerKey: ControllerKey{ + Name: "test-deployment", Kind: "Deployment", Namespace: "test-namesapce"}}, + objects: []runtime.Object{&appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment", + Namespace: "test-namesapce", + // Parent that does not support scale subresource and is not well known + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "Foo/Foo", + Controller: &trueVar, + Kind: "Foo", + Name: "bah", + }, + }, + }, + }}, + expectedKey: &ControllerKeyWithAPIVersion{ControllerKey: ControllerKey{ + Name: "test-deployment", Kind: "Deployment", Namespace: "test-namesapce"}}, // Parent does not support scale subresource so should return itself" + expectedError: nil, + }, + { + key: &ControllerKeyWithAPIVersion{ControllerKey: ControllerKey{ + Name: "test-deployment", Kind: "Deployment", Namespace: "test-namesapce"}}, + objects: []runtime.Object{&appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment", + Namespace: "test-namesapce", + // Parent that support scale subresource and is not well known + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "Foo/Foo", + Controller: &trueVar, + Kind: "Scale", + Name: "iCanScale", + }, + }, + }, + }}, + expectedKey: &ControllerKeyWithAPIVersion{ControllerKey: ControllerKey{ + Name: "iCanScale", Kind: "Scale", Namespace: "test-namesapce"}, ApiVersion: "Foo/Foo"}, // Parent supports scale subresource" + expectedError: nil, + }, } { t.Run(fmt.Sprintf("test case %d", i), func(t *testing.T) { f := simpleControllerFetcher() for _, obj := range tc.objects { addController(f, obj) } - topLevelController, err := f.FindTopLevel(tc.key) + topMostWellKnownOrScalableController, err := f.FindTopMostWellKnownOrScalable(tc.key) if tc.expectedKey == nil { - assert.Nil(t, topLevelController) + assert.Nil(t, topMostWellKnownOrScalableController) } else { - assert.Equal(t, tc.expectedKey, topLevelController) + assert.Equal(t, tc.expectedKey, topMostWellKnownOrScalableController) } if tc.expectedError == nil { assert.Nil(t, err) diff --git a/vertical-pod-autoscaler/vendor/k8s.io/client-go/scale/fake/client.go b/vertical-pod-autoscaler/vendor/k8s.io/client-go/scale/fake/client.go new file mode 100644 index 000000000000..e3bc57ffbbeb --- /dev/null +++ b/vertical-pod-autoscaler/vendor/k8s.io/client-go/scale/fake/client.go @@ -0,0 +1,78 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package fake provides a fake client interface to arbitrary Kubernetes +// APIs that exposes common high level operations and exposes common +// metadata. +package fake + +import ( + autoscalingapi "k8s.io/api/autoscaling/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/scale" + "k8s.io/client-go/testing" +) + +// FakeScaleClient provides a fake implementation of scale.ScalesGetter. +type FakeScaleClient struct { + testing.Fake +} + +func (f *FakeScaleClient) Scales(namespace string) scale.ScaleInterface { + return &fakeNamespacedScaleClient{ + namespace: namespace, + fake: &f.Fake, + } +} + +type fakeNamespacedScaleClient struct { + namespace string + fake *testing.Fake +} + +func (f *fakeNamespacedScaleClient) Get(resource schema.GroupResource, name string) (*autoscalingapi.Scale, error) { + obj, err := f.fake. + Invokes(testing.NewGetSubresourceAction(resource.WithVersion(""), f.namespace, "scale", name), &autoscalingapi.Scale{}) + + if err != nil { + return nil, err + } + + return obj.(*autoscalingapi.Scale), err +} + +func (f *fakeNamespacedScaleClient) Update(resource schema.GroupResource, scale *autoscalingapi.Scale) (*autoscalingapi.Scale, error) { + obj, err := f.fake. + Invokes(testing.NewUpdateSubresourceAction(resource.WithVersion(""), f.namespace, "scale", scale), &autoscalingapi.Scale{}) + + if err != nil { + return nil, err + } + + return obj.(*autoscalingapi.Scale), err +} + +func (f *fakeNamespacedScaleClient) Patch(gvr schema.GroupVersionResource, name string, pt types.PatchType, patch []byte) (*autoscalingapi.Scale, error) { + obj, err := f.fake. + Invokes(testing.NewPatchSubresourceAction(gvr, f.namespace, name, pt, patch, "scale"), &autoscalingapi.Scale{}) + + if err != nil { + return nil, err + } + + return obj.(*autoscalingapi.Scale), err +} diff --git a/vertical-pod-autoscaler/vendor/modules.txt b/vertical-pod-autoscaler/vendor/modules.txt index fc818bc55918..096a6cddcb5c 100644 --- a/vertical-pod-autoscaler/vendor/modules.txt +++ b/vertical-pod-autoscaler/vendor/modules.txt @@ -371,6 +371,7 @@ k8s.io/client-go/rest k8s.io/client-go/rest/watch k8s.io/client-go/restmapper k8s.io/client-go/scale +k8s.io/client-go/scale/fake k8s.io/client-go/scale/scheme k8s.io/client-go/scale/scheme/appsint k8s.io/client-go/scale/scheme/appsv1beta1