Refactor the recommender to allow for integration tests #8985

adrianmoisey · 2026-01-01T12:52:29Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Currently the VPA doesn't have "integration" tests, which allows us to test various CLI flags to each component. The purpose of this change is to set the foundation to allow us to add integration tests. The pattern I tried to base this on is https://github.com/kubernetes/kubernetes/blob/master/test/integration/cloudprovider/ccm_test.go

The idea here is that we can write tests that execute faster without needing an entire Kubernetes cluster to validate these changes.

In theory this change also pushes us closer to the modern patterns that other controllers are using.

I'd also like some extra eyes on this on to see what everyone thinks.
This PR only works on the updater, but if it makes sense to make similar changes to the admission-controller or updater, I'm happy to do so (I think the config change does make sense for all components).

This PR also highlighted that we use stop channels and contexts in some places where we could get away with only the context. That's something we can fix in future PRs.

To run this, etcd is required to be available on the local machine. If this PR is accepted, a README and some scripts will be required, along with some automation to run this on PR and master.

/cc @omerap12 @iamzili @maxcao13 @jkyros

Which issue(s) this PR fixes:

Related to #8934

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2026-01-01T12:52:34Z

@adrianmoisey: GitHub didn't allow me to request PR reviews from the following users: jkyros.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Currently the VPA doesn't have "integration" tests, which allows us to test various CLI flags to each component. The purpose of this change is to set the foundation to allow us to add integration tests. The pattern I tried to base this on is https://github.com/kubernetes/kubernetes/blob/master/test/integration/cloudprovider/ccm_test.go

The idea here is that we can write tests that execute faster without needing an entire Kubernetes cluster to validate these changes.

In theory this change also pushes us closer to the modern patterns that other controllers are using.

I'd also like some extra eyes on this on to see what everyone thinks.
This PR only works on the updater, but if it makes sense to make similar changes to the admission-controller or updater, I'm happy to do so (I think the config change does make sense for all components).

This PR also highlighted that we use stop channels and contexts in some places where we could get away with only the context. That's something we can fix in future PRs.

To run this, etcd is required to be available on the local machine. If this PR is accepted, a README and some scripts will be required, along with some automation to run this on PR and master.

/cc @omerap12 @iamzili @maxcao13 @jkyros

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2026-01-01T12:52:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~vertical-pod-autoscaler/OWNERS~~ [adrianmoisey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adrianmoisey · 2026-01-01T12:58:13Z

vertical-pod-autoscaler/pkg/utils/metrics/quality/quality.go

+	_ = prometheus.Register(usageRecommendationRelativeDiff)
+	_ = prometheus.Register(usageMissingRecommendationCounter)
+	_ = prometheus.Register(cpuRecommendationOverUsageDiff)
+	_ = prometheus.Register(memoryRecommendationOverUsageDiff)
+	_ = prometheus.Register(cpuRecommendationLowerOrEqualUsageDiff)
+	_ = prometheus.Register(memoryRecommendationLowerOrEqualUsageDiff)
+	_ = prometheus.Register(cpuRecommendations)
+	_ = prometheus.Register(memoryRecommendations)
+	_ = prometheus.Register(relativeRecommendationChange)


I'm not 100% sure on this, it's required to startup multiple recommenders in the same process.
I'm unsure if other errors can trigger this, so I need to check if this is safe

omerap12

Since I already gave it a review before I would like to hear other folks.
Just small nits from me in the meantime.

omerap12 · 2026-01-01T13:05:20Z

vertical-pod-autoscaler/common/flags.go

 	IgnoredVpaObjectNamespaces string
 }

+func DefaultCommonConfig() *CommonFlags {


Not sure I understand, what's the lint?

Oh, exported function should have comment or be unexported

omerap12 · 2026-01-01T13:05:58Z

vertical-pod-autoscaler/pkg/recommender/app/config.go

+	}
+}
+
+func ValidateRecommenderConfig(config *RecommenderConfig) {


Configure a struct that can be passed around, and populate that with CLI flags

if block ends with a return statement, so drop this else and outdent its block

maxcao13 · 2026-01-02T20:37:33Z

Thanks for the PR!

Quick question, are we able to leverage the envtest library from controller-runtime here, or is there a reason we cannot or should not? https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/envtest

adrianmoisey · 2026-01-03T12:31:08Z

Thanks for the PR!

Quick question, are we able to leverage the envtest library from controller-runtime here, or is there a reason we cannot or should not? pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/envtest

I wasn't aware of envtest. I asked Github Copilot to make a few changes, and I put it up as a draft PR: #8992

Some notes on that PR:

For some reason in this PR I thought we needed a new go.mod file, so that extra test dependencies wouldn't be bundled with the built binaries, it seems I was wrong and Go handles test dependencies for you, so I fixed that in the draft PR. If we chose to not go the envtest route, I'll make that fix to this non-envtest PR too
I also got Copilot to start etcd and the apiserver once for all the tests, that shaved off some time to execute these, again, I can back port those to the non-envtest variant if needed
In the envtest PR, I noticed that (for free) the output was nicer. We only get VPA and VPA test logs, in this PR we also get logs from the apiserver.

Honestly, I think I prefer the envtest PR. If everyone agrees on that path, I can clean it up a little before submitting a real PR for review.

Something else to note, this issue has been around for a long time: #6137
And using contoller-runtime for tests seems like a good start on that path.

adrianmoisey · 2026-01-03T12:36:06Z

vertical-pod-autoscaler/integration/utils.go

+	return kubeConfigFile.Name()
+}
+
+func makeVPACP(ns string) *vpa_types.VerticalPodAutoscalerCheckpoint {


Unused function

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jan 1, 2026

k8s-ci-robot requested review from iamzili and omerap12 January 1, 2026 12:52

k8s-ci-robot requested a review from maxcao13 January 1, 2026 12:52

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area area/vertical-pod-autoscaler labels Jan 1, 2026

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/needs-area labels Jan 1, 2026

adrianmoisey force-pushed the adrian-integration-tests branch from d54bdcd to ff6b4bb Compare January 1, 2026 12:57

adrianmoisey commented Jan 1, 2026

View reviewed changes

adrianmoisey force-pushed the adrian-integration-tests branch from ff6b4bb to 584e1d9 Compare January 1, 2026 13:01

omerap12 reviewed Jan 1, 2026

View reviewed changes

adrianmoisey force-pushed the adrian-integration-tests branch from 584e1d9 to 9f4a222 Compare January 1, 2026 13:54

adrianmoisey added 5 commits January 1, 2026 17:40

Refactor updater's flags

288990b

Configure a struct that can be passed around, and populate that with CLI flags

Move recommender start into it's own module

b02f555

Add some integration tests

f8a318c

Wire up context, add stop channels, etc

073d75c

Lint

f97b368

if block ends with a return statement, so drop this else and outdent its block

adrianmoisey force-pushed the adrian-integration-tests branch from 9f4a222 to f97b368 Compare January 1, 2026 15:41

adrianmoisey mentioned this pull request Jan 3, 2026

Refactor the recommender to allow for integration tests - With envtest #8992

Draft

adrianmoisey commented Jan 3, 2026

View reviewed changes

adrianmoisey mentioned this pull request Jan 3, 2026

Document how to run e2e tests locally #8988

Merged

Refactor the recommender to allow for integration tests #8985

Are you sure you want to change the base?

Refactor the recommender to allow for integration tests #8985

Conversation

adrianmoisey commented Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Jan 1, 2026

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Jan 1, 2026

Uh oh!

adrianmoisey Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

omerap12 left a comment

Choose a reason for hiding this comment

Uh oh!

omerap12 Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

adrianmoisey Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

adrianmoisey Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

omerap12 Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

maxcao13 commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrianmoisey commented Jan 3, 2026

Uh oh!

adrianmoisey Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adrianmoisey commented Jan 1, 2026 •

edited

Loading

maxcao13 commented Jan 2, 2026 •

edited

Loading