-
Notifications
You must be signed in to change notification settings - Fork 835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add Name tag to default launch template #529
Conversation
@@ -159,6 +159,10 @@ func (p *LaunchTemplateProvider) createLaunchTemplate(ctx context.Context, optio | |||
TagSpecifications: []*ec2.LaunchTemplateTagSpecificationRequest{{ | |||
ResourceType: aws.String(ec2.ResourceTypeInstance), | |||
Tags: []*ec2.Tag{ | |||
{ | |||
Key: aws.String("Name"), | |||
Value: aws.String(fmt.Sprintf("Karpenter/%s", ptr.StringValue(options.Cluster.Name))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this collide, since we have many LTs per cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can avoid collisions, maybe this should also have the provisioner as well, but it will still be the same name per instance per provisioner. It might be worse if two provisioners map to the same LT, which would seem to indicate that one of those provisioners is unneeded.
If we want to make these unique and use the actual node-name, then we'd need to call EC2 to add a tag after creation. I'm not sure that's the best thing to do. But open to other opinions. At the end-of-the-day, this is just a convenience tag utilized if a user is looking at the EC2 console denoting that these are Karpenter nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also not per node. It's per-unique-hash-of-scheduling-constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see a reasonable argument that they should just all have the same name, but the philosophy behind AWS's naming features is completely opaque to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind I was totally confused about what entity this was tagging. All good.
Issue, if available:
N/A
Description of changes:
The name format is
Karpenter/<Cluster-Name>
I can't add the K8s Node Name since the name doesn't exist until after an instance is launched.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.