refactor(vpa): make prometheus history provider config testable by ulascansenturk · Pull Request #9750 · kubernetes/autoscaler

ulascansenturk · 2026-06-05T10:19:31Z

/area vertical-pod-autoscaler
/kind cleanup

What this PR does / why we need it:

Follow-up to #9748, as discussed there. That PR was the minimal fix (cherry-picked to release-1.7). This PR does the refactor: it extracts the construction of the Prometheus history provider config out of initHistoryProvider into a newPrometheusHistoryProviderConfig helper, so the flag-to-config mapping can be unit tested, and adds a regression test asserting that --history-cpu-metric / --history-memory-metric are propagated.

No behavior change; pure refactor plus test.

Which issue(s) this PR fixes:

Refs #9747

Special notes for your reviewer:

Builds on the fix from #9748 (already merged). go test ./pkg/recommender/... passes; the new test fails without the field copies and passes with them.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2026-06-05T10:19:41Z

Hi @ulascansenturk. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

adrianmoisey · 2026-06-05T11:01:02Z

+// Prometheus history provider config. They were previously dropped, leaving the
+// queries without a metric name (kubernetes/autoscaler#9747).


Suggested change

// Prometheus history provider config. They were previously dropped, leaving the

// queries without a metric name (kubernetes/autoscaler#9747).

// Prometheus history provider config.

No need to mention why we have tests

Can this test be expanded to test more options too?

Trimmed the comment to just describe what the test checks, no rationale.

Expanded it to set every recommender flag and assert the full resulting config, so the whole flag-to-config mapping is covered, not just the metric names. Also added a case for an invalid query timeout.

adrianmoisey · 2026-06-05T11:01:43Z

/ok-to-test

adrianmoisey · 2026-06-05T11:23:04Z

+
+// TestNewPrometheusHistoryProviderConfig verifies that every recommender flag is
+// propagated to the Prometheus history provider config.
+func TestNewPrometheusHistoryProviderConfig(t *testing.T) {


🤔
If someone adds a flag, and forgets to update newPrometheusHistoryProviderConfig, I think this test won't catch that.
Any idea if it's possible to somehow test if they forgot to update it?

I don't know if it helps, but we could also move the prometheus specific settings within RecommenderConfig into it's own stuct inside of RecommenderConfig

Added TestNewPrometheusHistoryProviderConfigMapsEveryField: it sets every flag non-zero and reflects over the result, failing on any zero-valued field. Verified it flags an unmapped field by name.

Used the reflection test as the guard here. Happy to do the config restructure as a separate cleanup if you'd prefer.

omerap12

Can we merge all Test* function to one table driven unit test with different test cases?

omerap12 · 2026-06-11T17:00:18Z

/triage accepted

…ilder Extract the construction of the Prometheus history provider config out of initHistoryProvider into newPrometheusHistoryProviderConfig so the flag-to-config mapping can be unit tested, and add a regression test that the history metric name flags are propagated. Refs kubernetes#9747 Signed-off-by: ulascansenturk <ulascansenturk@protonmail.com>

k8s-ci-robot · 2026-06-11T17:08:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ulascansenturk
Once this PR has been reviewed and has the lgtm label, please assign omerap12 for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

vertical-pod-autoscaler/OWNERS

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

k8s-ci-robot requested review from kwiesmueller and raywainman June 5, 2026 10:19

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 5, 2026

ulascansenturk mentioned this pull request Jun 5, 2026

fix(vpa): honor --history-cpu-metric and --history-memory-metric flags #9748

Merged

adrianmoisey reviewed Jun 5, 2026

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 5, 2026

ulascansenturk force-pushed the refactor/9747-followup branch from 8b8a402 to 07e366f Compare June 5, 2026 11:05

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 5, 2026

adrianmoisey reviewed Jun 5, 2026

View reviewed changes

ulascansenturk force-pushed the refactor/9747-followup branch from 07e366f to 8cf6842 Compare June 5, 2026 11:36

ulascansenturk requested a review from adrianmoisey June 5, 2026 12:45

omerap12 reviewed Jun 11, 2026

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 11, 2026

ulascansenturk force-pushed the refactor/9747-followup branch from 8cf6842 to 16bb7d0 Compare June 11, 2026 17:08

		// Prometheus history provider config. They were previously dropped, leaving the
		// queries without a metric name (kubernetes/autoscaler#9747).

	// Prometheus history provider config. They were previously dropped, leaving the
	// queries without a metric name (kubernetes/autoscaler#9747).
	// Prometheus history provider config.

Conversation

ulascansenturk commented Jun 5, 2026

Uh oh!

k8s-ci-robot commented Jun 5, 2026

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!

adrianmoisey commented Jun 5, 2026

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!

omerap12 left a comment

Choose a reason for hiding this comment

Uh oh!

omerap12 commented Jun 11, 2026

Uh oh!

k8s-ci-robot commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants