Skip to content

fix(chart): webserver probes timeout and period. - #30609

Merged
potiuk merged 6 commits into
apache:mainfrom
mikaeld:fix/helm/webserver-probe-config
Apr 14, 2023
Merged

fix(chart): webserver probes timeout and period.#30609
potiuk merged 6 commits into
apache:mainfrom
mikaeld:fix/helm/webserver-probe-config

Conversation

@mikaeld

@mikaeld mikaeld commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

Description

This PR fixes the default probes timeout and period values. Generally, timeoutSeconds should be smaller than periodSeconds. Here are a SO question addressing this issue and a blog post detailing the Liveness and Readiness Probes. Furthermore, some specific k8s implementations such as GKE do not allow timeoutSeconds > periodSeconds.

The timeout is the amount of time that Google Cloud waits for a response to a probe. Its value must be less than or equal to the check interval. Units are seconds.

Finally, the webserver probes are the only ones with default values where timeoutSeconds > periodSeconds.

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg Bot added the area:helm-chart Airflow Helm Chart label Apr 12, 2023

@hussein-awala hussein-awala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you update the default values in the webserver deployment too?

initialDelaySeconds: {{ .Values.webserver.livenessProbe.initialDelaySeconds | default 15 }}
timeoutSeconds: {{ .Values.webserver.livenessProbe.timeoutSeconds | default 30 }}
failureThreshold: {{ .Values.webserver.livenessProbe.failureThreshold | default 20 }}
periodSeconds: {{ .Values.webserver.livenessProbe.periodSeconds | default 5 }}

and
initialDelaySeconds: {{ .Values.webserver.readinessProbe.initialDelaySeconds | default 15 }}
timeoutSeconds: {{ .Values.webserver.readinessProbe.timeoutSeconds | default 30 }}
failureThreshold: {{ .Values.webserver.readinessProbe.failureThreshold | default 20 }}
periodSeconds: {{ .Values.webserver.readinessProbe.periodSeconds | default 5 }}

@jedcunningham

Copy link
Copy Markdown
Member

Just remove them instead. They'll never get used.

@hussein-awala hussein-awala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep. Looks like those defaults are much more reasonable @jedcunningham @ephraimbuddy @dstandish -> do you recal (or maybe have some Astronomer setup for the chart) to know where the original settings were derived from ? Maybe there are good reasons the original settings were like that?

@jedcunningham

Copy link
Copy Markdown
Member

I don't see anything interesting in the history, to be honest. These seem reasonable to me too.

@potiuk
potiuk merged commit 8413c0e into apache:main Apr 14, 2023
@mikaeld
mikaeld deleted the fix/helm/webserver-probe-config branch April 14, 2023 16:07
wookiist pushed a commit to wookiist/airflow that referenced this pull request Apr 19, 2023
* fix(chart): webserver probes timeout and period

* Update default values in JSON schema to reflect values.yaml

* remove defautl templated values
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants