From bd74115003ea81f981bd9d645a470f3f39b90335 Mon Sep 17 00:00:00 2001 From: Marco Voelz Date: Thu, 19 Dec 2024 13:55:41 +0100 Subject: [PATCH 1/6] Move NilControllerFetcher --- .../controller_fetcher/controller_fetcher_fake.go | 8 ++++++++ vertical-pod-autoscaler/pkg/utils/vpa/api_test.go | 13 ++----------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go index b2a8d5eb0cc0..67fd53f0b349 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go @@ -26,4 +26,12 @@ func (f FakeControllerFetcher) FindTopMostWellKnownOrScalable(_ context.Context, 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{} diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go index e8cbf3f7c275..772780df066b 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go @@ -141,15 +141,6 @@ 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() @@ -188,7 +179,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 +312,7 @@ func TestFindParentControllerForPod(t *testing.T) { OwnerReferences: nil, }, }, - ctrlFetcher: &NilControllerFetcher{}, + ctrlFetcher: &controllerfetcher.NilControllerFetcher{}, expected: nil, }, { From d9199305460360585cf628f5470c813819edd395 Mon Sep 17 00:00:00 2001 From: Marco Voelz Date: Thu, 19 Dec 2024 13:56:47 +0100 Subject: [PATCH 2/6] Make FakeControllerFetcher also return error for Node kind --- .../controller_fetcher/controller_fetcher_fake.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go index 67fd53f0b349..a4a50998314f 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go @@ -16,13 +16,20 @@ limitations under the License. package controllerfetcher -import "context" +import ( + "context" + "fmt" +) // 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, fmt.Errorf("node is not a valid owner") + } return controller, nil } From c191b6dcad0ab2425aafc934b790fa14bc5e8a28 Mon Sep 17 00:00:00 2001 From: Marco Voelz Date: Thu, 19 Dec 2024 13:57:20 +0100 Subject: [PATCH 3/6] Cleanup: use ptr.To for reference --- vertical-pod-autoscaler/pkg/utils/vpa/api_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go index 772780df066b..3e88798b6736 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go @@ -144,7 +144,6 @@ func TestPodMatchesVPA(t *testing.T) { 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{ @@ -152,7 +151,7 @@ func TestGetControllingVPAForPod(t *testing.T) { APIVersion: "apps/v1", Kind: "StatefulSet", Name: "test-sts", - Controller: &isController, + Controller: ptr.To(true), }, } From f335e9db73f5f7c47aebbc8378d6a1d96af9ac21 Mon Sep 17 00:00:00 2001 From: Marco Voelz Date: Thu, 19 Dec 2024 14:15:59 +0100 Subject: [PATCH 4/6] Handle NodeInvalidOwnerError instead of returning it --- .../controller_fetcher/controller_fetcher.go | 5 ++++- .../controller_fetcher_fake.go | 7 ++----- vertical-pod-autoscaler/pkg/utils/vpa/api.go | 12 +++++++++++- .../pkg/utils/vpa/api_test.go | 18 ++++++++++++++++++ 4 files changed, 35 insertions(+), 7 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go index 080451329df4..86d636cd8f5b 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go @@ -18,6 +18,7 @@ package controllerfetcher import ( "context" + "errors" "fmt" "time" @@ -59,6 +60,8 @@ const ( discoveryResetPeriod time.Duration = 5 * time.Minute ) +var NodeInvalidOwnerError = errors.New("node is not a valid owner") + // ControllerKey identifies a controller. type ControllerKey struct { Namespace string @@ -290,7 +293,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, NodeInvalidOwnerError } mappings, err := f.mapper.RESTMappings(groupKind) if err != nil { diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go index a4a50998314f..357c5cecb8e7 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go @@ -16,10 +16,7 @@ limitations under the License. package controllerfetcher -import ( - "context" - "fmt" -) +import "context" // FakeControllerFetcher should be used in test only. It returns exactly the same controllerKey type FakeControllerFetcher struct{} @@ -28,7 +25,7 @@ type FakeControllerFetcher struct{} // 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, fmt.Errorf("node is not a valid owner") + return nil, NodeInvalidOwnerError } return controller, nil } diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api.go b/vertical-pod-autoscaler/pkg/utils/vpa/api.go index 8fbf11c98356..b1ffa079369b 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api.go +++ b/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.NodeInvalidOwnerError) { + return nil, err + } + return controller, nil } // GetUpdateMode returns the updatePolicy.updateMode for a given VPA. diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go index 3e88798b6736..89f032b3fec9 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go @@ -373,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) From 4c98e459f1b8010deacaeeb8741155fd7fb7a367 Mon Sep 17 00:00:00 2001 From: Marco Voelz Date: Thu, 19 Dec 2024 16:14:04 +0100 Subject: [PATCH 5/6] Rename Error to fit linting rules --- .../pkg/target/controller_fetcher/controller_fetcher.go | 4 ++-- .../pkg/target/controller_fetcher/controller_fetcher_fake.go | 2 +- vertical-pod-autoscaler/pkg/utils/vpa/api.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go index 86d636cd8f5b..89785740a15d 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go @@ -60,7 +60,7 @@ const ( discoveryResetPeriod time.Duration = 5 * time.Minute ) -var NodeInvalidOwnerError = errors.New("node is not a valid owner") +var ErrNodeInvalidOwner = errors.New("node is not a valid owner") // ControllerKey identifies a controller. type ControllerKey struct { @@ -293,7 +293,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, NodeInvalidOwnerError + return nil, ErrNodeInvalidOwner } mappings, err := f.mapper.RESTMappings(groupKind) if err != nil { diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go index 357c5cecb8e7..f67dd2b11e16 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_fake.go @@ -25,7 +25,7 @@ type FakeControllerFetcher struct{} // 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, NodeInvalidOwnerError + return nil, ErrNodeInvalidOwner } return controller, nil } diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api.go b/vertical-pod-autoscaler/pkg/utils/vpa/api.go index b1ffa079369b..14c98cf20327 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api.go @@ -187,7 +187,7 @@ func FindParentControllerForPod(ctx context.Context, pod *core.Pod, ctrlFetcher // 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.NodeInvalidOwnerError) { + if err != nil && !errors.Is(err, controllerfetcher.ErrNodeInvalidOwner) { return nil, err } return controller, nil From 2edd0261b522c746aa3724c55a8e460dc833d441 Mon Sep 17 00:00:00 2001 From: Marco Voelz Date: Thu, 19 Dec 2024 16:19:37 +0100 Subject: [PATCH 6/6] Add comment to ErrNodeInvalidOwner --- .../pkg/target/controller_fetcher/controller_fetcher.go | 1 + 1 file changed, 1 insertion(+) diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go index 89785740a15d..3a2040e8b9f4 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go @@ -60,6 +60,7 @@ 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.