Arvados

First version ready for review at ea57684c255434bcd25ec150a3979ce783a2183c on branch 10477-upgrade-aws-s3-driver. Tests at developer-run-tests: #1967

New revision ready at 8f3b2dedef2677654197e9838939d9abe7cc3791 on branch 10477-upgrade-aws-s3-driver. This one has faster upload performance, because we don't use sha-256 anymore.

The benchmark results seem to me a bit out of place in the install guide. I wonder if it would be better to move these details to a separate page (maybe even wiki), and pare back the install guide to something along the lines of "driver A has had more production use, but driver B can improve read performance by 50-100%, see wiki for details"?

The name "AlternateDriver" seems a bit sketchy wrt the "never reuse config flag to mean something else" story. Perhaps something similar to the "alternate controller code path" flag, like "ExperimentalDriver10477: true"? Then, when this becomes the default, we'd drop this flag and add a flag like "UseOldGoamzDriver: true".

Uploader concurrency 5, uploader partsize 5 MiB, etc. should be defined as consts.

It seems like PutReader is only used to write empty objects for "trash" and "recent" markers, and it has a 3rd copy of the s3manager.NewUploaderWithClient() code that doesn't propagate context. Perhaps this could be cleaned up by replacing PutReader with

func (v *S3AWSVolume) writeObject(ctx context.Context, name string, r io.Reader) error { ... }

(If len(name)==32 then writeObject can re-encode it as base64 to set ContentMD5.)

Then, WriteBlock() would reduce to something like

r := NewCountingReader(...)
err := v.writeObject(ctx, loc, r)
if err != nil { return err }
return v.writeObject(ctx, "recent/"+loc, nil)

This comment (copied from old driver which had r=nil) needs to be updated to match the code (which presumably changed because r=nil didn't work). Aside: does bytes.NewReader(nil) work just as well?

+       if length == 0 {
+               // aws-sdk-go will only send Content-Length: 0 when reader
+               // is nil due to net.http.Request.ContentLength
+               // behavior.  Otherwise, Content-Length header is
+               // omitted which will cause some S3 services
+               // (including AWS and Ceph RadosGW) to fail to create
+               // empty objects.
+               r = bytes.NewReader([]byte{})

I'm not sure how to manage the copy-paste aspect here. A lot of stuff is copied from the goamz-based driver with slight modifications, and finding the differences is a bit of an exercise. I suppose the implication is that we'll be deleting the goamz driver very soon, because until then we'll be maintaining 2 divergent copies of a lot of stuff.

(*S3AWSVolume)check() should error out if V2Signature is true, since that's not supported (maybe also worth a mention in docs/config example).

This doesn't look right -- surely a base64-encoded md5 will never be "d41d8cd98f00b204e9800998ecf8427e"? -- is this an indication the special case isn't even needed?

+       contentMD5 = base64.StdEncoding.EncodeToString(md5)
+       // See if this is the empty block
+       if contentMD5 != "d41d8cd98f00b204e9800998ecf8427e" {
+               uploadInput.ContentMD5 = &contentMD5
+       }

Remove commented-out code:

+       //var contentMD5, contentSHA256 string

These look like they should be Debugf:

+               v.logger.Warnf("EmptyTrash: looking for trash marker %s with last modified date %s", *trash.Key, *trash.LastModified)
...
+               v.logger.Infof("HEEEEEEE trashT key: %s, type: %T val: %s, startT is %s", *trash.Key, trashT, trashT, startT)
...
+                       v.logger.Infof("HERE! trashT for %s is smaller than blobtrashlifetime: %s < %s", *trash.Key, startT.Sub(trashT), v.cluster.Collections.BlobTrashLifetime.Duration())
...
+               v.logger.Infof("HERE! trash.Key %s should have been deleted", *trash.Key)
...
+               v.logger.Infof("HERE! recent/%s should have been deleted", loc)

Various fmt.Printf() in tests would be better as c.Logf():

+               fmt.Printf("USING TIMESTAMP %s to write key %s", t, key)

The arg to Unmarshal here can be v rather than &v since v is already a pointer:

+       v := &S3Volume{cluster: cluster, volume: volume, metrics: metrics}
+       err := json.Unmarshal(volume.DriverParameters, &v)

@ 8f3b2dede, 3 tests are failing. Haven't investigated further.

Tom Clegg wrote:

The benchmark results seem to me a bit out of place in the install guide. I wonder if it would be better to move these details to a separate page (maybe even wiki), and pare back the install guide to something along the lines of "driver A has had more production use, but driver B can improve read performance by 50-100%, see wiki for details"?

Sure, done.

The name "AlternateDriver" seems a bit sketchy wrt the "never reuse config flag to mean something else" story. Perhaps something similar to the "alternate controller code path" flag, like "ExperimentalDriver10477: true"? Then, when this becomes the default, we'd drop this flag and add a flag like "UseOldGoamzDriver: true".

I've changed the flag to "UseAWSS3v2Driver". Including the ticket number would make this too unwieldy imo. The risk of future reuse of that flag seems low.

Uploader concurrency 5, uploader partsize 5 MiB, etc. should be defined as consts.

Sure, done.

It seems like PutReader is only used to write empty objects for "trash" and "recent" markers, and it has a 3rd copy of the s3manager.NewUploaderWithClient() code that doesn't propagate context. Perhaps this could be cleaned up by replacing PutReader with

[...]

(If len(name)==32 then writeObject can re-encode it as base64 to set ContentMD5.)

Then, WriteBlock() would reduce to something like

Yeah, it is much better refactored like that, thanks.

This comment (copied from old driver which had r=nil) needs to be updated to match the code (which presumably changed because r=nil didn't work).

r=nil still does not work; I've updated the comment along those lines. Could be improved still.

Aside: does bytes.NewReader(nil) work just as well?

Yes, bytes.NewReader(nil) does work as well; I had used the other form since that's what they did in the AWS driver doc. Is bytes.NewReader(nil) preferred?

I'm not sure how to manage the copy-paste aspect here. A lot of stuff is copied from the goamz-based driver with slight modifications, and finding the differences is a bit of an exercise.
I suppose the implication is that we'll be deleting the goamz driver very soon, because until then we'll be maintaining 2 divergent copies of a lot of stuff.

Yeah, that's what I was thinking. We have pretty good test coverage, and Kubernetes switched cold-turkey a few years ago so it can be done. I propose to make the new driver optional in 2.1, default in 2.2, and the old one removed in 2.3.

(*S3AWSVolume)check() should error out if V2Signature is true, since that's not supported (maybe also worth a mention in docs/config example).

Yes good catch, I've added an error if V2Signature is true. The docs already mention that V2Signature is not supported, at the bottom of the "Configure S3 object storage" page where this driver is introduced.

This doesn't look right -- surely a base64-encoded md5 will never be "d41d8cd98f00b204e9800998ecf8427e"? -- is this an indication the special case isn't even needed?

You are right, I've run the tests and did some more real-life testing as well, the special case is not needed. I've removed it.

Remove commented-out code:

Done.

These look like they should be Debugf:

They shouldn't even be there, I've removed them (and some other leftovers).

Various fmt.Printf() in tests would be better as c.Logf():

I removed those too, they didn't really serve a purpose anymore.

The arg to Unmarshal here can be v rather than &v since v is already a pointer:

Fixed. Same thing in the s3_volume.go driver so I fixed that too.

@ 8f3b2dede, 3 tests are failing. Haven't investigated further.

Hmm, all the Keepstore tests are passing for me locally. I've kicked off a jenkins test run at developer-run-tests: #1973

Ready for another look at 95babd9e21eb871eed9535fad3d2af8ecdeb471d on branch 10477-upgrade-aws-s3-driver

changed the flag to "UseAWSS3v2Driver"

Yeah, that seems safer.

r=nil still does not work; I've updated the comment along those lines. Could be improved still.

Makes much more sense with the new comment, thanks.

Yes, bytes.NewReader(nil) does work as well; I had used the other form since that's what they did in the AWS driver doc. Is bytes.NewReader(nil) preferred?

One less memory allocation. Wouldn't make a huge difference here, it's just a good habit. Generally, anything that takes a slice/map (and isn't expected to write to it) can take a nil slice/map. len() and range work, map lookups return zero value / not found, etc.

The arg to Unmarshal here can be v rather than &v since v is already a pointer:

Fixed. Same thing in the s3_volume.go driver so I fixed that too.

Ah, nice. Copy&paste&review win. :D

@ 8f3b2dede, 3 tests are failing. Haven't investigated further.

Hmm, all the Keepstore tests are passing for me locally. I've kicked off a jenkins test run at developer-run-tests: #1973

Ah. The tests that fail are all timestamp-related. Seems like a timezone thing -- try running tests with TZ=EDT. Could be a testing-only problem, but worth fixing either way.

FAIL: s3aws_volume_test.go:76: StubbedS3AWSSuite.TestGeneric

s3aws_volume_test.go:77:
    DoGenericVolumeTests(c, false, func(t TB, cluster *arvados.Cluster, volume arvados.Volume, logger logrus.FieldLogger, metrics *volumeMetricsVecs) TestableVolume {
        // Use a negative raceWindow so s3test's 1-second
        // timestamp precision doesn't confuse fixRace.
        return s.newTestableVolume(c, cluster, volume, metrics, -2*time.Second)
    })
volume_generic_test.go:438:
    t.Errorf("got %d for TestHash timestamp, expected %d <= t <= %d",
        mtime, minMtime, maxMtime)
... Error: got 1596128123118000000 for TestHash timestamp, expected 1596142523000000000 <= t <= 1596142524000000000

"got 1596128123118000000" is 4h earlier than expected, and my TZ is UTC-4.

Tom Clegg wrote:

Ah. The tests that fail are all timestamp-related. Seems like a timezone thing -- try running tests with TZ=EDT. Could be a testing-only problem, but worth fixing either way.

[...]

"got 1596128123118000000" is 4h earlier than expected, and my TZ is UTC-4.

This was reproducible by having /etc/timezone set to something other than UTC, and having TZ unset. The fix is in ba2e24710fb7c6d8236f81ee79ca30ca7dcbcf9c.

Nit: better to put the "always send UTC to gofakes3" rule in the s3AWSFakeClock code, instead of relying on the caller. This works for me:

diff --git a/services/keepstore/s3aws_volume_test.go b/services/keepstore/s3aws_volume_test.go
index 33e671f3b..97045a660 100644
--- a/services/keepstore/s3aws_volume_test.go
+++ b/services/keepstore/s3aws_volume_test.go
@@ -43,7 +43,7 @@ func (c *s3AWSFakeClock) Now() time.Time {
        if c.now == nil {
                return time.Now().UTC()
        }
-       return *c.now
+       return c.now.UTC()
 }

 func (c *s3AWSFakeClock) Since(t time.Time) time.Duration {
@@ -336,7 +336,7 @@ func (s *StubbedS3AWSSuite) TestBackendStates(c *check.C) {
                }
        }

-       t0 := time.Now().UTC()
+       t0 := time.Now()
        nextKey := 0
        for _, scenario := range []struct {
                label               string

Tom Clegg wrote:

Nit: better to put the "always send UTC to gofakes3" rule in the s3AWSFakeClock code, instead of relying on the caller. This works for me:

[...]

Cool, I've rebased like that in 5771cf273a4e09a5666122a7f67b4f088927e29d

Provisional migration plan: behind feature flag on 2.1; default in 2.2; remove old driver in 2.3.