-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fix for LocalKubernetesExecutor scheduler is not serving logs #28638
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
Fix for LocalKubernetesExecutor scheduler is not serving logs #28638
Conversation
|
How about we create flag |
| ## Airflow Scheduler Service | ||
| ################################# | ||
| {{- if eq .Values.executor "LocalExecutor" }} | ||
| {{- if or (eq .Values.executor "LocalExecutor") (eq .Values.executor "LocalKubernetesExecutor")}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
@potiuk i noticed that shall i create a new PR with all the helm chart related changes? or it can be part of this PR? |
|
Hey folks! I definitely agree with splitting out |
503faf7 to
814ef0e
Compare
|
@dstandish i have added |
814ef0e to
0524994
Compare
@Chen-Oliver has opened a PR for this particular issue: #28813 |
0524994 to
b069eb7
Compare
…github.com:snjypl/airflow into bugfix/LocalK8sExecutor-scheduler-log-serving-logs
|
Anyone have some time to give this a second review/approval? Would be nice to get this merged for @snjypl |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Could use a rebase before merging, it's 20+ commits behind main |
Done. |
For
LocalKubernetesExecutorwe are not able to fetch the logs for local tasks.the scheduler is not starting the
serve_logsprocess.airflow/airflow/cli/commands/scheduler_command.py
Lines 83 to 87 in 94b3b89
we need to set
is_localtotrueforLocalKubernetesExecutorairflow/airflow/executors/local_kubernetes_executor.py
Line 46 in 94b3b89
also in the helm chart, when the executor is
LocalKubernetesExecutorwe need to create thescheduler serviceso that the tasks logs can be fetched.^ Add meaningful description above
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.rstor{issue_number}.significant.rst, in newsfragments.