feat(nodeScaleDownTime): add a new metric to track unprocessed nodes during scaleDown #8614

shaikenov · 2025-10-05T19:17:13Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds a new metric to the exported prometheus metrics list: LongestNodeScaleDownTime

We want to track all the nodes that were marked as unneeded, but were unprocessed during the ScaleDown. If a node was unneeded, but unprocessed multiple times consecutively, we store only the earliest time it happened. The difference between the current time and the earliest time among all unprocessed nodes will give the longest time. This time can give us an indication of possible throttling and helps to better monitor what happens during ScaleDown.

Which issue(s) this PR fixes:

None

Does this PR introduce a user-facing change?

Introduced a new metric, tracking time to process all nodes in scale down simulations. `--longest-node-scaledown-eval-timetracker-enabled` flag enables the new metric.

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

linux-foundation-easycla · 2025-10-05T19:17:20Z

The committers listed above are authorized under a signed CLA.

✅ login: shaikenov / name: Olzhas Shaikenov (4e48044)

k8s-ci-robot · 2025-10-05T19:17:22Z

Welcome @shaikenov!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-10-05T19:17:23Z

Hi @shaikenov. 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.

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.

kada2004

very good

k8s-ci-robot · 2025-10-06T06:57:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

cluster-autoscaler/OWNERS

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

cluster-autoscaler/core/scaledown/planner/planner.go

cluster-autoscaler/core/scaledown/planner/planner_test.go

cluster-autoscaler/core/scaledown/planner/planner.go

cluster-autoscaler/metrics/metrics.go

cluster-autoscaler/core/scaledown/planner/planner.go

cluster-autoscaler/core/scaledown/planner/planner_test.go

cluster-autoscaler/metrics/metrics.go

cluster-autoscaler/config/autoscaling_options.go

cluster-autoscaler/core/scaledown/planner/planner.go

cluster-autoscaler/core/scaledown/planner/planner_test.go

cluster-autoscaler/config/flags/flags.go

cluster-autoscaler/core/scaledown/planner/planner_test.go

cluster-autoscaler/metrics/metrics.go

cluster-autoscaler/core/scaledown/planner/planner_test.go

cluster-autoscaler/metrics/metrics.go

cluster-autoscaler/core/scaledown/planner/planner.go

x13n · 2025-10-29T10:21:50Z

cluster-autoscaler/core/scaledown/planner/planner.go

+	var longestTime time.Duration
+	// if nodeNames is nil it means that all nodes were processed
+	if nodeNames == nil {
+		// if l.minimumTime is 0, then in previous iteration we also processed all the nodes, so the longest time is 0


Why do we require all nodes to be processed twice before resetting the metric?

For the first time we might have some leftovers from previous simulations (as in the next comment), so we want to calculate the time for these nodes and reset the node map. If we get here again the second time we will report 0. I refactored this part to fix the issue that I had, I hope now it is more correct and clear

This sounds like an implementation detail leading to surprising results. Can this be implemented in a way that doesn't require this?

I made this condition here for a case when we do not have any unprocessed nodes at all. In this case if this if is not here and we have something like this:

func (l *LongestNodeScaleDownEvalTime) Update(nodeNames []string, currentTime time.Time) time.Duration { minimumTime := l.getMin() newNodes := make(map[string]time.Time) for _, nodeName := range nodeNames { newNodes[nodeName] = l.get(nodeName) } l.NodeNamesWithTimeStamps = newNodes longestTime := currentTime.Sub(minimumTime) l.lastEvalTime = currentTime metrics.ObserveLongestUnneededNodeScaleDownEvalDurationSeconds(longestTime) return longestTime }

we will report time between simulations (currentTime - lastEvalTime).

In this case IIUC we want to report 0 because there are no unprocessed nodes and from the metric definition (longest time during which node was not processed during ScaleDown) this metric is about the skipped nodes. The snippet above is fine by me, but I think it a bit diverges from the metric definition if there are not unprocessed nodes. WDYT, @x13n ?

Maybe I'm misunderstanding semantic of the metric, but if you want to report longest time a node was not processed I expected something like this:

func (l *LongestNodeScaleDownEvalTime) Update(nodeNames []string, currentTime time.Time) time.Duration { newNodes := make(map[string]time.Time) for _, nodeName := range nodeNames { newNodes[nodeName] = l.get(nodeName) } l.NodeNamesWithTimeStamps = newNodes minimumTime := l.getMin() longestDuration := currentTime.Sub(minimumTime) l.lastEvalTime = currentTime metrics.ObserveLongestUnneededNodeScaleDownEvalDurationSeconds(longestDuration) return longestDuration }

I suppose the distinction is that when I have this called with currentTime equal to some times t₀, t₁ and t₂, only the nodes unprocessed at a given timestamp are considered by the metric in that time instant. So, say at t₀ everything was processed, at t₁ node A was skipped and at t₂ node B was skipped. The metric then is set to 0 at t₀, t₁-t₀ at t₁ and t₂-t₁ at t₂. If then at t₃ everything is processed again, the metric drops back to 0. Does that make sense? Or did I misunderstood the metric definition?

x13n · 2025-10-29T10:27:05Z

cluster-autoscaler/core/scaledown/planner/planner_test.go

+		wantLongestScaleDownEvalTime []time.Duration
+	}
+	start := time.Now()
+	testCases := []testCase{


Can you add a test case when the set of skipped nodes is different every time? Wouldn't it make more sense for the metric to not increase in such scenario?

added this test case. There was a small bug with incorrect duration calculation, but now it should be fine.

In a map we store the last time when a node was not skipped. If we have different unprocessed nodes and the intervals between iterations is 1 sec, we will report the following:

initialization

{n1} -> getMin() returns 0 sec, we store map:{n1: 0 sec} and we return 1 sec

{n2} -> getMin() returns 0 sec, we store map:{n2: 1 sec}, because 1 sec is the last time when n2 was not skipped and we return 2 sec

{n3} -> ... we return 2 sec

{n4} -> ... we return 2 sec

{} -> we still have leftovers for node n4 in the map and getMin() returns 3 sec and we return 2 sec

Similarly as in the other comment - this sounds like an artifact of the current implementation, rather than desired behavior. Why would we want the metric to be different for a single unprocessed node based on whether previous iteration had any unprocessed nodes or not?

x13n · 2025-10-29T13:52:28Z

/ok-to-test
/release-note-edit

Introduced a new metric, tracking time to process all nodes in scale down simulations. `--longest-node-scaledown-eval-timetracker-enabled` flag enables the new metric.

k8s-ci-robot · 2025-10-29T13:52:30Z

@x13n: /release-note-edit must be used with a single release note block.

In response to this:

/ok-to-test
/release-note-edit

Introduced a new metric, tracking time to process all nodes in scale down simulations. `--longest-node-scaledown-eval-timetracker-enabled` flag enables the new metric.

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.

x13n · 2025-10-29T13:55:25Z

Ah, looks like you removed the release note block entirely, please bring it back in the first comment.

jackfrancis · 2025-10-29T17:53:03Z

/release-note-edit

Introduced a new metric, tracking time to process all nodes in scale down simulations. `--longest-node-scaledown-eval-timetracker-enabled` flag enables the new metric.

… nodes during scaleDown

x13n · 2025-10-31T08:15:19Z

cluster-autoscaler/core/scaledown/planner/planner.go


+// handleUnprocessedNodes is used to track the longest time it take for a node to be evaluated as removable or not
+func (p *Planner) handleUnprocessedNodes(unprocessedNodeNames []string) {
+	// if p.longestNodeScaleDownEvalTime is not set (flag is disabled) or endedPrematurely is already true (nodes were already reported in this iteration) do not do anything


Please update the comment as well.

x13n · 2025-10-31T08:19:05Z

cluster-autoscaler/core/scaledown/planner/planner.go

+	var longestTime time.Duration
+	// if nodeNames is nil it means that all nodes were processed
+	if nodeNames == nil {
+		// if l.minimumTime is 0, then in previous iteration we also processed all the nodes, so the longest time is 0


This sounds like an implementation detail leading to surprising results. Can this be implemented in a way that doesn't require this?

x13n · 2025-10-31T08:20:40Z

cluster-autoscaler/core/scaledown/planner/planner_test.go

+		wantLongestScaleDownEvalTime []time.Duration
+	}
+	start := time.Now()
+	testCases := []testCase{


Similarly as in the other comment - this sounds like an artifact of the current implementation, rather than desired behavior. Why would we want the metric to be different for a single unprocessed node based on whether previous iteration had any unprocessed nodes or not?

x13n · 2025-10-31T08:23:17Z

cluster-autoscaler/core/scaledown/longestevaluationtracker/longest_node_evaluation_tracker.go

+	// lastEvalTime is the time of previous currentlyUnneededNodeNames parsing
+	lastEvalTime time.Time
+	// NodeNamesWithTimeStamps is maps of nodeNames with their time of last successful evaluation
+	NodeNamesWithTimeStamps map[string]time.Time


Why is this public? This should be an implementation detail. Ideally tests would just evaluate whether return value from Update is correct.

k8s-ci-robot added the do-not-merge/needs-area label Oct 5, 2025

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 5, 2025

k8s-ci-robot added area/cluster-autoscaler and removed do-not-merge/needs-area labels Oct 5, 2025

k8s-ci-robot requested review from feiskyer and vadasambar October 5, 2025 19:17

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 5, 2025

kada2004 approved these changes Oct 6, 2025

View reviewed changes

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from b9e7969 to 86a98d1 Compare October 6, 2025 11:40

MartynaGrotek reviewed Oct 7, 2025

View reviewed changes

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

jackfrancis reviewed Oct 7, 2025

View reviewed changes

cluster-autoscaler/core/scaledown/planner/planner.go Outdated Show resolved Hide resolved

jackfrancis reviewed Oct 7, 2025

View reviewed changes

cluster-autoscaler/core/scaledown/planner/planner_test.go Outdated Show resolved Hide resolved

jackfrancis reviewed Oct 7, 2025

View reviewed changes

cluster-autoscaler/core/scaledown/planner/planner_test.go Outdated Show resolved Hide resolved

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from 86a98d1 to 0457f73 Compare October 8, 2025 08:56

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

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from 0457f73 to c2756ca Compare October 8, 2025 15:22

MartynaGrotek reviewed Oct 9, 2025

View reviewed changes

cluster-autoscaler/metrics/metrics.go Outdated Show resolved Hide resolved

MartynaGrotek reviewed Oct 9, 2025

View reviewed changes

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from c2756ca to a279f58 Compare October 10, 2025 12:00

MartynaGrotek reviewed Oct 14, 2025

View reviewed changes

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch 2 times, most recently from e5f5131 to 65b09dd Compare October 14, 2025 11:39

MartynaGrotek reviewed Oct 16, 2025

View reviewed changes

cluster-autoscaler/core/scaledown/planner/planner_test.go Outdated Show resolved Hide resolved

cluster-autoscaler/core/scaledown/planner/planner_test.go Outdated Show resolved Hide resolved

cluster-autoscaler/metrics/metrics.go Outdated Show resolved Hide resolved

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from 65b09dd to 716b1c3 Compare October 17, 2025 07:21

shaikenov marked this pull request as ready for review October 20, 2025 08:20

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2025

k8s-ci-robot requested review from aleksandra-malinowska and x13n October 20, 2025 08:20

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from 716b1c3 to 1239ced Compare October 20, 2025 13:53

jackfrancis reviewed Oct 20, 2025

View reviewed changes

cluster-autoscaler/core/scaledown/planner/planner.go Outdated Show resolved Hide resolved

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from 1239ced to 44aac42 Compare October 23, 2025 14:45

x13n reviewed Oct 29, 2025

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 Oct 29, 2025

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 29, 2025

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from 44aac42 to 3a5135c Compare October 30, 2025 13:19

feat(nodeScaleDownTimeTracker): add a new metric to track unprocessed…

4e48044

… nodes during scaleDown

shaikenov force-pushed the shaikenov-scaledown-unprocessed-node-tracking branch from 3a5135c to 4e48044 Compare October 30, 2025 14:03

x13n reviewed Oct 31, 2025

View reviewed changes

feat(nodeScaleDownTime): add a new metric to track unprocessed nodes during scaleDown #8614

Are you sure you want to change the base?

feat(nodeScaleDownTime): add a new metric to track unprocessed nodes during scaleDown #8614

Conversation

shaikenov commented Oct 5, 2025 • 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:

Does this PR introduce a user-facing change?

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

Uh oh!

linux-foundation-easycla bot commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 5, 2025

Uh oh!

k8s-ci-robot commented Oct 5, 2025

Uh oh!

kada2004 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

shaikenov Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

shaikenov Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

x13n commented Oct 29, 2025

Uh oh!

k8s-ci-robot commented Oct 29, 2025

Uh oh!

x13n commented Oct 29, 2025

Uh oh!

jackfrancis commented Oct 29, 2025

shaikenov commented Oct 5, 2025 •

edited

Loading

linux-foundation-easycla bot commented Oct 5, 2025 •

edited

Loading

shaikenov Oct 31, 2025 •

edited

Loading

shaikenov Oct 30, 2025 •

edited

Loading