Allow kOps to set labels required for cluster operations #999
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 5039ef5 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61bb96526b500f000882c0a6 |
| @@ -90,6 +90,9 @@ func (c *Constraints) validateLabels() (errs *apis.FieldError) { | |||
|
|
|||
| func IsRestrictedLabelDomain(key string) bool { | |||
| labelDomain := getLabelDomain(key) | |||
| if labelDomain == "kops.k8s.io" { | |||
There was a problem hiding this comment.
Can you implement something similar to RestrictedLabelDomains (e.g. AllowedLabelDomains) and generalize this bit of code?
There was a problem hiding this comment.
Take a look if this is better now.
|
No. As far as I know, the entire k8s.io domain is banned, including subdomains. But these labels make no sense to set on kubelet, and kOps does not support setting labels as kubelet arguments. So for these we should be fine. |
olemarkus
force-pushed
the
relax-labels
branch
2 times, most recently
from
2a718bd to
cfcf6e5
Compare
Currently we pass all labels to the kubelet. This is to avoid an issue where if karpenter crashes right after launching the instance, but doesn't get a chance to create the node, the labels will still be applied. I can potentially see moving away from applying all labels in the kubelet args and instead moving towards a model where
Currently, karpenter only does this with a create call, but during periods of high congestion (e.g. fast node boot and kubeclient throttling), we could support this with a mechanism like CreateOrPatch. |
olemarkus
force-pushed
the
relax-labels
branch
from
cfcf6e5 to
e555a17
Compare
|
As mentioned on Slack, restricted labels should never be set by components other than those who own the respective keys. That is why they are restricted. For now, I suggest filtering out allowed labels in the launch template. It is a very curious edge case where EKS users would set flags that are owned by kOps, so I think the risk here is very low. |
olemarkus
force-pushed
the
relax-labels
branch
from
e555a17 to
e0cff03
Compare
| } | ||
| nodeLabelArgs.WriteString(strings.Join(labelStrings, ",")) |
There was a problem hiding this comment.
Thoughts on simplifying to:
fmt.Sprintf("--node-labels=%s", strings.Join(labelStrings, ","))
e0cff03 to
5039ef5
Compare
Wrong. It moved to another domain, as I checked the thing and it took me somewhere else. Try going into it and you should be redirected to another domain. |
|
@Tyler887 I suspect you are not talking about label domains/prefixes here |
1. Issue, if available:
*2. Description of changes:
kOps require two labels set on Nodes:
kops.k8s.io/instancegroupandkops.k8s.io/gpu.The latter is especially important as any pod using the
nvidiaruntimeclass will get that label as nodeSelector.To make this more future proof, I allowed the entire
kops.k8s.iodomain as there will not be a case where these labels are restricted from Karpenters point of view.3. 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.