Initial work on s3 blobstore #1863

jontro · 2019-05-16T14:56:40Z

I've created an initial implementation of the blobstore interface for s3.

Some notes

Credentials are assumed to be configured according to the section in "Configuring Credentials" https://docs.aws.amazon.com/sdk-for-go/api/
All buckets have to be in the same region, it would maybe make sense to be able to configure this per bucket

Is there an easy way to test archival of a workflow in a development environment?

andrewjdawson2016

Thanks for taking this task on. I have left comments on your review. One comment that I did not leave in code is it would be awesome if there was a way to run this locally.

For example a local docker image running S3 that we could connect to. Then we can include config in development.yaml to run this S3 based implementation on local dev.

common/blobstore/s3store/client.go

common/blobstore/s3store/config.go

andrewjdawson2016 · 2019-05-16T18:48:13Z

common/blobstore/s3store/config.go

+	// Config describes the configuration needed to construct a blobstore client backed by file system
+	Config struct {
+		Region        string         `yaml:"region"`
+		DefaultBucket BucketConfig   `yaml:"defaultBucket"`


The reason filestore has DefaultBucket and CustomBucket is on start up it actually creates these buckets. But in the case of S3 blobstore we won't be doing that. There should not be any DefaultBucket or CustomBuckets as part of this config. S3 blobstore should just know how to take a bucket name in its public APIs and attempt to operate on that bucket.

What do you think about the use case of having different buckets in different regions? Or is one sufficient? Doing this lookup on demand is quite expensive but could of course be cached if so.

Removed it for now

common/blobstore/s3store/config.go

common/service/config/config.go

There are still some items remaining to be fixed

common/blobstore/s3store/client.go

common/blobstore/s3store/config_test.go

andrewjdawson2016

Looks better. Thanks for working on this.

common/blobstore/s3store/client.go

common/blobstore/s3store/README.md

andrewjdawson2016 · 2019-05-17T16:33:30Z

common/blobstore/s3store/client.go

+)
+
+var (
+	// ErrListFiles could not list files


Both of these are common error types between filestore and s3 store. Let's factor these out and put them in interface.go. Can you also remove remove these error types from filestore. That way we only have these errors in one place.

common/blobstore/s3store/client.go

andrewjdawson2016 · 2019-05-17T17:07:17Z

common/blobstore/s3store/client_test.go

+*/
+
+func (s *ClientSuite) TestUploadDownload_Success_CustomBucket() {
+


Remove extra enter

andrewjdawson2016 · 2019-05-17T17:10:19Z

common/blobstore/s3store/client_test.go

+	downloadBlob, err := client.Download(context.Background(), customBucketName, key)
+	s.NoError(err)
+	s.NotNil(downloadBlob)
+	s.assertBlobEquals(map[string]string{}, "blob body", downloadBlob)


Can we include some tests with non-empty tags?

Added this, though maybe this whole test should be removed as a custom bucket name isn't anything special with the s3store any more

Yes agreed. Let's remove.

common/blobstore/s3store/config_test.go

common/blobstore/s3store/README.md

cmd/server/server.go

Alaways return original aws error so it can be retried if needed.

jontro · 2019-05-20T21:39:44Z

Hopefully I have fixed all feedback and left comments otherwise. One thing, I can't see why the tests are failing, locally the unit tests I've touched passes.

andrewjdawson2016

Looking good. I have a few small nits then let's get this landed. Thanks for the great work on this.

andrewjdawson2016 · 2019-05-21T17:02:01Z

Gopkg.toml

@@ -150,3 +150,8 @@ ignored = ["github.com/uber/cadence/.gen"]
 [[constraint]]
  name = "github.com/valyala/fastjson"
  version = "1.4.1"
+


Extra enter.

Fixed, somehow merging with master this automatically. Good find!

andrewjdawson2016 · 2019-05-21T17:10:54Z

common/blobstore/s3store/client.go

+	return tags, nil
+}
+
+// Exists returns false when the bucket does not exist or the bucket and key does not exist


Filestore currently returns error if bucket does not exist (e.g. bucket not existing is an error but key not existing is just false, nil) I think both APIs are reasonable but I want them to be the same. Lets change this to return an error if bucket does not exist.

Problem is it would be more inefficient. I.e. I would need to call a different api method. The same error is returned from the api whether or not the bucket exists. What do you think?

andrewjdawson2016 · 2019-05-21T17:11:46Z

common/blobstore/s3store/client.go

+	})
+	if err != nil {
+		aerr, ok := err.(awserr.Error)
+		if ok && (aerr.Code() == "NotFound") {


Why do we need this hardcoded string? Is there no error code or constant in s3 lib we can check against?

andrewjdawson2016 · 2019-05-21T17:13:22Z

common/blobstore/s3store/client.go

+	}
+	retentionDays := 0
+	for _, v := range lifecycleResults.Rules {
+		if *v.Status == "Enabled" && retentionDays < int(*v.Expiration.Days) {


Any S3 library constant we can use instead of this hardcoded string?

andrewjdawson2016 · 2019-05-21T17:26:18Z

common/blobstore/s3store/client.go

+		return nil, err
+	}
+	retentionDays := 0
+	for _, v := range lifecycleResults.Rules {


Are we iterating through these rules in chronological order? If so will retention days always be correct at the end of this loop? The way I am reading this code is we select the highest Expiration.Days that has ever been seen even if that is higher than the current Expiration.Days? Maybe I am just misunderstanding these APIs?

andrewjdawson2016 · 2019-05-21T17:26:59Z

common/blobstore/s3store/client.go

+}
+
+func (c *client) IsRetryableError(err error) bool {
+	return c.isStatusCodeRetryable(err) || request.IsErrorRetryable(err) || request.IsErrorThrottle(err)


common/blobstore/s3store/client.go

andrewjdawson2016 · 2019-05-21T17:30:03Z

common/blobstore/s3store/client_test.go

+	downloadBlob, err = client.Download(context.Background(), defaultBucketName, key)
+	s.NoError(err)
+	s.NotNil(downloadBlob)
+	s.assertBlobEquals(map[string]string{"key": "value"}, "body version 2", downloadBlob)


andrewjdawson2016 · 2019-05-21T17:30:38Z

common/blobstore/s3store/client_test.go

+	downloadBlob, err := client.Download(context.Background(), customBucketName, key)
+	s.NoError(err)
+	s.NotNil(downloadBlob)
+	s.assertBlobEquals(map[string]string{}, "blob body", downloadBlob)


Yes agreed. Let's remove.

andrewjdawson2016

Awesome.

jontro added 2 commits May 16, 2019 17:14

Initial work on s3 blobstore

88185f2

Fix failing tests

a74a394

jontro force-pushed the s3-blobstore branch from 28c40d2 to a74a394 Compare May 16, 2019 15:14

venkat1109 requested a review from andrewjdawson2016 May 16, 2019 16:49

andrewjdawson2016 suggested changes May 16, 2019

View reviewed changes

jontro added 2 commits May 16, 2019 23:13

S3 blobstore - Style issues and adjustments from feedback

d944c15

There are still some items remaining to be fixed

Add missing error handling

547e2ef

shreyassrivatsan reviewed May 16, 2019

View reviewed changes

common/blobstore/s3store/client.go Show resolved Hide resolved

common/blobstore/s3store/config_test.go Show resolved Hide resolved

Add instructions on how to setup an local s3 server

eecdf81

andrewjdawson2016 suggested changes May 17, 2019

View reviewed changes

jontro added 15 commits May 18, 2019 01:10

Ensure context times out. Add retryable errors.

aa37719

Alaways return original aws error so it can be retried if needed.

Cleanup tests

ec25467

Fix potential null pointer when fetching owner

f729ce0

Disallow configuration of both blobstores

11d9a5d

Expose s3 interface in NewClient

48411a5

Rewrite mocking interface using mockery

272f691

Do not delete after upload in test

0bd4cb5

Add test for non-empty tags

29feefd

Retry logic fixes

08ebc42

Factor out common errors in filestore and s3store

6260840

Gofmt

0ed855f

Import order

5ac78f9

Fix compiler error

ea58480

Add s3store example configuration

c13ebd6

Merge remote-tracking branch 'origin/master' into s3-blobstore

c0ce567

andrewjdawson2016 reviewed May 21, 2019

View reviewed changes

jontro added 3 commits May 21, 2019 23:18

Remove test and use s3 constant for ExpirationStatus

d6ac519

Add license

8bd44fa

Make sure GetTags returns ErrBucketNotExists and ErrBlobNotExists

ddc7b8f

jontro added 2 commits May 22, 2019 00:10

Make sure Exists returns ErrCodeNoSuchBucket

73ceae4

Factor out common occurences of "NotFound" error

c666a20

andrewjdawson2016 approved these changes May 21, 2019

View reviewed changes

Merge branch 'master' into s3-blobstore

86cd169

andrewjdawson2016 merged commit ffa6a99 into uber:master May 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial work on s3 blobstore #1863

Initial work on s3 blobstore #1863

jontro commented May 16, 2019 •

edited

Loading

andrewjdawson2016 left a comment

andrewjdawson2016 May 16, 2019

jontro May 16, 2019

jontro May 17, 2019

andrewjdawson2016 left a comment

andrewjdawson2016 May 17, 2019

andrewjdawson2016 May 17, 2019

andrewjdawson2016 May 17, 2019

jontro May 20, 2019

andrewjdawson2016 May 21, 2019

jontro commented May 20, 2019

andrewjdawson2016 left a comment

andrewjdawson2016 May 21, 2019

jontro May 21, 2019

andrewjdawson2016 May 21, 2019

jontro May 21, 2019

andrewjdawson2016 May 21, 2019

andrewjdawson2016 May 21, 2019

jontro May 21, 2019

andrewjdawson2016 May 21, 2019

andrewjdawson2016 May 21, 2019

andrewjdawson2016 May 21, 2019

andrewjdawson2016 May 21, 2019

andrewjdawson2016 left a comment

		*/

		func (s *ClientSuite) TestUploadDownload_Success_CustomBucket() {

Initial work on s3 blobstore #1863

Initial work on s3 blobstore #1863

Conversation

jontro commented May 16, 2019 • edited Loading

andrewjdawson2016 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

andrewjdawson2016 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jontro commented May 20, 2019

andrewjdawson2016 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

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

andrewjdawson2016 left a comment

Choose a reason for hiding this comment

jontro commented May 16, 2019 •

edited

Loading