Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/traefik] Adding support for Dynamic tolerations and nodeSelector #1382

Merged
merged 1 commit into from
Jun 29, 2017
Merged

[stable/traefik] Adding support for Dynamic tolerations and nodeSelector #1382

merged 1 commit into from
Jun 29, 2017

Conversation

djsly
Copy link
Contributor

@djsly djsly commented Jun 26, 2017

Adding supports for dynamically applying a node selector to allow the pods to runs on specific nodes. See https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#nodeselector for more details.

Adding supports for dynamically allowing the pods to runs on tainted nodes using tolerations.
See https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#taints-and-tolerations-beta-feature for more details.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @djsly. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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 the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 26, 2017
@djsly
Copy link
Contributor Author

djsly commented Jun 26, 2017

/cc @krancour

@k8s-ci-robot
Copy link
Contributor

@djsly: GitHub didn't allow me to request PR reviews from the following users: krancour.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @krancour

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.

@djsly djsly changed the title Adding support for Dynamic tolerations and nodeSelector [stable/traefik] Adding support for Dynamic tolerations and nodeSelector Jun 26, 2017
@djsly
Copy link
Contributor Author

djsly commented Jun 26, 2017

/cc @lachie83
@krancour

Can you guys review? Thanks!

@k8s-ci-robot k8s-ci-robot requested a review from lachie83 June 26, 2017 20:40
krancour
krancour reviewed Jun 28, 2017
View reviewed changes
stable/traefik/README.md Outdated
@@ -95,6 +95,8 @@ The following tables lists the configurable parameters of the Traefik chart and
| `memoryRequest` | Initial share of memory requested per Traefik pod | `20Mi` |
| `cpuLimit` | CPU limit per Traefik pod | `200m` |
| `memoryLimit` | Memory limit per Traefik pod | `30Mi` |
| `nodeSelector` | node labels for pod assignment | {} |
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, capitalize the first word in the description. Also use backticks around the empty map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

krancour
krancour reviewed Jun 28, 2017
View reviewed changes
stable/traefik/README.md Outdated
@@ -95,6 +95,8 @@ The following tables lists the configurable parameters of the Traefik chart and
| `memoryRequest` | Initial share of memory requested per Traefik pod | `20Mi` |
| `cpuLimit` | CPU limit per Traefik pod | `200m` |
| `memoryLimit` | Memory limit per Traefik pod | `30Mi` |
| `nodeSelector` | node labels for pod assignment | {} |
| `tolerations` | List of node taints to tolerate (requires Kubernetes >= 1.6) | [] |
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, use backticks around the empty array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

krancour
krancour reviewed Jun 28, 2017
View reviewed changes
stable/traefik/templates/deployment.yaml Outdated
@@ -21,6 +21,10 @@ spec:
heritage: "{{ .Release.Service }}"
spec:
terminationGracePeriodSeconds: 60
{{- if .Values.nodeSelector }}
nodeSelector:
{{ toYaml .Values.nodeSelector | indent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Very clean. I like this line. :)

@krancour
Copy link
Contributor

I haven't tried this, but this looks very useful and cleanly implemented. LGTM. @lachie83 or @viglesiasce can one of you please mark this as ok to test?

Note, as always, version number should also be incremented accordingly before merge.

@djsly djsly force-pushed the toleration-and-nodeselector-support branch from ab4f308 to d350fb3 Compare June 28, 2017 15:10
@djsly
Copy link
Contributor Author

djsly commented Jun 28, 2017

I'm not sure what is expected for the version number increment. Let me know if this is ok.

@djsly djsly force-pushed the toleration-and-nodeselector-support branch from 45eec91 to 92b90f9 Compare June 28, 2017 19:33
@krancour
Copy link
Contributor

I'm not sure what is expected for the version number increment. Let me know if this is ok.

I'm not sure yet either. afaik, charts are continuously (or near continuously) deployed, so 1 PR implicitly == 1 release. This means we need to be careful about incrementing the version number properly, which requires coordination with other open PRs for the same chart...

I am LGTM on this. I'll ask @viglesiasce or @lachie83 to just double-check the version bump before this gets merged.

@kachkaev
Copy link
Contributor

kachkaev commented Jun 29, 2017 •
edited
Loading

V 1.4.0 has already been taken by #1225 and 1.5.0 has been just mentioned by me in #1401. So the chart version in this PR might need to change depending on the merge order.

@djsly djsly force-pushed the toleration-and-nodeselector-support branch 2 times, most recently from 0465e2b to 01a308b Compare June 29, 2017 13:59
@djsly
Copy link
Contributor Author

djsly commented Jun 29, 2017

@kachkaev, @krancour Ok, I have used version: 1.6.0 and fixed the merge conflic. Please merge the PR for upgrading to traefik 1.3.1 first!

Who can give the /ok-to-test ?

@viglesiasce
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 29, 2017
@djsly
Copy link
Contributor Author

djsly commented Jun 29, 2017

@viglesiasce please merge #1401 before this one.

@djsly djsly force-pushed the toleration-and-nodeselector-support branch from a5f000e to 6bcd73c Compare June 29, 2017 14:26
@djsly
Copy link
Contributor Author

djsly commented Jun 29, 2017

Merge conflicts have been fixed.

@viglesiasce viglesiasce merged commit 9477c26 into helm:master Jun 29, 2017
@djsly djsly deleted the toleration-and-nodeselector-support branch June 29, 2017 14:36
mikesplain pushed a commit to barklyprotects/charts that referenced this pull request Jul 6, 2017
@djsly @mikesplain
Adding support for dynamic tolerations (alpha/beta) and nodeSelector (h…
3fd3728 
…elm#1382)
yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017
@djsly
Adding support for dynamic tolerations (alpha/beta) and nodeSelector (h…
d71c2b2 
…elm#1382)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krancour krancour krancour left review comments

@lachie83 lachie83 Awaiting requested review from lachie83

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@djsly @k8s-ci-robot @krancour @kachkaev @viglesiasce