From b52ddf2dc4df063a9d97bb580b68eda0f3e04094 Mon Sep 17 00:00:00 2001 From: Marco Voelz Date: Tue, 16 Apr 2024 09:56:54 +0200 Subject: [PATCH 1/2] parentController may be nil when owner isn't scalable --- vertical-pod-autoscaler/pkg/utils/vpa/api.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api.go b/vertical-pod-autoscaler/pkg/utils/vpa/api.go index d6b9fbc22e5e..529d76961727 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api.go @@ -153,6 +153,9 @@ func GetControllingVPAForPod(pod *core.Pod, vpas []*VpaWithSelector, ctrlFetcher klog.Errorf("fail to get pod controller: pod=%s err=%s", pod.Name, err.Error()) return nil } + if parentController == nil { + return nil + } var controlling *VpaWithSelector var controllingVpa *vpa_types.VerticalPodAutoscaler From 6b6dfd43189eddd21d4fc777320ad6b150a26df0 Mon Sep 17 00:00:00 2001 From: Marco Voelz Date: Wed, 17 Apr 2024 11:41:45 +0200 Subject: [PATCH 2/2] Add tests for Pods owner that doesn't implement /scale --- .../controller_fetcher/controller_fetcher_test.go | 10 ++++++++++ vertical-pod-autoscaler/pkg/utils/vpa/api_test.go | 15 +++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go index 2ab3a26188f0..23eeb73321c9 100644 --- a/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go +++ b/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go @@ -389,6 +389,16 @@ func TestControllerFetcher(t *testing.T) { expectedKey: nil, expectedError: fmt.Errorf("Unhandled targetRef v1 / Node / node, last error node is not a valid owner"), }, + { + name: "custom resource with no scale subresource", + key: &ControllerKeyWithAPIVersion{ + ApiVersion: "Foo/Foo", ControllerKey: ControllerKey{ + Name: "bah", Kind: "Foo", Namespace: testNamespace}, + }, + objects: []runtime.Object{}, + expectedKey: nil, // Pod owner does not support scale subresource so should return nil" + expectedError: nil, + }, } { t.Run(tc.name, func(t *testing.T) { f := simpleControllerFetcher() diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go index 46787247eaea..ecee638d8766 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go @@ -139,6 +139,15 @@ func TestPodMatchesVPA(t *testing.T) { } } +type NilControllerFetcher struct{} + +// FindTopMostWellKnownOrScalable returns the same key for that fake implementation +func (f NilControllerFetcher) FindTopMostWellKnownOrScalable(_ *controllerfetcher.ControllerKeyWithAPIVersion) (*controllerfetcher.ControllerKeyWithAPIVersion, error) { + return nil, nil +} + +var _ controllerfetcher.ControllerFetcher = &NilControllerFetcher{} + func TestGetControllingVPAForPod(t *testing.T) { isController := true pod := test.Pod().WithName("test-pod").AddContainer(test.Container().WithName(containerName).WithCPURequest(resource.MustParse("1")).WithMemRequest(resource.MustParse("100M")).Get()).Get() @@ -171,6 +180,12 @@ func TestGetControllingVPAForPod(t *testing.T) { {nonMatchingVPA, parseLabelSelector("app = other")}, }, &controllerfetcher.FakeControllerFetcher{}) assert.Equal(t, vpaA, chosen.Vpa) + + // 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(pod, []*VpaWithSelector{{vpaA, parseLabelSelector("app = testingApp")}}, &NilControllerFetcher{}) + assert.Nil(t, chosen) } func TestGetContainerResourcePolicy(t *testing.T) {