Merge pull request kubernetes#7010 from ialidzhikov/enh/improve-get-m…

…atching-vpa

vpa-admission-controller: Improve finding the matching VPA for Pod

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go

@@ -52,30 +52,47 @@ 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 {
+			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 ||
+			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
 }

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.

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]

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 {
@@ -162,21 +143,46 @@ 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 ||
 			vpaWithSelector.Vpa.Namespace != parentController.Namespace ||
 			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
 		}
 	}
 	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 {

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)
+		})
+	}
+}