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 2022/12/29 14:33:38 UTC
[GitHub] [airflow] snjypl opened a new pull request, #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
snjypl opened a new pull request, #28638:
URL: https://github.com/apache/airflow/pull/28638
For `LocalKubernetesExecutor` we are not able to fetch the logs for local tasks.
the scheduler is not starting the `serve_logs` process.
https://github.com/apache/airflow/blob/94b3b897e2f94902777c4b24fb10c915279d8967/airflow/cli/commands/scheduler_command.py#L83-L87
we need to set `is_local` to `true` for `LocalKubernetesExecutor`
https://github.com/apache/airflow/blob/94b3b897e2f94902777c4b24fb10c915279d8967/airflow/executors/local_kubernetes_executor.py#L46
also in the helm chart, when the executor is `LocalKubernetesExecutor` we need to create the `scheduler service` so that the tasks logs can be fetched.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065031567
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
i think all Celery* executors need to serve logs, so i'd assume (without having looked) that this one needs to be true (unless something weird happens and it ends up serving anyway because of its child executor)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
snjypl commented on PR #28638:
URL: https://github.com/apache/airflow/pull/28638#issuecomment-1367544158
@potiuk i noticed that `airflow-worker` service account is not getting created for `LocalKubernetesExecutor`.
shall i create a new PR with all the helm chart related changes? or it can be part of this PR?
```
kubernetes.client.exceptions.ApiException: (403)
Reason: Forbidden
HTTP response headers: HTTPHeaderDict({'Audit-Id': '6000d700-1b7b-413b-a49f-3eba7114bbf7', 'Cache-Control': 'no-cache, private', 'Content-Type': 'application/json', 'X-Kubernetes-Pf-Flowschema-Uid': 'e6bfeec0-ef68-4420-88d8-b7024eed9fea', 'X-Kubernetes-Pf-Prioritylevel-Uid': '0e598fd5-a817-46f7-be85-6ac94aeb7872', 'Date': 'Thu, 29 Dec 2022 18:57:06 GMT', 'Content-Length': '400'})
HTTP response body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pods \"example-local-kubernetes-executor-task-with-kubernetes-c5u03pq1\" is forbidden: error looking up service account airflow/airflow-worker: serviceaccount \"airflow-worker\" not found","reason":"Forbidden","details":{"name":"example-local-kubernetes-executor-task-with-kubernetes-c5u03pq1","kind":"pods"},"code":403}
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #28638:
URL: https://github.com/apache/airflow/pull/28638#issuecomment-1371561421
Hey folks!
I definitely agree with splitting out `is_local` into more fine grained compatibility flags/properties ([see here](https://github.com/apache/airflow/issues/28276#issuecomment-1371553409) for more context). Also expect an AIP soon which will deliver native support for hybrid executors (of any combination) rather than these handbuilt classes which combine two explicit executors :rocket:
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #28638:
URL: https://github.com/apache/airflow/pull/28638#issuecomment-1384527196
Anyone have some time to give this a second review/approval? Would be nice to get this merged for @snjypl
Maybe @potiuk, @dstandish or @pierrejeambrun?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065185560
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
Oh, durr
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
snjypl commented on PR #28638:
URL: https://github.com/apache/airflow/pull/28638#issuecomment-1376452346
> @potiuk i noticed that `airflow-worker` service account is not getting created for `LocalKubernetesExecutor`.
>
> shall i create a new PR with all the helm chart related changes? or it can be part of this PR?
>
> ```
> kubernetes.client.exceptions.ApiException: (403)
> Reason: Forbidden
> HTTP response headers: HTTPHeaderDict({'Audit-Id': '6000d700-1b7b-413b-a49f-3eba7114bbf7', 'Cache-Control': 'no-cache, private', 'Content-Type': 'application/json', 'X-Kubernetes-Pf-Flowschema-Uid': 'e6bfeec0-ef68-4420-88d8-b7024eed9fea', 'X-Kubernetes-Pf-Prioritylevel-Uid': '0e598fd5-a817-46f7-be85-6ac94aeb7872', 'Date': 'Thu, 29 Dec 2022 18:57:06 GMT', 'Content-Length': '400'})
> HTTP response body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pods \"example-local-kubernetes-executor-task-with-kubernetes-c5u03pq1\" is forbidden: error looking up service account airflow/airflow-worker: serviceaccount \"airflow-worker\" not found","reason":"Forbidden","details":{"name":"example-local-kubernetes-executor-task-with-kubernetes-c5u03pq1","kind":"pods"},"code":403}
> [2022-12-29T18:57:06.871+0000] {kubernetes_executor.py:686} WARNING - ApiException when attempting to run task, re-queueing. Reason: 'Forbidden'. Message: pods "example-local-kubernetes-executor-task-with-kubernetes-c5u03pq1" is forbidden: error looking up service account airflow/airflow-worker: serviceaccount "airflow-worker" not found
> ```
@Chen-Oliver has opened a PR for this particular issue: https://github.com/apache/airflow/pull/28813
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
snjypl commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065667221
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065032207
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
i think maybe only k8s executor doesn't log serve... we read the pod logs instead (while task running)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
snjypl commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065667221
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
@o-nikolas i have made the changes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1059097979
##########
chart/templates/scheduler/scheduler-service.yaml:
##########
@@ -18,7 +18,7 @@
################################
## Airflow Scheduler Service
#################################
-{{- if eq .Values.executor "LocalExecutor" }}
+{{- if or (eq .Values.executor "LocalExecutor") (eq .Values.executor "LocalKubernetesExecutor")}}
Review Comment:
I think if we want to make our Chart fully compatible with AIP-51 and allow "any" executors we should also implement some way to override Helm Chart behaviour for custom executors. But we can do it later. CC: @o-nikolas
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
snjypl commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065175667
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
for `celery executor` the logs are served by the `workers`.
https://github.com/apache/airflow/blob/3ececb2c79307bd943aad116d7b0b5933af8185a/airflow/cli/commands/celery_command.py#L88-L96
this flag check if the logs need to be served by the `scheduler`.
should we rename this flag to `scheduler_serve_logs`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
snjypl commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065176714
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
> You'll need to add a default for the `CeleryKubernetesExecutor` as well (with False being the value I reckon), since that class does not inherit (directly) from BaseExecutor (see [this comment](https://github.com/apache/airflow/issues/28276#issuecomment-1344899475) for more details).
should we do `hasattr(executor.serve_logs)` to be sure?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065202396
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
> > You'll need to add a default for the `CeleryKubernetesExecutor` as well (with False being the value I reckon), since that class does not inherit (directly) from BaseExecutor (see [this comment](https://github.com/apache/airflow/issues/28276#issuecomment-1344899475) for more details).
>
> should we do `hasattr(executor.serve_logs)` to be sure?
@snjypl
I'd prefer to have a default added to the CeleryKubernetesExecutor so that code authors in the future don't need to remember/know that access to that flag needs to be protected with hasattr, but if you'd like to do both, then I'm fine with that as well!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28638:
URL: https://github.com/apache/airflow/pull/28638#issuecomment-1367470058
How about we create flag `serve_logs` and add this to base executor and use _this_ instead of `is_local` in all cases?
CC @potiuk @o-nikolas
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
snjypl commented on PR #28638:
URL: https://github.com/apache/airflow/pull/28638#issuecomment-1384637725
> Could use a rebase before merging, it's 20+ commits behind master
Done.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1065026422
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -48,6 +48,8 @@ class LocalKubernetesExecutor(LoggingMixin):
is_local: bool = False
+ serve_logs: bool = True
Review Comment:
You'll need to add a default for the `CeleryKubernetesExecutor` as well (with False being the value I reckon), since that class does not inherit (directly) from BaseExecutor (see [this comment](https://github.com/apache/airflow/issues/28276#issuecomment-1344899475) for more details).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
snjypl commented on PR #28638:
URL: https://github.com/apache/airflow/pull/28638#issuecomment-1374243272
@dstandish i have added `serve_logs` flag
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #28638:
URL: https://github.com/apache/airflow/pull/28638#issuecomment-1384545009
Could use a rebase before merging, it's 20+ commits behind master
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun merged pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun merged PR #28638:
URL: https://github.com/apache/airflow/pull/28638
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #28638: Fix for LocalKubernetesExecutor scheduler is not serving logs
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #28638:
URL: https://github.com/apache/airflow/pull/28638#discussion_r1059098844
##########
chart/templates/scheduler/scheduler-service.yaml:
##########
@@ -18,7 +18,7 @@
################################
## Airflow Scheduler Service
#################################
-{{- if eq .Values.executor "LocalExecutor" }}
+{{- if or (eq .Values.executor "LocalExecutor") (eq .Values.executor "LocalKubernetesExecutor")}}
Review Comment:
Ah.. I see @dstandish already had some comments on that one too :) Yeah - serve_logs should be fine and it should have default value depending on the "built-in" executors - so that we do not have to configure it manually for built-in executors.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org