Add concurrency to tag Lookup

[bainsy88]

This was to address performance issues when deleting images in repos with a large number of tagstore

#3525

Signed-off-by: jack-baines jack.baines@uk.ibm.com

[codecov-commenter]

Codecov Report

Merging #3589 (76c29bf) into main (b609265) will increase coverage by 0.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3589      +/-   ##
==========================================
+ Coverage   56.33%   56.56%   +0.22%     
==========================================
  Files         101      101              
  Lines        7313     7354      +41     
==========================================
+ Hits         4120     4160      +40     
- Misses       2536     2537       +1     
  Partials      657      657              

Impacted Files	Coverage Δ
...ribution/distribution/registry/storage/tagstore.go	81.63% <0.00%> (+6.16%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4d9db5...76c29bf. Read the comment docs.

[bainsy88]

This PR fixes 1 of the 2 things described in the issue which is that we have to look up every tag in the repo to detect whether it matches the digest being deleted and needs to be untagged and previously this was done one tag at a time.

The 2nd bit has much lower impact from manual testing and is that once we have identified which tags match the digest we untag them sequentially. My view was this second change can be done standalone at a later date.

[bainsy88]

My testing on a a relatively small repo showed a 3x improvement so deleting an image in a repo with 500 tags took ~6s down from ~18s.

[bainsy88]

Final thing to point out is that I have hardcoded the concurrency limit to 10 obviously may be some advantages to that being configurable but wasn't entirely sure on how that could be achieved, so if that is deemed required would appreciate a pointer of a similar code that gets config into something like the tag store.

[milosgajdos]

Thanks, this looks good. I have a few nits if you don't mind :)

[milosgajdos]

can we remove this empty line?

[milosgajdos]

can we move this code into a goroutine (maybe allocate a chan and wait for it to finish?); it would be great to get rid of the break label 🤔

[wy65701436]

+1 for use gorountine,

done := make(chan bool, 1)
go func() {
		defer func() {
			done <- true
		}()
                for {
		                select {
		                case err, workRemaining := <-errChan:
			                if !workRemaining {
				                break read
			                }
			                cancel()
			                return nil, err
		                case tag, workRemaining := <-tagChan:
			                if !workRemaining {
				                close(errChan)
				                break read
			                }
			                tags = append(tags, tag)
		                }
                
	                }
}
<-done

[milosgajdos]

can we make 10 a constant?

[wy65701436]

and why chooses 10 or make it configurable just by env is enough?

[milosgajdos]

Yeah I'm all for making this configurable, but we could also have a fallthrough default value which should be a constant

[milosgajdos]

Just wondering since we are addressing these slow deletes...this code in S3 delete driver could use a bit of TLC, too. If the repo contains a lot of data, we end up growing s3Objects slice into huge size; the actual deletes are batched, though that is of little help since we must first go through all contents of the repo.

[wy65701436]

I'd like we can have a struct to wrap chan to provide public methods, that could be easy to understand.

My suggestion:

func NewWorkerPool(size int32) *WorkerPool {
	wp := &WorkerPool{}
	wp.queue = make(chan struct{}, size)
	return wp
}

type WorkerPool struct {
	queue chan struct{}
}

func (w *WorkerPool) GetWorker() {
	w.queue <- struct{}{}
}

func (w *WorkerPool) ReleaseWorker() {
	<-w.queue
}

[tiny1990]

LGTM...

800TB files in ceph ,its' hard to delete.
patch it by this pr

delete file with 3TB / min

[Antiarchitect]

Is there any news on this?

[milosgajdos]

Ping @bainsy88

[davidspek]

Friendly ping @bainsy88

[milosgajdos]

There is also #3890 which is more comprehensive.

We need to decide which one to keep focussing on.

[davidspek]

If #3890 is more comprehensive and more (recently) active then I’d say it’s safe to close this PR (also given that the author hasn’t responded to the reviews). As always, feel free to re-open this PR.