s3: improve walk performance #2455

sargun · 2017-11-29T23:49:52Z

No description provided.

GordonTheTurtle · 2017-11-29T23:49:54Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "make-walk-faster" [email protected]:sargun/distribution.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842357011424
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

codecov · 2017-11-30T00:12:28Z

Codecov Report

Merging #2455 into master will decrease coverage by 10.24%.
The diff coverage is 15.38%.

@@             Coverage Diff             @@
##           master    #2455       +/-   ##
===========================================
- Coverage    61.4%   51.15%   -10.25%     
===========================================
  Files         128      129        +1     
  Lines       11727    11818       +91     
===========================================
- Hits         7201     6046     -1155     
- Misses       3628     5011     +1383     
+ Partials      898      761      -137

Impacted Files	Coverage Δ
registry/storage/driver/storagedriver.go	`33.33% <ø> (ø)`	⬆️
registry/storage/driver/filesystem/driver.go	`65.4% <0%> (-0.63%)`	⬇️
registry/storage/driver/inmemory/driver.go	`73.71% <0%> (-0.96%)`	⬇️
registry/storage/driver/base/base.go	`69.23% <0%> (-4.89%)`	⬇️
registry/storage/driver/s3-goamz/s3.go	`0.5% <0%> (-51.4%)`	⬇️
registry/storage/driver/s3-aws/s3.go	`4.15% <0%> (-56.66%)`	⬇️
registry/storage/driver/oss/oss.go	`0.56% <0%> (-57.23%)`	⬇️
registry/storage/driver/swift/swift.go	`58.26% <0%> (-0.25%)`	⬇️
registry/storage/driver/gcs/gcs.go	`0.39% <0%> (-68.94%)`	⬇️
registry/storage/driver/azure/azure.go	`0.77% <0%> (-0.01%)`	⬇️
... and 12 more

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 b5db8ee...cbcbcb0. Read the comment docs.

sargun · 2017-11-30T17:12:37Z

I have no idea what this codecov error is, any ideas?

stevvooe · 2017-12-01T23:39:52Z

@sargun Don't worry about it. codecov is just noise.

Could you describe the design and the approach, including how this works and what caveats it may introduce?

sargun · 2017-12-02T22:13:44Z

The root problem is that the Walk function is implemented in a manner that assumes that metadata ops are free on the filesystem driver that you're using. Unfortunately, this is not true.

This changes it so that it is the responsibility of the backend to implement a Walk Function. This walk function is a little bit different than the walk function that previously existed, as in your can return an ErrSkipDir in the middle to stop walking this directory.

This adds an implementation of a generic version of this new walk function, and it adds an S3-specific implementation.

Since there are only a few code paths that exercise this code, I'm not overly worried about negative impact. I think that the correctness of the algorithm is the primary concern.

sargun · 2017-12-13T11:51:02Z

@stevvooe Any opinions?

sargun · 2017-12-19T18:22:00Z

Bump.

stevvooe · 2018-01-03T01:34:27Z

@sargun That sounds great!

This walk function is a little bit different than the walk function that previously existed, as in your can return an ErrSkipDir in the middle to stop walking this directory.

I would prefer that we preserve this capability. It is the same thing supported in the standard library with filepath.Walk.

sargun · 2018-01-03T01:53:30Z

@stevvooe The "new" walk function has the ErrSkipDir to stop going through that directory, whereas the old code didn't have that. The old one could only do ErrSkipDir when initially entering the directory, and not while mid-way through a directory.

stevvooe · 2018-01-03T18:46:12Z

@hinshun @dmcgowan PTAL

hinshun

I took a first pass at this, but its seems that the new WalkFallback function isn't necessary as the original already provided the ability to stop traversal with a non ErrSkipDir error. It's up to the caller of Walk to decide how to handle the error.

hinshun · 2018-01-04T02:51:02Z

registry/storage/blobstore.go

@@ -109,11 +110,16 @@ func (bs *blobStore) Enumerate(ctx context.Context, ingester func(dgst digest.Di

 		digest, err := digestFromPath(currentPath)
 		if err != nil {
+			retErr = err


Is this necessary? It seems that the return value of WalkFn eventually gets returned from Walk unless its ErrSkipDir.

hinshun · 2018-01-04T02:55:38Z

registry/storage/catalog.go

@@ -40,22 +41,20 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri

 		// if we've filled our array, no need to walk any further
 		if len(foundRepos) == len(repos) {
-			return errFinishedWalk


It looks like the previous Walk function already had the ability to stop walking early by providing a non ErrSkipDir error. I think its a little confusing to overload the use of ErrSkipDir to stop the traversal in the middle.

This is the way that the one from the Golang stdlib works: https://golang.org/pkg/path/filepath/#Walk

--

I think it's somewhat preferential to move in this direction, because we could eventually get rid of our custom walk function users and just use stdlib for filesystems, and such.

It also means that people don't have to do mid-directory interruptions "by hand"

hinshun · 2018-01-04T03:05:43Z

registry/storage/driver/s3-aws/s3.go

+	var objectCount int64
+
+	path := from
+	if path != "/" && path[len(path)-1] != '/' {


Just path[len(path)-1] != '/' is sufficient.

hinshun · 2018-01-04T03:06:37Z

registry/storage/driver/s3-aws/s3.go

+// Walk traverses a filesystem defined within driver, starting
+// from the given path, calling f on each file
+func (d *driver) Walk(ctx context.Context, from string, f storagedriver.WalkFn) error {
+	var objectCount int64


Perhaps its better to declare this closer to where it's used, i.e. near the invocation of doWalk.

hinshun · 2018-01-04T03:07:52Z

registry/storage/driver/s3-aws/s3.go

+		return err
+	}
+
+	if objectCount == 0 {


Could you leave a comment why this errors? Does this mean it'll error on an empty directory?

The idea of an "empty" directory doesn't exist on EC2, see:

(titusagent) ~ $ aws s3 ls s3://bucket/titan/devvpc7/ PRE logs/ PRE registry/ PRE t/ (titusagent) ~ $ aws s3 cp foo s3://bucket/titan/devvpc7/sargun/ upload: ./foo to s3://bucket/titan/devvpc7/sargun/foo (titusagent) ~ $ aws s3 ls s3://bucket/titan/devvpc7/ PRE logs/ PRE registry/ PRE sargun/ PRE t/ (titusagent) ~ $ aws s3 rm s3://bucket/titan/devvpc7/sargun/foo delete: s3://bucket/titan/devvpc7/sargun/foo (titusagent) ~ $ aws s3 ls s3://bucket/titan/devvpc7/ PRE logs/ PRE registry/ PRE t/

hinshun · 2018-01-04T03:08:42Z

registry/storage/driver/s3-aws/s3.go

+func (d *driver) doWalk(parentCtx context.Context, objectCount *int64, path, prefix string, f storagedriver.WalkFn) error {
+	var retError error
+
+	// fmt.Printf("Called with path=%s prefix=%s", path, prefix)


Leftover debugging statement.

hinshun · 2018-01-04T03:10:21Z

registry/storage/driver/s3-aws/s3.go

+		*objectCount += *objects.KeyCount
+		walkInfos := make([]walkInfoContainer, *objects.KeyCount)
+		i := 0
+		for _, dir := range objects.CommonPrefixes {


for i, dir := range objects.CommonPrefixes will give you the index you're accessing.

hinshun · 2018-01-04T03:10:33Z

registry/storage/driver/s3-aws/s3.go

+			i++
+		}
+
+		for _, file := range objects.Contents {


for i, file := range objects.Contents will give you the index you're accessing.

No, because the range is len(objects.CommonPrefixes) + i. I refactored this to use append.

hinshun · 2018-01-04T03:18:33Z

registry/storage/driver/s3-aws/s3.go

+				prefix: dir.Prefix,
+				FileInfoFields: storagedriver.FileInfoFields{
+					IsDir: true,
+					Path:  strings.Replace(commonPrefix[0:len(commonPrefix)-1], d.s3Path(""), prefix, 1),


For array slices, the 0 is implicit so you can simply do: commonPrefix[:len(commonPrefix)-1]. Also note the end index is exclusive, is this intentional?

Yeah, this path shouldn't have the trailing '/' from my understanding.

sargun · 2018-01-04T18:07:19Z

make fmt passes fine on my system. Any idea why it's failing on Circle-CI?

This upgrade, and vendors aws-sdk-go to version v1.12.36. This is because it has new API calls accessible to the S3 client, specifically S3.ListObjectsV2PagesWithContext Signed-off-by: Sargun Dhillon <[email protected]>

Move the Walk types into registry/storage/driver, and add a Walk method to each storage driver. Although this is yet another API to implement, there is a fall back implementation that relies on List and Stat. For some filesystems this is very slow. Also, this WalkDir Method conforms better do a traditional WalkDir (a la filepath). This change is in preparation for refactoring. Signed-off-by: Sargun Dhillon <[email protected]>

This changes the Walk Method used for catalog enumeration. Just to show how much an effect this has on our s3 storage: Original: List calls: 6839 real 3m16.636s user 0m0.000s sys 0m0.016s New: ListObjectsV2 Calls: 1805 real 0m49.970s user 0m0.008s sys 0m0.000s This is because it no longer performs a list and stat per item, and instead is able to use the metadata gained from the list as a replacement to stat. Signed-off-by: Sargun Dhillon <[email protected]>

sargun · 2018-01-08T07:09:45Z

@hinshun PTAL.

dmcgowan · 2018-01-18T18:50:31Z

registry/storage/linkedblobstore.go

 			return err
 		}

 		return nil
 	})

-	if err != nil {
-		return err
+	if retErr != nil {


I don't see the case where retErr != err in this scope

If Walk is returning a different error than what retErr is set to, can you add a comment explaining that

I guess we should make it so the walk functions never transmute err? Is that a reasonable expectation from Driver implementations?

I added a comment.

stevvooe · 2018-01-18T19:03:17Z

LGTM

@sargun Do you have a way to run the unit tests against s3?

dmcgowan · 2018-01-18T19:23:46Z

registry/storage/catalog.go

@@ -40,22 +41,20 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri

 		// if we've filled our array, no need to walk any further
 		if len(foundRepos) == len(repos) {
-			return errFinishedWalk
+			finishedWalk = true


Can you also remove the errFinishedWalk variable

sargun · 2018-01-18T19:29:29Z

@stevvooe Yeah, I did. And also, I've run this against our registry for a bit.

dmcgowan · 2018-01-18T19:51:43Z

registry/storage/linkedblobstore.go

 			return err
 		}

 		return nil
 	})

-	if err != nil {
-		return err
+	// We check retErr first, in case the Walk function somehow mutates the error


If the error is mutated, wouldn't we still want the mutated error? I guess I am just trying to figure out if there is a good reason not to just return lbs.driver.Walk(....

dmcgowan · 2018-01-18T19:58:37Z

registry/storage/driver/walk.go

+// If the returned error from the WalkFn is ErrSkipDir and fileInfo refers
+// to a directory, the directory will not be entered and Walk
+// will continue the traversal.  If fileInfo refers to a normal file, processing stops
+func WalkFallback(ctx context.Context, driver StorageDriver, from string, f WalkFn) error {


I don't see why this and the implementation from registry/storage/walk.go must both exist. Can we kill one?

This change is primarily to make GC faster. Signed-off-by: Sargun Dhillon <[email protected]>

This removes the old global walk function, and changes all the code to use the per-driver walk functions. Signed-off-by: Sargun Dhillon <[email protected]>

sargun · 2018-01-18T21:32:50Z

@dmcgowan @stevvooe Fixed.

dmcgowan · 2018-01-18T21:36:30Z

LGTM

I would still like someone more familiar with the s3 driver to take a look and give an LGTM.

I am not a huge fan of overloading ErrSkipDir to different behavior based on whether the value is a file or directory. Not a big deal though, might be worth considering cleaning that up later and just adding a second error type (maybe to allow stopping traversal when a specific directory is seen).

sargun · 2018-01-18T21:46:02Z

Maybe @BrianBland or @RichardScothern?

dmcgowan · 2018-01-18T22:06:45Z

Maybe @BrianBland or @RichardScothern?

We could dream

Let me check to see if I can find anyone interested in taking a look, otherwise we have reached the LGTM threshold already 😄

BrianBland

Overall this change looks good to me.

BrianBland · 2018-01-19T06:16:56Z

registry/storage/driver/s3-aws/s3.go

+	return wi.FileInfoFields.IsDir
+}
+
+func (d *driver) doWalk(parentCtx context.Context, objectCount *int64, path, prefix string, f storagedriver.WalkFn) error {


This is pretty nit-picky, but it's generally better practice to return the objectCount and error values instead of mutating a pointer argument.

Thanks for taking a look! :D.
I think there's just the awkwardness of using a direct variable that there's no way to keep it across callbacks from ListObjectsV2PagesWithContext, so you need some pointer mutation at a minimum.

GordonTheTurtle added the dco/no label Nov 29, 2017

sargun force-pushed the make-walk-faster branch from 89eefd5 to e0557fd Compare November 29, 2017 23:56

GordonTheTurtle removed the dco/no label Nov 29, 2017

stevvooe changed the title ~~Make walk faster~~ s3: improve walk performance Dec 1, 2017

hinshun suggested changes Jan 4, 2018

View reviewed changes

sargun force-pushed the make-walk-faster branch 2 times, most recently from 34d4fd8 to b22b62c Compare January 4, 2018 17:42

sargun added 3 commits January 7, 2018 22:45

Upgrade AWS library to v1.12.36

9f66446

This upgrade, and vendors aws-sdk-go to version v1.12.36. This is because it has new API calls accessible to the S3 client, specifically S3.ListObjectsV2PagesWithContext Signed-off-by: Sargun Dhillon <[email protected]>

sargun force-pushed the make-walk-faster branch from b22b62c to b2b1560 Compare January 8, 2018 06:45

dmcgowan reviewed Jan 18, 2018

View reviewed changes

sargun force-pushed the make-walk-faster branch 2 times, most recently from b879326 to 37b1e8d Compare January 18, 2018 19:29

dmcgowan reviewed Jan 18, 2018

View reviewed changes

Use the new walk method for catalog enumeration

c7b0da2

This change is primarily to make GC faster. Signed-off-by: Sargun Dhillon <[email protected]>

sargun force-pushed the make-walk-faster branch 2 times, most recently from e885ec4 to a78edb3 Compare January 18, 2018 20:44

Remove old walk function

cbcbcb0

This removes the old global walk function, and changes all the code to use the per-driver walk functions. Signed-off-by: Sargun Dhillon <[email protected]>

sargun force-pushed the make-walk-faster branch from a78edb3 to cbcbcb0 Compare January 18, 2018 21:06

BrianBland reviewed Jan 19, 2018

View reviewed changes

dmcgowan merged commit 5cb406d into distribution:master Jan 19, 2018

sargun deleted the make-walk-faster branch January 19, 2018 22:03

hrak mentioned this pull request Mar 20, 2018

crash in s3-aws doWalk() since #2455 was merged #2553

Closed

MorrisLaw mentioned this pull request Jun 24, 2021

registry/cshoop/storagedriver Added StorageDriver method WalkFiles, optimize blobstore enumeration with S3 digitalocean/docker-distribution#16

Closed

s3: improve walk performance #2455

s3: improve walk performance #2455

Conversation

sargun commented Nov 29, 2017

GordonTheTurtle commented Nov 29, 2017

codecov bot commented Nov 30, 2017 • edited Loading

Codecov Report

sargun commented Nov 30, 2017

stevvooe commented Dec 1, 2017

sargun commented Dec 2, 2017

sargun commented Dec 13, 2017

sargun commented Dec 19, 2017

stevvooe commented Jan 3, 2018

sargun commented Jan 3, 2018

stevvooe commented Jan 3, 2018

hinshun left a 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

sargun Jan 4, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hinshun Jan 4, 2018 • edited Loading

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

sargun commented Jan 4, 2018

sargun commented Jan 8, 2018

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

stevvooe commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sargun commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sargun commented Jan 18, 2018

dmcgowan commented Jan 18, 2018

sargun commented Jan 18, 2018

dmcgowan commented Jan 18, 2018

BrianBland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 30, 2017 •

edited

Loading

sargun Jan 4, 2018 •

edited

Loading

hinshun Jan 4, 2018 •

edited

Loading