ocdav: upload file to storage provider after assembling chunks #1253

ishank011 · 2020-10-16T16:52:50Z

Closes #860

labkode · 2020-10-19T09:43:31Z

@ishank011 let me know when is non wip

ishank011 · 2020-10-21T10:29:53Z

@labkode @butonic the reason for the memory leaks in tus uploads was that the tus client expects an io.ReadSeeker object to be passed but accepts io.Reader as well, which it tries to typecast by copying the whole thing to an in-memory buffer declared on the heap. This stays in memory until the next invocation of the garbage collector, which causes reva to crash due to lack of free memory. A few solutions I tried are:

Modify the tus client to initialize the buffer of a size equal to the content length, so that it is declared on the stack and that memory is freed when the function execution ends. This doesn't work because bytes internally declares slices.

(pprof) top
Showing nodes accounting for 1GB, 99.85% of 1GB total
Dropped 51 nodes (cum <= 0.01GB)
     flat  flat%   sum%        cum   cum%
      1GB 99.85% 99.85%        1GB 99.85%  bytes.makeSlice
        0     0% 99.85%        1GB 99.85%  bytes.(*Buffer).ReadFrom
        0     0% 99.85%        1GB 99.85%  bytes.(*Buffer).grow
        0     0% 99.85%        1GB 99.80%  github.com/eventials/go-tus.NewUpload

Use a sync.pool of buffers but that won't scale as well, even if you have tens of concurrent uploads each worth let's say 100 MB each.
Invoke the garbage collector manually using FreeOSMemory. That cleans a portion of the memory but not the whole thing, given that buffer tries to have a bigger capacity than its length.

The solution which does work is writing the content to a temporary file and passing the file descriptor to that. It introduces a bit of added latency but almost doesn't add at all to the memory overhead, so I think this would be the best solution. Please let me know your thoughts.

ishank011 · 2020-10-21T10:35:33Z

I’ll make the change to other places where we initiate the tus client as well.

butonic

We should not write temporary files for every upload and always stream it to the datagateway. See the comment above.

Also you are mixing changes to reuse the httpclient with fixing the chunking ... please send two PRs ... makes it easier to review and get smaller things in asap.

butonic · 2020-10-21T13:36:42Z

internal/http/services/owncloud/ocdav/put.go

+	fileName, fd, err := s.createChunkTempFile()
+	if err != nil {
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}
+	defer os.RemoveAll(fileName)
+	defer fd.Close()
+	if _, err := io.Copy(fd, r.Body); err != nil {
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}


with this you are writing the file twice.
once on the machine that is runnung the dataprovider and once when writing it at the destination.

The whole point of tus and resumable uploads is to not touch the disk of any intermediate services. It is too slow.

The storage provider is responsible for putting any new bytes as close as possible to the target destination, so that the final assembly does not have to copy the bytes across storage boundaries.

A simple PUT request is the worst case because ill the bytes need to make it through ... and it cannot be resumed. Which makes it problematic for large files. That might be the reason why you increased the timeout in the gateway by 10 min.

Let me assure you that there are end users trying to upload 80GB ... in the browser ... which in the past used PUT ... to a machine with a 100mbit connection ... to the nas ...

TUS goes a long way to deal with this. it disconnects from the request context so it can write as many bytes as possible. That is the reason why we are having a hard time getting debug level logging from actual uploads.

Internally, handlePut should just read the body directly and us TUS to send it to the datagateway/dataprovider.

now ... chunking v1 is a different thing. Did you check if you can translate chinking v1 to TUS? because AFAICT we should be able to internally append the incoming PUT requests for every chunk to a single TUS upload to the datagateway/provider.

But to get things going I would be ok to just collect the chunks in ocdav and then do the upload using TUS internally.

Sorry about the multiple changes. Let's focus only on the tus part.

With the current /remote.php/webdav PUT endpoint, we're not supporting resumable uploads anyway. We get the whole body at once (80 GB), store that in memory (the tus client does that for the reader to readseeker conversion), and once the transfer is complete, persist that 80 GB allocation until the next run of the garbage collector.

So with the current implementation, PUTting 80 GB won't be successful unless you have 80 GB of memory. If you're able to provide that 80 GB, for the next PUT call, you'll have to allocate another 80 GB. This is the part where the problem creeps in https://github.com/eventials/go-tus/blob/45c7ec8f5d595db5dd50a926fc8420a2c70404fb/upload.go#L87-L89

buf := new(bytes.Buffer) buf.ReadFrom(reader) stream = bytes.NewReader(buf.Bytes())

On my 4GB VM, I'm not able to write even 3 GB files without reva crashing.

Sure, the POST handler works, but that assumes that the client itself knows tus and is sending binary chunks by itself, which won't be the case in a browser.

ouch ...

hm, ok so we consider PUT requests legacy ... because they cannot really be implemented without this hack ...

The owncloud clients (including phoenix) will either learn TUS or already have, so for legacy clients this might be ok.

butonic · 2020-10-21T13:38:04Z

internal/http/services/owncloud/ocdav/copy.go

@@ -293,16 +294,26 @@ func (s *svc) descend(ctx context.Context, client gateway.GatewayAPIClient, src
 			return fmt.Errorf("status code %d", httpDownloadRes.StatusCode)
 		}

+		fileName, fd, err := s.createChunkTempFile()


see above ... this copies the file to disk before sending it out again ... just read from the body

butonic · 2020-10-21T14:10:27Z

Ok, we should merge this and optimize handling of many small files in a subsequent PR. For files under 2MB we don't need to create temp files ... or maybe 10MB

butonic · 2020-10-21T14:14:49Z

also, with temp files, uploading large files requires more free disk space ... which is another reason for using tus in the first place.

so if anyone tries to upload large files .... tell him to use tus ... ;-)

ishank011 · 2020-10-21T16:10:02Z

Sure, we can optimize it for smaller uploads. Will add that part as well. Thanks @butonic!

butonic · 2020-10-23T12:19:17Z

@ishank011 any update? I'm ok to merge this as is ... we can improve performance for small files in a subsequent PR

ishank011 · 2020-10-23T14:36:57Z

@butonic running a few more tests. Will merge it then

ishank011 · 2020-10-27T12:30:42Z

@phil-davis this PR fixes the old chunking, so I removed the corresponding tests from the expected failures here fef229a. Please update these in ocis-reva as well.

There are some tests which still fail due to not being able to clear the log so maybe we can fix those as well https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload2/uploadFileUsingOldChunking.feature#L45

ishank011 force-pushed the ocdav-putchunked branch 3 times, most recently from bb49b9f to bc22212 Compare October 19, 2020 07:57

ishank011 force-pushed the ocdav-putchunked branch from bc22212 to 41762bf Compare October 21, 2020 08:32

ishank011 marked this pull request as ready for review October 21, 2020 08:37

ishank011 requested a review from labkode as a code owner October 21, 2020 08:37

ishank011 force-pushed the ocdav-putchunked branch from 41762bf to c28cf60 Compare October 21, 2020 10:18

ishank011 requested a review from butonic October 21, 2020 10:35

butonic requested changes Oct 21, 2020

View reviewed changes

butonic previously approved these changes Oct 21, 2020

View reviewed changes

ishank011 dismissed butonic’s stale review via 72e6e9c October 27, 2020 09:15

ishank011 and others added 7 commits October 27, 2020 11:13

ocdav: upload file to storage provider after assembling chunks

90a363d

Remove temp files after reading from EOS

c6913b0

Pass io.ReadSeeker to tus client instead of io.Reader

bc0a403

Run goimports

4d3917c

Convert io.Reader to io.ReadSeeker to pass to tus client

4f0417b

Reuse HTTP clients

acf5338

Append namespace in putChunked method

13a882e

ishank011 force-pushed the ocdav-putchunked branch from 72e6e9c to 13a882e Compare October 27, 2020 10:13

Move http utils to pkg

6f307f7

ishank011 force-pushed the ocdav-putchunked branch 2 times, most recently from e543531 to 74bd3e3 Compare October 27, 2020 11:54

Update expected failed test scenarios

fef229a

ishank011 force-pushed the ocdav-putchunked branch from 74bd3e3 to fef229a Compare October 27, 2020 11:55

ishank011 requested a review from phil-davis October 27, 2020 12:31

labkode approved these changes Oct 27, 2020

View reviewed changes

labkode merged commit acf5ed6 into cs3org:master Oct 27, 2020

ishank011 deleted the ocdav-putchunked branch October 27, 2020 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ocdav: upload file to storage provider after assembling chunks #1253

ocdav: upload file to storage provider after assembling chunks #1253

ishank011 commented Oct 16, 2020 •

edited

Loading

labkode commented Oct 19, 2020

ishank011 commented Oct 21, 2020

ishank011 commented Oct 21, 2020

butonic left a comment

butonic Oct 21, 2020

ishank011 Oct 21, 2020

ishank011 Oct 21, 2020

butonic Oct 21, 2020 •

edited

Loading

butonic Oct 21, 2020 •

edited

Loading

butonic commented Oct 21, 2020

butonic commented Oct 21, 2020

ishank011 commented Oct 21, 2020

butonic commented Oct 23, 2020

ishank011 commented Oct 23, 2020

ishank011 commented Oct 27, 2020

ocdav: upload file to storage provider after assembling chunks #1253

ocdav: upload file to storage provider after assembling chunks #1253

Conversation

ishank011 commented Oct 16, 2020 • edited Loading

labkode commented Oct 19, 2020

ishank011 commented Oct 21, 2020

ishank011 commented Oct 21, 2020

butonic left a comment

Choose a reason for hiding this comment

butonic Oct 21, 2020

Choose a reason for hiding this comment

ishank011 Oct 21, 2020

Choose a reason for hiding this comment

ishank011 Oct 21, 2020

Choose a reason for hiding this comment

butonic Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

butonic Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

butonic commented Oct 21, 2020

butonic commented Oct 21, 2020

ishank011 commented Oct 21, 2020

butonic commented Oct 23, 2020

ishank011 commented Oct 23, 2020

ishank011 commented Oct 27, 2020

ishank011 commented Oct 16, 2020 •

edited

Loading

butonic Oct 21, 2020 •

edited

Loading

butonic Oct 21, 2020 •

edited

Loading