Introduce a generic export for containerized executor logging #34903
Conversation
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.
| @@ -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: | |||
There was a problem hiding this 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...
There was a problem hiding this 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.
There was a problem hiding this 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 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) |
Context
Pulling this change out of #34381 into a separate PR as requested here 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
^ 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.