add the ability to remove nodes older than a certain age even when the node group has reached the min #240

awprice · 2024-05-27T08:07:06Z

adds max_node_age field to node group configuration
max_node_age by default is disabled, and needs to be a positive, greater than zero duration to enable
when the node group has reached the minimum and there are nodes older than the max_node_age, the scale delta will be set to +1
this will then force escalator to replace the oldest node

fixes #239

mwhittington21 · 2024-05-28T00:23:25Z

pkg/controller/controller.go

+	}
+
+	// We don't want to attempt to rotate nodes that have reached the max age if we haven't reached the minimum node
+	// count, as the scaling down of the node group will remove the oldest first anyway.


There's still an edge case here which is a node group that somehow has very consistent load such that it always maintains X number of nodes (e.g. always 2) and thus never gets cycled. I think this is very unlikely with the kinds of workloads that run on Escalator, but worth calling out in case others think it's worth considering here.

I had thought of this edge case whilst writing this feature - I guess it's a been an edge case in Escalator since it's existence, and won't be fixed or made worse by this feature being added.

Ideally if there is a consistent base load, the mininum nodes in the ASG should really be configured to a value that meets the base load, then this feature can be used to ensure the nodes are cycled out.

I can check if this edge case is documented in Escalator's documentation

I can check if this edge case is documented in Escalator's documentation

This isn't documented anywhere - I've raised #241

mwhittington21 · 2024-05-28T00:26:33Z

pkg/controller/controller.go

+
+	// We don't want to attempt to rotate nodes that have reached the max age if we haven't reached the minimum node
+	// count, as the scaling down of the node group will remove the oldest first anyway.
+	if len(untaintedNodes) != nodeGroup.Opts.MinNodes || len(untaintedNodes) == 0 {


Should this be <= to catch weird edge cases?

No I think we want to keep len(untaintedNodes) != nodeGroup.Opts.MinNodes, as we're only wanting this feature to come into play when we're at the minimum node level.

What I meant was using <= is more defensive in case we (or the cloud provider) have a bug elsewhere in the code that would allow the number of nodes present to be less than the number of MinNodes. In that case, we would actually start checking their age and potentially mess with the scale delta here, which complicates getting back to the MinNodes regardless.

All of this said, it's an unlikely edge case and I'm fine with this.

What I meant was using <= is more defensive in case we (or the cloud provider) have a bug elsewhere in the code that would allow the number of nodes present to be less than the number of MinNodes.

Exceeding the max and being under the min is already caught by Escalator here -

escalator/pkg/controller/controller.go

Lines 237 to 254 in 97246b1

if len(allNodes) < nodeGroup.Opts.MinNodes {

err = errors.New("node count less than the minimum")

log.WithField("nodegroup", nodegroup).Warningf(

"Node count of %v less than minimum of %v",

len(allNodes),

nodeGroup.Opts.MinNodes,

)

return 0, err

}

if len(allNodes) > nodeGroup.Opts.MaxNodes {

err = errors.New("node count larger than the maximum")

log.WithField("nodegroup", nodegroup).Warningf(

"Node count of %v larger than maximum of %v",

len(allNodes),

nodeGroup.Opts.MaxNodes,

)

return 0, err

}

mwhittington21 · 2024-05-28T00:27:53Z

pkg/controller/controller_scale_node_group_test.go

+				},
+				listerOptions: ListerOptions{},
+			},
+			1,


So in this case, the behaviour is that we scale up one node, it would be replaced and then we would do another scale up to replace the other older node once we made it back to our minimum node count again. Is that correct?

Yep - that's the desired behaviour

mwhittington21 · 2024-05-28T00:29:24Z

pkg/controller/controller_scale_node_group_test.go

+			nil,
+		},
+		{
+			"max_node_age enabled, some nodes are tainted",


Should there be a test case where we're at the MinNodes but some are tainted? I don't think that should occur in regular operation though, just throwing this out there in case it makes sense.

Had a look into adding this test case - it doesn't make sense to add to this feature's tests, as Escalator should get its self out of this scenario prior to the maxNodeAge code running (it will untaint the tainted nodes in an attempt to get the untainted nodes equal to the min nodes).

Again, this should be tested elsewhere already.

mwhittington21 · 2024-05-28T00:31:55Z

pkg/controller/node_group_test.go

@@ -471,6 +476,31 @@ func TestValidateNodeGroup(t *testing.T) {
 					HardDeleteGracePeriod:              "1h10m",
 					ScaleUpCoolDownPeriod:              "55m",
 					TaintEffect:                        "",
+					MaxNodeAge:                         "",


Piggy backing this particular edge case onto the case where there's an empty TaintEffect means that us also checking that we're parsing an empty MaxNodeAge is kinda lost. Either make a new table test case here or add that we're checking this field in the name of the table test case.

Have added a test case for this

Jacobious52 · 2024-05-28T00:46:32Z

pkg/controller/controller_scale_node_group_test.go

+		args              args
+		expectedNodeDelta int
+		err               error
+	}{


could we add a test for num nodes == maxNodes, and all expired, with scale up enabled

What are you worried about here? Are we expecting delta to be 0 because we're at max, or to be positive but unable to go further due to max? Either way I think that particular case isn't related to this feature

yeah just the very unlikely case of nodes at max for a long time, and this min of +1 node delta to cycle them trying to push above maxASG.

It's actually perfectly fine for node delta to be +1 when at the max ASG size - we have logic later on when trying to increase the ASG in the cloud provider above the max that will reject the scale up.

I don't think this test case is really relevant for this specific feature and the case of node delta exceeding the ASG max should be tested elsewhere.

Ideally we're really testing if nodeDelta is either +1 or 0 given the criteria for the feature and assuming whatever Escalator does with that nodeDelta is appropriately tested later on

Jacobious52 · 2024-05-28T00:48:59Z

pkg/controller/controller.go

+	if c.scaleOnMaxNodeAge(nodeGroup, untaintedNodes) {
+		log.WithField("nodegroup", nodegroup).
+			Info("Setting scale to minimum of 1 to rotate out a node older than the max node age")
+		nodesDelta = int(math.Max(float64(nodesDelta), 1))


should this also protect against nodesDelta exceeding ASGMax? Or is it protected against in normal scale functions

This is somewhat related to the other conversation, but I don't think this is relevant for the tests of this feature - this should already be covered by the general scaling tests that check that if nodeDelta would breach the ASG max, that Escalator prevents it.

mwhittington21

lgtm

…e node group has reached the min - adds max_node_age field to node group configuration - max_node_age by default is disabled, and needs to be a positive, greater than zero duration to enable - when the node group has reached the minimum and there are nodes older than the max_node_age, the scale delta will be set to +1 - this will then force escalator to replace the oldest node fixes #239 Signed-off-by: Alex Price <aprice@atlassian.com>

…o rotate nodes when at the ASG min, but there is a tainted node Signed-off-by: Alex Price <aprice@atlassian.com>

Signed-off-by: Alex Price <aprice@atlassian.com>

awprice requested review from Jacobious52, vincentportella, mwhittington21, MinyiZ and dtnyn May 27, 2024 08:07

awprice force-pushed the aprice/issue-239 branch from d5813c5 to 045ee4e Compare May 27, 2024 08:12

mwhittington21 reviewed May 28, 2024

View reviewed changes

Jacobious52 reviewed May 28, 2024

View reviewed changes

mwhittington21 self-requested a review May 28, 2024 23:14

mwhittington21 previously approved these changes May 28, 2024

View reviewed changes

Jacobious52 previously approved these changes May 29, 2024

View reviewed changes

awprice dismissed stale reviews from Jacobious52 and mwhittington21 via f83a85b May 30, 2024 01:57

awprice force-pushed the aprice/issue-239 branch from 045ee4e to f83a85b Compare May 30, 2024 01:57

Jacobious52 previously approved these changes May 30, 2024

View reviewed changes

mwhittington21 self-requested a review May 30, 2024 04:49

mwhittington21 previously approved these changes May 30, 2024

View reviewed changes

add test case for scale down to zero, also fix edge case that tries t…

6e227c5

…o rotate nodes when at the ASG min, but there is a tainted node Signed-off-by: Alex Price <aprice@atlassian.com>

awprice dismissed stale reviews from mwhittington21 and Jacobious52 via 6e227c5 May 30, 2024 06:07

mwhittington21 previously approved these changes May 30, 2024

View reviewed changes

add docs

72d1da3

Signed-off-by: Alex Price <aprice@atlassian.com>

awprice dismissed mwhittington21’s stale review via 72d1da3 June 6, 2024 04:56

mwhittington21 approved these changes Jun 6, 2024

View reviewed changes

Jacobious52 approved these changes Jun 6, 2024

View reviewed changes

awprice merged commit 679092c into master Jun 6, 2024
3 checks passed

awprice deleted the aprice/issue-239 branch June 6, 2024 05:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add the ability to remove nodes older than a certain age even when the node group has reached the min #240

add the ability to remove nodes older than a certain age even when the node group has reached the min #240

awprice commented May 27, 2024

mwhittington21 May 28, 2024

awprice May 28, 2024

awprice May 29, 2024

mwhittington21 May 28, 2024

awprice May 28, 2024

mwhittington21 May 28, 2024

awprice May 29, 2024

mwhittington21 May 28, 2024

awprice May 28, 2024

mwhittington21 May 28, 2024

awprice May 30, 2024

mwhittington21 May 28, 2024

awprice May 30, 2024

Jacobious52 May 28, 2024

mwhittington21 May 28, 2024

Jacobious52 May 28, 2024

awprice May 28, 2024

Jacobious52 May 28, 2024

awprice May 28, 2024

mwhittington21 left a comment

	if len(allNodes) < nodeGroup.Opts.MinNodes {
	err = errors.New("node count less than the minimum")
	log.WithField("nodegroup", nodegroup).Warningf(
	"Node count of %v less than minimum of %v",
	len(allNodes),
	nodeGroup.Opts.MinNodes,
	)
	return 0, err
	}
	if len(allNodes) > nodeGroup.Opts.MaxNodes {
	err = errors.New("node count larger than the maximum")
	log.WithField("nodegroup", nodegroup).Warningf(
	"Node count of %v larger than maximum of %v",
	len(allNodes),
	nodeGroup.Opts.MaxNodes,
	)
	return 0, err
	}

add the ability to remove nodes older than a certain age even when the node group has reached the min #240

add the ability to remove nodes older than a certain age even when the node group has reached the min #240

Conversation

awprice commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwhittington21 left a comment

Choose a reason for hiding this comment