Add Helm chart support for k8s node tolerations #13214

ZhuTopher · 2021-04-12T18:36:38Z

This PR is being made to address git issue #13203.

I tested that the Helm chart rendered using the following in my config.yaml

tolerations: [ {"key": "test", "operator": "Equal", "value": "test", "effect": "NoSchedule"} ]

and that the tolerations worked as intended on k8s nodes I tainted using kubectl taint nodes <node> test=test:NoSchedule

Note: I tested the Helm chart using all default values; so no FUSE nor logging-server deployments.

jiacheliu3 · 2021-04-16T07:48:45Z

@ZhuTopher Thanks for adding this! You will need to rebase when #13226 is in. It took 0.6.17 so you'll need to be 0.6.18.

integration/kubernetes/helm-chart/alluxio/values.yaml

jiacheliu3 · 2021-04-20T00:52:33Z

@nirav-chotai PTAL too, thanks!

jiacheliu3

Thanks @ZhuTopher ! I left some comments

jiacheliu3 · 2021-04-20T00:54:44Z

integration/kubernetes/helm-chart/alluxio/templates/master/statefulset.yaml

+      {{- if .Values.master.tolerations }}
+{{ toYaml .Values.master.tolerations | trim | indent 8  }}
+      {{- else if .Values.tolerations }}


How do you like .Values.master.tolerations + .Values.tolerations? In other words I think this should be a union instead of if-else.

It would be good if .Values.tolerations is there which can add tolerations to all the Alluxio components rather than specifying them individually. And if someone wants to run only some components on dedicated nodes, then they can use .Values.XYZ.tolerations

Yeah that makes sense to me, I can change it to a union of the tolerations.

ZhuTopher · 2021-04-20T19:09:25Z

integration/kubernetes/helm-chart/alluxio/templates/worker/daemonset.yaml

+{{ toYaml .Values.worker.tolerations | trim | indent 8  }}
+      {{- end }}
+      {{- if .Values.tolerations }}
+{{ toYaml .Values.tolerations | trim | indent 8  }}


@jiacheliu3 Is this what you meant by union? This will include duplicate definitions if the same toleration is in both .Values.tolerations and .Values.xxx.tolerations but I think that's on the user if they do that. Also I tested duplicate toleration definitions and I don't think it matters to Kubernetes anyway

Exactly this is what I meant

jiacheliu3

Thanks! LGTM except for the comment below. Please remove the change I mentioned in the comment then I'll merge it.

jiacheliu3 · 2021-04-21T06:00:53Z

integration/kubernetes/helm-chart/alluxio/templates/logserver/deployment.yaml

+      {{- if .Values.logserver.nodeSelector }}
+{{ toYaml .Values.logserver.nodeSelector | trim | indent 8  }}
+      {{- else if .Values.nodeSelector }}
+{{ toYaml .Values.nodeSelector | trim | indent 8  }}
+      {{- end }}


I can see you choose if+else if because the other code are doing this way. However I think if+if is a better option because it allows you to have general node selectors and more specific ones.

I think this PR (and helm chart version) is not intended for this nodeSelector change so let's remove this few lines. In a separate PR you can address this one and if you get a chance, change all node selectors to be if + if.

Actually I personally think it makes sense to have nodeSelector be XORs while letting tolerations be ANDs, I'll explain why in a moment but I agree this doesn't need to be in this PR so I'll remove it.

As for why I don't think nodeSelector should be AND, it is because in order for the resource to get scheduled it must match all of the labels provided. For taints, it is just a signifier to the Kubernetes scheduler that it may schedule onto nodes with the matching taints.

If we were talking about specifically "soft" node affinity then I think AND would make sense just like tolerations

Having this if-else order of 'most-specific first' allows the user to "override" the default set by the top-level nodeSelector. The alternative if it was AND would be that you would have to define the same selector for all resources except for the exception manually since you would no longer be able to use a top-level nodeSelector.

The implemented behaviour should be explained in the values.yaml comments more clearly regardless of what we choose too

ZhuTopher · 2021-04-22T17:07:48Z

integration/kubernetes/helm-chart/alluxio/values.yaml

@@ -416,7 +416,6 @@ logserver:
  # dnsPolicy: ClusterFirst
  # JVM options specific to the logserver container
  jvmOptions:
-  nodeSelector: {}


@jiacheliu3 I removed this because it was (and will still be) unused

Actually, let's keep it this way (don't remove this line).

One use case I can imagine is, there are nodes that have storage and nodes that do not. By defining a selector, the logserver is scheduled to a node that has storage, so that the logs are persisted to the storage.

Another argument is, let's do not take away what's there, unless the whole PR is intended for that. In this PR we add toleration, so let's not touch unrelated stuff. In the next PR just for nodeSelector, we can do the refactor as we redesign.

@ZhuTopher I'll merge after this is reverted. Thanks!

@ZhuTopher ooops sorry I seem to have clicked resolve unintendedly. Could you revert this line and then I'll merge? Thanks!

Oh sorry, I didn't notice this comment previously my bad! The reason I removed this from the Helm chart values.yaml is because this field is unused in templates/logserver/deployment.yaml.

I tried to add the nodeSelector behaviour from our other templates to it but since we decided to leave it out of this PR I also removed the related chart value to avoid confusion (chart implies you can specify a nodeSelector when you actually can't).

Anyway, again we can leave all this to a separate 'nodeSelector'-PR so I'll revert it for now.

codecov-commenter · 2021-04-22T17:14:35Z

Codecov Report

❗ No coverage uploaded for pull request base (master@830d4a8). Click here to learn what that means.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##             master   #13214   +/-   ##
=========================================
  Coverage          ?   17.37%           
  Complexity        ?     2625           
=========================================
  Files             ?     1362           
  Lines             ?    78502           
  Branches          ?     9519           
=========================================
  Hits              ?    13640           
  Misses            ?    63663           
  Partials          ?     1199

Impacted Files	Coverage Δ	Complexity Δ
...alluxio/cli/fs/command/DistributedLoadCommand.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 830d4a8...c1609bf. Read the comment docs.

jiacheliu3 · 2021-04-29T00:53:09Z

the failed test is due to flaky codecov, irrelevant to the change

jiacheliu3 · 2021-04-29T00:53:23Z

alluxio-bot, merge this please

jiacheliu3 · 2021-04-29T01:07:25Z

Same as the other PRs please add to the doc page too. Let's do the doc change in a separate PR in order not to block yourself. Thanks!

This PR is being made to address git [issue Alluxio#13203](Alluxio#13203). I tested that the Helm chart rendered using the following in my `config.yaml` ``` tolerations: [ {"key": "test", "operator": "Equal", "value": "test", "effect": "NoSchedule"} ] ``` and that the tolerations worked as intended on k8s nodes I tainted using `kubectl taint nodes <node> test=test:NoSchedule` **Note:** I tested the Helm chart using all default values; so no FUSE nor logging-server deployments. pr-link: Alluxio#13214 change-id: cid-81d850f3b53308a78b5bb207531b294ae09d8cdb

### What changes were proposed in this pull request? This PR proposes some additions to the [Alluxio k8s guide](https://docs.alluxio.io/os/user/stable/en/deploy/Running-Alluxio-On-Kubernetes.html) to reflect some of the recent additions to the Alluxio Helm chart: - `serviceAccount`: #13297 - `tolerations`: #13214 - `hostAliases`: #13226 - `strategy`: #13423 ### Why are the changes needed? Doc validation ### Does this PR introduce _any_ user-facing change? Yes, the documentation. pr-link: #13579 change-id: cid-cd456e708547bb121cdab5df1de7599860e6e089

alluxio-bot assigned jiacheliu3 Apr 12, 2021

ZhuTopher added 2 commits April 19, 2021 11:37

Add Helm chart support for k8s node tolerations

c4d4aed

Bump Helm chart changelog version

3a540fe

ZhuTopher force-pushed the helm-add-tolerations branch from 2570c5e to 3a540fe Compare April 19, 2021 15:39

ZhuTopher requested a review from jiacheliu3 April 19, 2021 18:02

jiacheliu3 reviewed Apr 20, 2021

View reviewed changes

integration/kubernetes/helm-chart/alluxio/values.yaml Show resolved Hide resolved

jiacheliu3 reviewed Apr 20, 2021

View reviewed changes

Change node tolerations to union instead of exclusive or

3c8f0a2

ZhuTopher commented Apr 20, 2021

View reviewed changes

jiacheliu3 approved these changes Apr 21, 2021

View reviewed changes

jiacheliu3 reviewed Apr 21, 2021

View reviewed changes

Remove addition of nodeSelector to logserver for this PR

c1609bf

ZhuTopher commented Apr 22, 2021

View reviewed changes

Revert removing nodeSelector from logserver in chart

bcfeb7f

alluxio-bot merged commit 7964c9f into Alluxio:master Apr 29, 2021

ZhuTopher mentioned this pull request Jun 7, 2021

[DOCFIX] Update k8s doc with new Helm additions #13579

Merged

alluxio-bot mentioned this pull request Jun 8, 2021

Port changes of [#13579] to branch-2.6 #13604

Merged

ssz1997 mentioned this pull request Jan 20, 2023

Helm chart add support for toleration #13203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Helm chart support for k8s node tolerations #13214

Add Helm chart support for k8s node tolerations #13214

ZhuTopher commented Apr 12, 2021

jiacheliu3 commented Apr 16, 2021

jiacheliu3 commented Apr 20, 2021

jiacheliu3 left a comment

jiacheliu3 Apr 20, 2021

nirav-chotai Apr 20, 2021

ZhuTopher Apr 20, 2021

ZhuTopher Apr 20, 2021

jiacheliu3 Apr 21, 2021

jiacheliu3 left a comment

jiacheliu3 Apr 21, 2021

ZhuTopher Apr 22, 2021

ZhuTopher Apr 22, 2021

jiacheliu3 Apr 23, 2021

jiacheliu3 Apr 23, 2021

jiacheliu3 Apr 28, 2021

ZhuTopher Apr 28, 2021

codecov-commenter commented Apr 22, 2021

jiacheliu3 commented Apr 29, 2021

jiacheliu3 commented Apr 29, 2021

jiacheliu3 commented Apr 29, 2021

Add Helm chart support for k8s node tolerations #13214

Add Helm chart support for k8s node tolerations #13214

Conversation

ZhuTopher commented Apr 12, 2021

jiacheliu3 commented Apr 16, 2021

jiacheliu3 commented Apr 20, 2021

jiacheliu3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiacheliu3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 22, 2021

Codecov Report

jiacheliu3 commented Apr 29, 2021

jiacheliu3 commented Apr 29, 2021

jiacheliu3 commented Apr 29, 2021