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.

Sign up for GitHub

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

Jump to bottom

helm: configurable nodeSelector and tolerations for all charts #13267

Merged
merged 1 commit into from
Sep 28, 2020
Merged

helm: configurable nodeSelector and tolerations for all charts #13267

merged 1 commit into from
Sep 28, 2020

Conversation

mvisonneau
Copy link
Contributor

@mvisonneau mvisonneau commented Sep 23, 2020 •
edited
Loading

Added configuration options nodeSelector (deployments) and tolerations (daemonsets and deployments)
for all the existing charts:

  • agent
  • hubble-relay
  • hubble-ui
  • managed-etcd
  • nodeinit
  • operator
  • preflight

On the preflight one, I also simplified the tolerations with a single 'operator: Exists'. Other than
that, the behaviour with default values should remain identical.

Initially, my use case was to be able to avoid having the hubble-relay pods running on tainted nodes.
I went forward with updating all the charts as I felt this could probably be useful for others.

Signed-off-by: Maxime VISONNEAU maxime.visonneau@gmail.com

Configurable nodeSelector and tolerations for all charts

@mvisonneau mvisonneau requested review from a team as code owners September 23, 2020 14:48
@mvisonneau mvisonneau requested review from a team September 23, 2020 14:48
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 23, 2020
@mvisonneau mvisonneau force-pushed the helm_configure_node_selector_and_tolerations branch from d105a01 to 1779b34 Compare September 23, 2020 14:58
@aanm
Copy link
Member

aanm commented Sep 24, 2020

@seanmwinn do we need to wait before #13259 is merged or we can go ahead with this one?

@seanmwinn
Copy link
Contributor

seanmwinn commented Sep 24, 2020 •
edited
Loading

@seanmwinn do we need to wait before #13259 is merged or we can go ahead with this one?

13259 also incorporates these changes, however it will be incompatible with this commit. This patch will be great to backport to 1.8 and 1.7 branches. No need to wait.

@aanm aanm added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 25, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 25, 2020
@aanm
Copy link
Member

aanm commented Sep 25, 2020

@mvisonneau it seems the unit tests are failing

@mvisonneau mvisonneau force-pushed the helm_configure_node_selector_and_tolerations branch from 1779b34 to 44605ed Compare September 25, 2020 14:17
@mvisonneau
Copy link
Contributor Author

indeed @aanm, sorry about that, they should be fixed now! 🤞

@mvisonneau mvisonneau force-pushed the helm_configure_node_selector_and_tolerations branch from 44605ed to 111ecd6 Compare September 25, 2020 14:19
@sayboras
Copy link
Member

@mvisonneau you can use make -C install/kubernetes all locally to generate yaml files, and do some static check.

@mvisonneau mvisonneau force-pushed the helm_configure_node_selector_and_tolerations branch from 111ecd6 to 8b6cd7f Compare September 25, 2020 14:37
@mvisonneau
Copy link
Contributor Author

mvisonneau commented Sep 25, 2020 •
edited
Loading

@mvisonneau you can use make -C install/kubernetes all locally to generate yaml files, and do some static check.

Indeed, it was much more efficient thanks @sayboras ! 🙇 🙏

@mvisonneau mvisonneau force-pushed the helm_configure_node_selector_and_tolerations branch from 8b6cd7f to 05feb3b Compare September 25, 2020 14:44
@mvisonneau
helm: configurable nodeSelector and tolerations for all charts
ddb6613 
Added configuration options nodeSelector (deployments) and tolerations (daemonsets and deployments)
for all the existing charts:
  - agent
  - hubble-relay
  - hubble-ui
  - managed-etcd
  - nodeinit
  - operator
  - preflight

On the preflight one, I also simplified the tolerations with a single 'operator: Exists'. Other than
that, the behaviour with default values should remain identical.

Initially, my use case was to be able to avoid having the hubble-relay pods running on tainted nodes.
I went forward with updating all the charts as I felt this could probably be useful for others.

Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>
@mvisonneau mvisonneau force-pushed the helm_configure_node_selector_and_tolerations branch from 05feb3b to ddb6613 Compare September 25, 2020 19:09
sayboras
sayboras approved these changes Sep 26, 2020
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM 💯, thanks :)

@qmonnet
Copy link
Member

qmonnet commented Sep 28, 2020

I'm merging without the full test suite since it only touches the charts.

I'll add the backports labels as suggested by Sean in his comment.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.4 Sep 28, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.10 Sep 28, 2020
@qmonnet qmonnet merged commit 5b68613 into cilium:master Sep 28, 2020
@joestringer
Copy link
Member

Do we have a strong argument for backporting to v1.7, @seanmwinn ? There are conflicts throughout the patch on that branch and we typically only backport major bugfixes and security-relevant fixes.

This was referenced Sep 29, 2020
Merged
Merged
@mvisonneau mvisonneau deleted the helm_configure_node_selector_and_tolerations branch September 30, 2020 09:18
@joestringer joestringer removed this from Needs backport from master in 1.7.10 Sep 30, 2020
@joestringer joestringer removed this from Needs backport from master in 1.8.4 Sep 30, 2020
@joestringer joestringer added this to Needs backport from master in 1.7.11 Sep 30, 2020
@joestringer joestringer added this to Needs backport from master in 1.8.5 Sep 30, 2020
@errordeveloper errordeveloper mentioned this pull request Oct 2, 2020
Merged
@pchaigno
Copy link
Member

pchaigno commented Oct 5, 2020

Removing this patch from 1.7 backports. There are a lot of conflicts and it doesn't seem to address a major bug.

@tklauser tklauser moved this from Needs backport from master to Backport done to v1.8 in 1.8.5 Oct 13, 2020
@tklauser tklauser moved this from Needs backport from master to Backport done to v1.7 in 1.7.11 Oct 13, 2020
@christarazi christarazi mentioned this pull request Oct 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@christarazi christarazi christarazi approved these changes

@seanmwinn seanmwinn seanmwinn approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.7.11
Backport done to v1.7
1.8.5
Backport done to v1.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@mvisonneau @aanm @seanmwinn @sayboras @qmonnet @joestringer @pchaigno @rolinh @christarazi @errordeveloper