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

Chart: set workers.safeToEvict default to False #40229

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

jedcunningham
Copy link
Member

This is a safer default for our workers.

This can be safe to set to true if you have a long enough workers.terminationGracePeriodSeconds set, but what is "long enough" is very situational, so I feel its better to default to not evicting worker pods.

This is a safer default for our workers.

This can be safe to set to true if you have a long enough
`workers.terminationGracePeriodSeconds` set, but what is
"long enough" is very situational, so I feel its better to
default to not evicting worker pods.
@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Jun 14, 2024
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

What is the default value for cluster-autoscaler.kubernetes.io/safe-to-evict?

@jedcunningham
Copy link
Member Author

There isn't a binary default. The short version is CA considers pods safe to evict unless the annotation is set to False, or the pod isn't backed by a controller object (e.g. deployment/statefulset). For Airflow, this means that the CE workers would be considered evictable and KE workers would not, by default, without the annotation.

There are other conditions as well, which you can read about in the CA FAQ.

@jedcunningham jedcunningham merged commit 4c9f12d into apache:main Jun 14, 2024
62 checks passed
@jedcunningham jedcunningham deleted the chart_worker_not_safe_to_evict branch June 14, 2024 18:36
jannisko pushed a commit to jannisko/airflow that referenced this pull request Jun 15, 2024
This is a safer default for our workers.

This can be safe to set to true if you have a long enough
`workers.terminationGracePeriodSeconds` set, but what is
"long enough" is very situational, so I feel its better to
default to not evicting worker pods.
@utkarsharma2 utkarsharma2 added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants