Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NPE in vpa-updater when Pod owner isn't scalable #6712

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
3 changes: 3 additions & 0 deletions vertical-pod-autoscaler/pkg/utils/vpa/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions vertical-pod-autoscaler/pkg/utils/vpa/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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) {
Expand Down
Loading