Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

Added support for tolerations in local-volume provisioner daemonset helm template #792

Merged
merged 1 commit into from
Jun 26, 2018

Conversation

dubuc
Copy link
Contributor

@dubuc dubuc commented Jun 6, 2018

Added support for tolerations in the DaemonsSet Helm template for the static local volume provisioner.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 6, 2018
@dubuc
Copy link
Contributor Author

dubuc commented Jun 6, 2018

/area local-volume
/assign @msau42

@msau42
Copy link
Contributor

msau42 commented Jun 6, 2018

cc @cofyc

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 6, 2018
@cofyc
Copy link
Contributor

cofyc commented Jun 7, 2018

This makes sense to me. Could you wait a while? There are some refactorings in #789, you can make changes based on new helm chart values/template files after that pr is merged.

# operator: Equal|Exist
# value: "{Value is the taint value the toleration matches to. If the operator is Exists, the value should be empty, otherwise just a regular string.}"
# effect: NoSchedule|PreferNoSchedule|NoExecution
# tolerationSeconds: <optional, by default not set>
Copy link
Contributor

@cofyc cofyc Jun 7, 2018

Choose a reason for hiding this comment

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

Because toleration is a spec of kubernetes api, add a reference to its docs should be enough.

  # Node tolerations for local-volume-provisioner scheduling to nodes with taints.
  # Ref: https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/
  tolerations: []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for that input, it does make more sense :) Fixed.

@dubuc
Copy link
Contributor Author

dubuc commented Jun 8, 2018

Yes no worries, I manually packaged a chart in the meantime. :) I can refactor a little after your updates. Thanks for the feedback.

@cofyc
Copy link
Contributor

cofyc commented Jun 14, 2018

@dubuc #789 has been merged, please rebase on latest master.
btw please also

  • add a example in helm/examples (and also added a expected yaml file in helm/test/expected, test it with helm/test/run.sh script)
  • update helm/README.md configurations section

@dubuc dubuc force-pushed the tolerations branch 2 times, most recently from 1a8fd86 to e2703a0 Compare June 25, 2018 21:05
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 25, 2018
@dubuc
Copy link
Contributor Author

dubuc commented Jun 25, 2018

@cofyc Hi! Sorry for the delay, but was quite busy the past few weeks. I've amended the PR to reflect the changes required. Thanks for the work you did with the tests and documentation! :) 👍 Please let me know if something else is required.

@msau42
Copy link
Contributor

msau42 commented Jun 25, 2018

/lgtm
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2018
@wongma7 wongma7 merged commit 5ecd0f5 into kubernetes-retired:master Jun 26, 2018
@msau42
Copy link
Contributor

msau42 commented Jun 28, 2018

@dubuc I had a follow up question on this. In your example, you are tolerating a master node. Does that mean you want to expose the local disks on your master node and also have workload pods scheduled to the master node?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/local-volume cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants