Code review comments addressed

Author: ttetyanka

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

@@ -34,8 +34,8 @@ type LatencyTracker interface {
 	GetTrackedNodes() []string
 }
 type nodeInfo struct {
-	UnneededSince time.Time
-	Threshold     time.Duration
+	unneededSince time.Time
+	threshold     time.Duration
 }
 
 // NodeLatencyTracker is a concrete implementation of LatencyTracker.
@@ -64,21 +64,21 @@ func (t *NodeLatencyTracker) UpdateStateWithUnneededList(
 
 		if _, exists := t.nodes[node.Name]; !exists {
 			t.nodes[node.Name] = nodeInfo{
-				UnneededSince: timestamp,
-				Threshold:     0,
+				unneededSince: timestamp,
+				threshold:     0,
 			}
-			klog.V(2).Infof("Started tracking unneeded node %s at %v", node.Name, timestamp)
+			klog.V(4).Infof("Started tracking unneeded node %s at %v", node.Name, timestamp)
 		}
 	}
 
 	for name, info := range t.nodes {
 		if _, stillUnneeded := currentSet[name]; !stillUnneeded {
 			if _, inDeletion := currentlyInDeletion[name]; !inDeletion {
-				duration := timestamp.Sub(info.UnneededSince)
-				metrics.UpdateScaleDownNodeDeletionDuration("false", duration-info.Threshold)
+				duration := timestamp.Sub(info.unneededSince)
+				metrics.UpdateScaleDownNodeRemovalLatency(false, duration-info.threshold)
 				delete(t.nodes, name)
-				klog.V(2).Infof("Node %q reported as deleted/missing (unneeded for %s, threshold %s)",
-					name, duration, info.Threshold)
+				klog.V(4).Infof("Node %q reported as deleted/missing (unneeded for %s, threshold %s)",
+					name, duration, info.threshold)
 			}
 		}
 	}
@@ -87,24 +87,24 @@ func (t *NodeLatencyTracker) UpdateStateWithUnneededList(
 // ObserveDeletion is called by the actuator just before node deletion.
 func (t *NodeLatencyTracker) ObserveDeletion(nodeName string, timestamp time.Time) {
 	if info, exists := t.nodes[nodeName]; exists {
-		duration := timestamp.Sub(info.UnneededSince)
+		duration := timestamp.Sub(info.unneededSince)
 
-		klog.V(2).Infof(
+		klog.V(4).Infof(
 			"Observing deletion for node %s, unneeded for %s (threshold was %s).",
-			nodeName, duration, info.Threshold,
+			nodeName, duration, info.threshold,
 		)
 
-		metrics.UpdateScaleDownNodeDeletionDuration("true", duration-info.Threshold)
+		metrics.UpdateScaleDownNodeRemovalLatency(true, duration-info.threshold)
 		delete(t.nodes, nodeName)
 	}
 }
 
 // UpdateThreshold updates the scale-down threshold for a tracked node.
 func (t *NodeLatencyTracker) UpdateThreshold(nodeName string, threshold time.Duration) {
 	if info, exists := t.nodes[nodeName]; exists {
-		info.Threshold = threshold
+		info.threshold = threshold
 		t.nodes[nodeName] = info
-		klog.V(2).Infof("Updated threshold for node %q to %s", nodeName, threshold)
+		klog.V(4).Infof("Updated threshold for node %q to %s", nodeName, threshold)
 	} else {
 		klog.Warningf("Attempted to update threshold for unknown node %q", nodeName)
 	}

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

@@ -28,65 +28,65 @@ func TestNodeLatencyTracker(t *testing.T) {
 	baseTime := time.Now()
 
 	tests := []struct {
-		name                  string
-		setupNodes            map[string]nodeInfo
-		unneededList          []string
-		currentlyInDeletion   map[string]bool
-		updateThresholds      map[string]time.Duration
-		observeDeletion       []string
-		expectedTrackedNodes  []string
-		expectedDeletionTimes map[string]time.Duration
+		name                string
+		setupNodes          map[string]nodeInfo
+		unneededList        []string
+		currentlyInDeletion map[string]bool
+		updateThresholds    map[string]time.Duration
+		observeDeletion     []string
+		wantTrackedNodes    []string
+		wantDeletionTimes   map[string]time.Duration
 	}{
 		{
-			name:                 "add new unneeded nodes",
-			setupNodes:           map[string]nodeInfo{},
-			unneededList:         []string{"node1", "node2"},
-			currentlyInDeletion:  map[string]bool{},
-			updateThresholds:     map[string]time.Duration{},
-			observeDeletion:      []string{},
-			expectedTrackedNodes: []string{"node1", "node2"},
+			name:                "add new unneeded nodes",
+			setupNodes:          map[string]nodeInfo{},
+			unneededList:        []string{"node1", "node2"},
+			currentlyInDeletion: map[string]bool{},
+			updateThresholds:    map[string]time.Duration{},
+			observeDeletion:     []string{},
+			wantTrackedNodes:    []string{"node1", "node2"},
 		},
 		{
 			name: "observe deletion with threshold",
 			setupNodes: map[string]nodeInfo{
-				"node1": {UnneededSince: baseTime, Threshold: 2 * time.Second},
+				"node1": {unneededSince: baseTime, threshold: 2 * time.Second},
 			},
-			unneededList:         []string{},
-			currentlyInDeletion:  map[string]bool{},
-			updateThresholds:     map[string]time.Duration{},
-			observeDeletion:      []string{"node1"},
-			expectedTrackedNodes: []string{},
-			expectedDeletionTimes: map[string]time.Duration{
+			unneededList:        []string{},
+			currentlyInDeletion: map[string]bool{},
+			updateThresholds:    map[string]time.Duration{},
+			observeDeletion:     []string{"node1"},
+			wantTrackedNodes:    []string{},
+			wantDeletionTimes: map[string]time.Duration{
 				"node1": 3 * time.Second, // simulate observation 5s after UnneededSince, threshold 2s
 			},
 		},
 		{
 			name: "remove unneeded node not in deletion",
 			setupNodes: map[string]nodeInfo{
-				"node1": {UnneededSince: baseTime, Threshold: 1 * time.Second},
-				"node2": {UnneededSince: baseTime, Threshold: 0},
+				"node1": {unneededSince: baseTime, threshold: 1 * time.Second},
+				"node2": {unneededSince: baseTime, threshold: 0},
 			},
-			unneededList:         []string{"node2"}, // node1 is removed from unneeded
-			currentlyInDeletion:  map[string]bool{},
-			updateThresholds:     map[string]time.Duration{},
-			observeDeletion:      []string{},
-			expectedTrackedNodes: []string{"node2"},
-			expectedDeletionTimes: map[string]time.Duration{
+			unneededList:        []string{"node2"}, // node1 is removed from unneeded
+			currentlyInDeletion: map[string]bool{},
+			updateThresholds:    map[string]time.Duration{},
+			observeDeletion:     []string{},
+			wantTrackedNodes:    []string{"node2"},
+			wantDeletionTimes: map[string]time.Duration{
 				"node1": 5*time.Second - 1*time.Second, // assume current timestamp baseTime+5s
 			},
 		},
 		{
 			name: "update threshold",
 			setupNodes: map[string]nodeInfo{
-				"node1": {UnneededSince: baseTime, Threshold: 1 * time.Second},
+				"node1": {unneededSince: baseTime, threshold: 1 * time.Second},
 			},
 			unneededList:        []string{"node1"},
 			currentlyInDeletion: map[string]bool{},
 			updateThresholds: map[string]time.Duration{
 				"node1": 4 * time.Second,
 			},
-			observeDeletion:      []string{},
-			expectedTrackedNodes: []string{"node1"},
+			observeDeletion:  []string{},
+			wantTrackedNodes: []string{"node1"},
 		},
 	}
 
@@ -116,7 +116,7 @@ func TestNodeLatencyTracker(t *testing.T) {
 			// Check tracked nodes
 			gotTracked := tracker.GetTrackedNodes()
 			expectedMap := make(map[string]struct{})
-			for _, n := range tt.expectedTrackedNodes {
+			for _, n := range tt.wantTrackedNodes {
 				expectedMap[n] = struct{}{}
 			}
 			for _, n := range gotTracked {
@@ -129,12 +129,12 @@ func TestNodeLatencyTracker(t *testing.T) {
 				t.Errorf("expected node %q to be tracked, but was not", n)
 			}
 
-			for node, expectedDuration := range tt.expectedDeletionTimes {
+			for node, expectedDuration := range tt.wantDeletionTimes {
 				info, ok := tt.setupNodes[node]
 				if !ok {
 					continue
 				}
-				duration := currentTime.Sub(info.UnneededSince) - info.Threshold
+				duration := currentTime.Sub(info.unneededSince) - info.threshold
 				if duration != expectedDuration {
 					t.Errorf("node %q expected deletion duration %v, got %v", node, expectedDuration, duration)
 				}

cluster-autoscaler/core/static_autoscaler_test.go

@@ -166,7 +166,7 @@ func (m *onNodeGroupDeleteMock) Delete(id string) error {
 
 func setUpScaleDownActuator(ctx *context.AutoscalingContext, autoscalingOptions config.AutoscalingOptions) {
 	deleteOptions := options.NewNodeDeleteOptions(autoscalingOptions)
-	ctx.ScaleDownActuator = actuation.NewActuator(ctx, nil, deletiontracker.NewNodeDeletionTracker(0*time.Second), latencytracker.NewNodeLatencyTracker(), deleteOptions, rules.Default(deleteOptions), processorstest.NewTestProcessors(ctx).NodeGroupConfigProcessor)
+	ctx.ScaleDownActuator = actuation.NewActuator(ctx, nil, deletiontracker.NewNodeDeletionTracker(0*time.Second), nil, deleteOptions, rules.Default(deleteOptions), processorstest.NewTestProcessors(ctx).NodeGroupConfigProcessor)
 }
 
 type nodeGroup struct {
@@ -212,7 +212,6 @@ type commonMocks struct {
 	podDisruptionBudgetLister *podDisruptionBudgetListerMock
 	daemonSetLister           *daemonSetListerMock
 	nodeDeletionTracker       *deletiontracker.NodeDeletionTracker
-	nodeLatencyTracker        *latencytracker.NodeLatencyTracker
 
 	resourceClaimLister *fakeAllObjectsLister[*resourceapi.ResourceClaim]
 	resourceSliceLister *fakeAllObjectsLister[*resourceapi.ResourceSlice]
@@ -323,12 +322,8 @@ func setupAutoscaler(config *autoscalerSetupConfig) (*StaticAutoscaler, error) {
 	if nodeDeletionTracker == nil {
 		nodeDeletionTracker = deletiontracker.NewNodeDeletionTracker(0 * time.Second)
 	}
-	nodeLatencyTracker := config.mocks.nodeLatencyTracker
-	if nodeLatencyTracker == nil {
-		nodeLatencyTracker = latencytracker.NewNodeLatencyTracker()
-	}
-	ctx.ScaleDownActuator = actuation.NewActuator(&ctx, clusterState, nodeDeletionTracker, nodeLatencyTracker, deleteOptions, drainabilityRules, processors.NodeGroupConfigProcessor)
-	sdPlanner := planner.New(&ctx, processors, deleteOptions, drainabilityRules, nodeLatencyTracker)
+	ctx.ScaleDownActuator = actuation.NewActuator(&ctx, clusterState, nodeDeletionTracker, nil, deleteOptions, drainabilityRules, processors.NodeGroupConfigProcessor)
+	sdPlanner := planner.New(&ctx, processors, deleteOptions, drainabilityRules, nil)
 
 	processorCallbacks.scaleDownPlanner = sdPlanner
 
@@ -384,6 +379,21 @@ func TestStaticAutoscalerRunOnce(t *testing.T) {
 	ng1 := reflect.ValueOf(provider.GetNodeGroup("ng1")).Interface().(*testprovider.TestNodeGroup)
 	assert.NotNil(t, ng1)
 	assert.NotNil(t, provider)
+	// NodeLatencyTracker mock
+	nltMock := &latencytrackerMock{}
+	nltMock.On("ObserveDeletion",
+		"n2",
+		mock.MatchedBy(func(t time.Time) bool { return !t.IsZero() }),
+	).Return()
+	nltMock.On("UpdateStateWithUnneededList",
+		mock.MatchedBy(func(nodes []*apiv1.Node) bool { return true }),
+		mock.MatchedBy(func(m map[string]bool) bool { return true }),
+		mock.Anything,
+	).Return()
+	nltMock.On("UpdateThreshold",
+		"n2",
+		time.Minute,
+	).Return()
 
 	// Create context with mocked lister registry.
 	options := config.AutoscalingOptions{
@@ -416,7 +426,7 @@ func TestStaticAutoscalerRunOnce(t *testing.T) {
 	}
 	processors := processorstest.NewTestProcessors(&context)
 	clusterState := clusterstate.NewClusterStateRegistry(provider, clusterStateConfig, context.LogRecorder, NewBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(options.NodeGroupDefaults), processors.AsyncNodeGroupStateChecker)
-	sdPlanner, sdActuator := newScaleDownPlannerAndActuator(&context, processors, clusterState, nil, nil)
+	sdPlanner, sdActuator := newScaleDownPlannerAndActuator(&context, processors, clusterState, nil, nltMock)
 	suOrchestrator := orchestrator.New()
 	suOrchestrator.Initialize(&context, processors, clusterState, newEstimatorBuilder(), taints.TaintConfig{})
 
@@ -2473,7 +2483,7 @@ func TestStaticAutoscalerUpcomingScaleDownCandidates(t *testing.T) {
 	csr := clusterstate.NewClusterStateRegistry(provider, csrConfig, ctx.LogRecorder, NewBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: 15 * time.Minute}), processors.AsyncNodeGroupStateChecker)
 
 	// Setting the Actuator is necessary for testing any scale-down logic, it shouldn't have anything to do in this test.
-	actuator := actuation.NewActuator(&ctx, csr, deletiontracker.NewNodeDeletionTracker(0*time.Second), latencytracker.NewNodeLatencyTracker(), options.NodeDeleteOptions{}, nil, processorstest.NewTestProcessors(&ctx).NodeGroupConfigProcessor)
+	actuator := actuation.NewActuator(&ctx, csr, deletiontracker.NewNodeDeletionTracker(0*time.Second), nil, options.NodeDeleteOptions{}, nil, processorstest.NewTestProcessors(&ctx).NodeGroupConfigProcessor)
 	ctx.ScaleDownActuator = actuator
 
 	// Fake planner that keeps track of the scale-down candidates passed to UpdateClusterState.
@@ -3134,7 +3144,7 @@ func waitForDeleteToFinish(t *testing.T, deleteFinished <-chan bool) {
 	}
 }
 
-func newScaleDownPlannerAndActuator(ctx *context.AutoscalingContext, p *ca_processors.AutoscalingProcessors, cs *clusterstate.ClusterStateRegistry, nodeDeletionTracker *deletiontracker.NodeDeletionTracker, nodeLatencyTracker *latencytracker.NodeLatencyTracker) (scaledown.Planner, scaledown.Actuator) {
+func newScaleDownPlannerAndActuator(ctx *context.AutoscalingContext, p *ca_processors.AutoscalingProcessors, cs *clusterstate.ClusterStateRegistry, nodeDeletionTracker *deletiontracker.NodeDeletionTracker, nodeDeletionLatencyTracker latencytracker.LatencyTracker) (scaledown.Planner, scaledown.Actuator) {
 	ctx.MaxScaleDownParallelism = 10
 	ctx.MaxDrainParallelism = 1
 	ctx.NodeDeletionBatcherInterval = 0 * time.Second
@@ -3149,11 +3159,8 @@ func newScaleDownPlannerAndActuator(ctx *context.AutoscalingContext, p *ca_proce
 	if nodeDeletionTracker == nil {
 		nodeDeletionTracker = deletiontracker.NewNodeDeletionTracker(0 * time.Second)
 	}
-	if nodeLatencyTracker == nil {
-		nodeLatencyTracker = latencytracker.NewNodeLatencyTracker()
-	}
-	planner := planner.New(ctx, p, deleteOptions, nil, nodeLatencyTracker)
-	actuator := actuation.NewActuator(ctx, cs, nodeDeletionTracker, nodeLatencyTracker, deleteOptions, nil, p.NodeGroupConfigProcessor)
+	planner := planner.New(ctx, p, deleteOptions, nil, nodeDeletionLatencyTracker)
+	actuator := actuation.NewActuator(ctx, cs, nodeDeletionTracker, nodeDeletionLatencyTracker, deleteOptions, nil, p.NodeGroupConfigProcessor)
 	return planner, actuator
 }
 
@@ -3269,13 +3276,13 @@ func buildStaticAutoscaler(t *testing.T, provider cloudprovider.CloudProvider, a
 	processors.ScaleDownNodeProcessor = cp
 
 	csr := clusterstate.NewClusterStateRegistry(provider, clusterstate.ClusterStateRegistryConfig{OkTotalUnreadyCount: 1}, ctx.LogRecorder, NewBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: 15 * time.Minute}), processors.AsyncNodeGroupStateChecker)
-	actuator := actuation.NewActuator(&ctx, csr, deletiontracker.NewNodeDeletionTracker(0*time.Second), latencytracker.NewNodeLatencyTracker(), options.NodeDeleteOptions{}, nil, processors.NodeGroupConfigProcessor)
+	actuator := actuation.NewActuator(&ctx, csr, deletiontracker.NewNodeDeletionTracker(0*time.Second), nil, options.NodeDeleteOptions{}, nil, processors.NodeGroupConfigProcessor)
 	ctx.ScaleDownActuator = actuator
 
 	deleteOptions := options.NewNodeDeleteOptions(ctx.AutoscalingOptions)
 	drainabilityRules := rules.Default(deleteOptions)
 
-	sdPlanner := planner.New(&ctx, processors, deleteOptions, drainabilityRules, latencytracker.NewNodeLatencyTracker())
+	sdPlanner := planner.New(&ctx, processors, deleteOptions, drainabilityRules, nil)
 
 	autoscaler := &StaticAutoscaler{
 		AutoscalingContext:   &ctx,
@@ -3325,3 +3332,25 @@ func assertNodesSoftTaintsStatus(t *testing.T, fakeClient *fake.Clientset, nodes
 		assert.Equal(t, tainted, taints.HasDeletionCandidateTaint(newNode))
 	}
 }
+
+// latencytrackerMock implements LatencyTracker for mocking
+type latencytrackerMock struct {
+	mock.Mock
+}
+
+func (m *latencytrackerMock) ObserveDeletion(nodeName string, timestamp time.Time) {
+	m.Called(nodeName, timestamp)
+}
+
+func (m *latencytrackerMock) UpdateStateWithUnneededList(list []*apiv1.Node, currentlyInDeletion map[string]bool, timestamp time.Time) {
+	m.Called(list, currentlyInDeletion, timestamp)
+}
+
+func (m *latencytrackerMock) UpdateThreshold(nodeName string, threshold time.Duration) {
+	m.Called(nodeName, threshold)
+}
+
+func (m *latencytrackerMock) GetTrackedNodes() []string {
+	args := m.Called()
+	return args.Get(0).([]string)
+}

cluster-autoscaler/metrics/metrics.go

@@ -18,6 +18,7 @@ package metrics
 
 import (
 	"fmt"
+	"strconv"
 	"time"
 
 	"k8s.io/autoscaler/cluster-autoscaler/simulator"
@@ -428,12 +429,12 @@ var (
 		}, []string{"instance_type", "cpu_count", "namespace_count"},
 	)
 
-	scaleDownNodeDeletionDuration = k8smetrics.NewHistogramVec(
+	scaleDownNodeRemovalLatency = k8smetrics.NewHistogramVec(
 		&k8smetrics.HistogramOpts{
 			Namespace: caNamespace,
-			Name:      "node_deletion_duration_seconds",
+			Name:      "node_deletion_latency_seconds",
 			Help:      "Latency from planning (node marked) to final outcome (deleted, aborted, rescued).",
-			Buckets:   k8smetrics.ExponentialBuckets(1, 2, 12), //1, 2, 4, 8, ..., 2048
+			Buckets:   k8smetrics.ExponentialBuckets(1, 2, 18), //1, 2, 4, 8, ..., 131072 approx 1.5 days
 		}, []string{"deleted"},
 	)
 )
@@ -472,7 +473,7 @@ func RegisterAll(emitPerNodeGroupMetrics bool) {
 	legacyregistry.MustRegister(nodeTaintsCount)
 	legacyregistry.MustRegister(inconsistentInstancesMigsCount)
 	legacyregistry.MustRegister(binpackingHeterogeneity)
-	legacyregistry.MustRegister(scaleDownNodeDeletionDuration)
+	legacyregistry.MustRegister(scaleDownNodeRemovalLatency)
 
 	if emitPerNodeGroupMetrics {
 		legacyregistry.MustRegister(nodesGroupMinNodes)
@@ -761,8 +762,8 @@ func ObserveBinpackingHeterogeneity(instanceType, cpuCount, namespaceCount strin
 	binpackingHeterogeneity.WithLabelValues(instanceType, cpuCount, namespaceCount).Observe(float64(pegCount))
 }
 
-// UpdateScaleDownNodeDeletionDuration records the time after which node was deleted/needed
+// UpdateScaleDownNodeRemovalLatency records the time after which node was deleted/needed
 // again after being marked unneded
-func UpdateScaleDownNodeDeletionDuration(deleted string, duration time.Duration) {
-	scaleDownNodeDeletionDuration.WithLabelValues(deleted).Observe(duration.Seconds())
+func UpdateScaleDownNodeRemovalLatency(deleted bool, duration time.Duration) {
+	scaleDownNodeRemovalLatency.WithLabelValues(strconv.FormatBool(deleted)).Observe(duration.Seconds())
 }