RedHatInsights / insights-core

Insights Core is a data collection and processing framework used by Red Hat Insights
https://cloud.redhat.com/insights
Apache License 2.0
153 stars 183 forks source link

Catch Timeout on test urls #4248

Closed Glutexo closed 1 month ago

Glutexo commented 1 month ago

All Pull Requests:

Check all that apply:

Complete Description of Additions/Changes:

The internal methods used by the --test-connection command only used to catch ConnectionError, including ConnectionTimeout, but not other timeouts such as ReadTimeoutError. As a result, in case of a read timeout,  the failure is not fully handled. That means:

Re-used the existing REQUEST_FAILED_EXCEPTIONS list to process all timeouts in the same way.

Card IDs:

codecov-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.10%. Comparing base (416ed6b) to head (bafcb3d).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4248 +/- ## ========================================== + Coverage 77.01% 77.10% +0.09% ========================================== Files 761 761 Lines 41471 41471 Branches 8763 8763 ========================================== + Hits 31937 31976 +39 + Misses 8481 8438 -43 - Partials 1053 1057 +4 ``` | [Flag](https://app.codecov.io/gh/RedHatInsights/insights-core/pull/4248/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RedHatInsights) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/RedHatInsights/insights-core/pull/4248/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RedHatInsights) | `77.09% <100.00%> (+0.09%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RedHatInsights#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Glutexo commented 1 month ago

I extracted this fix from https://github.com/RedHatInsights/insights-core/pull/4237 and originally from https://github.com/RedHatInsights/insights-core/pull/4211. https://github.com/RedHatInsights/insights-core/pull/4211 only changes exception handling in the outer test_connection method, because that is the minimal patch to fix the reported issue of a stack trace being dumped on the standard output. This new PR, on the other hand, fixes error logging and fallback mechanism in case of a read timeout.

Moreover, this would unblock https://github.com/RedHatInsights/insights-core/pull/4246 in case of https://github.com/RedHatInsights/insights-core/pull/4211 being merged first of the two. See my comment in https://github.com/RedHatInsights/insights-core/pull/4246 for more details.

Glutexo commented 1 month ago

This patch only updates the inner _test_urls and _legacy_test_urls methods, not the outer test_connection method. It doesn’t even change any printing or logging logic. Therefore, unlike https://github.com/RedHatInsights/insights-core/pull/4211 and https://github.com/RedHatInsights/insights-core/pull/4246, there are no tests of test_connection or integration tests.

The error log and standard out tests are the only way how to find out whether the except block fired as intended. That’s why there are log/stdout tests in place. They, however, don’t test the contents of the messages as that’s not the subject of this pull request. That will change after merging with the tests from other related PRs.

Glutexo commented 1 month ago

Open for review. @m-horky

Glutexo commented 1 month ago

To make things simpler, let‘s say this is a blocker of https://github.com/RedHatInsights/insights-core/pull/4246, which is a blocker of https://github.com/RedHatInsights/insights-core/pull/4211. Merging this first will make further reviewing and merging easier.

Glutexo commented 1 month ago

There is also https://github.com/RedHatInsights/insights-core/pull/4254 in the family. It neither blocks anything nor is blocked by anything. It only shares some tests.

m-horky commented 1 month ago

Hi, @xiangce, good to merge!