Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

worker: Remove tons of work arounds and quirks in upload #826

Merged
merged 3 commits into from Aug 12, 2016

Conversation

coolo
Copy link
Contributor

@coolo coolo commented Aug 11, 2016

Just use blocking post - possibly all this was necessary with mojo < 7, but
surely not in current:

http://mojolicious.org/perldoc/Mojolicious/Guides/Cookbook#Large-file-upload

Just use blocking post - possibly all this was necessary with mojo < 7, but
surely not in current:

http://mojolicious.org/perldoc/Mojolicious/Guides/Cookbook#Large-file-upload
@okurz
Copy link
Member

okurz commented Aug 11, 2016

LGTM

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 71.01% when pulling f2547e9 on simplify_upload into d8fe6a7 on master.

@coolo
Copy link
Contributor Author

coolo commented Aug 11, 2016

the coverage increase is fake - I just replaced uncovered code :)

@okurz
Copy link
Member

okurz commented Aug 11, 2016

Finally not deleting covered code ;-P

@coolo
Copy link
Contributor Author

coolo commented Aug 11, 2016

I implemented a chksum check - verify link: https://tortuga.suse.de/tests/427#downloads

See the log:

uploading test.qcow2
Checksum 3644698332:3644698332 Sizes:1041956864:1041956864

Hopefully we can find out the root problem with this - uploading works 100% on my staging instance ;(

@coolo
Copy link
Contributor Author

coolo commented Aug 11, 2016

I applied the patch on 4 workers on openqa.opensuse.org and we didn't see a single failure since then ;(

So I claim f2547e9 is the actual fix - but to prove that we would need to have the two other commits run independent of it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 70.828% when pulling 08add35 on simplify_upload into d8fe6a7 on master.

@okurz
Copy link
Member

okurz commented Aug 11, 2016

could very well be. LGTM

@coolo coolo merged commit c93ddd2 into master Aug 12, 2016
@coolo coolo deleted the simplify_upload branch August 12, 2016 05:03
@okurz
Copy link
Member

okurz commented Aug 18, 2016

we have this deployed on osd as well as my change to not always auto-duplicate and we see problems saving and uploading images, e.g.
https://openqa.suse.de/tests/522357/file/autoinst-log.txt
https://openqa.suse.de/tests/522368/file/autoinst-log.txt
https://openqa.suse.de/tests/522432/file/autoinst-log.txt

@aaannz
Copy link
Contributor

aaannz commented Aug 19, 2016

I think this is exactly what @nadvornik tried to solve by that removed hacks. :)

@nadvornik
Copy link
Contributor

I just checked the new Mojo. The default boundary calculation is still the same: Mojo generates a random string and then checks if the file contains it - that means the file is completely read from disk before sending anything. It can take up to several minutes and the server closes the connection meanwhile.

So I think the boundary hack is still needed.

@nadvornik
Copy link
Contributor

Maybe the most correct solution is to split the file to ~16MB chunks and upload each in separate request.

@okurz
Copy link
Member

okurz commented Aug 19, 2016

some of the stuff coolo applied certainly make sense. What he failed to do is reproduce the error first locally to confirm that his "fix" is really a fix. OTOH http://mojolicious.org/perldoc/Mojolicious/Guides/Cookbook#Large-file-upload should still apply. Maybe the images we are handling are still a bit too big and 16MB might make sense.

Maybe these references can help:

@nadvornik
Copy link
Contributor

This concrete problem is mentioned here:
https://groups.google.com/forum/#!topic/mojolicious/6DNYc3nKAvA/discussion

The official mojo documentation speaks about large files in size of megabytes, we need gigabytes.

I don't see any way how to override the boundary calculation in ua->post() api.

@okurz
Copy link
Member

okurz commented Aug 24, 2016

I am pretty sure in perl everything can be monkey patched :-)

@nadvornik
Copy link
Contributor

See #843

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants