Skip to content

Commit b1f30f2

Browse files
sfotonysfotony
committed
Park nodes by ago, oldest first
1 parent 1195cd1 commit b1f30f2

File tree

3 files changed

+230
-33
lines changed

3 files changed

+230
-33
lines changed

‎README.md‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,9 @@ When `MaxParkedNodes` is set to a non-zero value:
142142
- For integers: `limit = configured value`
143143
2. **Count parked nodes**: k8s-shredder counts the number of currently parked nodes
144144
3. **Calculate available slots**: `availableSlots = limit - currentlyParked`
145-
4. **Limit parking**: If the number of eligible nodes exceeds available slots, only the first `availableSlots` nodes are parked
146-
5. **Skip if full**: If no slots are available (currentlyParked >= limit), parking is skipped for that eviction interval
145+
4. **Sort by age**: Eligible nodes are sorted by creation timestamp (oldest first) to ensure predictable parking order
146+
5. **Limit parking**: If the number of eligible nodes exceeds available slots, only the oldest `availableSlots` nodes are parked
147+
6. **Skip if full**: If no slots are available (currentlyParked >= limit), parking is skipped for that eviction interval
147148

148149
**Examples:**
149150
- `MaxParkedNodes: "0"` (default): No limit, all eligible nodes are parked
@@ -156,7 +157,16 @@ When `MaxParkedNodes` is set to a non-zero value:
156157
- **Proportional safety**: Maintains a consistent percentage of available capacity regardless of cluster size
157158
- **Auto-scaling friendly**: Works well with cluster auto-scaling by recalculating limits each cycle
158159

159-
This limit applies to both Karpenter drift detection and node label detection features. When multiple nodes are eligible for parking but the limit would be exceeded, k8s-shredder will park the nodes in the order they are discovered and skip the remaining nodes until the next eviction interval.
160+
**Predictable Parking Order:**
161+
Eligible nodes are **always sorted by creation timestamp (oldest first)**, regardless of whether `MaxParkedNodes` is set. This ensures:
162+
- **Consistent behavior**: The same nodes will be parked first across multiple eviction cycles
163+
- **Fair rotation**: Oldest nodes are prioritized for replacement during rolling upgrades
164+
- **Predictable capacity**: You can anticipate which nodes will be parked next when slots become available
165+
- **Deterministic ordering**: Even when parking all eligible nodes (no limit), they are processed in a predictable order
166+
167+
This sorting behavior applies to both Karpenter drift detection and node label detection features. When multiple nodes are eligible for parking:
168+
- **With no limit** (`MaxParkedNodes: "0"`): All nodes are parked in order from oldest to newest
169+
- **With a limit**: Only the oldest nodes up to the limit are parked; newer nodes wait for the next eviction interval
160170

161171
**Use cases:**
162172
- **Gradual node replacement**: Control the pace of node cycling during cluster upgrades

‎pkg/utils/parking.go‎

Lines changed: 84 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -550,11 +550,83 @@ func ParseMaxParkedNodes(ctx context.Context, k8sClient kubernetes.Interface, ma
550550
return limit, nil
551551
}
552552

553+
// sortNodesByCreationTime sorts a list of NodeInfo by creation timestamp (oldest first)
554+
// Returns a new sorted slice without modifying the original
555+
func sortNodesByCreationTime(ctx context.Context, k8sClient kubernetes.Interface, nodes []NodeInfo, logger *log.Entry) ([]NodeInfo, error) {
556+
if len(nodes) <= 1 {
557+
return nodes, nil
558+
}
559+
560+
// Create a map to store creation times
561+
type nodeWithTime struct {
562+
info NodeInfo
563+
createTime time.Time
564+
}
565+
566+
nodesWithTime := make([]nodeWithTime, 0, len(nodes))
567+
568+
// Fetch creation times for all nodes
569+
for _, node := range nodes {
570+
k8sNode, err := k8sClient.CoreV1().Nodes().Get(ctx, node.Name, metav1.GetOptions{})
571+
if err != nil {
572+
logger.WithError(err).WithField("nodeName", node.Name).Warn("Failed to get node for sorting, will use current position")
573+
// If we can't get the node, add it with zero time (will be sorted to end)
574+
nodesWithTime = append(nodesWithTime, nodeWithTime{
575+
info: node,
576+
createTime: time.Time{},
577+
})
578+
continue
579+
}
580+
581+
nodesWithTime = append(nodesWithTime, nodeWithTime{
582+
info: node,
583+
createTime: k8sNode.CreationTimestamp.Time,
584+
})
585+
}
586+
587+
// Sort by creation time (oldest first)
588+
slices.SortFunc(nodesWithTime, func(a, b nodeWithTime) int {
589+
// Nodes with zero time (failed to fetch) go to the end
590+
if a.createTime.IsZero() && !b.createTime.IsZero() {
591+
return 1
592+
}
593+
if !a.createTime.IsZero() && b.createTime.IsZero() {
594+
return -1
595+
}
596+
// Both have valid times or both are zero - compare normally
597+
if a.createTime.Before(b.createTime) {
598+
return -1
599+
}
600+
if a.createTime.After(b.createTime) {
601+
return 1
602+
}
603+
return 0
604+
})
605+
606+
// Extract sorted NodeInfo
607+
sortedNodes := make([]NodeInfo, len(nodesWithTime))
608+
for i, nwt := range nodesWithTime {
609+
sortedNodes[i] = nwt.info
610+
}
611+
612+
logger.WithField("nodeCount", len(sortedNodes)).Debug("Sorted nodes by creation time (oldest first)")
613+
614+
return sortedNodes, nil
615+
}
616+
553617
// LimitNodesToPark limits the number of nodes to park based on MaxParkedNodes configuration
554618
// It returns the nodes that should be parked, prioritizing the oldest nodes first
555619
func LimitNodesToPark(ctx context.Context, k8sClient kubernetes.Interface, nodes []NodeInfo, maxParkedNodesStr string, upgradeStatusLabel string, logger *log.Entry) ([]NodeInfo, error) {
556620
logger = logger.WithField("function", "LimitNodesToPark")
557621

622+
// Always sort nodes by creation timestamp (oldest first) to ensure predictable parking order
623+
// This happens regardless of whether MaxParkedNodes is set
624+
sortedNodes, err := sortNodesByCreationTime(ctx, k8sClient, nodes, logger)
625+
if err != nil {
626+
logger.WithError(err).Warn("Failed to sort nodes by creation time, using original order")
627+
sortedNodes = nodes
628+
}
629+
558630
// Parse MaxParkedNodes to get the actual limit
559631
maxParkedNodes, err := ParseMaxParkedNodes(ctx, k8sClient, maxParkedNodesStr, logger)
560632
if err != nil {
@@ -563,8 +635,8 @@ func LimitNodesToPark(ctx context.Context, k8sClient kubernetes.Interface, nodes
563635
}
564636

565637
if maxParkedNodes <= 0 {
566-
logger.Debug("MaxParkedNodes is not set or invalid, parking all eligible nodes")
567-
return nodes, nil
638+
logger.Debug("MaxParkedNodes is not set or invalid, parking all eligible nodes in order (oldest first)")
639+
return sortedNodes, nil
568640
}
569641

570642
// Count currently parked nodes
@@ -578,7 +650,7 @@ func LimitNodesToPark(ctx context.Context, k8sClient kubernetes.Interface, nodes
578650
"currentlyParked": currentlyParked,
579651
"maxParkedNodes": maxParkedNodes,
580652
"maxParkedNodesStr": maxParkedNodesStr,
581-
"eligibleNodes": len(nodes),
653+
"eligibleNodes": len(sortedNodes),
582654
}).Info("Checking parking limits")
583655

584656
// Calculate how many nodes we can park
@@ -594,32 +666,31 @@ func LimitNodesToPark(ctx context.Context, k8sClient kubernetes.Interface, nodes
594666
}
595667

596668
// If we have more eligible nodes than available slots, limit to the oldest nodes
597-
if len(nodes) > availableSlots {
669+
if len(sortedNodes) > availableSlots {
598670
logger.WithFields(log.Fields{
599-
"eligibleNodes": len(nodes),
671+
"eligibleNodes": len(sortedNodes),
600672
"availableSlots": availableSlots,
601673
"nodesToPark": availableSlots,
602-
"nodesToSkip": len(nodes) - availableSlots,
674+
"nodesToSkip": len(sortedNodes) - availableSlots,
603675
}).Info("Limiting nodes to park based on MaxParkedNodes configuration")
604676

605-
// For now, we'll take the first availableSlots nodes
606-
// In a future enhancement, we could sort by node creation time or other criteria
607-
limitedNodes := nodes[:availableSlots]
677+
// Take the first availableSlots nodes (oldest nodes due to sorting)
678+
limitedNodes := sortedNodes[:availableSlots]
608679

609680
// Log which nodes are being skipped
610-
for i := availableSlots; i < len(nodes); i++ {
611-
logger.WithField("skippedNode", nodes[i].Name).Debug("Skipping node due to MaxParkedNodes limit")
681+
for i := availableSlots; i < len(sortedNodes); i++ {
682+
logger.WithField("skippedNode", sortedNodes[i].Name).Debug("Skipping node due to MaxParkedNodes limit")
612683
}
613684

614685
return limitedNodes, nil
615686
}
616687

617688
logger.WithFields(log.Fields{
618-
"eligibleNodes": len(nodes),
689+
"eligibleNodes": len(sortedNodes),
619690
"availableSlots": availableSlots,
620691
}).Info("All eligible nodes can be parked within MaxParkedNodes limit")
621692

622-
return nodes, nil
693+
return sortedNodes, nil
623694
}
624695

625696
// UnparkNode unparks a node by removing parking labels, taints, and uncordoning it

‎pkg/utils/parking_test.go‎

Lines changed: 133 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -27,20 +27,43 @@ import (
2727

2828
func TestLimitNodesToPark_NoLimit(t *testing.T) {
2929
// Test case: MaxParkedNodes = "0" (no limit)
30+
// Even with no limit, nodes should be sorted by creation time (oldest first)
3031
cfg := config.Config{
3132
MaxParkedNodes: "0",
3233
UpgradeStatusLabel: "test-upgrade-status",
3334
}
3435

35-
nodes := []NodeInfo{
36-
{Name: "node1"},
37-
{Name: "node2"},
38-
{Name: "node3"},
39-
}
40-
4136
// Create a fake k8s client
4237
fakeClient := fake.NewSimpleClientset()
4338

39+
// Create nodes with different creation times (node3 is oldest, node1 is newest)
40+
baseTime := time.Now()
41+
node3 := &v1.Node{
42+
ObjectMeta: metav1.ObjectMeta{
43+
Name: "node3",
44+
CreationTimestamp: metav1.Time{Time: baseTime.Add(-3 * time.Hour)}, // Oldest
45+
},
46+
}
47+
node2 := &v1.Node{
48+
ObjectMeta: metav1.ObjectMeta{
49+
Name: "node2",
50+
CreationTimestamp: metav1.Time{Time: baseTime.Add(-2 * time.Hour)},
51+
},
52+
}
53+
node1 := &v1.Node{
54+
ObjectMeta: metav1.ObjectMeta{
55+
Name: "node1",
56+
CreationTimestamp: metav1.Time{Time: baseTime.Add(-1 * time.Hour)}, // Newest
57+
},
58+
}
59+
60+
_, err := fakeClient.CoreV1().Nodes().Create(context.Background(), node1, metav1.CreateOptions{})
61+
assert.NoError(t, err)
62+
_, err = fakeClient.CoreV1().Nodes().Create(context.Background(), node2, metav1.CreateOptions{})
63+
assert.NoError(t, err)
64+
_, err = fakeClient.CoreV1().Nodes().Create(context.Background(), node3, metav1.CreateOptions{})
65+
assert.NoError(t, err)
66+
4467
// Add some parked nodes to simulate existing parked nodes
4568
parkedNode := &v1.Node{
4669
ObjectMeta: metav1.ObjectMeta{
@@ -50,34 +73,67 @@ func TestLimitNodesToPark_NoLimit(t *testing.T) {
5073
},
5174
},
5275
}
53-
_, err := fakeClient.CoreV1().Nodes().Create(context.Background(), parkedNode, metav1.CreateOptions{})
76+
_, err = fakeClient.CoreV1().Nodes().Create(context.Background(), parkedNode, metav1.CreateOptions{})
5477
assert.NoError(t, err)
5578

79+
// Pass nodes in arbitrary order - they should be sorted by creation time
80+
nodes := []NodeInfo{
81+
{Name: "node1"},
82+
{Name: "node2"},
83+
{Name: "node3"},
84+
}
85+
5686
logger := log.WithField("test", "TestLimitNodesToPark_NoLimit")
5787

5888
result, err := LimitNodesToPark(context.Background(), fakeClient, nodes, cfg.MaxParkedNodes, cfg.UpgradeStatusLabel, logger)
5989

6090
assert.NoError(t, err)
61-
assert.Equal(t, len(nodes), len(result))
62-
assert.Equal(t, nodes, result)
91+
assert.Equal(t, 3, len(result), "Should return all nodes when no limit")
92+
// Verify nodes are sorted by creation time (oldest first)
93+
assert.Equal(t, "node3", result[0].Name, "Oldest node should be first")
94+
assert.Equal(t, "node2", result[1].Name, "Middle node should be second")
95+
assert.Equal(t, "node1", result[2].Name, "Newest node should be last")
6396
}
6497

6598
func TestLimitNodesToPark_WithLimit(t *testing.T) {
6699
// Test case: MaxParkedNodes = "2", 1 already parked, 3 eligible nodes
100+
// Nodes should be sorted by creation time (oldest first)
67101
cfg := config.Config{
68102
MaxParkedNodes: "2",
69103
UpgradeStatusLabel: "test-upgrade-status",
70104
}
71105

72-
nodes := []NodeInfo{
73-
{Name: "node1"},
74-
{Name: "node2"},
75-
{Name: "node3"},
76-
}
77-
78106
// Create a fake k8s client
79107
fakeClient := fake.NewSimpleClientset()
80108

109+
// Create nodes with different creation times (node3 is oldest, node1 is newest)
110+
baseTime := time.Now()
111+
node3 := &v1.Node{
112+
ObjectMeta: metav1.ObjectMeta{
113+
Name: "node3",
114+
CreationTimestamp: metav1.Time{Time: baseTime.Add(-3 * time.Hour)}, // Oldest
115+
},
116+
}
117+
node2 := &v1.Node{
118+
ObjectMeta: metav1.ObjectMeta{
119+
Name: "node2",
120+
CreationTimestamp: metav1.Time{Time: baseTime.Add(-2 * time.Hour)},
121+
},
122+
}
123+
node1 := &v1.Node{
124+
ObjectMeta: metav1.ObjectMeta{
125+
Name: "node1",
126+
CreationTimestamp: metav1.Time{Time: baseTime.Add(-1 * time.Hour)}, // Newest
127+
},
128+
}
129+
130+
_, err := fakeClient.CoreV1().Nodes().Create(context.Background(), node1, metav1.CreateOptions{})
131+
assert.NoError(t, err)
132+
_, err = fakeClient.CoreV1().Nodes().Create(context.Background(), node2, metav1.CreateOptions{})
133+
assert.NoError(t, err)
134+
_, err = fakeClient.CoreV1().Nodes().Create(context.Background(), node3, metav1.CreateOptions{})
135+
assert.NoError(t, err)
136+
81137
// Add one parked node to simulate existing parked nodes
82138
parkedNode := &v1.Node{
83139
ObjectMeta: metav1.ObjectMeta{
@@ -87,17 +143,25 @@ func TestLimitNodesToPark_WithLimit(t *testing.T) {
87143
},
88144
},
89145
}
90-
_, err := fakeClient.CoreV1().Nodes().Create(context.Background(), parkedNode, metav1.CreateOptions{})
146+
_, err = fakeClient.CoreV1().Nodes().Create(context.Background(), parkedNode, metav1.CreateOptions{})
91147
assert.NoError(t, err)
92148

149+
// Pass nodes in arbitrary order - they should be sorted by creation time
150+
nodes := []NodeInfo{
151+
{Name: "node1"},
152+
{Name: "node2"},
153+
{Name: "node3"},
154+
}
155+
93156
logger := log.WithField("test", "TestLimitNodesToPark_WithLimit")
94157

95158
result, err := LimitNodesToPark(context.Background(), fakeClient, nodes, cfg.MaxParkedNodes, cfg.UpgradeStatusLabel, logger)
96159

97160
assert.NoError(t, err)
98161
// Should only park 1 node (2 max - 1 already parked = 1 available slot)
99162
assert.Equal(t, 1, len(result))
100-
assert.Equal(t, "node1", result[0].Name)
163+
// Should park node3 as it's the oldest
164+
assert.Equal(t, "node3", result[0].Name)
101165
}
102166

103167
func TestLimitNodesToPark_NoAvailableSlots(t *testing.T) {
@@ -272,6 +336,58 @@ func TestLimitNodesToPark_PercentageLimit_NoSlots(t *testing.T) {
272336
assert.Equal(t, 0, len(result))
273337
}
274338

339+
func TestLimitNodesToPark_SortingByAge(t *testing.T) {
340+
// Test case: Verify nodes are sorted by creation time (oldest first)
341+
cfg := config.Config{
342+
MaxParkedNodes: "2",
343+
UpgradeStatusLabel: "test-upgrade-status",
344+
}
345+
346+
// Create a fake k8s client
347+
fakeClient := fake.NewSimpleClientset()
348+
349+
// Create 5 nodes with different creation times
350+
baseTime := time.Now()
351+
nodeCreationTimes := map[string]time.Time{
352+
"node-newest": baseTime.Add(-1 * time.Hour),
353+
"node-older": baseTime.Add(-2 * time.Hour),
354+
"node-middle": baseTime.Add(-3 * time.Hour),
355+
"node-very-old": baseTime.Add(-5 * time.Hour),
356+
"node-oldest-ever": baseTime.Add(-10 * time.Hour), // Should be parked first
357+
}
358+
359+
for name, createTime := range nodeCreationTimes {
360+
node := &v1.Node{
361+
ObjectMeta: metav1.ObjectMeta{
362+
Name: name,
363+
CreationTimestamp: metav1.Time{Time: createTime},
364+
},
365+
}
366+
_, err := fakeClient.CoreV1().Nodes().Create(context.Background(), node, metav1.CreateOptions{})
367+
assert.NoError(t, err)
368+
}
369+
370+
// Pass nodes in arbitrary order
371+
nodes := []NodeInfo{
372+
{Name: "node-newest"},
373+
{Name: "node-middle"},
374+
{Name: "node-oldest-ever"},
375+
{Name: "node-older"},
376+
{Name: "node-very-old"},
377+
}
378+
379+
logger := log.WithField("test", "TestLimitNodesToPark_SortingByAge")
380+
381+
result, err := LimitNodesToPark(context.Background(), fakeClient, nodes, cfg.MaxParkedNodes, cfg.UpgradeStatusLabel, logger)
382+
383+
assert.NoError(t, err)
384+
// Should park 2 oldest nodes
385+
assert.Equal(t, 2, len(result))
386+
// Should be sorted oldest first
387+
assert.Equal(t, "node-oldest-ever", result[0].Name, "Oldest node should be first")
388+
assert.Equal(t, "node-very-old", result[1].Name, "Second oldest node should be second")
389+
}
390+
275391
func TestCountParkedNodes(t *testing.T) {
276392
// Test case: Count parked nodes
277393
upgradeStatusLabel := "test-upgrade-status"

0 commit comments

Comments
 (0)