Add extra error handling to S3 remote logging (#9118)

[JPonte]

#9118

Make sure to mark the boxes below before creating PR: [x]

• Description above provides context of the change
• Unit tests coverage for changes (not needed for documentation changes)
• Target Github ISSUE in description if exists
• Commits follow "How to write a good git commit message"
• Relevant documentation is updated including usage instructions.
• I will engage committers as explained in Contribution Workflow Example.

---

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.
Read the Pull Request Guidelines for more information.

[dimon222]

This works very well!

[RosterIn]

@feluelle can you check?

[feluelle]

LGTM 👍

[ashb]

Can you give me an example of what the log view now shows with an error?

[JPonte]

Depending on the type of the error it might show:
*** Failed to verify remote log exists if the HEAD to the s3 object fails along with the error.
If the HEAD returns 404 (meaning that the log file doesn't exist in s3) it shows:
*** Falling back to local log.
And if the previous log to append fails to be read, it gets skipped.

[ashb]

Oh, so it doesn't include any of the error message from S3?

[JPonte]

It shows the error from the HEAD:
'*** Failed to verify remote log exists {}.\n{}\n'.format(remote_loc, str(error))
Shows the supposed location of the remote log and the error.

[ashb]

Could you add a new test along the lines of test_read_when_s3_log_missing but when an error is returned.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ashb]

Sorry, missed the rereview request.

Lgtm

[github-actions]

The PR should be OK to be merged with just subset of tests as it does not modify Core of Airflow. The committers might merge it or can add a label 'full tests needed' and re-run it to run all tests if they see it is needed!

[feluelle]

Sorry @JPonte but there are some conflicting files. Can you fix this? When you've done this I am merging it.

[JPonte]

@feluelle done

[github-actions]

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

[feluelle]

@JPonte there is an error with the Static checks

-            msg = 'Could not read logs from {} with error: {}'.format(remote_log_location, error)
+            msg = f'Could not read logs from {remote_log_location} with error: {error}'

We force to use f-strings

[JPonte]

@feluelle Ah, my bad. Fixed again.

[github-actions]

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

[ashb]

Tests failing because of trailing spaces in strings