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

[cherry-pick 2.7] GC: correctly handle manifest unknown (404) condition in v2DeleteManifest retry loop #18534

Conversation

dkulchinsky
Copy link
Contributor

@dkulchinsky dkulchinsky commented Apr 14, 2023

Comprehensive Summary of your change

cherry pick of #18386 for release 2.7

/cc @Vad1mo & @zyyw

Issue being fixed

Fixes #18381

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

…n v2DeleteManifest retry loop

Signed-off-by: Danny Kulchinsky <dkulchinsky@fastly.com>
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #18534 (6cb6f22) into release-2.7.0 (de4cab1) will increase coverage by 0.14%.
The diff coverage is 0.00%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release-2.7.0   #18534      +/-   ##
=================================================
+ Coverage          66.40%   66.55%   +0.14%     
=================================================
  Files               1012     1012              
  Lines             108614   108966     +352     
  Branches            2677     2677              
=================================================
+ Hits               72129    72519     +390     
+ Misses             32522    32484      -38     
  Partials            3963     3963              
Flag Coverage Δ
unittests 66.55% <0.00%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/jobservice/job/impl/gc/util.go 26.92% <0.00%> (-1.65%) ⬇️

... and 21 files with indirect coverage changes

@dkulchinsky
Copy link
Contributor Author

UTTEST is failing, but I fail to see how this could be related to my change?

@Vad1mo Vad1mo added the release-note/update Update or Fix label Apr 17, 2023
@Vad1mo
Copy link
Member

Vad1mo commented Apr 17, 2023

@dkulchinsky I suggest also opening up a PR for 2.8.

@dkulchinsky dkulchinsky changed the title [cherry-pick] GC: correctly handle manifest unknown (404) condition in v2DeleteManifest retry loop [cherry-pick 2.7] GC: correctly handle manifest unknown (404) condition in v2DeleteManifest retry loop Apr 17, 2023
@dkulchinsky
Copy link
Contributor Author

@dkulchinsky I suggest also opening up a PR for 2.8.

Hey @Vad1mo, cherry picked to release-2.8.0

#18542

@dkulchinsky
Copy link
Contributor Author

Hey @Vad1mo, I think this is good to go?

@dkulchinsky
Copy link
Contributor Author

Hey @zyyw 👋🏼 hoping you could approve this cherry-pick based on #18386 for v2.7 🙏🏼

Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

we should revise this PR in main branch.

@wy65701436
Copy link
Contributor

closed by closed by #18662 (comment)

@Vad1mo
Copy link
Member

Vad1mo commented May 26, 2023

I would like to see this feature in 2.7 and 2.8, we have a dozen of harbor instances failing because of this error.
Not many users can upgrade from 2.7 to 2.8 because they need the helm chart support.

@dkulchinsky
Copy link
Contributor Author

I would like to see this feature in 2.7 and 2.8, we have a dozen of harbor instances failing because of this error. Not many users can upgrade from 2.7 to 2.8 because they need the helm chart support.

Thanks @Vad1mo, agree 100% and was asking for the same here

I'm happy to do the PRs as long as there's intention to accept them.

@wy65701436
Copy link
Contributor

@Vad1mo @dkulchinsky If we want to have this enhancement in versions 2.7 and 2.8, we should backport this change instead of the current pull request.

@dkulchinsky
Copy link
Contributor Author

@wy65701436 that's great! I'm going to prepare PRs today for 2.7 & 2.8 based on #18662

@dkulchinsky dkulchinsky closed this Jun 8, 2023
@dkulchinsky
Copy link
Contributor Author

Cherry picks based on #18662

release 2.7: #18802
release 2.8: #18803

@Vad1mo @wy65701436 @zyyw please 🙏🏼

@dkulchinsky dkulchinsky reopened this Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/update Update or Fix
Projects
Status: Closed PRs
Development

Successfully merging this pull request may close these issues.

None yet

5 participants