You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/07 03:55:07 UTC
[GitHub] [airflow] dstandish opened a new pull request #13526: Make helm chart commands compatible with 1.10.14 image
dstandish opened a new pull request #13526:
URL: https://github.com/apache/airflow/pull/13526
["airflow", "webserver"] does not work with apache/airflow:1.10.14
["webserver"] works, as does ["bash", "-c", ...] as done in this PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang edited a comment on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
xinbinhuang edited a comment on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757535276
> @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?
Now I am taking a second look at it. Actually, I don't think this PR is doing anything, 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757363328
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#discussion_r553253450
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -94,7 +94,7 @@ spec:
- name: webserver
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
- args: ["airflow", "webserver"]
+ args: ["bash", "-c", "airflow webserver"]
Review comment:
```suggestion
args: ["bash", "-c", "exec airflow webserver"]
```
##########
File path: chart/templates/scheduler/scheduler-deployment.yaml
##########
@@ -107,7 +107,7 @@ spec:
- name: scheduler
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
- args: ["airflow", "scheduler"]
+ args: ["bash", "-c", "airflow scheduler"]
Review comment:
```suggestion
args: ["bash", "-c", "exec airflow scheduler"]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-773808291
@ashb i think i resolved your concerns
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757363328
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757535276
> @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?
Now I am taking a second look at it. Actually, I don't think this PR is doing anything, because `["airflow", "scheduler"]` is essentially the same as `["bash", "-c", "exec airflow scheduler"]`, which is preferred.
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #13526:
URL: https://github.com/apache/airflow/pull/13526
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757540337
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-773808291
@ashb i think i resolved your concerns
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757377728
@xinbinhuang i am testing this with 1.10.14 and seeing bad termination behavior of celery workers. documented here: https://github.com/apache/airflow/issues/13591
perhaps you have some insight?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757377728
@xinbinhuang i am testing this with 1.10.14 and seeing bad termination behavior of celery workers. documented here: https://github.com/apache/airflow/issues/13591
perhaps you have some insight?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757544097
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang edited a comment on pull request #13526: Make helm chart commands compatible with 1.10.14 image
Posted by GitBox <gi...@apache.org>.
xinbinhuang edited a comment on pull request #13526:
URL: https://github.com/apache/airflow/pull/13526#issuecomment-757535276
> @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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org