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

feat: add failure-tolerance for gc #16094

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Conversation

zyyw
Copy link
Contributor

@zyyw zyyw commented Dec 8, 2021

Closes: #15469
Closes: #15822

Signed-off-by: Shengwen Yu yshengwen@vmware.com

@zyyw zyyw requested a review from wy65701436 December 8, 2021 08:19
@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #16094 (f390c38) into main (0276906) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

❗ Current head f390c38 differs from pull request most recent head d2ae016. Consider uploading reports for the commit d2ae016 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16094      +/-   ##
==========================================
- Coverage   66.93%   66.91%   -0.02%     
==========================================
  Files         944      944              
  Lines       78447    78467      +20     
  Branches     2304     2304              
==========================================
+ Hits        52508    52509       +1     
- Misses      21891    21907      +16     
- Partials     4048     4051       +3     
Flag Coverage Δ
unittests 66.91% <33.33%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
src/jobservice/job/impl/gc/garbage_collection.go 44.66% <33.33%> (-0.34%) ⬇️
src/lib/cache/util.go 73.68% <0.00%> (-15.79%) ⬇️
src/controller/event/topic.go 1.80% <0.00%> (-7.21%) ⬇️
src/controller/event/handler/auditlog/auditlog.go 60.86% <0.00%> (+4.34%) ⬆️
src/common/utils/passports.go 89.74% <0.00%> (+5.12%) ⬆️
src/common/rbac/system/namespace.go 44.44% <0.00%> (+11.11%) ⬆️

@@ -265,9 +268,11 @@ func (gc *GarbageCollector) sweep(ctx job.Context) error {
if err := ignoreNotFound(func() error {
return gc.markDeleteFailed(ctx, blob)
}); err != nil {
gc.logger.Errorf("failed to call gc.markDeleteFailed() after v2DeleteManifest() error out: %s, %v", blob.Digest, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

the error was recorded in the markDeleteFailed() method, dup here.

@@ -255,6 +257,7 @@ func (gc *GarbageCollector) sweep(ctx job.Context) error {
}

// remove tags and revisions of a manifest
skippedBlobs := map[string]interface{}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

bool varaible is enough.

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.

LGTM

Signed-off-by: Shengwen Yu <yshengwen@vmware.com>
@zyyw zyyw merged commit c0b4963 into goharbor:main Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GC fails when manifest not found GC should be more resilient for flaky backends
3 participants