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

Reinstate retries for image pulls #1712

Merged
merged 5 commits into from Aug 22, 2019
Merged

Reinstate retries for image pulls #1712

merged 5 commits into from Aug 22, 2019

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Aug 6, 2019

No description provided.

@rnorth rnorth mentioned this pull request Aug 6, 2019
bsideup
bsideup previously approved these changes Aug 6, 2019
@rnorth
Copy link
Member Author

rnorth commented Aug 7, 2019

@kiview if you could please review as well I'd be grateful. After @bsideup's review I'm afraid I made a couple of changes:

  • I noticed that pull failures were still occuring in CI. This really shouldn't happen, but my hypothesis is that when CI->Docker Hub network glitches occur they are brief periods of 0% availability. 3 quick retries in these periods are pointless. So instead of this I've modified the retry logic to allow as many retries as necessary up to a maximum of 2 minutes. As a result, temporary network blips should get enough time to recover but there is a sanity limit for retries.

  • I also noticed that the retries were a bit blunt - we were retrying in the case of authentication or 'not found' failures, which is clearly unhelpful. We obviously don't want to retry these for up to 2 minutes! Instead, I've changed the code to let these kind of failures fail-fast and bubble up the exception immediately - zero retries.

I hope this makes sense!

@rnorth rnorth dismissed bsideup’s stale review August 8, 2019 09:10

I've made significant changes after review

@rnorth rnorth added this to the next milestone Aug 10, 2019
Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

👍

@rnorth rnorth merged commit 62d646a into master Aug 22, 2019
@rnorth rnorth deleted the reinstate-pull-retries branch August 22, 2019 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants