Fix/21994 liveness probe #22041

Wilderone · 2022-03-07T09:13:39Z

This MR resolve two problems:

Remove "exec" argument for sh in livenessProbe, because of this leads to false-success of probe check
Move livenessProbe's code to values, so client can redefine it.

Related issue #21994

I am not sure about version of this change, so just upgrade patch version.

boring-cyborg · 2022-03-07T09:13:43Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

potiuk · 2022-03-07T10:55:33Z

chart/Chart.yaml

@@ -19,7 +19,7 @@
 ---
 apiVersion: v2
 name: airflow
-version: 1.5.0-dev
+version: 1.5.1-dev


I tihnk this should not be changed. We are voting on 1.5.0 and my bet is that we will issue and rc2

Yeah, we don't need to touch this here.

potiuk

I like this but I need @ephraimbuddy @jedcunningham @kaxil chiming in :)

github-actions · 2022-03-07T10:57:06Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

ephraimbuddy

LGTM.

potiuk · 2022-03-07T12:59:10Z

The mypy static checks are unrelated (and already fixed in main - can you please rebase and fix the other static check (which is related :)). Pre-commit might help you with automatically fixing it

potiuk · 2022-03-07T12:59:33Z

(and removal of the version change too :))

jedcunningham

Thanks for opening this! Can you also add some tests?

jedcunningham · 2022-03-07T16:54:09Z

chart/values.yaml

+        - sh
+        - -c
+        - |
+          CONNECTION_CHECK_MAX_COUNT=0 /entrypoint python -Wignore -c "


Suggested change

CONNECTION_CHECK_MAX_COUNT=0 /entrypoint python -Wignore -c "

CONNECTION_CHECK_MAX_COUNT=0 exec /entrypoint python -Wignore -c "

This is where the exec should be, and I think we want to keep it so timeoutSeconds can actually function.

jedcunningham · 2022-03-07T16:57:56Z

chart/values.schema.json

@@ -1504,6 +1504,20 @@
                            "description": "How often (in seconds) to perform the probe. Minimum value is 1.",
                            "type": "integer",
                            "default": 60
+                        },
+                        "exec": {
+                            "description": "Custom exec command for livenessProbe",


Suggested change

"description": "Custom exec command for livenessProbe",

"description": "Command for livenessProbe",

I'm also not sure we need to maintain the nesting of exec.command. I'd rather we flatten it and just have livenessProbe.command. My 2c, thoughts?

jedcunningham · 2022-03-07T16:58:35Z

chart/Chart.yaml

@@ -19,7 +19,7 @@
 ---
 apiVersion: v2
 name: airflow
-version: 1.5.0-dev
+version: 1.5.1-dev


Yeah, we don't need to touch this here.

potiuk · 2022-03-08T11:15:08Z

chart/tests/test_scheduler.py

@@ -235,6 +236,9 @@ def test_livenessprobe_values_are_configurable(self):
            "spec.template.spec.containers[0].livenessProbe.failureThreshold", docs[0]
        )
        assert 444 == jmespath.search("spec.template.spec.containers[0].livenessProbe.periodSeconds", docs[0])
+        assert ["sh", "-c", "echo", "wow such test"] == jmespath.search(


boring-cyborg · 2022-03-08T16:06:42Z

Awesome work, congrats on your first merged pull request!

jedcunningham · 2022-03-08T16:10:42Z

Thanks @Wilderone! Congrats on your first commit 🎉

Move livenessProbe exec to values, remove 'exec' argument from sh -c

1c9bc2d

Wilderone requested review from dstandish and jedcunningham as code owners March 7, 2022 09:13

boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Mar 7, 2022

potiuk requested review from kaxil and ephraimbuddy March 7, 2022 10:54

potiuk reviewed Mar 7, 2022

View reviewed changes

potiuk approved these changes Mar 7, 2022

View reviewed changes

potiuk added this to the Airflow Helm Chart 1.5.0 milestone Mar 7, 2022

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 7, 2022

ephraimbuddy approved these changes Mar 7, 2022

View reviewed changes

jedcunningham requested changes Mar 7, 2022

View reviewed changes

maksim.vilde added 5 commits March 8, 2022 11:39

Merge branch 'main' of github.com:Wilderone/airflow

978c774

update chart version

d6ebf6e

Uncomment dependencies

f60b60f

add -dev suffix to version

e9cccd4

Add exec before /entrypoint, remove nested 'command' for livenessprobe

6c5b226

Wilderone force-pushed the fix/21994-livenessProbe branch from a752741 to 6c5b226 Compare March 8, 2022 07:41

+correct schema.json for scheduler

8bfccb7

potiuk approved these changes Mar 8, 2022

View reviewed changes

potiuk requested a review from jedcunningham March 8, 2022 10:46

add tests for livenessProbe exec command

6193ae1

potiuk reviewed Mar 8, 2022

View reviewed changes

linting

18c5b40

jedcunningham approved these changes Mar 8, 2022

View reviewed changes

ephraimbuddy mentioned this pull request Mar 8, 2022

Update changelog for helm chart 1.5.0 #22090

Merged

potiuk merged commit 99cf75d into apache:main Mar 8, 2022

Wilderone deleted the fix/21994-livenessProbe branch March 8, 2022 16:30

ephraimbuddy mentioned this pull request Mar 8, 2022

Status of testing of Apache Airflow Helm Chart 1.5.0rc2 #22096

Closed

15 tasks

jedcunningham mentioned this pull request Mar 8, 2022

scheduler: livenessProbe redundant exec leads to false-positive result #21994

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/21994 liveness probe #22041

Fix/21994 liveness probe #22041

Wilderone commented Mar 7, 2022

boring-cyborg bot commented Mar 7, 2022

potiuk Mar 7, 2022

jedcunningham Mar 7, 2022

potiuk left a comment

github-actions bot commented Mar 7, 2022

ephraimbuddy left a comment

potiuk commented Mar 7, 2022

potiuk commented Mar 7, 2022

jedcunningham left a comment

jedcunningham Mar 7, 2022

potiuk Mar 7, 2022

jedcunningham Mar 7, 2022

jedcunningham Mar 7, 2022

potiuk Mar 8, 2022

boring-cyborg bot commented Mar 8, 2022

jedcunningham commented Mar 8, 2022

	CONNECTION_CHECK_MAX_COUNT=0 /entrypoint python -Wignore -c "
	CONNECTION_CHECK_MAX_COUNT=0 exec /entrypoint python -Wignore -c "

	"description": "Custom exec command for livenessProbe",
	"description": "Command for livenessProbe",

Fix/21994 liveness probe #22041

Fix/21994 liveness probe #22041

Conversation

Wilderone commented Mar 7, 2022

boring-cyborg bot commented Mar 7, 2022

potiuk Mar 7, 2022

Choose a reason for hiding this comment

jedcunningham Mar 7, 2022

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 7, 2022

ephraimbuddy left a comment

Choose a reason for hiding this comment

potiuk commented Mar 7, 2022

potiuk commented Mar 7, 2022

jedcunningham left a comment

Choose a reason for hiding this comment

jedcunningham Mar 7, 2022

Choose a reason for hiding this comment

potiuk Mar 7, 2022

Choose a reason for hiding this comment

jedcunningham Mar 7, 2022

Choose a reason for hiding this comment

jedcunningham Mar 7, 2022

Choose a reason for hiding this comment

potiuk Mar 8, 2022

Choose a reason for hiding this comment

boring-cyborg bot commented Mar 8, 2022

jedcunningham commented Mar 8, 2022