chore: add nodeclass name as label to nodeclaim #6125
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
Pull Request Test Coverage Report for Build 8946404523Details
💛 - Coveralls |
rschalo
force-pushed
the
label-nodeclaim-with-nodeclass-name
branch
from
20e2de1 to
406f2f1
Compare
| @@ -131,6 +131,7 @@ func (p *DefaultProvider) List(ctx context.Context, kc *corev1beta1.KubeletConfi | |||
| blockDeviceMappingsHash, | |||
| aws.StringValue((*string)(nodeClass.Spec.InstanceStorePolicy)), | |||
| aws.StringValue(nodeClass.Spec.AMIFamily), | |||
| nodeClass.Name, | |||
There was a problem hiding this comment.
I'm a bit worried with how this aligns with other changes we've made in the past that modifies our controller's memory usage + cache hits. Realistically, if every other detail is the same, and only the nodeClass name is different, we shouldn't need to maintain two cache values, especially since the data we're storing is pretty large.
There was a problem hiding this comment.
I'm not sure if adding this requirement into the cloudprovider.InstanceType is the right way to do this in the long run, especially as the nodeClass requirement is applicable for all instance types in the NodePool. I'd like to explore the idea of adding in a karpenter.sh/nodeclass-name label in the upstream code, so that we can better model this in the scheduling code. @jonathan-innis @engedaam thoughts?
There was a problem hiding this comment.
aligns with other changes we've made in the past that modifies our controller's memory usage + cache hits
IMO, this is the strongest point. This will definitely increase our memory footprint for storing these instance types in a cache. Given this trade-off, I'd agree that the neutral code is probably the best way to attack this, though I'd recommend that we take a format more akin to <nodeClassRef.group>/<nodeClassRef.kind>: <nodeClassRef.name>
|
This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity. |
Fixes #5178
Description
Adds the node class name to node claim.
How was this change tested?
Unit testing.
Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.