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

kubernetes · Jul 8, 2024 · 4bd7ae0 · 4bd7ae0
1 parent 02521ed
commit 4bd7ae0
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 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
@@ -50,30 +50,46 @@ func NewMatcher(vpaLister vpa_lister.VerticalPodAutoscalerLister,
 }
 
 func (m *matcher) GetMatchingVPA(pod *core.Pod) *vpa_types.VerticalPodAutoscaler {
+	parentController, err := vpa_api_util.FindParentControllerForPod(pod, m.controllerFetcher)
+	if err != nil {
+		klog.Errorf("fail to get parent controller for pod: pod=%s err=%s", klog.KObj(pod), err.Error())
+		return nil
+	}
+	if parentController == nil {
+		return nil
+	}
+
 	configs, err := m.vpaLister.VerticalPodAutoscalers(pod.Namespace).List(labels.Everything())
 	if err != nil {
 		klog.Errorf("failed to get vpa configs: %v", err)
 		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(vpaConfig)
 		if err != nil {
 			klog.V(3).Infof("skipping VPA object %s because we cannot fetch selector: %s", klog.KObj(vpaConfig), err)
 			continue
 		}
-		onConfigs = append(onConfigs, &vpa_api_util.VpaWithSelector{
-			Vpa:      vpaConfig,
-			Selector: selector,
-		})
-	}
-	klog.V(2).Infof("Let's choose from %d configs for pod %s", len(onConfigs), klog.KObj(pod))
-	result := vpa_api_util.GetControllingVPAForPod(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
@@ -56,10 +56,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
@@ -78,12 +84,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",
@@ -99,10 +118,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(),
@@ -124,10 +142,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)
@@ -141,7 +159,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
@@ -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(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(k)
+	parentController, err := FindParentControllerForPod(pod, ctrlFetcher)
 	if err != nil {
-		klog.Errorf("fail to get pod controller: pod=%s err=%s", klog.KObj(pod), err.Error())
+		klog.Errorf("fail to get parent controller for pod: pod=%s err=%s", klog.KObj(pod), err.Error())
 		return nil
 	}
 	if parentController == nil {
@@ -169,14 +150,38 @@ func GetControllingVPAForPod(pod *core.Pod, vpas []*VpaWithSelector, ctrlFetcher
 			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(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(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 {