VPA: Refactor how flags are set by adrianmoisey · Pull Request #9195 · kubernetes/autoscaler

adrianmoisey · 2026-02-07T15:09:57Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This is step 1 of #9156

Which issue(s) this PR fixes:

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.:

/hold

I only plan for this to be merged after the VPA 1.6 release

k8s-ci-robot · 2026-02-07T15:10:25Z

[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-02-08T08:21:48Z

Oh, some flags live in vertical-pod-autoscaler/pkg/updater/priority/update_priority_calculator.go that I still need to move

maxcao13

Thanks for this! Works fine for me locally, just one comment.

maxcao13 · 2026-02-10T18:03:29Z

vertical-pod-autoscaler/pkg/updater/main.go

 	scaleCacheEntryJitterFactor  float64       = 1.
 )

+var config *updater_config.UpdaterConfig


We have a global (file?) scoped variable for the config now instead of the individual flags but now it includes commonFlags, but we still also pass commonFlags to run: https://github.com/kubernetes/autoscaler/pull/9195/changes#diff-a64cbe814d071ca8df41b2890583bd9e92c3fb39fbbe57e17f013e434b2d108aR150. I don't think we need to pass commonFlag anymore, or maybe there's some other way to not duplicate?

This happens in all 3 components I believe.

Right, good question.
That's a transition to get the flags moved out of main. Once the logic for each component is changed to allow for integration tests, all configs will be passed in and the config shouldn't be global.

See https://github.com/kubernetes/autoscaler/pull/9156/changes#diff-bc201689c0c988297d4f93cf844937aa4f4755bfdc5bc912746148eaeed3bb64R48
And https://github.com/kubernetes/autoscaler/pull/9156/changes#diff-bc201689c0c988297d4f93cf844937aa4f4755bfdc5bc912746148eaeed3bb64R55

I made the config global just to keep the change as light as possible in this PR, with the plans to move things around later.

adrianmoisey · 2026-02-10T18:14:37Z

Thanks for this! Works fine for me locally, just one comment.

Thanks for the review!
Unfortunately I have more changes to push, I found a BUNCH of flags that I had forgotten about, in various other files.
I'll keep those changes as new commits to help with the review.

adrianmoisey · 2026-02-14T06:38:45Z

Unfortunately I have more changes to push, I found a BUNCH of flags that I had forgotten about, in various other files.
I'll keep those changes as new commits to help with the review.

Pushed, it's a little more difficult to review, but kept to a single commit

omerap12

Some comments from me.

omerap12 · 2026-02-14T14:29:38Z

vertical-pod-autoscaler/pkg/admission-controller/certs.go

+// CertsConfig holds configuration related to TLS certificates
+type CertsConfig struct {
+	ClientCaFile, TlsCertFile, TlsPrivateKey *string
+	Reload                                   *bool


I can't find a place where use this. maybe we should remove it instead?

I do see where are using: https://github.com/kubernetes/autoscaler/pull/9195/changes#diff-1d9e38171029851f3ff7fab43babd31245cdd3a21201ad79e42a6dd1596ae5dcR27 instead.

omerap12 · 2026-02-14T14:34:55Z

vertical-pod-autoscaler/pkg/admission-controller/config/config.go

+// ValidateAdmissionControllerConfig performs validation of the admission-controller flags
+func ValidateAdmissionControllerConfig(config *AdmissionControllerConfig) {
+	common.ValidateCommonConfig(config.CommonFlags)
+}


Does it make sense to you to use ValidateAdmissionControllerConfig inside InitAdmissionControllerFlags function and then return error if ValidateCommonConfig returns error?
Then we won't need https://github.com/kubernetes/autoscaler/pull/9195/changes#diff-0d60ce44b7823f02b8363916a2843a184b7f4cb4f1d75ab45d3954af4f25def4R68.
My concern is that someone could use InitAdmissionControllerFlags and will forget to validate.

This is true for all of the components

Moving this required a bit of other work related to the kubernetes flag we're using, I hope the end result makes sense

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

I had missed these on previous commits since they aren't in main.go

k8s-ci-robot requested review from omerap12 and raywainman February 7, 2026 15:10

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-area labels Feb 7, 2026

adrianmoisey mentioned this pull request Feb 7, 2026

Add integration tests for recommender #9156

Open

maxcao13 reviewed Feb 10, 2026

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 14, 2026

adrianmoisey force-pushed the adrian-flags branch from c5684a7 to 40b1605 Compare February 14, 2026 06:48

omerap12 reviewed Feb 14, 2026

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2026

adrianmoisey force-pushed the adrian-flags branch from c0bc7ca to cee795b Compare February 14, 2026 18:29

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 14, 2026

adrianmoisey added 5 commits February 16, 2026 15:00

Refactor updater's flags

ae99eb5

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

Refactor other flags

e1bb346

Update flags

d50897d

Move more recommender flags into config

5cb3c02

I had missed these on previous commits since they aren't in main.go

Remove unused CertsConfig struct

39d8025

adrianmoisey force-pushed the adrian-flags branch from cee795b to de98f82 Compare February 16, 2026 13:07

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2026

DRY up the validation of common flags

1d1521e

adrianmoisey force-pushed the adrian-flags branch from de98f82 to 1d1521e Compare February 16, 2026 13:10

Conversation

adrianmoisey commented Feb 7, 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 Feb 7, 2026

Uh oh!

adrianmoisey commented Feb 8, 2026

Uh oh!

maxcao13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianmoisey commented Feb 10, 2026

Uh oh!

adrianmoisey commented Feb 14, 2026

Uh oh!

omerap12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants