Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Propagate logs to stdout when in k8s executor pod #28440
Propagate logs to stdout when in k8s executor pod #28440
Changes from 41 commits
febf15f
170402e
f867323
c0301cd
3947617
cba5be4
b744c41
1b4e967
54124d2
beaf6df
71f270b
937535c
3e7baa8
9face81
8602005
ac3bfd4
9817191
b980c01
6c0dbe0
095a509
6aa0dfc
0f54b60
59bdf38
a032e2c
650220f
65ebef6
52ce222
c0ceed0
9adb850
dd0f252
0adb2b3
590b3f5
486d5b5
4f8aeea
67bc6a7
7e148b5
1a9db65
6b8b153
6280275
776b819
0d4a208
777dd4e
1c1f1c9
49228a2
1695b44
da74320
b7e8d7c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@malthe observe, we already copy (temporarily) to root logger the thandlers from airflow.task
This causes problems because we have to have complicated propagation rules at the airflow.task logger because we leave it there.
While my solution here is a few more lines (and a lot more comments) it's not very complicated. There are two parts.
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 didn't quite understand what "stdout redirection" means – is that within Python or is that something happening outside of Python (in a shell script perhaps) – ?
When I think about this problem, it seems that there is an orthogonal concern in the logging setup of Airflow which is whether or not the task logger (during execution) should be emitted to the stdout stream.
That's something you could want in any situation, not just K8S.
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 am not sure what comment you refer to but, what this means is there are stdlib helpers to take things that would ordinarily go to stdout (e.g. print) and send them to some other stream.
see
from contextlib import redirect_stdout
. it just monkey patchessys.stdout
temporarily.we use them (for better or worse) to redirect stdout to task logs.
our log read logic when task is running is usually, read from flask log server. most task logging is redirected to log (and therefore file) and our "console" handler respects this redirection. for obvious reasons -- the celery worker log, or local executor log would get unreasonably chatty. but in k8s executor context, there is no log server on the worker, and there is no problem with keeping the stdout, and importantly our log read logic assumes everything will be forwarded to stdout -- and this is what's broken that i am here fixing.
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.
Is this related to this logging PR in anyway?
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 don't think this will fail if we remove this change. you could call it a drive by and i can remove if you like.
task runner tests failed with this change. then i was like "oh why is it just throwing away whole log config, let's try and not do that". then after leaving the FTH intact for that test, that i discovered there was an extraneous session being created in created in _render_filename. as part of troubleshooting the failures and discovering that though, also saw that this reconfigure is happening earlier than it probably ought to. should be done as late as possible i think.
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.
Why is this parsed within the session context now?
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.
if you look a few lines below, we need to keep
context = ti.get_template_context(session=session)
in the same session. otherwise it creates a new session (provided by provide_session), and this can cause problems post-os.fork. this is why the test for standard task runner previously blew away the entire log config because. with this, we don't have to do that anymore.