Node removal latency metrics added #8485

ttetyanka · 2025-08-28T14:25:49Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

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

k8s-ci-robot · 2025-08-28T14:25:52Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

linux-foundation-easycla · 2025-08-28T14:25:56Z

❌ - login: @ttetyanka / name: Tetiana Yeremenko . The commit (ef7c537) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

k8s-ci-robot · 2025-08-28T14:25:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ttetyanka
Once this PR has been reviewed and has the lgtm label, please assign towca 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

k8s-ci-robot · 2025-08-28T14:25:58Z

Welcome @ttetyanka!

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-08-28T14:25:59Z

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

elmiko · 2025-08-28T16:32:33Z

/ok-to-test

MartynaGrotek · 2025-08-29T06:03:25Z

cluster-autoscaler/core/scaledown/latencytracker/nodelatencytracker.go

+)
+
+type NodeInfo struct {
+	Name          string


Looks like we have duplicated information. Name is already in key.

MartynaGrotek · 2025-08-29T06:10:45Z

cluster-autoscaler/core/scaledown/latencytracker/nodelatencytracker.go

File name not aligned with convention - using underscore between words.

MartynaGrotek · 2025-08-29T06:30:05Z

cluster-autoscaler/core/scaledown/latencytracker/latencytracker_test.go

Names of the test file and file implementation do not match.

MartynaGrotek · 2025-08-29T06:32:00Z

cluster-autoscaler/core/scaledown/latencytracker/nodelatencytracker.go

+		klog.V(2).Infof(
+			"Observing deletion for node %s, unneeded for %s (threshold was %s).",
+			nodeName, duration, info.Threshold,
+		)
+
+		metrics.UpdateScaleDownNodeDeletionDuration("true", duration-info.Threshold)


Logging and updating metric don't need and shouldn’t be done under lock.

MartynaGrotek · 2025-08-29T07:27:27Z

cluster-autoscaler/core/scaledown/unneeded/nodes.go

+	limitsFinder      *resource.LimitsFinder
+	cachedList        []*apiv1.Node
+	byName            map[string]*node
+	unneededTimeCache map[string]time.Duration


I would try to store here latency tracker. I think it would simplify code - eg. we could get rid of this cache and GetUnneededTimeForNode() method. We could save the GetScaleDownUnneededTime result when it is called during unremovableReason() method.

MartynaGrotek · 2025-08-29T09:48:26Z

cluster-autoscaler/core/scaledown/actuation/actuator.go

@@ -324,6 +327,9 @@ func (a *Actuator) deleteNodesAsync(nodes []*apiv1.Node, nodeGroup cloudprovider
 	}

 	for _, node := range nodes {
+		if a.nodeLatencyTracker != nil {
+			a.nodeLatencyTracker.ObserveDeletion(node.Name, time.Now())


We could consider covering this logic with test.

MartynaGrotek · 2025-08-29T10:05:00Z

cluster-autoscaler/metrics/metrics.go

@@ -750,3 +760,7 @@ func UpdateInconsistentInstancesMigsCount(migCount int) {
 func ObserveBinpackingHeterogeneity(instanceType, cpuCount, namespaceCount string, pegCount int) {
 	binpackingHeterogeneity.WithLabelValues(instanceType, cpuCount, namespaceCount).Observe(float64(pegCount))
 }
+
+func UpdateScaleDownNodeDeletionDuration(deleted string, duration time.Duration) {


Do we ever pass deleted = false?

MartynaGrotek · 2025-08-29T10:09:41Z

cluster-autoscaler/metrics/metrics.go

+			Namespace: caNamespace,
+			Name:      "node_deletion_duration_seconds",
+			Help:      "Latency from planning (node marked) to final outcome (deleted, aborted, rescued).",
+			Buckets:   k8smetrics.ExponentialBuckets(10, 2, 12),


I would consider having better resolution.

MartynaGrotek · 2025-08-29T10:16:42Z

cluster-autoscaler/core/scaledown/latencytracker/latencytracker_test.go

I don't see any test checking the reported value.

MartynaGrotek · 2025-08-29T12:48:59Z

cluster-autoscaler/core/scaledown/actuation/actuator.go

@@ -324,6 +327,9 @@ func (a *Actuator) deleteNodesAsync(nodes []*apiv1.Node, nodeGroup cloudprovider
 	}

 	for _, node := range nodes {
+		if a.nodeLatencyTracker != nil {


It doesn’t seem like the best placement. Deletion still might fail (look at the checks below). I think we would want to report only successful deletion.

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

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 28, 2025

k8s-ci-robot requested review from aleksandra-malinowska and elmiko August 28, 2025 14:25

k8s-ci-robot added the area/cluster-autoscaler label Aug 28, 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 Aug 28, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2025

Node removal latency metrics added

ef7c537

ttetyanka force-pushed the feature/deletionlatencytracker branch from aff3480 to ef7c537 Compare August 28, 2025 14:34

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 Aug 28, 2025

MartynaGrotek reviewed Aug 29, 2025

View reviewed changes

Node removal latency metrics added #8485

Are you sure you want to change the base?

Node removal latency metrics added #8485

Uh oh!

Conversation

ttetyanka commented Aug 28, 2025

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 Aug 28, 2025

Uh oh!

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

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 28, 2025

Uh oh!

k8s-ci-robot commented Aug 28, 2025

Uh oh!

k8s-ci-robot commented Aug 28, 2025

Uh oh!

elmiko commented Aug 28, 2025

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!

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!

Uh oh!

linux-foundation-easycla bot commented Aug 28, 2025 •

edited

Loading