diff --git a/pkg/hold/gc/blobs_test.go b/pkg/hold/gc/blobs_test.go new file mode 100644 index 0000000..328bc72 --- /dev/null +++ b/pkg/hold/gc/blobs_test.go @@ -0,0 +1,153 @@ +package gc + +import ( + "context" + "errors" + "testing" + "time" + + "atcr.io/pkg/s3" +) + +// errAllDeletesFail stands in for whatever S3 returns when an object cannot be +// removed: a permissions change, an object lock, a bucket in a bad state. +var errAllDeletesFail = errors.New("access denied") + +// blobKey builds the S3 key layout deleteOrphanedBlobs walks. The digest is +// recovered from this path, so the shape matters: a key that does not parse is +// silently skipped rather than deleted. +func blobKey(digest string) string { + return "docker/registry/v2/blobs/sha256/" + digest[:2] + "/" + digest + "/data" +} + +// TestDeleteOrphanedBlobs covers the only part of GC that removes bytes. +// +// It had no test, and could not have had one: the in-process harness stamps +// every object with time.Now(), and the S3 mock stamped nothing at all, so +// neither could express "this blob is nine days old". Object age is the input +// that decides whether a blob is deletable, so both halves of that rule went +// unexercised. MockS3Client.ObjectTimes exists to close exactly this gap. +// +// The four cases are the four ways this function must behave, and three of them +// are reasons NOT to delete. That ratio is the point: the failure that matters +// here is deleting something, not keeping it. +// +// Verified by mutation: disabling the grace check deletes the young blob, +// disabling the referenced check deletes the live one. Both fail this test. +func TestDeleteOrphanedBlobs(t *testing.T) { + const ( + orphanOld = "aaaa000000000000000000000000000000000000000000000000000000000001" + orphanYoung = "bbbb000000000000000000000000000000000000000000000000000000000002" + referencedID = "cccc000000000000000000000000000000000000000000000000000000000003" + ) + + mock := s3.NewMockS3Client("") + svc := &s3.S3Service{Client: mock, Bucket: "test-bucket"} + + longAgo := time.Now().Add(-30 * 24 * time.Hour) + justNow := time.Now().Add(-1 * time.Hour) + + // Unreferenced and well past the grace period: the one blob that should go. + mock.Objects[blobKey(orphanOld)] = make([]byte, 4096) + mock.ObjectTimes[blobKey(orphanOld)] = longAgo + + // Unreferenced but young. A push in flight looks exactly like this, and its + // records may not exist yet, so age is the only thing protecting it. + mock.Objects[blobKey(orphanYoung)] = make([]byte, 128) + mock.ObjectTimes[blobKey(orphanYoung)] = justNow + + // Referenced and old. Age alone must never be enough. + mock.Objects[blobKey(referencedID)] = make([]byte, 256) + mock.ObjectTimes[blobKey(referencedID)] = longAgo + + // Not a /data object. The link file sits beside the blob under the same + // prefix; deleting it is not a byte reclaim, it is corruption of the layout. + // + // Guarded twice, and worth knowing which one carries it: the HasSuffix + // check in deleteOrphanedBlobs is a redundant early-out, because + // extractDigestFromPath anchors its regex on /data$ and returns "" for + // anything else. Removing the suffix check leaves this case passing. + linkKey := "docker/registry/v2/blobs/sha256/dd/dddd000000000000000000000000000000000000000000000000000000000004/link" + mock.Objects[linkKey] = []byte("sha256:dddd") + mock.ObjectTimes[linkKey] = longAgo + + gc := &GarbageCollector{s3: svc, logger: newTestLogger()} + referenced := map[string]bool{"sha256:" + referencedID: true} + + result := &GCResult{} + if err := gc.deleteOrphanedBlobs(context.Background(), referenced, result); err != nil { + t.Fatalf("deleteOrphanedBlobs: %v", err) + } + + if result.BlobsDeleted != 1 { + t.Errorf("BlobsDeleted = %d, want 1", result.BlobsDeleted) + } + if result.OrphanedBlobs != 1 { + t.Errorf("OrphanedBlobs = %d, want 1: only the old unreferenced blob is a candidate", + result.OrphanedBlobs) + } + if result.BytesReclaimed != 4096 { + t.Errorf("BytesReclaimed = %d, want 4096: reclaimed bytes are what the operator "+ + "is shown, and they come from the listing rather than the delete", + result.BytesReclaimed) + } + + survivors := map[string]string{ + blobKey(orphanYoung): "unreferenced but inside the blob grace period", + blobKey(referencedID): "referenced by a live manifest", + linkKey: "not a /data object", + } + for key, why := range survivors { + if _, ok := mock.Objects[key]; !ok { + t.Errorf("deleted a blob that should have survived (%s): %s", why, key) + } + } + if _, ok := mock.Objects[blobKey(orphanOld)]; ok { + t.Error("the old unreferenced blob was counted but never actually removed from S3") + } +} + +// TestDeleteOrphanedBlobs_KeepsCountingAfterADeleteFails pins the deliberate +// choice to log and continue rather than abort. One unreclaimable object must +// not strand every blob behind it in the walk, which on a large hold would mean +// the sweep never finishes its first pass. +// +// The counters have to stay honest through it: the blob is a candidate either +// way, but only a blob that actually left storage may be counted as deleted or +// have its bytes reported as reclaimed. +func TestDeleteOrphanedBlobs_KeepsCountingAfterADeleteFails(t *testing.T) { + const ( + doomed = "eeee000000000000000000000000000000000000000000000000000000000005" + stubborn = "ffff000000000000000000000000000000000000000000000000000000000006" + ) + + mock := s3.NewMockS3Client("") + svc := &s3.S3Service{Client: mock, Bucket: "test-bucket"} + longAgo := time.Now().Add(-30 * 24 * time.Hour) + + for _, d := range []string{doomed, stubborn} { + mock.Objects[blobKey(d)] = make([]byte, 512) + mock.ObjectTimes[blobKey(d)] = longAgo + } + + // DeleteObject fails for everything; both blobs are candidates, neither goes. + mock.DeleteObjectError = errAllDeletesFail + + gc := &GarbageCollector{s3: svc, logger: newTestLogger()} + result := &GCResult{} + if err := gc.deleteOrphanedBlobs(context.Background(), map[string]bool{}, result); err != nil { + t.Fatalf("deleteOrphanedBlobs returned an error instead of continuing: %v", err) + } + + if result.OrphanedBlobs != 2 { + t.Errorf("OrphanedBlobs = %d, want 2: the walk must reach the second blob "+ + "after the first one fails to delete", result.OrphanedBlobs) + } + if result.BlobsDeleted != 0 { + t.Errorf("BlobsDeleted = %d, want 0: nothing left storage", result.BlobsDeleted) + } + if result.BytesReclaimed != 0 { + t.Errorf("BytesReclaimed = %d, want 0: reporting reclaimed space for bytes that "+ + "are still there is how a hold quietly runs out of disk", result.BytesReclaimed) + } +} diff --git a/pkg/s3/mock.go b/pkg/s3/mock.go index deed775..997945e 100644 --- a/pkg/s3/mock.go +++ b/pkg/s3/mock.go @@ -29,6 +29,16 @@ type MockS3Client struct { // Objects stores in-memory blobs for PutObject/HeadObject/DeleteObject/CopyObject/ListObjectsV2. Objects map[string][]byte + // ObjectTimes optionally sets LastModified per key for ListObjectsV2. A key + // with no entry is listed without a timestamp, which is what this mock has + // always done — WalkBlobs then reports the zero time, and anything reading + // it as an age sees the object as arbitrarily old. + // + // It exists because object age is a real input to behaviour: the GC blob + // sweep protects anything inside gcBlobGracePeriod, and a mock that cannot + // make an object young cannot test the protecting half of that rule. + ObjectTimes map[string]time.Time + // Track calls for verification in tests mu sync.Mutex CreateMultipartCalls []CreateMultipartCall @@ -45,6 +55,7 @@ type MockS3Client struct { AbortError error HeadObjectError error CopyObjectError error + DeleteObjectError error } // CreateMultipartCall records a CreateMultipartUpload call @@ -99,6 +110,7 @@ func NewMockS3Client(testServerURL string) *MockS3Client { return &MockS3Client{ TestServerURL: testServerURL, Objects: make(map[string][]byte), + ObjectTimes: make(map[string]time.Time), CreateMultipartCalls: []CreateMultipartCall{}, CompleteCalls: []CompleteCall{}, AbortCalls: []AbortCall{}, @@ -266,8 +278,13 @@ func (m *MockS3Client) DeleteObject(ctx context.Context, input *awss3.DeleteObje m.mu.Lock() defer m.mu.Unlock() + if m.DeleteObjectError != nil { + return nil, m.DeleteObjectError + } + key := aws.ToString(input.Key) delete(m.Objects, key) + delete(m.ObjectTimes, key) return &awss3.DeleteObjectOutput{}, nil } @@ -302,10 +319,14 @@ func (m *MockS3Client) ListObjectsV2(ctx context.Context, input *awss3.ListObjec size := int64(len(data)) k := key - contents = append(contents, s3types.Object{ + obj := s3types.Object{ Key: &k, Size: &size, - }) + } + if lm, ok := m.ObjectTimes[key]; ok { + obj.LastModified = &lm + } + contents = append(contents, obj) } var cps []s3types.CommonPrefix