Fixes Docker xcom functionality #21175
Conversation
|
@uranusjr - You were looking at someone else's attempt at this, which has been sitting for a while. (link in the description) |
Please see: apache#19027 Co-Author: [email protected]
|
Rebased and corrected the co-author in the commit message. |
|
See #19027 (comment). I still think having a separate API call is safer. |
| line = line.strip() | ||
| res_lines.append(line) | ||
| self.log.info(line) | ||
| log_chunk = log_chunk.decode('utf-8') |
There was a problem hiding this comment.
We probably should have error="surrogateescape" here (something like this, I don’t remember the exact name) because there’s no guaratee the log is outputting valid strings, and those characters shouldn’t crash the entire job.
There was a problem hiding this comment.
I haven't run into that before. I'll read up on it and get the change in on Monday.
There was a problem hiding this comment.
Added in 12df34c
Just making sure I understand, adding the surrogateescape here allows our decode to pick up and fix anything that was escaped in the initial encode, assuming they used the same surrogateescape there? Is that right?
Co-authored-by: Tzu-ping Chung <[email protected]>
Isn't this what you were asking for with that discussion? https://github.com/apache/airflow/pull/21175/files#diff-5ec82e1fcaaa38e04471f56b64ec319f67b146d3a18d8bdb69c2f973ea3dd585R306-R311 |
|
Ah right, I misread, sorry. |
| log_lines = "\n".join(log_lines) | ||
| raise AirflowException(f'Docker container failed: {repr(result)} lines {log_lines}') |
There was a problem hiding this comment.
| log_lines = "\n".join(log_lines) | |
| raise AirflowException(f'Docker container failed: {repr(result)} lines {log_lines}') | |
| joined_log_lines = "\n".join(log_lines) | |
| raise AirflowException(f'Docker container failed: {repr(result)} lines {joined_log_lines}') |
to satisfy Mypy.
There was a problem hiding this comment.
Thanks. For the future, what didn't it like there? Reusing the same variable name?
There was a problem hiding this comment.
Yeah from previous lines Mypy sets the type of log_lines to List[str], so assigning str to it raises an error.
Co-authored-by: Tzu-ping Chung <[email protected]>
github-actions
bot
added
the
okay to merge
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
The fix from apache#21175 did not actually fix the logging behaviour with non-ascii characters returned by docker logs when xcom push was enabled. The problem is that DockerOperator uses different methods to stream the logs as they come (using attach stream) and different method to retrieve the logs to actually return the Xcom value. The latter uses "logs" method of docker client. The tests have not caught it, because the two methods were mocked in two different places. This PR uses the same "stringify()" function to convert both "logged" logs and those that are pushed as xcom. Also added test for "no lines returned" case. Fixes: apache#19027
* Fix docker behaviour with byte lines returned The fix from #21175 did not actually fix the logging behaviour with non-ascii characters returned by docker logs when xcom push was enabled. The problem is that DockerOperator uses different methods to stream the logs as they come (using attach stream) and different method to retrieve the logs to actually return the Xcom value. The latter uses "logs" method of docker client. The tests have not caught it, because the two methods were mocked in two different places. This PR uses the same "stringify()" function to convert both "logged" logs and those that are pushed as xcom. Also added test for "no lines returned" case. Fixes: #19027 * Update tests/providers/docker/operators/test_docker.py Co-authored-by: Jed Cunningham <[email protected]> Co-authored-by: Jed Cunningham <[email protected]>
closes: #18874
completes: #19027 (someone else has a PR in for this but it appears to be abandoned)
Co-Author: [email protected]
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.