Disable chunking v1 by default #678

PVince81 · 2020-04-22T13:30:17Z

This is because chunking v1 is not working correctly currently with desktop clients, so we better not advertise the capability until it's stable. That is, if there is interest at all to make it stable considering that we're moving to TUS: #661

In this PR I've set the default config to not set version 1 for the chunking capabilitiy.

Now:

how to access the capabilities from inside ocdav to disable the chunking logic ? (see TODO in code)

@butonic

PVince81 · 2020-04-22T13:32:56Z

@labkode I heard there might not be interest in keeping and maintaining the old chunking v1. If that's the case maybe we could just remove all the related code instead ?

labkode · 2020-04-22T13:40:05Z

Hi @PVince81 nice to see you hacking again.

OCIS needs to come with production support for the existing sync algorithms.
V1 can be easily mapped TUS as they follow similar conventions to finish uploads. And it scales. So far the V2 to TUS mapping doesn't scale as it needs to storage the chunks in a temporary filesystem at the HTTP layer and without introducing complexity (sticky sessions) will not scale.

So for now we can promote V2 as default sync algo but definetely not retiring the V1 code yet.

@butonic any ideas of how to push the chunks from the V2 and finish the upload on the storage?

labkode · 2020-04-22T13:41:53Z

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

@@ -120,6 +121,15 @@ func (s *svc) handlePut(w http.ResponseWriter, r *http.Request, ns string) {
 	}

 	if ok {
+		// TODO: disable if chunking capability is turned off in config


just remove the code

assuming you meant remove all the commented out code and don't add a check ? (not remove the chunking code)

PVince81 · 2020-04-22T13:50:23Z

I got a bit confused with the versions.

the capability "bigfilechunking" is about chunking v1 (the one with the "-chunking-" file names)
the capability "dav/chunking" with value "1.0" is about chunking v2

As far as I understood, according to owncloud/ocis#195 the chunking v1 in OCIS has troubles with the final PUT. So this is the one we should disable.
But as far as I can see in an HTTP request to the capabilities, "bigfilechunking" is already set to false by default. So the clients are not properly detecting v1 support and are assuming it's there, ticket here owncloud/client#7849.

Also is it correct that chunking v2 is not implemented yet / not ready ? If yes, then this PR is still valid to disable v2 chunking for production until it is stable (or remapped to TUS).

butonic · 2020-04-22T16:07:10Z

@labkode based on our experience with implementing tus in phoenix we would prefer to implement tus in the clients instead of supporting chunking v1 / v2 in ocdav.

labkode · 2020-04-23T13:36:33Z

@butonic supporting TUS alone and forget about the existing chunk algorithms makes sense when all the clients will support it, and we need to go for production with OCIS in short term.
How much time would it take to have TUS support on all the clients (mobile, sync) and properly tested?

michaelstingl · 2020-04-23T18:46:40Z

How much time would it take to have TUS support on all the clients (mobile, sync) and properly tested?

Sounds like a challenge…

Careful estimation:

iOS ==> 1 Month from now
Desktop ==> 2 Months from now
Android ==> 3 Months from now

labkode · 2020-05-05T09:05:12Z

Thanks @michaelstingl, how would you support a smooth transition from the current algorithm to the new one? Will the clients support both protocols for some time and they will use the one or another based on the capabilities offered by the server?

michaelstingl · 2020-05-05T11:29:18Z

Will the clients support both protocols for some time and they will use the one or another based on the capabilities offered by the server?

Yeah, strictly based on capabilities. iOS clients have no chunking at all, and Android has incomplete implementation.

I could imagine this:

Client	No Chunking	Chunking V1	Chunking NG	TUS
Desktop 2.6.x (current)	n.a.* (no capability)	✅ (capability)	✅ (capability)	Not implemented
Desktop 2.7 (next)	✅ (no capability)	✅ (capability)	✅ (capability)	👁️ Preview (capability)
Android 2.15 (current beta)	✅	Not implemented	Not implemented	Not implemented
Android 2.16 (next)	✅ (no capability)	Not implemented	Not implemented	👁️ Preview (capability)
iOS 3.8.2 (old)	✅	Not implemented	Not implemented	Not implemented
iOS 1.3.2 (current)	✅	Not implemented	Not implemented	Not implemented
iOS 11.4 (next)	✅ (no capability)	Not implemented	Not implemented	👁️ Preview (capability)

*: If no capability is set, Desktop Sync client assumes Chunking V1 works. This need to be fixed.

michaelstingl · 2020-05-06T11:42:06Z

@labkode @butonic Chunking V1 and Chunking NG both are broken in oCIS. Please approve the PR do set capabilities accordingly. This can be changed later, if decision changes again, and Chunking V1 and Chunking NG gets fixed, capabilities can be added again. This blocks development and testing of clients!! /cc @felixboehm @micbar

felixboehm · 2020-05-06T11:59:05Z

yes, please merge to unblock.
@labkode transition needs more consideration. Let us talk this through.

PVince81 · 2020-05-06T12:33:19Z

note: we don't need this PR to disable capabilities, this was already done in owncloud/ocis-reva#145

not sure if this PR here is still needed for that purpose

labkode · 2020-05-06T13:15:33Z

@michaelstingl @felixboehm let me know what is blocking you, capabilities are configurable, if you don't want to expose one protocol is very easy to do so in the config of the OCS service.

labkode · 2020-05-06T13:20:32Z

This PR only changes the default value for the chunking algorithm, from v1 to nothing. If you have a better default, of course, we can change it.

However, I don't think this PR has been blocking you from testing the clients, as @PVince81 pointed out is a matter of changing the configuration to enable one protocol or another.

PVince81 · 2020-05-06T13:31:39Z

I've just tested and it seems that owncloud/ocis-reva#145 is not enough.

When sitting on this PR here the chunking is properly disabled.

Before this PR:

curl -u einstein:relativity -k 'https://localhost:9200/ocs/v1.php/cloud/capabilities?format=json' | jsonlint -p | grep chunk
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1356  100  1356    0     0  16740      0 --:--:-- --:--:-- --:--:-- 16950
          "bigfilechunking": false,
          "chunking": "1.0",
          "chunkingParallelUploadDisabled": false

After this PR:

curl -u einstein:relativity -k 'https://localhost:9200/ocs/v1.php/cloud/capabilities?format=json' | jsonlint -p | grep chunk
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1353  100  1353    0     0   6567      0 --:--:-- --:--:-- --:--:--  6536
          "bigfilechunking": false,
          "chunking": "",
          "chunkingParallelUploadDisabled": false

I'll adjust the PR to be minimal

PVince81 · 2020-05-06T13:34:25Z

I've adjusted the PR, so now it defaults to disabled.

I'm not sure how the higher level config stuff is working, but apparently in our current deployment it seems to be using the default provided by reva.

In any case, since it's not working properly it is safer to disable it by default.

PVince81 requested a review from labkode as a code owner April 22, 2020 13:30

PVince81 mentioned this pull request Apr 22, 2020

Remove chunking from capabilities owncloud/ocis#203

Closed

labkode reviewed Apr 22, 2020

View reviewed changes

WIP Disable chunking v1 by default

5a33c6a

PVince81 changed the title ~~WIP Disable chunking v1 by default~~ Disable chunking v1 by default May 6, 2020

labkode merged commit 1f678dc into cs3org:master May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable chunking v1 by default #678

Disable chunking v1 by default #678

PVince81 commented Apr 22, 2020 •

edited

Loading

PVince81 commented Apr 22, 2020

labkode commented Apr 22, 2020

labkode Apr 22, 2020

PVince81 Apr 22, 2020

PVince81 commented Apr 22, 2020 •

edited

Loading

butonic commented Apr 22, 2020

labkode commented Apr 23, 2020

michaelstingl commented Apr 23, 2020 •

edited

Loading

labkode commented May 5, 2020 •

edited

Loading

michaelstingl commented May 5, 2020

michaelstingl commented May 6, 2020

felixboehm commented May 6, 2020

PVince81 commented May 6, 2020

labkode commented May 6, 2020

labkode commented May 6, 2020 •

edited

Loading

PVince81 commented May 6, 2020

PVince81 commented May 6, 2020

Disable chunking v1 by default #678

Disable chunking v1 by default #678

Conversation

PVince81 commented Apr 22, 2020 • edited Loading

PVince81 commented Apr 22, 2020

labkode commented Apr 22, 2020

labkode Apr 22, 2020

Choose a reason for hiding this comment

PVince81 Apr 22, 2020

Choose a reason for hiding this comment

PVince81 commented Apr 22, 2020 • edited Loading

butonic commented Apr 22, 2020

labkode commented Apr 23, 2020

michaelstingl commented Apr 23, 2020 • edited Loading

labkode commented May 5, 2020 • edited Loading

michaelstingl commented May 5, 2020

michaelstingl commented May 6, 2020

felixboehm commented May 6, 2020

PVince81 commented May 6, 2020

labkode commented May 6, 2020

labkode commented May 6, 2020 • edited Loading

PVince81 commented May 6, 2020

PVince81 commented May 6, 2020

PVince81 commented Apr 22, 2020 •

edited

Loading

PVince81 commented Apr 22, 2020 •

edited

Loading

michaelstingl commented Apr 23, 2020 •

edited

Loading

labkode commented May 5, 2020 •

edited

Loading

labkode commented May 6, 2020 •

edited

Loading