Automatically create ASG tag from nodegroup's labels and taints - Round 2

[Skarlso]

In #4785 we tied the auto generation and population of labels and taints to desired capacity = 0. A follow up discussion in the PR determined that that is sub-optimal and that it should rather always do it based on a setting. Something like tagForAutoscaler or propagateASGTags: true or something in that direction.

Summary

• So we will add propagateASGTags: true setting to automatically propagate labels and taints to tags upon nodegroup creation.
• We will also need to update some docs around this.

[TBBle]

I lean towards tagForAutoscaler over propagateASGTags as that the same config option could be used to implement #2263, adding the other tags needed for the Cluster Autoscaler to recognise the node-group.

However, perhaps we want to keep those two things separate, e.g., if the user is going over the 50-tag limit, they may want to disable copying all the labels as tags, but keep the behaviour of #2263, so they only have to manage their own custom tags.

[Skarlso]

I lean towards tagForAutoscaler over propagateASGTags as that the same config option could be used to implement #2263, adding the other tags needed for the Cluster Autoscaler to recognise the node-group.

Wasn't the point that we do this automatically rather than users have to repeat themselves?

[TBBle]

I don't understand your question... I don't think we should always add the magic tags, I think it would be surprising if node groups were enabled for Cluster Autoscaler by default.

[Skarlso]

Yeah, no, exactly. I would tie it to a setting, saying -> propagate my tags because I will use them later please. Something like that. Not always always. Hence my suggestion-> propagateASGLabelsAndTaints or something like that. :)

[TBBle]

That's what I'm suggesting. Maybe what I'm suggesting isn't clear. To be succint:

For this ticket, I'm suggesting that tagForAutoscaler: true means "Copy tags from node labels", and for #2263, tagForAutoscaler: true means "Add the magic tags that enable a NodeGroup for CA".

Or conversely, if tagForAutoscaler: true:

• Adds the relevant magic CA tags, to enable CA to auto-scale the group; and
• Creates tags for all the specified labels and taints, to enable CA to scale-from-zero;

then this ticket is resolved and #2263 is mostly resolved, and the user is repeating themselves less than they do now, because with one config option, they "Enable a node group for CA, including scale-from-zero".

The remaining part of #2263 would be to remove the automatic adding of the magic CA tags from the setting which enables CA's required IRSA permissions, because that's combining two unrelated things. It would also be a breaking change, so I would keep it separate from this discussion. Edit: Thinking about it, I'd close #2263 with the above changes, and open a new ticket to make the change in this paragraph.

[Skarlso]

Alrighty then. :D

[TBBle]

That said, and to clarify my original comment, it might also make sense for there to be two config options:

• tagForAutoscaler: true: Adds the relevant magic CA tags, to enable CA to auto-scale the group
• propagateASGTags: true: Creates tags for all the specified labels and taints, to enable CA to scale-from-zero

And perhaps enabling propagateASGTags implies tagForAutoscaler. (There's also various other possibilities for defaulting the combinations of these behaviours)

That way, if you're going to hit the 50-tag limit using propagateASGTags, you can still use tagForAutoscaler, and manually define the tags that actually matter to autoscaling in order to stay under the 50-tag limit.

My leaning is to just have the one config option, and perhaps an option to disable propagateASGTags when tagForAutoscaler is on for the 50-tag cases, so that the default case ("just make it work") remains a single option to enable.

[Himangini]

@TBBle Thanks for all the comments. The team will review and see whether this functionality fits in with the project. In the meantime, if you would like to demonstrate a solution, please open a PR. 👍🏻

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions]

This issue was closed because it has been stalled for 5 days with no activity.

[Skarlso]

#5002 is pending for managed nodegroups.

[Himangini]

@Skarlso Can we close this since #5002 PR is merged?

[Skarlso]

yep, this can be closed. :) thanks, Gini!