fix(dedupe): use a common blobs dir to simplify #3314

rchincha · 2025-08-17T05:19:53Z

Currently, our dedupe scheme is very complicated since master copy is kept in one of the repo dirs and if that image is deleted, the owning repo is changed, requiring multiple repo locks causing unnecessary contention.

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rchincha · 2025-08-19T16:06:41Z

Should pair with #2968

andaaron · 2025-09-04T16:35:00Z

pkg/storage/constants/constants.go

 	DefaultGCInterval       = 1 * time.Hour
 	S3StorageDriverName     = "s3"
 	LocalStorageDriverName  = "local"
+	GlobalBlobsRepo         = "_blobstore"


Make sure this blobs repo is per store and substore. Because substores may be on different partitions.

Currently, zot uses one of the repos as the master copy for a blob to achieve dedupe. However, blobs can be deleted from repos and this complicates dedupe tracking logic. Now use a single hidden global repo as a blob store instead. Signed-off-by: Ramkumar Chinchani <[email protected]>

Signed-off-by: Ramkumar Chinchani <[email protected]>

andaaron

I think the logging is too verbose, and should be trimmed

andaaron · 2025-10-20T07:01:30Z

pkg/api/controller.go

 			substore := c.StoreController.SubStore[route]
 			if substore != nil {
-				substore.RunDedupeBlobs(time.Duration(0), c.taskScheduler)
+				//substore.RunDedupeBlobs(time.Duration(0), c.taskScheduler)


Is this just temporary, or we're dropping dedupe altogether?

andaaron · 2025-10-20T07:06:05Z

pkg/storage/cache/boltdb.go

+			return nil
+		}
+
+		// create nested deduped bucket where we store all the deduped blobs + original blob


Suggested change

// create nested deduped bucket where we store all the deduped blobs + original blob

// create nested deduped bucket where we store all the deduped blobs while excluding the original blob

Correct?

andaaron · 2025-10-20T07:11:00Z

pkg/storage/cache/boltdb.go

I think the logging is too verbose for merge. It would be too much information to include logs for the code paths not returning errors.

andaaron · 2025-10-20T07:26:05Z

pkg/storage/cache/boltdb.go

+
+				dedupeBlob := d.getOne(deduped)
+				if dedupeBlob != nil {
+					d.log.Debug().Str("digest", digest.String()).Str("path", path).Msg("more in dedupe bucket, leaving original alone")


Previous logic was replacing the old original blob with one of the duplicates, and then delete the blob.
This new logic would keep the original blob around forever? How/when would it be GCed?

andaaron · 2025-10-20T07:32:39Z

pkg/storage/cache/boltdb.go

 		}

-		return nil
+		return zerr.ErrCacheMiss


Under which scenario would this return the error?
The updated code is a bit confusing as it is returning nil in several other places and it is unclear in which scenario this line is reached.

andaaron · 2025-10-20T07:37:32Z

pkg/storage/cache/dynamodb.go

Are there no other needed changes in this file?

Also the redis implementation should be reviewed

andaaron · 2025-10-20T07:41:14Z

pkg/storage/cache/boltdb.go

 		}

 		origin := bucket.Bucket([]byte(constants.OriginalBucket))
 		if origin != nil {


And if origin is nil, then we should return nil, because the blob does not exist.

It is not in the duplicates list and it is not the original, so it does not exist, so the deletion should be marked as successful, correct?

andaaron · 2025-10-20T07:51:51Z

pkg/storage/gc/gc.go

 	}

+	// skip the global blobs repo
+	if repo == constants.GlobalBlobsRepo {


Something is fishy.
Why is this here? constants.GlobalBlobsRepo is not an actual repo because it has an invalid repo name. How is GC detecting it when "walking" the disk if it doesn't have a valid repo name? It shouldn't call this function at all for constants.GlobalBlobsRepo?

andaaron · 2025-10-20T08:13:37Z

pkg/storage/imagestore/imagestore.go


-				return err
-			}
+		goto retry


Since we're refactoring this, can you pick up #2969?
Or do you want me to rebase and merge that in advance?

Is this code working right now, wouldn't it potentially execute the code multiple times?

if err := is.storeDriver.Move(src, gdst); err != nil { is.log.Error().Err(err).Str("src", src).Str("dst", gdst).Str("component", "dedupe"). Msg("failed to rename blob") return err }

rchincha force-pushed the fix-dedupe branch from 8bee9dc to f789a29 Compare August 19, 2025 07:45

rchincha force-pushed the fix-dedupe branch 5 times, most recently from 6edb9fb to b4c3c46 Compare September 4, 2025 05:23

andaaron reviewed Sep 4, 2025

View reviewed changes

rchincha force-pushed the fix-dedupe branch 6 times, most recently from 4b2da37 to da2884a Compare September 11, 2025 05:24

rchincha force-pushed the fix-dedupe branch 3 times, most recently from 49521a6 to 23f1ebc Compare October 10, 2025 06:47

rchincha added 13 commits October 14, 2025 22:15

fix: dedupe fix

a0fd8d7

Signed-off-by: Ramkumar Chinchani <[email protected]>

fix: fix dedupe

f6b708f

Signed-off-by: Ramkumar Chinchani <[email protected]>

fix: tests

aa98aef

fix: test

6df7e79

Signed-off-by: Ramkumar Chinchani <[email protected]>

fix: disable dedupe task generators

2cece57

fix: dedupe

b3639c3

Signed-off-by: Ramkumar Chinchani <[email protected]>

fix: dedupe

05b5556

fix: dedupe

f045116

Signed-off-by: Ramkumar Chinchani <[email protected]>

fix: add more debugs

3da4db6

fix: fix cache logic

42a40dd

fix: dyanmodb cache logic

1b18dd3

fix: test

03e532e

rchincha added 4 commits October 14, 2025 22:15

fix: cache

28a2b58

fix: some boltdb changes

8409301

Signed-off-by: Ramkumar Chinchani <[email protected]>

fix: botldb

b57c349

Signed-off-by: Ramkumar Chinchani <[email protected]>

fix: botdb fixes

3c6902e

rchincha force-pushed the fix-dedupe branch from 23f1ebc to 3c6902e Compare October 16, 2025 06:46

fix: test cases

f628cfb

andaaron reviewed Oct 20, 2025

View reviewed changes

	// create nested deduped bucket where we store all the deduped blobs + original blob
	// create nested deduped bucket where we store all the deduped blobs while excluding the original blob

fix(dedupe): use a common blobs dir to simplify #3314

Are you sure you want to change the base?

fix(dedupe): use a common blobs dir to simplify #3314

Uh oh!

Conversation

rchincha commented Aug 17, 2025

Uh oh!

rchincha commented Aug 19, 2025

Uh oh!

andaaron Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andaaron Sep 4, 2025 •

edited

Loading