Add karpenter.k8s.aws/instance-ami-id label

[yuanmwang-wf]

Description

Observed Behavior:
Noticed nodes created by Karpenter after we upgraded to 0.29.2 are missing the karpenter.k8s.aws/instance-ami-id label. We have some operation scripts (if a specific AMI has issues for example) that rely on this label so it would be great if we could get it back.

Expected Behavior:
Current Karpenter labels added by one of our provisioners.

    karpenter.k8s.aws/instance-category: c
    karpenter.k8s.aws/instance-cpu: "16"
    karpenter.k8s.aws/instance-encryption-in-transit-supported: "true"
    karpenter.k8s.aws/instance-family: c5n
    karpenter.k8s.aws/instance-generation: "5"
    karpenter.k8s.aws/instance-hypervisor: nitro
    karpenter.k8s.aws/instance-memory: "43008"
    karpenter.k8s.aws/instance-network-bandwidth: "15000"
    karpenter.k8s.aws/instance-pods: "234"
    karpenter.k8s.aws/instance-size: 4xlarge
    karpenter.sh/capacity-type: on-demand
    karpenter.sh/initialized: "true"
    karpenter.sh/provisioner-name: key-server
    karpenter.sh/registered: "true"

Reproduction Steps (Please include YAML):
A node created by the same provisioner last week:

    karpenter.k8s.aws/instance-ami-id: ami-0158858fcc1c5f6a3
    karpenter.k8s.aws/instance-category: c
    karpenter.k8s.aws/instance-cpu: "16"
    karpenter.k8s.aws/instance-encryption-in-transit-supported: "true"
    karpenter.k8s.aws/instance-family: c5n
    karpenter.k8s.aws/instance-generation: "5"
    karpenter.k8s.aws/instance-hypervisor: nitro
    karpenter.k8s.aws/instance-memory: "43008"
    karpenter.k8s.aws/instance-network-bandwidth: "15000"
    karpenter.k8s.aws/instance-pods: "234"
    karpenter.k8s.aws/instance-size: 4xlarge
    karpenter.sh/capacity-type: on-demand
    karpenter.sh/initialized: "true"
    karpenter.sh/provisioner-name: key-server
    karpenter.sh/registered: "true"

Versions:

• Chart Version: 0.29.2
• Kubernetes Version (kubectl version): 1.23

Server Version: version.Info{Major:"1", Minor:"23+", GitVersion:"v1.23.17-eks-a5565ad", GitCommit:"23f44c7dd4e8e23856387345e7cb80bf0f93ced6", GitTreeState:"clean", BuildDate:"2023-06-15T21:19:39Z", GoVersion:"go1.19.6", Compiler:"gc", Platform:"linux/amd64"}

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

[sftim]

We could also (as in: do both):

• populate the ARN of that AMI into the .status of the NodeClaim (BTW that's a mooted future API kind)
• annotate the node with that full ARN

[jonathan-innis]

Also, for context on why this was changed from a bug to a feature request: this was originally added as part of the drift work and was intended to be an implementation detail. It was left around and later removed as a result of the Machine migration work to ensure that we are only using label data obtained from the CreateFleet response to hydrate the labels on the node. At the time, the AMI Id was not a CreateFleet override, which meant that we couldn't get the AMI Id from the CreateFleet response, so we removed it from the label propagation.

That state has since changed so we should be able to pass the AMI ID through the overrides in the request (actually avoiding creating multiple launch templates due to architecture differences) and then pull the AMI ID detail from the selected override in the CreateFleet response.

[jonathan-innis]

Code Pointer to us getting the overrides from the CreateFleet response here: https://github.com/aws/karpenter/blob/main/pkg/providers/instance/types.go#L60

[gfcroft]

Hi, please assign me

[JeremyBolster]

Hi, I am also affected by this removal. I recently wrote a tool for checking the status of patching, just a simple "are all nodes on the most recent AMI?", and was using this label. Not having this label makes my tool more involved since it will need permissions to reach out to AWS.

[gfcroft]

We could also (as in: do both):

• populate the ARN of that AMI into the .status of the NodeClaim (BTW that's a mooted future API kind)
• annotate the node with that full ARN

I'm happy to go ahead adding this to the PR currently linked to this issue (#4637) if we wouldn't consider that too much change for one PR?

[jonathan-innis]

@yuanmwang-wf @gfcroft @JeremyBolster Was thinking through this a little bit. What if the AMI ID was stored in the Machine/NodeClaim status field and your script could grab the NodeClaim owner for the Node and looking up the NodeClaim status field to grab the AMI? Would that satisfy your use-case?

[yuanmwang-wf]

We'll need to update our script a little, but yep that would work for me