Skip to content
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

ECSOperator returns last logs when ECS task fails #17209

Merged
merged 14 commits into from
Sep 9, 2021

Conversation

pmalafosse
Copy link
Contributor

closes: #17038

This PR changes the message in the AirflowException when the ECS task launched by ECSOperator is stopped.

Before:
The message when it failed was:
This task is not in success state {<huge JSON from AWS containing all the ECS task details>}

Now:
The message is:

This task is not in success state - last logs from Cloudwatch:
<last_logs_from_cloudwatch>

which makes it much more useful to understand what failed in the underlying code directly from the alert.

The number of logs can be customized with the parameter number_logs_exception.

FYI @uranusjr

@boring-cyborg boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Jul 25, 2021
airflow/providers/amazon/aws/operators/ecs.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/operators/ecs.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/operators/ecs.py Outdated Show resolved Hide resolved
tests/providers/amazon/aws/operators/test_ecs.py Outdated Show resolved Hide resolved
tests/providers/amazon/aws/operators/test_ecs.py Outdated Show resolved Hide resolved
@uranusjr
Copy link
Member

(Forgot to comment when hitting submit: the implementation itself looks good to me, but the tests can be improved.)

@pmalafosse
Copy link
Contributor Author

Thanks for your review @uranusjr !

@pmalafosse
Copy link
Contributor Author

I added some other tests for _last_log_messages and making sure it works if there are no messages at all.

I wanted to do all of that in the same test instead of splitting it into 2 (one for _last_log_messages() and one for the exception message) but unfortunately there are issues with mocks and generators (can only be called once because then it's empty) and fixing this with https://stackoverflow.com/a/65019164/5706548 might be confusing.

@pmalafosse pmalafosse requested a review from uranusjr July 31, 2021 17:19
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Flake8 is complaining about these; logic-wise this looks good to me.

tests/providers/amazon/aws/operators/test_ecs.py Outdated Show resolved Hide resolved
tests/providers/amazon/aws/operators/test_ecs.py Outdated Show resolved Hide resolved
@uranusjr
Copy link
Member

uranusjr commented Aug 2, 2021

(I could have missed something. You should run pre-commit locally to pass the linters; see https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#contribution-workflow)

@pmalafosse
Copy link
Contributor Author

Thanks @uranusjr I just ran the flake8 tests locally now after your changes and it works.
Thanks for the link, I will see how to set up properly all those pre-commit things that will make it easier next time.

@pmalafosse pmalafosse requested a review from uranusjr August 2, 2021 09:30
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Aug 3, 2021
@github-actions
Copy link

github-actions bot commented Aug 3, 2021

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.

@kaxil kaxil merged commit e6cb2f7 into apache:main Sep 9, 2021
@pmalafosse pmalafosse deleted the pmalafosse/ecs_operator_logs_in_alert branch September 14, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECSOperator returns last logs when ECS task fails
3 participants