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

Metadata: Retry dockerhub fetch on metadata post publish fail #32626

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented Nov 17, 2023

Problem

The dockerhub api can be stale and not return an image even though its published

From this slack discussion
https://airbytehq-team.slack.com/archives/C03VDJ4FMJB/p1700182656932749

Solution

Metadata validation will retry up to 3 times 30 seconds apart to find the image

Future

We may be able to rework this to work off the local instance of docker which downloads these images much faster

@bnchrch bnchrch requested a review from a team November 17, 2023 01:27
Copy link

vercel bot commented Nov 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Nov 24, 2023 0:50am

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Nice improvement 👍
I'd appreciate:

  • Not swallowing DockerHub API errors by being more specific about the status code we handle (404)
    *A unit test of is_image_on_dockerhubto prevent future regression

# Allow for retries as the DockerHub API is not always reliable with returning the latest publish.
for _ in range(retries + 1):
response = requests.get(tag_url, headers=headers)
if response.ok:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we retry on 404 only and raise if the response status is not 200 or 404? The current implementation will swallow DockerHub API errors and end up wit the "Image does not exists" error message, even if the failure is not related to the existence/absence of the image.

try:
  response = requests.get(tag_url, headers=headers)
  response.raise_for_status()
  if response.ok:
    break
except requests.HTTPError as e:
  if e.status_code == 404:
    continue
  raise e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im worried that dockerhub during intermittent issues is not throwing just a 404. But could also throw a 500 or a 401.

Is that a valid fear?

also wdyt about the retry then an the end if still failing raising an error with the error message so we dont swallow it?

@bnchrch bnchrch merged commit e39aec4 into master Nov 29, 2023
19 checks passed
@bnchrch bnchrch deleted the bnchrch/metadata/add-docker-hub-retry branch November 29, 2023 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants