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

Make helm chart commands compatible with 1.10.14 image #13526

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

dstandish
Copy link
Contributor

@dstandish dstandish commented Jan 7, 2021

["airflow", "webserver"] does not work with apache/airflow:1.10.14

["webserver"] works, as does ["bash", "-c", ...] as done in this PR

i tested with both 1.10.14 and 2.0.0 but no earlier versions

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Jan 7, 2021
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Using exec means we don't have a "needless" bash process hanging around.

@dstandish dstandish requested a review from ashb January 7, 2021 16:43
@dstandish dstandish force-pushed the fix-command-in-chart branch from dd5de0a to 31b520d Compare January 8, 2021 08:08
dstandish and others added 3 commits January 9, 2021 09:37
* ["airflow", "webserver"] does not work with the apache image
* ["webserver"] works, as does "bash" "-c" etc
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
@dstandish dstandish force-pushed the fix-command-in-chart branch from 31b520d to b886f8b Compare January 9, 2021 17:37
@mik-laj mik-laj requested a review from potiuk January 9, 2021 18:22
@xinbinhuang
Copy link
Contributor

Using exec means we don't have a "needless" bash process hanging around.

Just a small note: without the exec, bash will be the PID 1, and the airflow process may not exit properly upon container exit. But I think dumb-init takes care of it in our case.

@dstandish
Copy link
Contributor Author

@xinbinhuang i am testing this with 1.10.14 and seeing bad termination behavior of celery workers. documented here: #13591

perhaps you have some insight?

@xinbinhuang
Copy link
Contributor

xinbinhuang commented Jan 10, 2021

@xinbinhuang i am testing this with 1.10.14 and seeing bad termination behavior of celery workers. documented here: #13591

perhaps you have some insight?

Does ["bash", "-c", "exec airflow scheduler"] work for your case?

I have a feeling that the current PR does not do anything differently because ["airflow", "scheduler"] is essentially the same as ["bash", "-c", "exec airflow scheduler"].
As for ["bash", "-c", "airflow webserver"] is working, it's probably due to the fact that Docker does not signal the process properly which leaves it hanging, making it feels like a "graceful termination".

I will comment further on the original issue

@dstandish
Copy link
Contributor Author

have a feeling that the current PR does not do anything differently

the issue @xinbinhuang is how entrypoint handles bash vs airflow

try to run 1.10.14 with this chart and you'll see

@dstandish
Copy link
Contributor Author

and to clarify, this PR is not about worker termination -- its just about making 1.10.14 run at all

but in some testing i noticed termination issue, which doesn't seem to have been working before anyway.

@dstandish
Copy link
Contributor Author

@ashb i think i resolved your concerns

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