store/s3: try to guess size to enable multipart uploads #617

adrien-f · 2018-11-06T16:02:00Z

Signed-off-by: Adrien Fillon adrien.fillon@cdiscount.com

I noticed an issue where a S3 multipart upload would fail with a 413 Entity Too Large error. After investigating, I noticed we were not sending the size of the buffer/chunk/file/... to the minio-go library and that would cause the multipart upload to be a single part upload.

From what I've seen, the Upload method is only called with a os.File object : https://github.com/improbable-eng/thanos/blob/a09d41dd6534f28365eede43fd024fed3dcc0750/pkg/objstore/objstore.go#L80

I'm currently not satisfied with what I did but it works ! I open this PR so we can figure out the best way to handle this. In case of real buffers, to compute their size I would need to duplicate it in memory from what I read.

Changes

s3: Send correct file size to support multipart uploads

Verification

Deployed the patched binary on our production clusters, uploads started again.

For your information, we use Ceph as our S3 provider.

bwplotka · 2018-11-26T23:48:37Z

I think we had some discussion offline on this. Definitely this PR is wrong as it will panic if io.Reader is not os.File.

So we have 3 options:

extend interface with sizeBytes int64 argument.
try to cast reader into os.File safely.
ask minio maintainers to not make any assumptions based on size - the same way as GCS implementation does.

As the 3rd option is most likely long and potentially not possible due to s3 API limitations and taking into consideration fact that in most cases we have os.File passed as reader I think option 1 makes sense...

Option 2 is too hidden and it makes assumptions that are easy to miss.

Thoughts @domgreen?

adrien-f · 2018-12-03T14:43:16Z

I went with option 1. I guess that should help with #467 @jojohappy ?

bwplotka

Yes, @jojohappy would be unblocked.

It's interface change so it's quite major - affects all implementations, let's get some feedback from more people before merging.

From my side LGTM, just minor nits.

bwplotka · 2018-12-03T19:55:17Z

pkg/objstore/objstore.go

+ fileSize := int64(-1)
+ fileInfo, err := r.Stat()
+ if err != nil {
+ return errors.Wrapf(err, "Could not stat file %s", src)


errors should not be capitalized (as they will be wrapped)

bwplotka · 2018-12-03T19:55:37Z

pkg/objstore/objstore.go

+ fileInfo, err := r.Stat()
+ if err != nil {
+ return errors.Wrapf(err, "Could not stat file %s", src)
+ } else {


no need for this else as on error we reurn

bwplotka · 2018-12-03T19:58:15Z

pkg/objstore/swift/swift.go

@@ -154,7 +154,7 @@ func (c *Container) IsObjNotFoundErr(err error) bool {
 }

 // Upload writes the contents of the reader as an object into the container.
-func (c *Container) Upload(ctx context.Context, name string, r io.Reader) error {
+func (c *Container) Upload(ctx context.Context, name string, r io.Reader, sizeBytes int64) error {


Please do _ for this int64 to indicate this one is not used.

Also ping @sudhi-vm - maybe @sudhi-vm has quick idea how to use this additional info nicely. (:

bwplotka · 2018-12-03T19:59:45Z

pkg/objstore/objtesting/acceptance_e2e_test.go

+ testutil.Ok(t, bkt.Upload(context.Background(), "id1/obj_2.some", testDataReader2, testDataReader2.Size()))
+ testutil.Ok(t, bkt.Upload(context.Background(), "id1/obj_3.some", testDataReader3, testDataReader3.Size()))
+ testutil.Ok(t, bkt.Upload(context.Background(), "id2/obj_4.some", testDataReader4, testDataReader4.Size()))
+ testutil.Ok(t, bkt.Upload(context.Background(), "obj_5.some", testDataReader5, testDataReader5.Size()))


I wonder if it is a good test to actually try to... put wrong size (too small or too big) to ensure we have a knowledge how each provider handles this scenario.. What do you think? Might be not worthy.

bwplotka · 2018-12-03T20:01:14Z

pkg/objstore/inmem/inmem.go

@@ -124,7 +124,7 @@ func (b *Bucket) Exists(_ context.Context, name string) (bool, error) {
 }

 // Upload writes the file specified in src to into the memory.
-func (b *Bucket) Upload(_ context.Context, name string, r io.Reader) error {
+func (b *Bucket) Upload(_ context.Context, name string, r io.Reader, sizeBytes int64) error {


bwplotka · 2018-12-03T20:01:27Z

pkg/objstore/gcs/gcs.go

@@ -124,7 +124,7 @@ func (b *Bucket) Exists(ctx context.Context, name string) (bool, error) {
 }

 // Upload writes the file specified in src to remote GCS location specified as target.
-func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader) error {
+func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader, sizeBytes int64) error {


I am pretty sure we can reuse this knowledge, but you can leave that to others. Just please use _ for arg name that is not used.

bwplotka · 2018-12-03T20:02:14Z

pkg/objstore/azure/azure.go

@@ -211,7 +211,7 @@ func (b *Bucket) Exists(ctx context.Context, name string) (bool, error) {
 }

 // Upload the contents of the reader as an object into the bucket.
-func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader) error {
+func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader, sizeBytes int64) error {


@vglafirov is this something we can use in azure?

If not @adrien-f let's make it _ as well

jojohappy

LGTM!

But I shouldn't know the size of Reader. Maybe the client should be handled that will be the best solution. Just my opinion.

Anyway, thank you for your PR. 👍

bwplotka · 2018-12-04T10:25:25Z

@jojohappy

But I shouldn't know the size of Reader. Maybe the client should be handled that will be the best solution. Just my opinion.

What do you mean? I means who? Consumers of the interfaces sets the size now.

jojohappy · 2018-12-04T10:49:29Z

@bwplotka sorry for my poor English, I means the consumer, and IMO, it should not give the size of Reader to the object storage client particularly, I mean I like the 3rd option.

bwplotka · 2018-12-04T11:08:58Z

Well but for COS you need size as well? Otherwise it does not work either: https://github.com/improbable-eng/thanos/pull/467/files#r209909773

jojohappy · 2018-12-04T11:29:49Z

Yes, actually the 1st option will solve the problem now and make sence. I accept it and will ask the cos-client maintainer to not make the size sensitively.

domgreen

I agree option 1 is better than the other options at the moment, my main concern is extending the interface with extra parameters for the func that are not used by other implementations. Doesnt feel to clean tbh.

If we start adding more parameters we would want to make this into an uploadOpts that deal with all these extra pesky options for different providers, but lets wait till that becomes an issue.

Would be good to ensure that the maintainers of each provider know of this change and if it optimises uploads for them make correlating PRs.

domgreen

My other idea was to cast it to a *File like in the COS implementation but again that doesnt feel like the best solution to the problem due recasting etc. not sure how much overhead this has in go.

brancz

I'm not sure it makes sense to specify this size on the upload interface. I feel attempting to cast to a file and or doing an intermediate buffer copy might be better than leak this into the interface.

brancz · 2018-12-05T12:49:00Z

pkg/objstore/objstore.go

@@ -83,7 +83,14 @@ func UploadFile(ctx context.Context, logger log.Logger, bkt Bucket, src, dst str
 }
 defer runutil.CloseWithLogOnErr(logger, r, "close file %s", src)

- if err := bkt.Upload(ctx, dst, r); err != nil {
+ fileSize := int64(-1)


Maybe I'm missing something, but this initialization doesn't seem to be necessary as the Stat call must succeed.

Yes, let's remove it

domgreen · 2018-12-05T14:41:36Z

The COS implementation does this here https://github.com/improbable-eng/thanos/pull/467/files#r209909773
I tend to agree not extending the interface feels better and therefore only the needed implementations do this extra step. Also from my quick look type assertion does not seem to be overly expensive in Go.

bwplotka · 2018-12-05T18:06:14Z

It's not about cost of type assertion - it's marginal.

It's about fact that provider has different logic or even fails if the passed io.Reader is NOT os.File. The fact that implmentation assumes os.File is wrong as our interface contract does not mention it. We expect user to pass any io.Reader.

So in future someone might add usage that will wrap *os.File or like we do now pass bytes.Buffer (like we do in unit tests). This is literally abstraction leak, as implementation poses limitation that cause unexpected side effects (multipart disabled for mino or failure for COS).

More details why this problem can be categorized as abstraction leak: https://medium.com/@fagnerbrack/repairing-the-leaky-abstraction-e726baab91b5

So there is no other way than either:

Expose leak/limitation and be open about this in form of passing the size. Mentioning that io.Reader has to be os.File in docs is clearly not enough (also we don't need whole file, just size int64, and we want abstraction to be as thin as possible)
Guess size by doing buffering as @brancz suggested. Quite complex for us to do it on top of minio or COS, but doable. Worth to discuss this with authors of packages we use for problematic providers.

For example, GCS package we use is doing 2, that's why it does not need preknown size.

bwplotka · 2018-12-07T11:02:47Z

any friday thoughts @domgreen @brancz ^^

stefan-improbable · 2018-12-07T14:02:18Z

@bwplotka @adrien-f can we change the interface to Upload(ctx context.Context, name string, r os.File) error or do we really need that io.Reader there?

bwplotka · 2018-12-07T18:10:10Z

@stefan-improbable Why is that better? Also passing os.File would be violation of idiomatic "Accept interfaces, return struct" guideline. Well explained with actually similar example here: http://idiomaticgo.com/post/best-practice/accept-interfaces-return-structs/

Also, implementation does not need os.File just io.Reader and size. So rather ReaderSize interface would be something slightly better here, but it would be more difficult to use than io.Reader and just single int size at the end.

brancz · 2018-12-08T20:16:30Z

I think we should consult the minio people, as to me it seems that this should be possible without a change in the interface, looking at their internal handling, this should actually work, as it should attempt to perform multi part on a best effort basis: https://github.com/minio/minio-go/blob/70799fe8dae6ecfb6c7d7e9e048fce27f23a1992/api-put-object.go#L157

(mind you I'm well jetlagged right now so may very well be missing something)

adrien-f · 2018-12-09T14:32:25Z

I think we should consult the minio people, as to me it seems that this should be possible without a change in the interface, looking at their internal handling, this should actually work, as it should attempt to perform multi part on a best effort basis: https://github.com/minio/minio-go/blob/70799fe8dae6ecfb6c7d7e9e048fce27f23a1992/api-put-object.go#L157

(mind you I'm well jetlagged right now so may very well be missing something)

While digging this issue, I looked into this piece of code I got to https://github.com/minio/minio-go/blob/70799fe8dae6ecfb6c7d7e9e048fce27f23a1992/api-put-object.go#L184 then https://github.com/minio/minio-go/blob/2508a98a820e993b0307bbac4db9731e4e6f89a0/api-put-object-common.go#L72 that goes with this:

        // object size is '-1' set it to 5TiB.
	if objectSize == -1 {
		objectSize = maxMultipartPutObjectSize
	}

The following issue minio/minio-go#848 (comment) propose an alternative solution that involves manually uploading the blob parts by parts. Adding such a relative complexity to Thanos felt unneeded.

stefan-improbable · 2018-12-10T10:15:16Z

@bwplotka

@stefan-improbable Why is that better? Also passing os.File would be violation of idiomatic "Accept interfaces, return struct" guideline. Well explained with actually similar example here: http://idiomaticgo.com/post/best-practice/accept-interfaces-return-structs/

Passing reader and length seems a bit weird, but I don't have any other argument. Possibly just that all callers will have to calculate size as opposed to it being calculated in the one AWS-specific function. As for the accept-interfaces-and-return-structs, you can open the file for reading/writing only and test it using ioutil.TempFile.
But in the end, the reader+size is not too bad and solves our current problem, so I'm happy to go with it.

adrien-f · 2018-12-12T12:59:59Z

Any last minute changes I should make ?

bwplotka

Yes @adrien-f there are minor outstanding comment.

So this is difficult decision. I totally agree with @brancz here that even if some magic is needed from our side to use mino library in bit more complicated way that deduce what upload operation it should use - it is worth in long term.

Additionally again, COS Tencent is depending on lenght as well: #467 and this might be something that @jojohappy could help us with longterm to put issues on https://github.com/mozillazg/go-cos (we don't know Chineese well) to make that happen/figure out how to use the library without pre known lenght.

I see this change as quick mitigation that does not hurt us in mid term but we should aim for having bucket providers that support working without known lenght for the sake of good patterns and need for streamed upload in future.

I am adding 2 issues on those two providers:
#678
#679

bwplotka · 2018-12-13T14:22:56Z

pkg/objstore/objstore.go

@@ -22,7 +22,7 @@ type Bucket interface {
 BucketReader

 // Upload the contents of the reader as an object into the bucket.
- Upload(ctx context.Context, name string, r io.Reader) error
+ Upload(ctx context.Context, name string, r io.Reader, sizeBytes int64) error


Let's comment properly here, why this has size and reader interface. Worth to mention PR #617 as we have reference discussion why etc.

I would also add a comment saying TODO(PR #617): Consider removing requirement on pre-known length when all providers will support this.

bwplotka · 2018-12-13T14:23:13Z

pkg/objstore/objstore.go

@@ -83,7 +83,14 @@ func UploadFile(ctx context.Context, logger log.Logger, bkt Bucket, src, dst str
 }
 defer runutil.CloseWithLogOnErr(logger, r, "close file %s", src)

- if err := bkt.Upload(ctx, dst, r); err != nil {
+ fileSize := int64(-1)


Yes, let's remove it

bwplotka · 2018-12-13T14:41:03Z

@adrien-f Can you address small comments and change title/commit title to exclude WIP prefix

bwplotka · 2018-12-13T14:45:31Z

@adrien-f And we need CHANGELOG maybe as this loosens our requirements towards client provider implementations (if someone wants to add one)?

domgreen · 2018-12-14T09:58:38Z

Does the update of the COS API effect this discussion? #679

I am happy to add the int to the interface to unblock and gain perf improvements however, think that we may have to revisit the interface in the future so it is more future proof.

domgreen · 2018-12-14T10:05:21Z

Re CHANGELOG ... I'm not sure we want to update as this is an internal cahnge that will not effect users really only provider devs.

To further followup on my previous comment now it is only minio that needs the size ... maybe we treat this as the special case and do the casting? Sorry to keep throwing the cat amongst the pigeons.

jojohappy · 2018-12-14T10:12:17Z

I have make a test and confirmed that the new version of COS provider had been supported uploading files without the length.

adrien-f · 2018-12-14T15:03:52Z

I made the last requested changes I think.
I'll be more than happy to help work on the transition to go-cloud 😄
Thanks everyone for the review and the feedback, I realize this change isn't the best solution, and nothing is more frustrating than this situation. I hope to be able to resolve this soon durably.

bwplotka · 2018-12-14T15:11:05Z

Oh boy, COS news means that this interface change is indeed weird.

Thanks for doing all this work @adrien-f and I feel like in this case casting to os.File and warn if we cannot do it so multipart upload will be disabled (and mention #678) is proper direction here as a mitigation.

Even if we fail to cast it s3 will still work. The COS was different as this was a blocker for it (but not anymore)

Thanks all for valuable discussion! (:

adrien-f · 2018-12-14T15:23:45Z

There you go !

bwplotka · 2018-12-14T15:32:35Z

pkg/objstore/s3/s3.go

+ // TODO(PR #617): Consider removing requirement on pre-known length when all providers will support this.
+ fileInfo, err := r.(*os.File).Stat()
+ if err != nil {
+ return errors.Wrapf(err, "could not stat local file %s", name)


Well, we agreed for best effort logic, so we need warning instead I think, right?

My bad. I really need holidays 🤕

bwplotka

Thanks!

This was referenced Dec 3, 2018

Object Storage: Support tencent cloud COS provider #467

Merged

To use real temporary block file for bucket e2e testing #600

Closed

adrien-f force-pushed the feature/s3_multipart branch from e40f6cb to bec24ab Compare December 3, 2018 14:39

bwplotka reviewed Dec 3, 2018

View reviewed changes

jojohappy reviewed Dec 4, 2018

View reviewed changes

domgreen reviewed Dec 5, 2018

View reviewed changes

brancz reviewed Dec 5, 2018

View reviewed changes

bwplotka requested a review from stefan-improbable December 7, 2018 11:38

stefan-improbable approved these changes Dec 10, 2018

View reviewed changes

adrien-f force-pushed the feature/s3_multipart branch from 67d62d5 to 876e122 Compare December 10, 2018 16:02

This was referenced Dec 13, 2018

s3/aliyunOSS: Support multipart upload without pre known object size. #678

Open

cos: Provider is not depent on preknown object size for upload. #679

Closed

bwplotka approved these changes Dec 13, 2018

View reviewed changes

adrien-f force-pushed the feature/s3_multipart branch from 876e122 to 3d55401 Compare December 14, 2018 15:01

adrien-f changed the title ~~[WIP] Support S3 multipart uploads~~ store: support S3 multipart uploads by passing the fileSize down the interface Dec 14, 2018

adrien-f force-pushed the feature/s3_multipart branch from 3d55401 to 5c35b8e Compare December 14, 2018 15:23

adrien-f changed the title ~~store: support S3 multipart uploads by passing the fileSize down the interface~~ store/s3: try to guess size to enable multipart uploads Dec 14, 2018

bwplotka requested changes Dec 14, 2018

View reviewed changes

store/s3: try to guess size to enable multipart uploads

66093ad

adrien-f force-pushed the feature/s3_multipart branch from 5c35b8e to 66093ad Compare December 14, 2018 16:04

bwplotka approved these changes Dec 14, 2018

View reviewed changes

bwplotka merged commit 26d4170 into thanos-io:master Dec 14, 2018

adrien-f mentioned this pull request Dec 24, 2018

Compact: thanos 0.1.0 compact only PUT got 403 #682

Closed

store/s3: try to guess size to enable multipart uploads #617

store/s3: try to guess size to enable multipart uploads #617

Conversation

adrien-f commented Nov 6, 2018

Changes

Verification

bwplotka commented Nov 26, 2018

adrien-f commented Dec 3, 2018

bwplotka 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

jojohappy left a comment

Choose a reason for hiding this comment

bwplotka commented Dec 4, 2018

jojohappy commented Dec 4, 2018

bwplotka commented Dec 4, 2018

jojohappy commented Dec 4, 2018

domgreen left a comment

Choose a reason for hiding this comment

domgreen left a comment

Choose a reason for hiding this comment

brancz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domgreen commented Dec 5, 2018

bwplotka commented Dec 5, 2018 • edited Loading

bwplotka commented Dec 7, 2018

stefan-improbable commented Dec 7, 2018

bwplotka commented Dec 7, 2018

brancz commented Dec 8, 2018 • edited Loading

adrien-f commented Dec 9, 2018

stefan-improbable commented Dec 10, 2018

adrien-f commented Dec 12, 2018

bwplotka 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

bwplotka commented Dec 13, 2018

bwplotka commented Dec 13, 2018

domgreen commented Dec 14, 2018

domgreen commented Dec 14, 2018

jojohappy commented Dec 14, 2018

adrien-f commented Dec 14, 2018

bwplotka commented Dec 14, 2018 • edited Loading

adrien-f commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Dec 5, 2018 •

edited

Loading

brancz commented Dec 8, 2018 •

edited

Loading

bwplotka commented Dec 14, 2018 •

edited

Loading