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

fix: Do NOT require list permissions on a single set of tags #228

Merged
merged 2 commits into from
Oct 29, 2020

Conversation

ivanilves
Copy link
Owner

closes #222

@ivanilves ivanilves force-pushed the ISSUE-222 branch 2 times, most recently from b159e10 to b4f82b2 Compare October 27, 2020 12:18
@coveralls
Copy link

coveralls commented Oct 27, 2020

Coverage Status

Coverage increased (+0.009%) to 91.379% when pulling 3872f9b on ISSUE-222 into 0c09910 on master.

@@ -172,8 +172,6 @@ func decodeTagData(body io.ReadCloser) ([]string, map[string]manifest.Manifest,
return nil, nil, err
}

tagManifests = manifest.MapByTag(tagManifests)
Copy link
Owner Author

Choose a reason for hiding this comment

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

this was a useless line 😆

@@ -263,6 +263,7 @@ func ParseRef(ref string) (*Repository, error) {
refParts := strings.Split(fullRef, "=")
fullRepo = refParts[0]
repoTags = strings.Split(refParts[1], ",")
isSingle = true
Copy link
Owner Author

Choose a reason for hiding this comment

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

a set of single tags should be also classified as "single"

tagManifests := make(map[string]manifest.Manifest)

for _, tagName := range repoTags {
tagManifests[tagName] = manifest.Manifest{}
Copy link
Owner Author

@ivanilves ivanilves Oct 27, 2020

Choose a reason for hiding this comment

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

This would be filled later... GCR allows you to get some extra information at this step, but it's available only on list operation which we will not perform if we are looking for the set of single tags only...

@cablespaghetti
Copy link
Collaborator

Looks cool! I'll try and build this and try it out :D

@cablespaghetti
Copy link
Collaborator

This is SO MUCH FASTER 😁

@cablespaghetti
Copy link
Collaborator

I may have spoken too soon. I think this behaves differently with a config file maybe? If I run a single instance locally it's lightning fast but when run in my environment I get this still:

image

@cablespaghetti cablespaghetti self-requested a review October 27, 2020 15:17
@ivanilves
Copy link
Owner Author

First, thank you for the swift review @cablespaghetti 🙇

I may have spoken too soon. I think this behaves differently with a config file maybe? If I run a single instance locally it's lightning fast but when run in my environment I get this still:

I think this may be related to quay.io API rate limits, the infamous yet frequent error 429.

Original issue cases were error 401 because of list requests instead of "blind" get ones 🙂

Could you please add concurrency limits for quay:

  • -c 1 - to disable concurrency (this may result in considerable slow down, just asking to test to see if it works);
  • ... or -w 50ms to throttle requests while keeping concurrency enabled (curious, how it will slow down and will it work for you);

  -c, --concurrent-requests=  Limit of concurrent requests to the registry (default: 16) [$CONCURRENT_REQUESTS]
  -w, --wait-between=         Time to wait between batches of requests (incl. pulls and pushes) (default: 0) [$WAIT_BETWEEN]

P.S.:
quay.io is pretty sensible/touchy on API rate limiting, I even think to change default concurrency setting for quay.io requests...

@vonrabbe
Copy link
Collaborator

I agree @ivanilves this "429 Too Many Requests" is a separate issue 👍

@vonrabbe vonrabbe merged commit cc3b49a into master Oct 29, 2020
@vonrabbe vonrabbe deleted the ISSUE-222 branch October 29, 2020 18:58
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.

Requires list permissions on source repository
4 participants