add NodeConfig api support for node labels and taints #1590
Conversation
| Labels map[string]string `json:"labels,omitempty"` | ||
| Taints []v1.Taint `json:"taints,omitempty"` |
There was a problem hiding this comment.
these are applied by kubelet, but they are "node" configuration so not sure if they feel best here or in a Spec.Kubelet.{Labels,Taints}
There was a problem hiding this comment.
I think it belongs in Kubelet
ndbaker1
force-pushed
the
labels+taints
branch
2 times, most recently
from
8d8c309
to
b8864a2
Compare
nodeadm/internal/kubelet/config.go
Outdated
| for labelKey, label := range cfg.Spec.Kubelet.Labels { | ||
| labelStrings = append(labelStrings, fmt.Sprintf("%s=%s", labelKey, label)) | ||
| } | ||
| k.additionalArguments["node-labels"] = strings.Join(labelStrings, ",") |
There was a problem hiding this comment.
Would like to double confirm these keys are supported for all versions?
node-labels and register-with-taints
There was a problem hiding this comment.
checked changelogs and only found a couple of relevant details:
- 1.23 :
--register-with-taintsoption is now available via the Kubelet config file field registerWithTaints- keeping the cli arg seems like the best things to do for now in the case that a cluster beyond 1.23 is spun up
- no important changes for
--node-labels
There was a problem hiding this comment.
however, does ^ incentivize us to put this in Kubelet.Config.RegisterWithTaints instead to keep on the edge of kubelet config adoption?
There was a problem hiding this comment.
Here's what we do in Karpenter, which has worked out very well: https://github.com/kubernetes-sigs/karpenter/blob/main/pkg/apis/v1beta1/nodeclaim.go#L70
There was a problem hiding this comment.
think this is a good idea, and will treat the config as first class. i.e. write the --register-with-taints argument if necessary based on a version check. similar to sichao's container runtime catch
| @@ -30,6 +31,7 @@ type NodeConfigList struct { | |||
|
|
|||
| type NodeConfigSpec struct { | |||
| Cluster ClusterDetails `json:"cluster,omitempty"` | |||
| Kubelet KubeletOptions `json:"kubelet,omitempty"` | |||
| FeatureGates map[string]bool `json:"featureGates,omitempty"` | |||
There was a problem hiding this comment.
Are feature gates scoped to our API, or is this kubelet feature gates?
There was a problem hiding this comment.
yep its ours not kubelet's; to opt-in for a particular "preset" of the options (it could use a better name though :P)
There was a problem hiding this comment.
Is there an example of how to configure this, or is it something we think well use in the future. If the latter, I'd recommend only introducing it when we need it. Sounds like the kind of thing that's design worthy in its own right.
There was a problem hiding this comment.
not really documented yet. i took some real actions from the bootstrap.sh which could be provided as feature gate toggles, but it might be wiser to keep it under wraps until someone explicitly asks for it 🤔
ndbaker1
force-pushed
the
labels+taints
branch
from
b8864a2
to
cfb5b2e
Compare
|
some branch shenanigans happened while updating the scope of this PR, so redirecting to #1593 |
Issue #, if available:
Description of changes:
extend
NodeConfigspec to include label and taint fields that pass through to kubelet--node-labelsand--register-with-taintsrespectively.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Testing Done
add new e2e tests for labels and taints
See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.