GC: correctly handle manifest unknown (404) condition in v2DeleteManifest retry loop

[dkulchinsky]

Comprehensive Summary of your change

Currently when a 404 error is returned from the registry in v2DeleteManifest retry loop, it will be considered is a temporary/retryable error and will eventually result in the v2DeleteManifest step to fail, although a 404 (MANIFEST_UNKNOWN) indicates that the manifest no longer exists.

This PR adds a check inside the retry loop that marks the v2DeleteManifest as success if registry returns a 404.

This change seem to correctly align with the next cleanup step that deletes all the manifests revisions using RegistryCtl which also treats a 404 error as success.

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.

[codecov]

Codecov Report

Merging #18386 (33b1d7c) into main (b201f98) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #18386      +/-   ##
==========================================
- Coverage   67.39%   67.38%   -0.01%     
==========================================
  Files         984      984              
  Lines      106987   106990       +3     
  Branches     2670     2670              
==========================================
- Hits        72101    72099       -2     
- Misses      31005    31012       +7     
+ Partials     3881     3879       -2     

Flag	Coverage Δ
unittests	67.38% <0.00%> (-0.01%)	⬇️

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 8 files with indirect coverage changes

[dkulchinsky]

Hey @Vad1mo, @wy65701436 was wondering if you had a chance to consider this PR?

I have a local build with this change running on our test Harbor and it is working very well, we're finally able to get GC running and actually cleaning up all these manifests & blobs.

[dkulchinsky]

Thanks for approving @zyyw!

I updated the branch, let me know if there's anything else needed to get this merged.

[dkulchinsky]

Thanks @Vad1mo & @zyyw!

I wonder if it's possible to backport/cherry-pick this change to v2.7? I realize v2.8 is almost out but we're not quite ready to upgrade yet as we're still working on our chartmuseum deprecation and would be great if we we could have this fix v2.7 for now

[Vad1mo]

Good question, would make sense as it is a fix.
I would recommend to back port it yourself to 2.7 and 2.8. Open a PR for that so that it can land in 2.7.2 or 2.8.1

[dkulchinsky]

@Vad1mo, @zyyw cherry picked this into release-2.7.0 branch: #18534 🙏🏼

[Vad1mo]

you should also make one for the 2.8 release.