Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

perf: reduce auth request count for manifest delete #618

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

Wwwsylvia
Copy link
Member

@Wwwsylvia Wwwsylvia commented Oct 17, 2023

Add pull and delete scope hints before attempting client-side indexing on manifest delete.
Resolve: #594

Before this change, the requests produced during deleting a manifest when Referrers API is not known to be available:

  • Request #0: GET manifest - 401
  • Request #1: POST token (scope: pull) - 200
  • Request #2: GET manifest - 200
  • [optional] Request #3: GET referrers index - 200
  • [optional] Request #4: PUT referrers index - 401
  • [optional] Request #5: POST token (scope: pull, push) - 200
  • [optional] Request #6: PUT referrers index - 201
  • Request #7: DELETE manifest - 401
  • Request #8: POST token (scope: delete)
  • Request #9: DELETE manifest - 202

After this change, the requests should become:

  • Request #0: GET manifest - 401
  • Request #1: POST token (scope: pull, delete) - 200
  • Request #2: GET manifest - 200
  • [optional] Request #3: GET referrers index - 200
  • [optional] Request #4: PUT referrers index - 401
  • [optional] Request #5: POST token (scope: pull, push) - 200
  • [optional] Request #6: PUT referrers index - 201
  • Request #7: DELETE manifest - 202

Signed-off-by: Lixia (Sylvia) Lei <lixlei@microsoft.com>
Signed-off-by: Lixia (Sylvia) Lei <lixlei@microsoft.com>
@codecov-commenter
Copy link

Codecov Report

Merging #618 (987b48c) into main (459a246) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##             main     #618   +/-   ##
=======================================
  Coverage   75.11%   75.11%           
=======================================
  Files          59       59           
  Lines        5420     5421    +1     
=======================================
+ Hits         4071     4072    +1     
  Misses        994      994           
  Partials      355      355           
Files Coverage Δ
registry/remote/repository.go 71.21% <100.00%> (+0.02%) ⬆️

Signed-off-by: Lixia (Sylvia) Lei <lixlei@microsoft.com>
Copy link
Contributor

@qweeah qweeah left a comment

Choose a reason for hiding this comment

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

LGTM, but IANAM

@Wwwsylvia Wwwsylvia changed the title perf: reduce potential auth request count for manifest delete perf: reduce auth request count for manifest delete Oct 18, 2023
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@Wwwsylvia Wwwsylvia merged commit 0f1dc30 into oras-project:main Oct 18, 2023
@Wwwsylvia Wwwsylvia deleted the auth_perf branch October 18, 2023 08:04
Wwwsylvia added a commit to Wwwsylvia/oras-go that referenced this pull request Oct 20, 2023
Add `pull` and `delete` scope hints before attempting client-side indexing on manifest delete.

Resolve: oras-project#594 
Signed-off-by: Lixia (Sylvia) Lei <lixlei@microsoft.com>
Wwwsylvia added a commit that referenced this pull request Oct 20, 2023
Backporting 0f1dc30 to the release
branch.
This change does the same thing as the original commit (#618) did but
using the old `registryutil.WithScopeHint` instead of
`auth.AppendRepositoryScope`, which is introduced by #604 and is not
available in `v2.3.x`.

Signed-off-by: Lixia (Sylvia) Lei <lixlei@microsoft.com>
@Wwwsylvia Wwwsylvia mentioned this pull request Jan 26, 2024
4 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize the number of auth requests
4 participants