add bounded concurrency for tag lookup and untag #4329

microyahoo · 2024-04-18T09:04:08Z

add concurrency limits for tag lookup and untag

Harbor is using the distribution for it's (harbor-registry) registry component.

The harbor GC will call into the registry to delete the manifest, which in turn then does a lookup for all tags that reference the deleted manifest.
To find the tag references, the registry will iterate every tag in the repository and read it's link file to check if it matches the deleted manifest (i.e. to see if uses the same sha256 digest). So, the more tags in repository, the worse the performance will be (as there will be more s3 API calls occurring for the tag directory lookups and tag file reads).

Therefore, we can use concurrent lookup and untag to optimize performance as described in goharbor/harbor#12948.

P.S. This optimization was originally contributed by @Antiarchitect, now I would like to take it over.
Thanks @Antiarchitect's efforts with PR #3890.

Signed-off-by: Liang Zheng zhengliang0901@gmail.com

registry/handlers/manifests.go

milosgajdos · 2024-04-18T16:16:26Z

@corhere do you mind batting an eye on this? It looks a ok to me, but I need to give it another look.

milosgajdos · 2024-04-21T09:11:56Z

@Jamstah @squizzi you folks mind having a look at this?

registry/storage/registry.go

registry/handlers/app.go

registry/handlers/context.go

registry/handlers/manifests.go

corhere · 2024-04-22T20:31:57Z

registry/handlers/app.go

+		if ok {
+			limit, ok := l.(int)
+			if !ok {
+				panic("tag lookup concurrency limit config key must have a integer value")


What are the semantics of an explicit concurrency limit of 0? -1?

During the initialization of the tagStore, a judgment is made, and if it is greater than 0, it will be set; otherwise, the concurrency limit is set to the default value.

limit := DefaultConcurrencyLimit if repo.tagLookupConcurrencyLimit > 0 { limit = repo.tagLookupConcurrencyLimit }

We should document that in the docs FWIW

We should document that in the docs FWIW

done

Defining all non-positive integers to mean GOMAXPROCS does not leave any values to signify the other possible special case: unbounded concurrency / no limit. To be clear, I don't think an option for unbounded concurrency should be added as part of this PR. I simply want to reserve the negative numbers for future use by having it be an error to set concurrencylimit to a negative value.

hi @corhere, I've added a check that directly throws an error if it's set to a negative value. However, I'm not quite clear on how you suggest handling unbounded / no limit concurrency. Should we maintain the current logic? Or are you saying that even if the user sets it to unbounded, we should treat it with a certain upper limit, such as GOMAXPROCS?

I suggest that we do not handle unbounded concurrency. At least not as part of this PR. What I am suggesting is the following:

concurrencylimit value Result

(unset) Tag lookups are limited to GOMAXPROCS concurrency (the default)

0 Tag lookups are limited to GOMAXPROCS concurrency (the default)

Positive integer, e.g. 3 Tag lookups are limited to the value, e.g. 3

Negative integer, e.g. -1 Configuration error

Making negative numbers an error today guarantees that no valid registry configuration can have a concurrencylimit option set to a negative number. That gives us the freedom to redefine negative values to mean something valid in the future as a non-breaking change. One potential future definition of negative concurrencylimit values is to configure unbounded concurrency. But we don't need that today.

Note that setting concurrencylimit: 0 should be allowed. It is very useful to have an "explicit default" value for distribution configuration as it allows the user to be explicit about their intent to use GOMAXPROCS as the concurrency limit in their configuration, or to override e.g. concurrencylimit: 4 in the YAML config by setting the environment variable REGISTRY_TAG_CONCURRENCYLIMIT=0.

Thanks for your nice explanation, I have updated it, PTAL.

registry/storage/tagstore.go

corhere · 2024-04-24T14:41:11Z

registry/handlers/app.go

+		if ok {
+			limit, ok := l.(int)
+			if !ok {
+				panic("tag lookup concurrency limit config key must have a integer value")


Defining all non-positive integers to mean GOMAXPROCS does not leave any values to signify the other possible special case: unbounded concurrency / no limit. To be clear, I don't think an option for unbounded concurrency should be added as part of this PR. I simply want to reserve the negative numbers for future use by having it be an error to set concurrencylimit to a negative value.

registry/storage/tagstore.go

Signed-off-by: Liang Zheng <zhengliang0901@gmail.com>

@Antiarchitect

Harbor is using the distribution for it's (harbor-registry) registry component. The harbor GC will call into the registry to delete the manifest, which in turn then does a lookup for all tags that reference the deleted manifest. To find the tag references, the registry will iterate every tag in the repository and read it's link file to check if it matches the deleted manifest (i.e. to see if uses the same sha256 digest). So, the more tags in repository, the worse the performance will be (as there will be more s3 API calls occurring for the tag directory lookups and tag file reads). Therefore, we can use concurrent lookup and untag to optimize performance as described in goharbor/harbor#12948. P.S. This optimization was originally contributed by @Antiarchitect, now I would like to take it over. Thanks @Antiarchitect's efforts with PR distribution#3890. Signed-off-by: Liang Zheng <zhengliang0901@gmail.com>

corhere

LGTM, thanks!

milosgajdos

Thanks for a nice contribution @microyahoo

Antiarchitect · 2024-04-26T22:32:55Z

@microyahoo Congrats! You made it!

dkulchinsky · 2024-04-27T01:13:36Z

Congrats @microyahoo! very excited to see this getting merged and thanks to @Antiarchitect for the initial work.

github-actions bot added area/storage area/config Related to registry config dependencies Pull requests that update a dependency file area/api labels Apr 18, 2024

microyahoo force-pushed the harbor-12948 branch 4 times, most recently from 9e15cc0 to f21b9a0 Compare April 18, 2024 10:56

microyahoo changed the title ~~WIP: add concurrency limits for tag lookup and untag~~ add concurrency limits for tag lookup and untag Apr 18, 2024

microyahoo mentioned this pull request Apr 18, 2024

[GC performance] The performance of v2 manifest deletion is not good in S3 environment goharbor/harbor#12948

Open

milosgajdos reviewed Apr 18, 2024

View reviewed changes

registry/handlers/manifests.go Outdated Show resolved Hide resolved

milosgajdos requested a review from corhere April 18, 2024 14:37

microyahoo force-pushed the harbor-12948 branch from f21b9a0 to 19e5292 Compare April 18, 2024 15:10

microyahoo requested a review from milosgajdos April 18, 2024 15:15

milosgajdos reviewed Apr 18, 2024

View reviewed changes

registry/handlers/manifests.go Outdated Show resolved Hide resolved

microyahoo force-pushed the harbor-12948 branch from 19e5292 to 7adf358 Compare April 18, 2024 15:43

microyahoo requested a review from milosgajdos April 18, 2024 15:45

milosgajdos requested review from Jamstah and squizzi April 21, 2024 09:12

Antiarchitect mentioned this pull request Apr 21, 2024

Concurrent tag lookup #3890

Closed

squizzi reviewed Apr 22, 2024

View reviewed changes

registry/storage/registry.go Outdated Show resolved Hide resolved

corhere requested changes Apr 22, 2024

View reviewed changes

microyahoo force-pushed the harbor-12948 branch from 7adf358 to b7f94f6 Compare April 23, 2024 03:43

microyahoo requested review from squizzi and corhere April 23, 2024 04:00

microyahoo force-pushed the harbor-12948 branch from b7f94f6 to a0424f0 Compare April 23, 2024 11:38

github-actions bot added the area/docs label Apr 23, 2024

microyahoo force-pushed the harbor-12948 branch from a0424f0 to 600402b Compare April 23, 2024 11:42

microyahoo force-pushed the harbor-12948 branch 3 times, most recently from 4b3671c to 7b6eae8 Compare April 24, 2024 03:00

corhere reviewed Apr 24, 2024

View reviewed changes

microyahoo force-pushed the harbor-12948 branch 2 times, most recently from fd7c7a6 to dfd3f03 Compare April 25, 2024 09:57

microyahoo requested a review from corhere April 25, 2024 10:04

microyahoo added 2 commits April 26, 2024 22:22

vendor: update manifest dependencies

a5882d6

Signed-off-by: Liang Zheng <zhengliang0901@gmail.com>

microyahoo force-pushed the harbor-12948 branch from dfd3f03 to a2afe23 Compare April 26, 2024 14:32

corhere approved these changes Apr 26, 2024

View reviewed changes

corhere changed the title ~~add concurrency limits for tag lookup and untag~~ add bounded concurrency for tag lookup and untag Apr 26, 2024

milosgajdos approved these changes Apr 26, 2024

View reviewed changes

milosgajdos merged commit e0795fc into distribution:main Apr 26, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add bounded concurrency for tag lookup and untag #4329

add bounded concurrency for tag lookup and untag #4329

microyahoo commented Apr 18, 2024 •

edited

milosgajdos commented Apr 18, 2024

milosgajdos commented Apr 21, 2024

corhere Apr 22, 2024

microyahoo Apr 23, 2024

milosgajdos Apr 23, 2024

microyahoo Apr 23, 2024

corhere Apr 24, 2024

microyahoo Apr 25, 2024 •

edited

corhere Apr 26, 2024

microyahoo Apr 26, 2024

corhere Apr 24, 2024

corhere left a comment

milosgajdos left a comment

Antiarchitect commented Apr 26, 2024

dkulchinsky commented Apr 27, 2024

`concurrencylimit` value	Result
(unset)	Tag lookups are limited to `GOMAXPROCS` concurrency (the default)
`0`	Tag lookups are limited to `GOMAXPROCS` concurrency (the default)
Positive integer, e.g. `3`	Tag lookups are limited to the value, e.g. `3`
Negative integer, e.g. `-1`	Configuration error

add bounded concurrency for tag lookup and untag #4329

add bounded concurrency for tag lookup and untag #4329

Conversation

microyahoo commented Apr 18, 2024 • edited

milosgajdos commented Apr 18, 2024

milosgajdos commented Apr 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microyahoo Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corhere left a comment

Choose a reason for hiding this comment

milosgajdos left a comment

Choose a reason for hiding this comment

Antiarchitect commented Apr 26, 2024

dkulchinsky commented Apr 27, 2024

microyahoo commented Apr 18, 2024 •

edited

microyahoo Apr 25, 2024 •

edited