Change UpdateStateWithUnneededList logic to also process nodes that are currently under deletion

Author: ttetyanka

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

@@ -3,16 +3,16 @@ package latencytracker
 import (
 	"time"
 
+	apiv1 "k8s.io/api/core/v1"
 	"k8s.io/autoscaler/cluster-autoscaler/metrics"
 	"k8s.io/klog/v2"
 )
 
 type LatencyTracker interface {
 	ObserveDeletion(nodeName string, timestamp time.Time)
-	UpdateStateWithUnneededList(list []NodeInfo, timestamp time.Time)
+	UpdateStateWithUnneededList(list []*apiv1.Node, currentlyInDeletion map[string]bool, timestamp time.Time)
 }
 type NodeInfo struct {
-	Name          string
 	UnneededSince time.Time
 	Threshold     time.Duration
 }
@@ -28,19 +28,34 @@ func NewNodeLatencyTracker() *NodeLatencyTracker {
 	}
 }
 
-func (t *NodeLatencyTracker) UpdateStateWithUnneededList(list []NodeInfo, timestamp time.Time) {
+// UpdateStateWithUnneededList records unneeded nodes and handles missing ones.
+func (t *NodeLatencyTracker) UpdateStateWithUnneededList(
+	list []*apiv1.Node,
+	currentlyInDeletion map[string]bool,
+	timestamp time.Time,
+) {
 	currentSet := make(map[string]struct{}, len(list))
-	for _, info := range list {
-		currentSet[info.Name] = struct{}{}
-		_, exists := t.nodes[info.Name]
-		if !exists {
-			t.nodes[info.Name] = NodeInfo{
-				Name:          info.Name,
-				UnneededSince: info.UnneededSince,
-				Threshold:     info.Threshold,
+	for _, node := range list {
+		currentSet[node.Name] = struct{}{}
+
+		if _, exists := t.nodes[node.Name]; !exists {
+			t.nodes[node.Name] = NodeInfo{
+				UnneededSince: timestamp,
+				Threshold:     0,
+			}
+			klog.V(2).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)
+				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(2).Infof("Started tracking unneeded node %s at %v with ScaleDownUnneededTime=%v",
-				info.Name, info.UnneededSince, info.Threshold)
 		}
 	}
 }

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

@@ -17,53 +17,10 @@ limitations under the License.
 package latencytracker
 
 import (
-	"sync"
 	"testing"
 	"time"
 )
 
-func TestUpdateStateWithUnneededList_AddsNewNodes(t *testing.T) {
-	tracker := NewNodeLatencyTracker()
-	now := time.Now()
-	node := NodeInfo{Name: "node1", UnneededSince: now, Threshold: 5 * time.Minute}
-
-	tracker.UpdateStateWithUnneededList([]NodeInfo{node}, now)
-
-	if _, ok := tracker.nodes["node1"]; !ok {
-		t.Errorf("expected node1 to be tracked, but was not")
-	}
-}
-
-func TestUpdateStateWithUnneededList_DoesNotDuplicate(t *testing.T) {
-	tracker := NewNodeLatencyTracker()
-	now := time.Now()
-	node := NodeInfo{Name: "node1", UnneededSince: now, Threshold: 5 * time.Minute}
-
-	tracker.UpdateStateWithUnneededList([]NodeInfo{node}, now)
-	tracker.UpdateStateWithUnneededList([]NodeInfo{node}, now.Add(time.Minute))
-
-	if len(tracker.nodes) != 1 {
-		t.Errorf("expected 1 tracked node, got %d", len(tracker.nodes))
-	}
-}
-
-func TestObserveDeletion_RemovesNode(t *testing.T) {
-	tracker := NewNodeLatencyTracker()
-	now := time.Now()
-	node := NodeInfo{
-		Name:          "node1",
-		UnneededSince: now.Add(-10 * time.Minute),
-		Threshold:     5 * time.Minute,
-	}
-	tracker.UpdateStateWithUnneededList([]NodeInfo{node}, now)
-
-	tracker.ObserveDeletion("node1", now)
-
-	if _, ok := tracker.nodes["node1"]; ok {
-		t.Errorf("expected node1 removed after ObserveDeletion")
-	}
-}
-
 func TestObserveDeletion_NoOpIfNodeNotTracked(t *testing.T) {
 	tracker := NewNodeLatencyTracker()
 	now := time.Now()
@@ -74,51 +31,3 @@ func TestObserveDeletion_NoOpIfNodeNotTracked(t *testing.T) {
 		t.Errorf("expected no nodes tracked, got %d", len(tracker.nodes))
 	}
 }
-
-func TestConcurrentUpdatesAndDeletions(t *testing.T) {
-	tracker := NewNodeLatencyTracker()
-	now := time.Now()
-
-	node := NodeInfo{
-		Name:          "node1",
-		UnneededSince: now,
-		Threshold:     2 * time.Minute,
-	}
-
-	var wg sync.WaitGroup
-	stop := make(chan struct{})
-
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
-		for {
-			select {
-			case <-stop:
-				return
-			default:
-				tracker.UpdateStateWithUnneededList([]NodeInfo{node}, time.Now())
-			}
-		}
-	}()
-
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
-		for {
-			select {
-			case <-stop:
-				return
-			default:
-				tracker.ObserveDeletion("node1", time.Now())
-			}
-		}
-	}()
-
-	time.Sleep(50 * time.Millisecond)
-	close(stop)
-	wg.Wait()
-
-	if len(tracker.nodes) > 1 {
-		t.Errorf("expected at most 1 tracked node, got %d", len(tracker.nodes))
-	}
-}

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

@@ -131,6 +131,9 @@ func (p *Planner) UpdateClusterState(podDestinations, scaleDownCandidates []*api
 	podDestinations = filterOutOngoingDeletions(podDestinations, deletions)
 	scaleDownCandidates = filterOutOngoingDeletions(scaleDownCandidates, deletions)
 	p.categorizeNodes(asMap(nodeNames(podDestinations)), scaleDownCandidates)
+	if p.nodeLatencyTracker != nil {
+		p.nodeLatencyTracker.UpdateStateWithUnneededList(p.unneededNodes.AsList(), deletions, p.latestUpdate)
+	}
 	p.rs.DropOldHints()
 	p.actuationInjector.DropOldHints()
 	return nil
@@ -310,19 +313,6 @@ func (p *Planner) categorizeNodes(podDestinations map[string]bool, scaleDownCand
 		}
 	}
 	p.unneededNodes.Update(removableList, p.latestUpdate)
-	if p.nodeLatencyTracker != nil {
-		var unneededList []latencytracker.NodeInfo
-		for _, n := range p.unneededNodes.AsList() {
-			if threshold, ok := p.unneededNodes.GetUnneededTimeForNode(p.context, n.Name); ok {
-				unneededList = append(unneededList, latencytracker.NodeInfo{
-					Name:          n.Name,
-					UnneededSince: p.latestUpdate,
-					Threshold:     threshold,
-				})
-			}
-		}
-		p.nodeLatencyTracker.UpdateStateWithUnneededList(unneededList, p.latestUpdate)
-	}
 	if unremovableCount > 0 {
 		klog.V(1).Infof("%v nodes found to be unremovable in simulation, will re-check them at %v", unremovableCount, unremovableTimeout)
 	}