diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index d2e412a..b91d377 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -35,6 +35,9 @@ type NodeGroupState struct { taintTracker []string forceTaintTracker []string + // tracks when nodes were last untainted, keyed by node name + untaintTracker map[string]time.Time + // used for tracking scale delta across runs, useful for reducing hysteresis scaleDelta int lastScaleOut time.Time @@ -100,7 +103,8 @@ func NewController(opts Opts, stopChan <-chan struct{}) (*Controller, error) { minimumLockDuration: nodeGroupOpts.ScaleUpCoolDownPeriodDuration(), nodegroup: nodeGroupOpts.Name, }, - scaleDelta: 0, + scaleDelta: 0, + untaintTracker: make(map[string]time.Time), } } @@ -222,6 +226,19 @@ func (c *Controller) scaleNodeGroup(nodegroup string, nodeGroup *NodeGroupState) return 0, err } + // Clean up untaint tracker for nodes that no longer exist + if len(nodeGroup.untaintTracker) > 0 { + activeNodes := make(map[string]bool, len(allNodes)) + for _, node := range allNodes { + activeNodes[node.Name] = true + } + for name := range nodeGroup.untaintTracker { + if !activeNodes[name] { + delete(nodeGroup.untaintTracker, name) + } + } + } + // store a cached version of node capacity if len(allNodes) > 0 { nodeGroup.cpuCapacity = *allNodes[0].Status.Allocatable.Cpu() diff --git a/pkg/controller/controller_scale_node_group_test.go b/pkg/controller/controller_scale_node_group_test.go index 5280311..17b762f 100644 --- a/pkg/controller/controller_scale_node_group_test.go +++ b/pkg/controller/controller_scale_node_group_test.go @@ -1497,3 +1497,405 @@ func TestScaleNodeGroupNodeMaxAge(t *testing.T) { }) } } + +func TestUntaintGracePeriod(t *testing.T) { + t.Run("recently untainted node is not re-tainted", func(t *testing.T) { + // Set up 10 nodes with low utilisation (triggers taint) + nodes := buildTestNodes(10, 1000, 1000) + pods := buildTestPods(5, 100, 100) // ~5% utilization, should trigger fast scale down + + nodeGroup := NodeGroupOptions{ + Name: "default", + CloudProviderGroupName: "default", + MinNodes: 2, + MaxNodes: 20, + ScaleUpThresholdPercent: 70, + TaintLowerCapacityThresholdPercent: 40, + TaintUpperCapacityThresholdPercent: 60, + FastNodeRemovalRate: 5, + SlowNodeRemovalRate: 2, + SoftDeleteGracePeriod: "1m", + HardDeleteGracePeriod: "2m", + ScaleUpCoolDownPeriod: "1m", + UntaintGracePeriod: "10m", + } + nodeGroups := []NodeGroupOptions{nodeGroup} + + client, opts, err := buildTestClient(nodes, pods, nodeGroups, ListerOptions{}) + require.NoError(t, err) + + testCloudProvider := test.NewCloudProvider(1) + testNodeGroup := test.NewNodeGroup( + nodeGroup.CloudProviderGroupName, + nodeGroup.Name, + int64(nodeGroup.MinNodes), + int64(nodeGroup.MaxNodes), + int64(len(nodes)), + ) + testCloudProvider.RegisterNodeGroup(testNodeGroup) + + nodeGroupsState := BuildNodeGroupsState(nodeGroupsStateOpts{ + nodeGroups: nodeGroups, + client: *client, + }) + + // Manually record an untaint timestamp for 2 nodes (recently untainted) + nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[0].Name] = stdtime.Now() + nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[1].Name] = stdtime.Now() + + controller := &Controller{ + Client: client, + Opts: opts, + stopChan: nil, + nodeGroups: nodeGroupsState, + cloudProvider: testCloudProvider, + } + + _, err = controller.scaleNodeGroup(nodeGroup.Name, nodeGroupsState[nodeGroup.Name]) + require.NoError(t, err) + + _, tainted, _, _ := controller.filterNodes(nodeGroupsState[nodeGroup.Name], nodes) + + for _, taintedNode := range tainted { + assert.NotEqual(t, nodes[0].Name, taintedNode.Name, "Node %s should not be tainted (within grace period)", nodes[0].Name) + assert.NotEqual(t, nodes[1].Name, taintedNode.Name, "Node %s should not be tainted (within grace period)", nodes[1].Name) + } + + assert.LessOrEqual(t, len(tainted), 5, "Should not taint more than FastNodeRemovalRate nodes") + assert.Greater(t, len(tainted), 0, "Should have tainted some nodes") + }) + + t.Run("expired grace period allows re-tainting", func(t *testing.T) { + nodes := buildTestNodes(10, 1000, 1000) + pods := buildTestPods(5, 100, 100) // ~5% utilization, triggers fast scale down + + nodeGroup := NodeGroupOptions{ + Name: "default", + CloudProviderGroupName: "default", + MinNodes: 2, + MaxNodes: 20, + ScaleUpThresholdPercent: 70, + TaintLowerCapacityThresholdPercent: 40, + TaintUpperCapacityThresholdPercent: 60, + FastNodeRemovalRate: 5, + SlowNodeRemovalRate: 2, + SoftDeleteGracePeriod: "1m", + HardDeleteGracePeriod: "2m", + ScaleUpCoolDownPeriod: "1m", + UntaintGracePeriod: "10m", + } + nodeGroups := []NodeGroupOptions{nodeGroup} + + client, opts, err := buildTestClient(nodes, pods, nodeGroups, ListerOptions{}) + require.NoError(t, err) + + testCloudProvider := test.NewCloudProvider(1) + testNodeGroup := test.NewNodeGroup( + nodeGroup.CloudProviderGroupName, + nodeGroup.Name, + int64(nodeGroup.MinNodes), + int64(nodeGroup.MaxNodes), + int64(len(nodes)), + ) + testCloudProvider.RegisterNodeGroup(testNodeGroup) + + nodeGroupsState := BuildNodeGroupsState(nodeGroupsStateOpts{ + nodeGroups: nodeGroups, + client: *client, + }) + + // Record untaint timestamp as 15 minutes ago (past the 10-minute grace period) + expiredTime := stdtime.Now().Add(-15 * stdtime.Minute) + nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[0].Name] = expiredTime + nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[1].Name] = expiredTime + + controller := &Controller{ + Client: client, + Opts: opts, + stopChan: nil, + nodeGroups: nodeGroupsState, + cloudProvider: testCloudProvider, + } + + _, err = controller.scaleNodeGroup(nodeGroup.Name, nodeGroupsState[nodeGroup.Name]) + require.NoError(t, err) + + _, tainted, _, _ := controller.filterNodes(nodeGroupsState[nodeGroup.Name], nodes) + + assert.Equal(t, 5, len(tainted), "Should taint FastNodeRemovalRate nodes when grace period expired") + }) + + t.Run("grace period of 0s has no effect (backwards compatibility)", func(t *testing.T) { + nodes := buildTestNodes(10, 1000, 1000) + pods := buildTestPods(5, 100, 100) // ~5% utilization + + nodeGroup := NodeGroupOptions{ + Name: "default", + CloudProviderGroupName: "default", + MinNodes: 2, + MaxNodes: 20, + ScaleUpThresholdPercent: 70, + TaintLowerCapacityThresholdPercent: 40, + TaintUpperCapacityThresholdPercent: 60, + FastNodeRemovalRate: 5, + SlowNodeRemovalRate: 2, + SoftDeleteGracePeriod: "1m", + HardDeleteGracePeriod: "2m", + ScaleUpCoolDownPeriod: "1m", + UntaintGracePeriod: "", // Empty = disabled + } + nodeGroups := []NodeGroupOptions{nodeGroup} + + client, opts, err := buildTestClient(nodes, pods, nodeGroups, ListerOptions{}) + require.NoError(t, err) + + testCloudProvider := test.NewCloudProvider(1) + testNodeGroup := test.NewNodeGroup( + nodeGroup.CloudProviderGroupName, + nodeGroup.Name, + int64(nodeGroup.MinNodes), + int64(nodeGroup.MaxNodes), + int64(len(nodes)), + ) + testCloudProvider.RegisterNodeGroup(testNodeGroup) + + nodeGroupsState := BuildNodeGroupsState(nodeGroupsStateOpts{ + nodeGroups: nodeGroups, + client: *client, + }) + + // Record untaint timestamps - should be ignored when grace period is 0 + nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[0].Name] = stdtime.Now() + nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[1].Name] = stdtime.Now() + + controller := &Controller{ + Client: client, + Opts: opts, + stopChan: nil, + nodeGroups: nodeGroupsState, + cloudProvider: testCloudProvider, + } + + _, err = controller.scaleNodeGroup(nodeGroup.Name, nodeGroupsState[nodeGroup.Name]) + require.NoError(t, err) + + _, tainted, _, _ := controller.filterNodes(nodeGroupsState[nodeGroup.Name], nodes) + + assert.Equal(t, 5, len(tainted), "Should taint FastNodeRemovalRate nodes when grace period is disabled") + }) + + t.Run("stale tracker entries are cleaned up", func(t *testing.T) { + nodes := buildTestNodes(5, 1000, 1000) + pods := buildTestPods(10, 100, 100) // normal utilization + + nodeGroup := NodeGroupOptions{ + Name: "default", + CloudProviderGroupName: "default", + MinNodes: 2, + MaxNodes: 20, + ScaleUpThresholdPercent: 70, + TaintLowerCapacityThresholdPercent: 40, + TaintUpperCapacityThresholdPercent: 60, + FastNodeRemovalRate: 5, + SlowNodeRemovalRate: 2, + SoftDeleteGracePeriod: "1m", + HardDeleteGracePeriod: "2m", + ScaleUpCoolDownPeriod: "1m", + UntaintGracePeriod: "10m", + } + nodeGroups := []NodeGroupOptions{nodeGroup} + + client, opts, err := buildTestClient(nodes, pods, nodeGroups, ListerOptions{}) + require.NoError(t, err) + + testCloudProvider := test.NewCloudProvider(1) + testNodeGroup := test.NewNodeGroup( + nodeGroup.CloudProviderGroupName, + nodeGroup.Name, + int64(nodeGroup.MinNodes), + int64(nodeGroup.MaxNodes), + int64(len(nodes)), + ) + testCloudProvider.RegisterNodeGroup(testNodeGroup) + + nodeGroupsState := BuildNodeGroupsState(nodeGroupsStateOpts{ + nodeGroups: nodeGroups, + client: *client, + }) + + // Add entries for existing nodes + nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[0].Name] = stdtime.Now() + nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[1].Name] = stdtime.Now() + + // Add stale entries for nodes that don't exist + nodeGroupsState[nodeGroup.Name].untaintTracker["non-existent-node-1"] = stdtime.Now() + nodeGroupsState[nodeGroup.Name].untaintTracker["non-existent-node-2"] = stdtime.Now() + + controller := &Controller{ + Client: client, + Opts: opts, + stopChan: nil, + nodeGroups: nodeGroupsState, + cloudProvider: testCloudProvider, + } + + assert.Equal(t, 4, len(nodeGroupsState[nodeGroup.Name].untaintTracker), "Should have 4 tracker entries before cleanup") + + _, err = controller.scaleNodeGroup(nodeGroup.Name, nodeGroupsState[nodeGroup.Name]) + require.NoError(t, err) + + assert.Equal(t, 2, len(nodeGroupsState[nodeGroup.Name].untaintTracker), "Should have 2 tracker entries after cleanup") + _, exists1 := nodeGroupsState[nodeGroup.Name].untaintTracker["non-existent-node-1"] + _, exists2 := nodeGroupsState[nodeGroup.Name].untaintTracker["non-existent-node-2"] + assert.False(t, exists1, "Stale entry 'non-existent-node-1' should be removed") + assert.False(t, exists2, "Stale entry 'non-existent-node-2' should be removed") + + _, exists := nodeGroupsState[nodeGroup.Name].untaintTracker[nodes[0].Name] + assert.True(t, exists, "Valid entry for existing node should remain") + }) + + t.Run("untaint records timestamp in wet mode", func(t *testing.T) { + // Set up nodes where some are tainted + taintedNodes := test.BuildTestNodes(3, test.NodeOpts{ + CPU: 1000, + Mem: 1000, + Tainted: true, + }) + untaintedNodes := test.BuildTestNodes(2, test.NodeOpts{ + CPU: 1000, + Mem: 1000, + }) + nodes := append(taintedNodes, untaintedNodes...) + + // High utilization to trigger scale up (untaint) + pods := buildTestPods(10, 500, 500) // High utilization + + nodeGroup := NodeGroupOptions{ + Name: "default", + CloudProviderGroupName: "default", + MinNodes: 5, + MaxNodes: 20, + ScaleUpThresholdPercent: 70, + TaintLowerCapacityThresholdPercent: 40, + TaintUpperCapacityThresholdPercent: 60, + FastNodeRemovalRate: 5, + SlowNodeRemovalRate: 2, + SoftDeleteGracePeriod: "1m", + HardDeleteGracePeriod: "2m", + ScaleUpCoolDownPeriod: "1m", + UntaintGracePeriod: "10m", + } + nodeGroups := []NodeGroupOptions{nodeGroup} + + client, opts, err := buildTestClient(nodes, pods, nodeGroups, ListerOptions{}) + require.NoError(t, err) + + testCloudProvider := test.NewCloudProvider(1) + testNodeGroup := test.NewNodeGroup( + nodeGroup.CloudProviderGroupName, + nodeGroup.Name, + int64(nodeGroup.MinNodes), + int64(nodeGroup.MaxNodes), + int64(len(nodes)), + ) + testCloudProvider.RegisterNodeGroup(testNodeGroup) + + nodeGroupsState := BuildNodeGroupsState(nodeGroupsStateOpts{ + nodeGroups: nodeGroups, + client: *client, + }) + + controller := &Controller{ + Client: client, + Opts: opts, + stopChan: nil, + nodeGroups: nodeGroupsState, + cloudProvider: testCloudProvider, + } + + assert.Equal(t, 0, len(nodeGroupsState[nodeGroup.Name].untaintTracker), "Tracker should be empty before untaint") + + beforeTime := stdtime.Now() + _, err = controller.scaleNodeGroup(nodeGroup.Name, nodeGroupsState[nodeGroup.Name]) + require.NoError(t, err) + afterTime := stdtime.Now() + + for nodeName, untaintTime := range nodeGroupsState[nodeGroup.Name].untaintTracker { + assert.True(t, untaintTime.After(beforeTime) || untaintTime.Equal(beforeTime), + "Untaint time for %s should be after or equal to beforeTime", nodeName) + assert.True(t, untaintTime.Before(afterTime) || untaintTime.Equal(afterTime), + "Untaint time for %s should be before or equal to afterTime", nodeName) + } + }) + + t.Run("dry mode also records untaint timestamps", func(t *testing.T) { + // Set up nodes + nodes := buildTestNodes(5, 1000, 1000) + + // High utilization to trigger scale up (untaint) + pods := buildTestPods(10, 500, 500) + + nodeGroup := NodeGroupOptions{ + Name: "default", + CloudProviderGroupName: "default", + MinNodes: 5, + MaxNodes: 20, + ScaleUpThresholdPercent: 70, + TaintLowerCapacityThresholdPercent: 40, + TaintUpperCapacityThresholdPercent: 60, + FastNodeRemovalRate: 5, + SlowNodeRemovalRate: 2, + SoftDeleteGracePeriod: "1m", + HardDeleteGracePeriod: "2m", + ScaleUpCoolDownPeriod: "1m", + UntaintGracePeriod: "10m", + DryMode: true, // Enable dry mode + } + nodeGroups := []NodeGroupOptions{nodeGroup} + + client, opts, err := buildTestClient(nodes, pods, nodeGroups, ListerOptions{}) + require.NoError(t, err) + + testCloudProvider := test.NewCloudProvider(1) + testNodeGroup := test.NewNodeGroup( + nodeGroup.CloudProviderGroupName, + nodeGroup.Name, + int64(nodeGroup.MinNodes), + int64(nodeGroup.MaxNodes), + int64(len(nodes)), + ) + testCloudProvider.RegisterNodeGroup(testNodeGroup) + + nodeGroupsState := BuildNodeGroupsState(nodeGroupsStateOpts{ + nodeGroups: nodeGroups, + client: *client, + }) + + // Simulate some nodes being tainted in dry mode + nodeGroupsState[nodeGroup.Name].taintTracker = []string{nodes[0].Name, nodes[1].Name, nodes[2].Name} + + controller := &Controller{ + Client: client, + Opts: opts, + stopChan: nil, + nodeGroups: nodeGroupsState, + cloudProvider: testCloudProvider, + } + + assert.Equal(t, 0, len(nodeGroupsState[nodeGroup.Name].untaintTracker), "Tracker should be empty before untaint") + + beforeTime := stdtime.Now() + _, err = controller.scaleNodeGroup(nodeGroup.Name, nodeGroupsState[nodeGroup.Name]) + require.NoError(t, err) + afterTime := stdtime.Now() + + assert.Greater(t, len(nodeGroupsState[nodeGroup.Name].untaintTracker), 0, "Should have recorded untaint timestamps in dry mode") + + for nodeName, untaintTime := range nodeGroupsState[nodeGroup.Name].untaintTracker { + assert.True(t, untaintTime.After(beforeTime) || untaintTime.Equal(beforeTime), + "Untaint time for %s should be after or equal to beforeTime", nodeName) + assert.True(t, untaintTime.Before(afterTime) || untaintTime.Equal(afterTime), + "Untaint time for %s should be before or equal to afterTime", nodeName) + } + }) +} diff --git a/pkg/controller/node_group.go b/pkg/controller/node_group.go index 97b9f06..78a0aa2 100644 --- a/pkg/controller/node_group.go +++ b/pkg/controller/node_group.go @@ -49,6 +49,11 @@ type NodeGroupOptions struct { MaxNodeAge string `json:"max_node_age,omitempty" yaml:"max_node_age,omitempty"` + // UntaintGracePeriod is the duration to wait after untainting a node before + // it can be re-tainted. This gives the scheduler time to fill the node and + // gives Escalator a stable signal before deciding whether to remove it again. + UntaintGracePeriod string `json:"untaint_grace_period,omitempty" yaml:"untaint_grace_period,omitempty"` + // UnhealthyNodeGracePeriod is the duration to wait before testing if a node // can be considered unhealthy. UnhealthyNodeGracePeriod string `json:"unhealthy_node_grace_period,omitempty" yaml:"unhealthy_node_grace_period,omitempty"` @@ -68,6 +73,7 @@ type NodeGroupOptions struct { hardDeleteGracePeriodDuration time.Duration scaleUpCoolDownPeriodDuration time.Duration maxNodeAgeDuration time.Duration + untaintGracePeriodDuration time.Duration unhealthyNodeGracePeriodDuration time.Duration } @@ -145,6 +151,11 @@ func ValidateNodeGroup(nodegroup NodeGroupOptions) []error { checkThat(validMaxNodeAgeDuration(nodegroup.MaxNodeAge), "max_node_age failed to parse into a time.Duration. Set to '0' or '' to disable, or a positive Go duration to enable.") + // UntaintGracePeriod is an optional parameter. + if len(nodegroup.UntaintGracePeriod) > 0 { + checkThat(nodegroup.UntaintGracePeriodDuration() >= 0, "untaint_grace_period failed to parse into a time.Duration. check your formatting.") + } + // UnhealthyNodeGracePeriod is an optional parameter. if len(nodegroup.UnhealthyNodeGracePeriod) > 0 { checkThat(nodegroup.UnhealthyNodeGracePeriodDuration() > 0, "unhealthy_node_grace_period failed to parse into a time.Duration. check your formatting.") @@ -227,6 +238,19 @@ func (n *NodeGroupOptions) MaxNodeAgeDuration() time.Duration { return n.maxNodeAgeDuration } +// UntaintGracePeriodDuration lazily returns/parses the untaintGracePeriod string into a duration +func (n *NodeGroupOptions) UntaintGracePeriodDuration() time.Duration { + if n.untaintGracePeriodDuration == 0 { + duration, err := time.ParseDuration(n.UntaintGracePeriod) + if err != nil { + return 0 + } + n.untaintGracePeriodDuration = duration + } + + return n.untaintGracePeriodDuration +} + // UnhealthyNodeGracePeriodDuration lazily returns/parses the unhealthyNodeGracePeriod string into a duration func (n *NodeGroupOptions) UnhealthyNodeGracePeriodDuration() time.Duration { if n.unhealthyNodeGracePeriodDuration != 0 { @@ -387,6 +411,7 @@ func BuildNodeGroupsState(opts nodeGroupsStateOpts) map[string]*NodeGroupState { minimumLockDuration: ng.ScaleUpCoolDownPeriodDuration(), nodegroup: ng.Name, }, + untaintTracker: make(map[string]time.Time), } } return nodeGroupsState diff --git a/pkg/controller/scale_up.go b/pkg/controller/scale_up.go index 2e8b356..90d6ec9 100644 --- a/pkg/controller/scale_up.go +++ b/pkg/controller/scale_up.go @@ -3,6 +3,7 @@ package controller import ( "fmt" "sort" + "time" "github.com/atlassian/escalator/pkg/k8s" "github.com/atlassian/escalator/pkg/metrics" @@ -145,6 +146,7 @@ func (c *Controller) untaintNewestN(nodes []*v1.Node, nodeGroup *NodeGroupState, } else { bundle.node = updatedNode untaintedIndices = append(untaintedIndices, bundle.index) + nodeGroup.untaintTracker[bundle.node.Name] = time.Now() } } } else { @@ -159,6 +161,7 @@ func (c *Controller) untaintNewestN(nodes []*v1.Node, nodeGroup *NodeGroupState, // Delete from tracker nodeGroup.taintTracker = append(nodeGroup.taintTracker[:deleteIndex], nodeGroup.taintTracker[deleteIndex+1:]...) untaintedIndices = append(untaintedIndices, bundle.index) + nodeGroup.untaintTracker[bundle.node.Name] = time.Now() log.WithField("drymode", c.dryMode(nodeGroup)).Infof("Untainting node %v", bundle.node.Name) } } diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 41f3baf..40703f9 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -2,6 +2,7 @@ package controller import ( "math" + "time" "github.com/atlassian/escalator/pkg/k8s" "github.com/pkg/errors" @@ -92,6 +93,21 @@ func (c *Controller) taintInstances(sortedNodes nodesByOldestCreationTime, nodeG break } + // Skip nodes within the untaint grace period + if nodeGroup.Opts.UntaintGracePeriodDuration() > 0 { + if untaintTime, exists := nodeGroup.untaintTracker[bundle.node.Name]; exists { + if time.Since(untaintTime) < nodeGroup.Opts.UntaintGracePeriodDuration() { + log.WithField("nodegroup", nodeGroup.Opts.Name).Infof( + "Skipping taint of node %v: untainted %v ago, grace period %v", + bundle.node.Name, + time.Since(untaintTime).Round(time.Second), + nodeGroup.Opts.UntaintGracePeriodDuration(), + ) + continue + } + } + } + // only actually taint in non-dry mode if c.dryMode(nodeGroup) { nodeGroup.taintTracker = append(nodeGroup.taintTracker, bundle.node.Name)