From c76dffa968d3fcc123d300b5c39132c519aee022 Mon Sep 17 00:00:00 2001 From: Ismail Alidzhikov Date: Mon, 8 Jul 2024 10:58:35 +0300 Subject: [PATCH 1/4] vpa-admission-controller: Improve finding the matching VPA for Pod --- .../resource/vpa/matcher.go | 38 +++++++++---- .../resource/vpa/matcher_test.go | 32 ++++++++--- vertical-pod-autoscaler/pkg/utils/vpa/api.go | 53 ++++++++++--------- 3 files changed, 82 insertions(+), 41 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go index 05ceff3026f7..8b9378cf1140 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go @@ -52,30 +52,46 @@ func NewMatcher(vpaLister vpa_lister.VerticalPodAutoscalerLister, } func (m *matcher) GetMatchingVPA(ctx context.Context, pod *core.Pod) *vpa_types.VerticalPodAutoscaler { + parentController, err := vpa_api_util.FindParentControllerForPod(ctx, pod, m.controllerFetcher) + if err != nil { + klog.ErrorS(err, "Failed to get parent controller for pod", "pod", klog.KObj(pod)) + return nil + } + if parentController == nil { + return nil + } + configs, err := m.vpaLister.VerticalPodAutoscalers(pod.Namespace).List(labels.Everything()) if err != nil { klog.ErrorS(err, "Failed to get vpa configs") return nil } - onConfigs := make([]*vpa_api_util.VpaWithSelector, 0) + + var controllingVpa *vpa_types.VerticalPodAutoscaler for _, vpaConfig := range configs { if vpa_api_util.GetUpdateMode(vpaConfig) == vpa_types.UpdateModeOff { continue } + if vpaConfig.Spec.TargetRef == nil { + continue + } + if vpaConfig.Spec.TargetRef.Kind != parentController.Kind || + vpaConfig.Namespace != parentController.Namespace || + vpaConfig.Spec.TargetRef.Name != parentController.Name { + continue // This pod is not associated to the right controller + } + selector, err := m.selectorFetcher.Fetch(ctx, vpaConfig) if err != nil { klog.V(3).InfoS("Skipping VPA object because we cannot fetch selector", "vpa", klog.KObj(vpaConfig), "error", err) continue } - onConfigs = append(onConfigs, &vpa_api_util.VpaWithSelector{ - Vpa: vpaConfig, - Selector: selector, - }) - } - klog.V(2).InfoS("Let's choose from", "configs", len(onConfigs), "pod", klog.KObj(pod)) - result := vpa_api_util.GetControllingVPAForPod(ctx, pod, onConfigs, m.controllerFetcher) - if result != nil { - return result.Vpa + + vpaWithSelector := &vpa_api_util.VpaWithSelector{Vpa: vpaConfig, Selector: selector} + if vpa_api_util.PodMatchesVPA(pod, vpaWithSelector) && vpa_api_util.Stronger(vpaConfig, controllingVpa) { + controllingVpa = vpaConfig + } } - return nil + + return controllingVpa } diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher_test.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher_test.go index ce149e3f48d0..b08d688d20c5 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher_test.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher_test.go @@ -57,10 +57,16 @@ func TestGetMatchingVpa(t *testing.T) { Name: sts.Name, APIVersion: sts.APIVersion, } + targetRefWithNoMatches := &v1.CrossVersionObjectReference{ + Kind: "ReplicaSet", + Name: "rs", + APIVersion: "apps/v1", + } podBuilderWithoutCreator := test.Pod().WithName("test-pod").WithLabels(map[string]string{"app": "test"}). AddContainer(test.Container().WithName("i-am-container").Get()) podBuilder := podBuilderWithoutCreator.WithCreator(&sts.ObjectMeta, &sts.TypeMeta) vpaBuilder := test.VerticalPodAutoscaler().WithContainer("i-am-container") + testCases := []struct { name string pod *core.Pod @@ -79,12 +85,25 @@ func TestGetMatchingVpa(t *testing.T) { expectedFound: true, expectedVpaName: "auto-vpa", }, { - name: "matching selector but not match ownerRef (orphan pod)", + name: "no matching ownerRef (orphan pod)", pod: podBuilderWithoutCreator.Get(), vpas: []*vpa_types.VerticalPodAutoscaler{ vpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).WithName("auto-vpa").WithTargetRef(targetRef).Get(), }, - labelSelector: "app = test", + expectedFound: false, + }, { + name: "vpa without targetRef", + pod: podBuilder.Get(), + vpas: []*vpa_types.VerticalPodAutoscaler{ + vpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).WithName("auto-vpa").Get(), + }, + expectedFound: false, + }, { + name: "no vpa with matching targetRef", + pod: podBuilder.Get(), + vpas: []*vpa_types.VerticalPodAutoscaler{ + vpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).WithName("auto-vpa").WithTargetRef(targetRefWithNoMatches).Get(), + }, expectedFound: false, }, { name: "not matching selector", @@ -100,10 +119,9 @@ func TestGetMatchingVpa(t *testing.T) { vpas: []*vpa_types.VerticalPodAutoscaler{ vpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).WithName("off-vpa").WithTargetRef(targetRef).Get(), }, - labelSelector: "app = test", expectedFound: false, }, { - name: "two vpas one in off mode", + name: "two vpas, one in off mode", pod: podBuilder.Get(), vpas: []*vpa_types.VerticalPodAutoscaler{ vpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).WithName("off-vpa").WithTargetRef(targetRef).Get(), @@ -125,10 +143,10 @@ func TestGetMatchingVpa(t *testing.T) { name: "no vpa objects", pod: podBuilder.Get(), vpas: []*vpa_types.VerticalPodAutoscaler{}, - labelSelector: "app = test", expectedFound: false, }, } + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { ctrl := gomock.NewController(t) @@ -142,7 +160,9 @@ func TestGetMatchingVpa(t *testing.T) { vpaLister := &test.VerticalPodAutoscalerListerMock{} vpaLister.On("VerticalPodAutoscalers", "default").Return(vpaNamespaceLister) - mockSelectorFetcher.EXPECT().Fetch(gomock.Any()).AnyTimes().Return(parseLabelSelector(tc.labelSelector), nil) + if tc.labelSelector != "" { + mockSelectorFetcher.EXPECT().Fetch(gomock.Any()).AnyTimes().Return(parseLabelSelector(tc.labelSelector), nil) + } // This test is using a FakeControllerFetcher which returns the same ownerRef that is passed to it. // In other words, it cannot go through the hierarchy of controllers like "ReplicaSet => Deployment" // For this reason we are using "StatefulSet" as the ownerRef kind in the test, since it is a direct link. diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api.go b/vertical-pod-autoscaler/pkg/utils/vpa/api.go index 303def859c70..053c08f8e68c 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api.go @@ -109,8 +109,8 @@ func PodLabelsMatchVPA(podNamespace string, labels labels.Set, vpaNamespace stri return vpaSelector.Matches(labels) } -// stronger returns true iff a is before b in the order to control a Pod (that matches both VPAs). -func stronger(a, b *vpa_types.VerticalPodAutoscaler) bool { +// Stronger returns true iff a is before b in the order to control a Pod (that matches both VPAs). +func Stronger(a, b *vpa_types.VerticalPodAutoscaler) bool { // Assume a is not nil and each valid object is before nil object. if b == nil { return true @@ -129,28 +129,9 @@ func stronger(a, b *vpa_types.VerticalPodAutoscaler) bool { // GetControllingVPAForPod chooses the earliest created VPA from the input list that matches the given Pod. func GetControllingVPAForPod(ctx context.Context, pod *core.Pod, vpas []*VpaWithSelector, ctrlFetcher controllerfetcher.ControllerFetcher) *VpaWithSelector { - var ownerRefrence *meta.OwnerReference - for i := range pod.OwnerReferences { - r := pod.OwnerReferences[i] - if r.Controller != nil && *r.Controller { - ownerRefrence = &r - } - } - if ownerRefrence == nil { - // If the pod has no ownerReference, it cannot be under a VPA. - return nil - } - k := &controllerfetcher.ControllerKeyWithAPIVersion{ - ControllerKey: controllerfetcher.ControllerKey{ - Namespace: pod.Namespace, - Kind: ownerRefrence.Kind, - Name: ownerRefrence.Name, - }, - ApiVersion: ownerRefrence.APIVersion, - } - parentController, err := ctrlFetcher.FindTopMostWellKnownOrScalable(ctx, k) + parentController, err := FindParentControllerForPod(ctx, pod, ctrlFetcher) if err != nil { - klog.ErrorS(err, "Failed to get pod controller", "pod", klog.KObj(pod)) + klog.ErrorS(err, "Failed to get parent controller for pod", "pod", klog.KObj(pod)) return nil } if parentController == nil { @@ -169,7 +150,7 @@ func GetControllingVPAForPod(ctx context.Context, pod *core.Pod, vpas []*VpaWith vpaWithSelector.Vpa.Spec.TargetRef.Name != parentController.Name { continue // This pod is not associated to the right controller } - if PodMatchesVPA(pod, vpaWithSelector) && stronger(vpaWithSelector.Vpa, controllingVpa) { + if PodMatchesVPA(pod, vpaWithSelector) && Stronger(vpaWithSelector.Vpa, controllingVpa) { controlling = vpaWithSelector controllingVpa = controlling.Vpa } @@ -177,6 +158,30 @@ func GetControllingVPAForPod(ctx context.Context, pod *core.Pod, vpas []*VpaWith return controlling } +// FindParentControllerForPod returns the parent controller (topmost well-known or scalable controller) for the given Pod. +func FindParentControllerForPod(ctx context.Context, pod *core.Pod, ctrlFetcher controllerfetcher.ControllerFetcher) (*controllerfetcher.ControllerKeyWithAPIVersion, error) { + var ownerRefrence *meta.OwnerReference + for i := range pod.OwnerReferences { + r := pod.OwnerReferences[i] + if r.Controller != nil && *r.Controller { + ownerRefrence = &r + } + } + if ownerRefrence == nil { + // If the pod has no ownerReference, it cannot be under a VPA. + return nil, nil + } + k := &controllerfetcher.ControllerKeyWithAPIVersion{ + ControllerKey: controllerfetcher.ControllerKey{ + Namespace: pod.Namespace, + Kind: ownerRefrence.Kind, + Name: ownerRefrence.Name, + }, + ApiVersion: ownerRefrence.APIVersion, + } + return ctrlFetcher.FindTopMostWellKnownOrScalable(ctx, k) +} + // GetUpdateMode returns the updatePolicy.updateMode for a given VPA. // If the mode is not specified it returns the default (UpdateModeAuto). func GetUpdateMode(vpa *vpa_types.VerticalPodAutoscaler) vpa_types.UpdateMode { From 2bbd0a84c4f5af508992962871b38455880b1041 Mon Sep 17 00:00:00 2001 From: Ismail Alidzhikov Date: Wed, 24 Jul 2024 12:01:04 +0300 Subject: [PATCH 2/4] Address review comment from plkokanov --- .../pkg/admission-controller/resource/vpa/matcher.go | 1 + vertical-pod-autoscaler/pkg/target/fetcher.go | 2 +- vertical-pod-autoscaler/pkg/utils/vpa/api.go | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go index 8b9378cf1140..472b6a35ed59 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go @@ -73,6 +73,7 @@ func (m *matcher) GetMatchingVPA(ctx context.Context, pod *core.Pod) *vpa_types. continue } if vpaConfig.Spec.TargetRef == nil { + klog.V(5).InfoS("Skipping VPA object because targetRef is not defined. If this is a v1beta1 object switch to v1", "vpa", klog.KObj(vpaConfig)) continue } if vpaConfig.Spec.TargetRef.Kind != parentController.Kind || diff --git a/vertical-pod-autoscaler/pkg/target/fetcher.go b/vertical-pod-autoscaler/pkg/target/fetcher.go index 9ad372587e18..237814dff022 100644 --- a/vertical-pod-autoscaler/pkg/target/fetcher.go +++ b/vertical-pod-autoscaler/pkg/target/fetcher.go @@ -118,7 +118,7 @@ type vpaTargetSelectorFetcher struct { func (f *vpaTargetSelectorFetcher) Fetch(ctx context.Context, vpa *vpa_types.VerticalPodAutoscaler) (labels.Selector, error) { if vpa.Spec.TargetRef == nil { - return nil, fmt.Errorf("targetRef not defined. If this is a v1beta1 object switch to v1beta2.") + return nil, fmt.Errorf("targetRef not defined. If this is a v1beta1 object switch to v1.") } kind := wellKnownController(vpa.Spec.TargetRef.Kind) informer, exists := f.informersMap[kind] diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api.go b/vertical-pod-autoscaler/pkg/utils/vpa/api.go index 053c08f8e68c..da13d5f90323 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api.go @@ -143,6 +143,7 @@ func GetControllingVPAForPod(ctx context.Context, pod *core.Pod, vpas []*VpaWith // Choose the strongest VPA from the ones that match this Pod. for _, vpaWithSelector := range vpas { if vpaWithSelector.Vpa.Spec.TargetRef == nil { + klog.V(5).InfoS("Skipping VPA object because targetRef is not defined. If this is a v1beta1 object switch to v1", "vpa", klog.KObj(vpaWithSelector.Vpa)) continue } if vpaWithSelector.Vpa.Spec.TargetRef.Kind != parentController.Kind || From 041de8e1212941dd123421e3d9dee1c64e65be44 Mon Sep 17 00:00:00 2001 From: Ismail Alidzhikov Date: Tue, 6 Aug 2024 13:12:55 +0300 Subject: [PATCH 3/4] Address review comments from plkokanov (2) --- .../pkg/admission-controller/resource/vpa/matcher.go | 2 +- vertical-pod-autoscaler/pkg/target/fetcher.go | 2 +- vertical-pod-autoscaler/pkg/utils/vpa/api.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go index 472b6a35ed59..704749649ad2 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go @@ -73,7 +73,7 @@ func (m *matcher) GetMatchingVPA(ctx context.Context, pod *core.Pod) *vpa_types. continue } if vpaConfig.Spec.TargetRef == nil { - klog.V(5).InfoS("Skipping VPA object because targetRef is not defined. If this is a v1beta1 object switch to v1", "vpa", klog.KObj(vpaConfig)) + klog.V(5).InfoS("Skipping VPA object because targetRef is not defined. If this is a v1beta1 object, switch to v1", "vpa", klog.KObj(vpaConfig)) continue } if vpaConfig.Spec.TargetRef.Kind != parentController.Kind || diff --git a/vertical-pod-autoscaler/pkg/target/fetcher.go b/vertical-pod-autoscaler/pkg/target/fetcher.go index 237814dff022..936b95d85bd5 100644 --- a/vertical-pod-autoscaler/pkg/target/fetcher.go +++ b/vertical-pod-autoscaler/pkg/target/fetcher.go @@ -118,7 +118,7 @@ type vpaTargetSelectorFetcher struct { func (f *vpaTargetSelectorFetcher) Fetch(ctx context.Context, vpa *vpa_types.VerticalPodAutoscaler) (labels.Selector, error) { if vpa.Spec.TargetRef == nil { - return nil, fmt.Errorf("targetRef not defined. If this is a v1beta1 object switch to v1.") + return nil, fmt.Errorf("targetRef not defined. If this is a v1beta1 object, switch to v1.") } kind := wellKnownController(vpa.Spec.TargetRef.Kind) informer, exists := f.informersMap[kind] diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api.go b/vertical-pod-autoscaler/pkg/utils/vpa/api.go index da13d5f90323..8fbf11c98356 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api.go @@ -143,7 +143,7 @@ func GetControllingVPAForPod(ctx context.Context, pod *core.Pod, vpas []*VpaWith // Choose the strongest VPA from the ones that match this Pod. for _, vpaWithSelector := range vpas { if vpaWithSelector.Vpa.Spec.TargetRef == nil { - klog.V(5).InfoS("Skipping VPA object because targetRef is not defined. If this is a v1beta1 object switch to v1", "vpa", klog.KObj(vpaWithSelector.Vpa)) + klog.V(5).InfoS("Skipping VPA object because targetRef is not defined. If this is a v1beta1 object, switch to v1", "vpa", klog.KObj(vpaWithSelector.Vpa)) continue } if vpaWithSelector.Vpa.Spec.TargetRef.Kind != parentController.Kind || From dcbd7164678527fb9ed867c20c4a74215d026c38 Mon Sep 17 00:00:00 2001 From: Ismail Alidzhikov Date: Wed, 30 Oct 2024 08:12:27 +0200 Subject: [PATCH 4/4] Address review comment from adrianmoisey --- .../pkg/utils/vpa/api_test.go | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go index 2b6f4679103d..e8cbf3f7c275 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go @@ -33,6 +33,7 @@ import ( vpa_fake "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned/fake" controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test" + "k8s.io/utils/ptr" ) const ( @@ -305,3 +306,88 @@ func TestGetContainerControlledResources(t *testing.T) { }) } } + +func TestFindParentControllerForPod(t *testing.T) { + for _, tc := range []struct { + name string + pod *core.Pod + ctrlFetcher controllerfetcher.ControllerFetcher + expected *controllerfetcher.ControllerKeyWithAPIVersion + }{ + { + name: "should return nil for Pod without ownerReferences", + pod: &core.Pod{ + ObjectMeta: meta.ObjectMeta{ + OwnerReferences: nil, + }, + }, + ctrlFetcher: &NilControllerFetcher{}, + expected: nil, + }, + { + name: "should return nil for Pod with ownerReference with controller=nil", + pod: &core.Pod{ + ObjectMeta: meta.ObjectMeta{ + OwnerReferences: []meta.OwnerReference{ + { + APIVersion: "apps/v1", + Controller: nil, + Kind: "ReplicaSet", + Name: "foo", + }, + }, + }, + }, + ctrlFetcher: &controllerfetcher.FakeControllerFetcher{}, + expected: nil, + }, + { + name: "should return nil for Pod with ownerReference with controller=false", + pod: &core.Pod{ + ObjectMeta: meta.ObjectMeta{ + OwnerReferences: []meta.OwnerReference{ + { + APIVersion: "apps/v1", + Controller: ptr.To(false), + Kind: "ReplicaSet", + Name: "foo", + }, + }, + }, + }, + ctrlFetcher: &controllerfetcher.FakeControllerFetcher{}, + expected: nil, + }, + { + name: "should pass the Pod ownerReference to the fake ControllerFetcher", + pod: &core.Pod{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "bar", + OwnerReferences: []meta.OwnerReference{ + { + APIVersion: "apps/v1", + Controller: ptr.To(true), + Kind: "ReplicaSet", + Name: "foo", + }, + }, + }, + }, + ctrlFetcher: &controllerfetcher.FakeControllerFetcher{}, + expected: &controllerfetcher.ControllerKeyWithAPIVersion{ + ControllerKey: controllerfetcher.ControllerKey{ + Namespace: "bar", + Kind: "ReplicaSet", + Name: "foo", + }, + ApiVersion: "apps/v1", + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := FindParentControllerForPod(context.Background(), tc.pod, tc.ctrlFetcher) + assert.NoError(t, err, "Unexpected error occurred.") + assert.Equal(t, got, tc.expected) + }) + } +}