Skip to content
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

settings: adds new node-labels, node-taints settings #390

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

etungsten
Copy link
Contributor

@etungsten etungsten commented Oct 10, 2019

Issue #, if available: Fixes #366

Description of changes:
Adds node-labels and node-taints settings for kubelet.service, both are modelled as HashMap<SingleLineString, SingleLineString>.
Allows users to label and taint the node through said settings in userdata.toml

Testing:
Launch Thar nodes with node-labels and node-taints set as follows in user data:

[settings.kubernetes.node-labels]
testLabel = "foo"
testLabel2 = "bar"

[settings.kubernetes.node-taints]
dedicated = "experimental:PreferNoSchedule"
special = "true:PreferNoSchedule"

Confirmed that kubelet starts successfully:

bash-5.0# systemctl status kubelet
● kubelet.service - Kubelet
   Loaded: loaded (/x86_64-thar-linux-gnu/sys-root/usr/lib/systemd/system/kubelet.service; enabled; vendor preset: enabled)
   Active: active (running) since Tue 2019-10-15 01:14:53 UTC; 17min ago
     Docs: https://github.com/kubernetes/kubernetes
  Process: 2684 ExecStartPre=/sbin/iptables -P FORWARD ACCEPT (code=exited, status=0/SUCCESS)
 Main PID: 2769 (kubelet)
    Tasks: 18
   Memory: 132.0M
      CPU: 12.846s
   CGroup: /system.slice/kubelet.service
           └─2769 /usr/bin/kubelet --cloud-provider aws --config /etc/kubernetes/kubelet/config --kubeconfig /etc/kubernetes/kubelet/kubeconfig --container-runt
ime=remote --container-runtime-endpoint=unix:///run/containerd/containerd.sock --network-plugin cni --root-dir /var/lib/kubelet --cert-dir /var/lib/kubelet/pk
i --volume-plugin-dir /var/lib/kubelet/plugins/volume/exec --node-ip 192.168.33.144 --node-labels testLabel=foo,testLabel2=bar --register-with-taints dedicate
d=experimental:PreferNoSchedule,special=true:PreferNoSchedule --pod-infra-container-image 602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/pause-amd64:3.1

And the nodes are labelled and tainted accordingly:

$ kubectl describe nodes ip-192-168-33-144.us-west-2.compute.internal                                                                
Name:               ip-192-168-33-144.us-west-2.compute.internal                                                                                              Roles:              <none>     
Labels:             beta.kubernetes.io/arch=amd64               
                    beta.kubernetes.io/instance-type=c5.large
                    beta.kubernetes.io/os=linux
                    failure-domain.beta.kubernetes.io/region=us-west-2
                    failure-domain.beta.kubernetes.io/zone=us-west-2a
                    kubernetes.io/arch=amd64
                    kubernetes.io/hostname=ip-192-168-33-144.us-west-2.compute.internal
                    kubernetes.io/os=linux
                    testLabel=foo
                    testLabel2=bar
Annotations:        node.alpha.kubernetes.io/ttl: 0
                    volumes.kubernetes.io/controller-managed-attach-detach: true
CreationTimestamp:  Mon, 14 Oct 2019 18:14:54 -0700
Taints:             dedicated=experimental:PreferNoSchedule
                    special=true:PreferNoSchedule
...

Without node-lables and node-taints specified. kubelet still successfully runs and registers the node:

bash-5.0# systemctl status kubelet                         
● kubelet.service - Kubelet                                                                                                                                   
   Loaded: loaded (/x86_64-thar-linux-gnu/sys-root/usr/lib/systemd/system/kubelet.service; enabled; vendor preset: enabled)                                   
   Active: active (running) since Tue 2019-10-15 19:22:18 UTC; 4min 31s ago
     Docs: https://github.com/kubernetes/kubernetes
  Process: 2634 ExecStartPre=/sbin/iptables -P FORWARD ACCEPT (code=exited, status=0/SUCCESS)
 Main PID: 2759 (kubelet)
    Tasks: 17
   Memory: 128.6M
      CPU: 3.726s
   CGroup: /system.slice/kubelet.service
           └─2759 /usr/bin/kubelet --cloud-provider aws --config /etc/kubernetes/kubelet/config --kubeconfig /etc/kubernetes/kubelet/kubeconfig --container-ru
nt
ime=remote --container-runtime-endpoint=unix:///run/containerd/containerd.sock --network-plugin cni --root-dir /var/lib/kubelet --cert-dir /var/lib/kubelet/pk
i --volume-plugin-dir /var/lib/kubelet/plugins/volume/exec --node-ip 192.168.35.147 --node-labels  --register-with-taints  --pod-infra-container-image 6024011
43452.dkr.ecr.us-west-2.amazonaws.com/eks/pause-amd64:3.1

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bcressey
Copy link
Contributor

bcressey commented Oct 10, 2019

What would be the level of effort to do this in a map type in the userdata?

E.g.

[settings.kubernetes]
node_labels = {
  testLabel = "foo",
  testLabel2 = "bar",
}

node_taints = {
  dedicated = "experimental:PreferNoSchedule",
}

That's a little awkward for taints since they have a more complex structure. key = "value:effect" isn't terrible though.

The more structured types buy us some free validation - we don't have to watch out for input like "foo=bar=baz" - and we can add logic that checks for legal characters in keys, values, effects if we need to.

@etungsten etungsten force-pushed the node-labels-and-taints-support branch from 1bed052 to 00d1bcb Compare October 10, 2019 18:07
@etungsten
Copy link
Contributor Author

etungsten commented Oct 10, 2019

Addresses issue with environment file not being generated if node-labels or node-taints not specified in user data. Removes = between option and parameter being passed to kubelet
Reran tests.

@etungsten etungsten force-pushed the node-labels-and-taints-support branch from 00d1bcb to b1065d9 Compare October 10, 2019 18:53
@etungsten
Copy link
Contributor Author

etungsten commented Oct 10, 2019

Replaces _ with - in documentation for node-taints and node-labels settings

@etungsten
Copy link
Contributor Author

etungsten commented Oct 10, 2019

What would be the level of effort to do this in a map type in the userdata?

Would need to model node_labels and node_taints as HashMap<String, String> then I'd imagine we would have to do something similar to what's currently being done in #386. Either an enhanced version of thar-be-settings or another handler would need to get the settings then generate the EnvironmentFile for kubelet.service? Thoughts @tjkirch?

@tjkirch
Copy link
Contributor

tjkirch commented Oct 10, 2019

Either an enhanced version of thar-be-settings or another handler would need to get the settings then generate the EnvironmentFile for kubelet.service?

Maybe we could do something clever with the Accept headers of requests to the API server to serialize in different ways; we could make node_labels serialize to key=value,key=value and then just use thar-be-settings as-is, instead of needing another program like host-containers...

But for now, yeah, to do a map I think you'd take the same approach as host-containers in #386.

The formatting example isn't quite right, though, it'd be more like this:

[settings.kubernetes.node_labels]
testLabel = "foo"
testLabel2 = "bar"

[settings.kubernetes.node_taints]
dedicated = "experimental:PreferNoSchedule"

I do agree that having each of those be a map would be better in a way because we wouldn't have to track/allow every possible label/taint, while allowing a more consistent input format for users. I don't feel strongly, though, because anyone using k8s labels/taints is already familiar with the label=thing,label=thing syntax, and accepting it as a setting seems fine to me, too, rather than making them adapt further to us. In other third-party-application-specific settings we've gone with the upstream format.

I guess I'd lean very slightly toward the current syntax for that reason. It would also mean several fewer components - you can see how simple this PR is now.

Thoughts?

@bcressey
Copy link
Contributor

Anyone using k8s labels/taints is already familiar with the label=thing,label=thing syntax, and accepting it as a setting seems fine to me, too, rather than making them adapt further to us.

The "canonical" form is a map per Labels and Selectors. The comma separated variant is just for kubelet args, it isn't really essential and may not be obvious to anyone who hasn't needed to customize the service.

@etungsten etungsten force-pushed the node-labels-and-taints-support branch from b1065d9 to 7f6d3f6 Compare October 13, 2019 16:56
@etungsten etungsten changed the title settings: adds new node-labels, node-taints settings node-painter: writes env file for setting node-labels and node-taints for kubelet Oct 13, 2019
@etungsten
Copy link
Contributor Author

Modelled node-labels and node-taints settings as HashMap<SingleLineString, SingleLineString>s.
Adds new node-painter tool to query the API settings and generate an EnvironmentFile from said settings to pass to kubelet.service to 'paint' the node when registering it.

@bcressey
Copy link
Contributor

Can you look into what's needed to do this with thar-be-settings, using handlebars to render the map to a template?

That's not necessarily a hard blocker here but we do need that functionality. One helper for a handful of settings isn't going to scale for us, and will be hard for downstream projects to extend.

@tjkirch
Copy link
Contributor

tjkirch commented Oct 14, 2019

Can you look into what's needed to do this with thar-be-settings, using handlebars to render the map to a template?

I think we'd need something like what I mentioned above, Accept headers on the server side, and helpers able to specify a desired format, along with multiple "Serialize" implementations for types in the API. It's not trivial.

Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you look into what's needed to do this with thar-be-settings, using handlebars to render the map to a template?

I think we'd need something like what I mentioned above, Accept headers on the server side, and helpers able to specify a desired format, along with multiple "Serialize" implementations for types in the API. It's not trivial.

bcressey had a simplifying idea - what if a template helper could accept a collection type instead of a scalar? Then we could have it format the labels/taints instead of needing another program. We'd only used helpers for scalars before and I didn't think of this. I'm going to test it.

README.md Outdated Show resolved Hide resolved
@tjkirch
Copy link
Contributor

tjkirch commented Oct 14, 2019

bcressey had a simplifying idea - what if a template helper could accept a collection type instead of a scalar? Then we could have it format the labels/taints instead of needing another program. We'd only used helpers for scalars before and I didn't think of this. I'm going to test it.

#408 adds the helper, which works on its own, but there's a higher level problem. thar-be-settings looks for keys referenced in templates, then queries the API for just those keys, for efficiency. This doesn't work if the "key" you're querying is really a prefix like settings.kubernetes.node-labels. If we're going with the simpler approach, t-b-s will have to be modified to query all settings, and associate the prefixes referenced in templates with the full structure of that part of the settings, which would have to be serde_json::Value form because of the varying types. I'm seeing what this would take to implement.

@tjkirch
Copy link
Contributor

tjkirch commented Oct 14, 2019

t-b-s will have to be modified to query all settings, and associate the prefixes referenced in templates with the full structure of that part of the settings, which would have to be serde_json::Value form because of the varying types. I'm seeing what this would take to implement.

Updated #408 to do this; seems to work OK. Hoping this PR will become quite simple with that improvement.

@etungsten etungsten force-pushed the node-labels-and-taints-support branch from 13bea61 to 0e9f07e Compare October 15, 2019 01:28
@etungsten
Copy link
Contributor Author

Rebases on top of 88165fc.

Removes node-painter as it is no longer necessary with thar-be-settings changes.
Generating node-taints and node-labels settings with thar-be-settings's join_maps helper.
Addresse @tjkirch 's comment on README

#408 should be merged before this.

@etungsten etungsten changed the title node-painter: writes env file for setting node-labels and node-taints for kubelet settings: adds new node-labels, node-taints settings Oct 15, 2019
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, nice work!

packages/api/settings-applier.service Outdated Show resolved Hide resolved
Comment on lines +22 to +23
--node-labels "${NODE_LABELS}" \
--register-with-taints "${NODE_TAINTS}" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the quotes needed? The curly-brace form of the variable should ensure that it's passed as a single argument.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use quotes anyway, even if it's the same, to clarify purpose? systemd's behavior is confusing; quotes aren't.

packages/api/settings-applier.service Outdated Show resolved Hide resolved
packages/kubernetes/kubelet-env Outdated Show resolved Hide resolved
Adds `node-labels` and `node-taints` settings for kubelet.
Allows users to label and taint the node through userdata

Updates README with descriptions of the new settings
@etungsten etungsten force-pushed the node-labels-and-taints-support branch from 0e9f07e to d2e60fe Compare October 15, 2019 19:25
@etungsten
Copy link
Contributor Author

Updated kubelet-env with the new join_map parameter no-fail-if-missing and verified it works even if node-labels and node-taints aren't set.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏷

@etungsten etungsten merged commit 294785c into develop Oct 15, 2019
@etungsten etungsten deleted the node-labels-and-taints-support branch October 15, 2019 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubelet: expose kubelet static labels
5 participants