Set download retries and log download exceptions for installer failures by dominikschubert · Pull Request #7600 · localstack/localstack · GitHub
Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

Set download retries and log download exceptions for installer failures#7600

Merged
dominikschubert merged 3 commits into
masterfrom
installer-download-logging-and-retries
Feb 3, 2023
Merged

Set download retries and log download exceptions for installer failures#7600
dominikschubert merged 3 commits into
masterfrom
installer-download-logging-and-retries

Conversation

@dominikschubert

Copy link
Copy Markdown
Member

Motivation

PR was motivated by failing downloads of ffmpeg where the Installer tries to extract the empty installer file instead of just re-trying to download the archive first. It also didn't log any meaningful exceptions in this case since it wasn't a requests.exceptions.ReadTimeout

Changes

  • Downloads are retried up to 3 times
  • Exception + retry attempt number are logged in case of a failure
  • Extraction is not attempted on an empty "archive" file.

@dominikschubert dominikschubert self-assigned this Feb 2, 2023
@dominikschubert dominikschubert temporarily deployed to localstack-ext-tests February 2, 2023 10:02 — with GitHub Actions Inactive
@dominikschubert dominikschubert temporarily deployed to localstack-ext-tests February 2, 2023 10:04 — with GitHub Actions Inactive
@github-actions

github-actions Bot commented Feb 2, 2023

Copy link
Copy Markdown

@dominikschubert dominikschubert temporarily deployed to localstack-ext-tests February 2, 2023 12:56 — with GitHub Actions Inactive
@dominikschubert dominikschubert marked this pull request as ready for review February 2, 2023 15:30

@alexrashed alexrashed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot for tackling one of the causes of the pipeline instability! 🚀
I only have a small nitpick, nothing blocking a merge.

Comment thread localstack/utils/archives.py Outdated

@baermat baermat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mostly LGTM 👍 I agree with what @alexrashed said, + one small thing that I wanted to clarify (but is not blocking in any way)

Comment on lines +208 to +209

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: can this actually ever be a negative value? The docs state that in case of inaccessibility, OSError is returned.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TBH, just copied this from L185.

Like you said, probably not, but I guess we still wouldn't want it if for some reason this would happen on some esoteric system 🤷‍♂️

@dominikschubert dominikschubert temporarily deployed to localstack-ext-tests February 3, 2023 10:44 — with GitHub Actions Inactive
@dominikschubert dominikschubert merged commit 0e9fa81 into master Feb 3, 2023
@dominikschubert dominikschubert deleted the installer-download-logging-and-retries branch February 3, 2023 12:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants