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

feat: add default toleration to tolerate all taints #215

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vikram-katkar
Copy link

Issue #, if available:

Description of changes:
Adding default toleration to the daemonSet to tolerate any taints.

Ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/

An empty key with operator Exists matches all keys, values and effects which means this will tolerate everything.

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

@smobyrne smobyrne added the enhancement New feature or request label Apr 18, 2023
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (8d3e503) 53.15% compared to head (7ce584b) 53.15%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #215   +/-   ##
=======================================
  Coverage   53.15%   53.15%           
=======================================
  Files           8        8           
  Lines         730      730           
=======================================
  Hits          388      388           
  Misses        332      332           
  Partials       10       10           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msardana94
Copy link

This is open for a while. Do you know @simonmarty if this can be reviewed? (not sure who will it be but saw that you contributed to one file recently so tagged you 🙂 )

@simonmarty
Copy link
Contributor

Could you explain your use case? Helm chart configuration options can be overriden based on your needs. Why does this need to be merged back into the default configuration?

@msardana94
Copy link

Yes I was able to work around this by setting the tolerations but I wasn't sure if it should be set as default? I am still new to EKS and saw this issue (#267). If we can't set that as default, may be we can add it in troubleshooting?

I followed the docs to use csi driver provider for secrets manager and ran into this issue.

@vikram-katkar
Copy link
Author

@simonmarty
Most folks would probably assume the Daemon set runs on all nodes by default. If not, users might end up dealing with issues and struggle with configurations, making it a bit of a hassle to give the tool a spin.

Setting it as the default would save users from this hassle.
The pros can still tweak things if they want to by using overrides.

@simonmarty simonmarty requested a review from a team as a code owner June 3, 2024 18:32
@simonmarty
Copy link
Contributor

I'm good to merge this based on the above info and the fact that the Secrets Store CSI Driver does the same thing in their helm chart

@simonmarty simonmarty enabled auto-merge (squash) June 3, 2024 18:34
@toretore
Copy link

toretore commented Jun 4, 2024

Another consideration before making this the default is that it works the other way when running some workloads on Fargate. When tolerating the Fargate nodes' taint, a DeamonSet will try to schedule pods on said nodes, which does not work. I've had to remove the default "tolerate all taints" from a number of chart defaults because of this (and it can be a PITA to work out exactly which magic incantations to use to get the correct value through Terraform and then Helm with their template languages).

@simonmarty
Copy link
Contributor

simonmarty commented Jun 6, 2024

@toretore Would #247 address the issue you're raising?

@toretore
Copy link

toretore commented Jun 6, 2024

@simonmarty Yes, most likely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set default toleration value to avoid error connecting to provider "aws"
6 participants