You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "o-nikolas (via GitHub)" <gi...@apache.org> on 2023/10/12 21:16:53 UTC
[PR] Introduce a generic export for containerized executor logging [airflow]
o-nikolas opened a new pull request, #34903:
URL: https://github.com/apache/airflow/pull/34903
### Context
Pulling this change out of #34381 into a separate PR as requested [here](https://github.com/apache/airflow/pull/34381#discussion_r1327980137) by @eladkal
### Overview
A new logging environment variable to enable task logs to work correctly in containerized executors. K8s has an approach for this, which is followed closely when making it generic in this change. However, I did not convert K8s executor te this new mechanism to keep the change set minimal and reduce blast radius. A follow-up change can be done for this.
### Testing
- unit test updated and passing as expected
- UAT testing done with the ECS Executor as part of #34381
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
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/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ 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
Re: [PR] Introduce a generic export for containerized executor logging [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34903:
URL: https://github.com/apache/airflow/pull/34903#discussion_r1357747434
##########
airflow/cli/commands/task_command.py:
##########
@@ -326,7 +326,7 @@ def _move_task_handlers_to_root(ti: TaskInstance) -> Generator[None, None, None]
console_handler = next((h for h in root_logger.handlers if h.name == "console"), None)
with LoggerMutationHelper(root_logger), LoggerMutationHelper(ti.log) as task_helper:
task_helper.move(root_logger)
- if IS_K8S_EXECUTOR_POD:
+ if IS_K8S_EXECUTOR_POD or IS_EXECUTOR_CONTAINER:
Review Comment:
From another direction, it seems that `IS_EXECUTOR_CONTAINER` is never used on its own, which makes me feel perhaps only the `IS_CONTAINERIZED` constant is needed, and `IS_EXECUTOR_CONTAINER` can be removed entirely. The same applies for the test parameters.
--
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
Re: [PR] Introduce a generic export for containerized executor logging [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #34903:
URL: https://github.com/apache/airflow/pull/34903#issuecomment-1766946879
This has a couple approvals now after clarification in the comment thread. Merging this one and will follow up with the K8s changes in a separate PR (the ECS executor already uses the new generic config)
--
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
Re: [PR] Introduce a generic export for containerized executor logging [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #34903:
URL: https://github.com/apache/airflow/pull/34903#discussion_r1360963519
##########
airflow/cli/commands/task_command.py:
##########
@@ -326,7 +326,7 @@ def _move_task_handlers_to_root(ti: TaskInstance) -> Generator[None, None, None]
console_handler = next((h for h in root_logger.handlers if h.name == "console"), None)
with LoggerMutationHelper(root_logger), LoggerMutationHelper(ti.log) as task_helper:
task_helper.move(root_logger)
- if IS_K8S_EXECUTOR_POD:
+ if IS_K8S_EXECUTOR_POD or IS_EXECUTOR_CONTAINER:
Review Comment:
Sorry folks (@ferruzzi @uranusjr), I thought I put enough details in the description but perhaps I should have added more!
This new constant will be a generalized replacement for the K8s constant we have now. Any new containerized executor should use this one. Which our new ECS executor does use (link in description). I did not remove the K8s environment variable in this PR to keep the scope small. This change is _just_ the core pieces. The K8s executor will be migrated over to this env var in a follow up 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce a generic export for containerized executor logging [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #34903:
URL: https://github.com/apache/airflow/pull/34903#discussion_r1357507527
##########
airflow/cli/commands/task_command.py:
##########
@@ -326,7 +326,7 @@ def _move_task_handlers_to_root(ti: TaskInstance) -> Generator[None, None, None]
console_handler = next((h for h in root_logger.handlers if h.name == "console"), None)
with LoggerMutationHelper(root_logger), LoggerMutationHelper(ti.log) as task_helper:
task_helper.move(root_logger)
- if IS_K8S_EXECUTOR_POD:
+ if IS_K8S_EXECUTOR_POD or IS_EXECUTOR_CONTAINER:
Review Comment:
Nittiest of nitpicks, non-blocking thought: in two places you use `if IS_K8S_EXECUTOR_POD or IS_EXECUTOR_CONTAINER:`, I wonder if a calculated constant somewhere like `IS_CONTAINERIZED = any(IS_K8S_EXECUTOR_POD, IS_EXECUTOR_CONTAINER)`... but that's likely pointless overkill right now, but if/when other providers add their own containerized executors, that may be needed...
--
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
Re: [PR] Introduce a generic export for containerized executor logging [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas merged PR #34903:
URL: https://github.com/apache/airflow/pull/34903
--
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