Merge pull request #7632 from voelzmo/enh/node-owner-ref-error-handling

Handle Error for Pods with `ownerRef: Node`

vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go

@@ -18,6 +18,7 @@ package controllerfetcher
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -59,6 +60,9 @@ const (
 	discoveryResetPeriod time.Duration = 5 * time.Minute
 )
 
+// ErrNodeInvalidOwner is thrown when a Pod is owned by a Node.
+var ErrNodeInvalidOwner = errors.New("node is not a valid owner")
+
 // ControllerKey identifies a controller.
 type ControllerKey struct {
 	Namespace string
@@ -290,7 +294,7 @@ func (f *controllerFetcher) getOwnerForScaleResource(ctx context.Context, groupK
 		// Some pods specify nodes as their owners. This causes performance problems
 		// in big clusters when VPA tries to get all nodes. We know nodes aren't
 		// valid controllers so we can skip trying to fetch them.
-		return nil, fmt.Errorf("node is not a valid owner")
+		return nil, ErrNodeInvalidOwner
 	}
 	mappings, err := f.mapper.RESTMappings(groupKind)
 	if err != nil {

vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go

@@ -21,9 +21,21 @@ import "context"
 // FakeControllerFetcher should be used in test only. It returns exactly the same controllerKey
 type FakeControllerFetcher struct{}
 
-// FindTopMostWellKnownOrScalable returns the same key for that fake implementation
+// FindTopMostWellKnownOrScalable returns the same key for that fake implementation and returns and error when the kind is Node
+// See pkg/target/controller_fetcher/controller_fetcher.go:296 where the original implementation does the same.
 func (f FakeControllerFetcher) FindTopMostWellKnownOrScalable(_ context.Context, controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
+	if controller.Kind == "Node" {
+		return nil, ErrNodeInvalidOwner
+	}
 	return controller, nil
 }
 
+// NilControllerFetcher is a fake ControllerFetcher which always returns 'nil'
+type NilControllerFetcher struct{}
+
+// FindTopMostWellKnownOrScalable always returns nil
+func (f NilControllerFetcher) FindTopMostWellKnownOrScalable(_ context.Context, _ *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
+	return nil, nil
+}
+
 var _ ControllerFetcher = &FakeControllerFetcher{}

vertical-pod-autoscaler/pkg/utils/vpa/api.go

@@ -19,6 +19,7 @@ package api
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -180,7 +181,16 @@ func FindParentControllerForPod(ctx context.Context, pod *core.Pod, ctrlFetcher
 		},
 		ApiVersion: ownerRefrence.APIVersion,
 	}
-	return ctrlFetcher.FindTopMostWellKnownOrScalable(ctx, k)
+	controller, err := ctrlFetcher.FindTopMostWellKnownOrScalable(ctx, k)
+
+	// ignore NodeInvalidOwner error when looking for the parent controller for a Pod. While this _is_ an error when
+	// validating the targetRef of a VPA, this is a valid scenario when iterating over all Pods and finding their owner.
+	// vpa updater and admission-controller don't care about these Pods, because they cannot have a valid VPA point to
+	// them, so it is safe to ignore this here.
+	if err != nil && !errors.Is(err, controllerfetcher.ErrNodeInvalidOwner) {
+		return nil, err
+	}
+	return controller, nil
 }
 
 // GetUpdateMode returns the updatePolicy.updateMode for a given VPA.

vertical-pod-autoscaler/pkg/utils/vpa/api_test.go

@@ -141,27 +141,17 @@ func TestPodMatchesVPA(t *testing.T) {
 	}
 }
 
-type NilControllerFetcher struct{}
-
-// FindTopMostWellKnownOrScalable returns the same key for that fake implementation
-func (f NilControllerFetcher) FindTopMostWellKnownOrScalable(_ context.Context, _ *controllerfetcher.ControllerKeyWithAPIVersion) (*controllerfetcher.ControllerKeyWithAPIVersion, error) {
-	return nil, nil
-}
-
-var _ controllerfetcher.ControllerFetcher = &NilControllerFetcher{}
-
 func TestGetControllingVPAForPod(t *testing.T) {
 	ctx := context.Background()
 
-	isController := true
 	pod := test.Pod().WithName("test-pod").AddContainer(test.Container().WithName(containerName).WithCPURequest(resource.MustParse("1")).WithMemRequest(resource.MustParse("100M")).Get()).Get()
 	pod.Labels = map[string]string{"app": "testingApp"}
 	pod.OwnerReferences = []meta.OwnerReference{
 		{
 			APIVersion: "apps/v1",
 			Kind:       "StatefulSet",
 			Name:       "test-sts",
-			Controller: &isController,
+			Controller: ptr.To(true),
 		},
 	}
 
@@ -188,7 +178,7 @@ func TestGetControllingVPAForPod(t *testing.T) {
 	// For some Pods (which are *not* under VPA), controllerFetcher.FindTopMostWellKnownOrScalable will return `nil`, e.g. when the Pod owner is a custom resource, which doesn't implement the /scale subresource
 	// See pkg/target/controller_fetcher/controller_fetcher_test.go:393 for testing this behavior
 	// This test case makes sure that GetControllingVPAForPod will just return `nil` in that case as well
-	chosen = GetControllingVPAForPod(ctx, pod, []*VpaWithSelector{{vpaA, parseLabelSelector("app = testingApp")}}, &NilControllerFetcher{})
+	chosen = GetControllingVPAForPod(ctx, pod, []*VpaWithSelector{{vpaA, parseLabelSelector("app = testingApp")}}, &controllerfetcher.NilControllerFetcher{})
 	assert.Nil(t, chosen)
 }
 
@@ -321,7 +311,7 @@ func TestFindParentControllerForPod(t *testing.T) {
 					OwnerReferences: nil,
 				},
 			},
-			ctrlFetcher: &NilControllerFetcher{},
+			ctrlFetcher: &controllerfetcher.NilControllerFetcher{},
 			expected:    nil,
 		},
 		{
@@ -383,6 +373,24 @@ func TestFindParentControllerForPod(t *testing.T) {
 				ApiVersion: "apps/v1",
 			},
 		},
+		{
+			name: "should not return an error for Node owner reference",
+			pod: &core.Pod{
+				ObjectMeta: meta.ObjectMeta{
+					Namespace: "bar",
+					OwnerReferences: []meta.OwnerReference{
+						{
+							APIVersion: "v1",
+							Controller: ptr.To(true),
+							Kind:       "Node",
+							Name:       "foo",
+						},
+					},
+				},
+			},
+			ctrlFetcher: &controllerfetcher.FakeControllerFetcher{},
+			expected:    nil,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			got, err := FindParentControllerForPod(context.Background(), tc.pod, tc.ctrlFetcher)