Generalizing upload code for minio #1035

manugupt1 · 2019-01-08T06:51:56Z

What is the problem I am trying to address?

Makes minio-storage implementation with other implementations like S3.

How is the fix applied?

Refactor the code to use modUploader and fix it in other places.

NB: It is a breaking change, so it has to be taken care while integrating this change.

pkg/storage/minio/saver.go

codecov-io · 2019-01-13T05:04:18Z

Codecov Report

Merging #1035 into master will decrease coverage by 0.1%.
The diff coverage is 79.36%.

@@            Coverage Diff             @@
##           master    #1035      +/-   ##
==========================================
- Coverage   53.05%   52.95%   -0.11%     
==========================================
  Files          80       80              
  Lines        2731     2744      +13     
==========================================
+ Hits         1449     1453       +4     
- Misses       1163     1170       +7     
- Partials      119      121       +2

Impacted Files	Coverage Δ
pkg/module/zip_read_closer.go	`54.16% <ø> (-33.34%)`	⬇️
pkg/storage/gcp/saver.go	`0% <0%> (ø)`	⬆️
pkg/storage/mongo/cataloger.go	`81.81% <100%> (+0.56%)`	⬆️
pkg/storage/fs/saver.go	`51.61% <100%> (ø)`	⬆️
pkg/storage/minio/getter.go	`79.59% <100%> (ø)`	⬆️
pkg/storage/minio/minio.go	`60.86% <100%> (ø)`	⬆️
pkg/storage/minio/lister.go	`90.62% <100%> (+2.16%)`	⬆️
pkg/storage/minio/checker.go	`80% <100%> (ø)`	⬆️
pkg/storage/minio/deleter.go	`55.55% <100%> (ø)`	⬆️
pkg/storage/module/upload.go	`97.56% <100%> (+0.33%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d379c20...12f2102. Read the comment docs.

pkg/storage/minio/saver.go

pkg/storage/module/upload.go

manugupt1 · 2019-01-16T06:36:31Z

@marpio I was able to get down the memory consumption without going through GoGetFetcher and get minio passing on windows also.

Let me know what do you think.

marpio · 2019-01-16T07:54:35Z

@manugupt1 this will make the tests work but won't solve the underlying problem.
You're still passing -1 in case of zip which makes minio allocate 600MiB per mod@ver. Imagine now that athens serves 20 requests concurrently - this means 20*600MiB. This is way to much in my opinion...
Btw the issue is not OS dependent - it will happen on Linux as well. It's just that the Travis build server has more free memory.

manugupt1 · 2019-01-16T15:05:24Z

true! Let me see if there is a way to get this down lower for zip and bracket it to the maximum size of the zip.

marpio · 2019-01-16T21:28:19Z

...and bracket it to the maximum size of the zip.

We actually have to do better than that b/c some module are big - i.e. cockroachdb so we should use only a small buffer or even none and stream directly if possible.
I took a glance at minio code and I think passing the size should help but it would be awesome if we could measure memory consumption somehow.

manugupt1 · 2019-01-17T06:05:44Z

This is what I am thinking:
The concern that you are really expressing is at the tail end of the spectrum. As of now, I do not think anyone uses Minio and anyone having low RAM would be an even lower probability IMO. While I am not advocating for this PR to go through, I honestly do not think that we should remove Minio for the very minority of the users who are on very old and limited hardware. Because by removing that, we will be actually removing a feature that is potentially useful for the other 90% of the user base.

About measurement:
Would not the observability dashboard help to measure memory consumption?
I can do it on my computer as well to measure change in memory before and after to see what it is.

What do you think?

manugupt1 · 2019-01-17T06:48:02Z

Also, @marpio I have been thinking a lot more. If you look at the minio docs, we would not ever require more than 600MB for zip if we do not provide a length. Granted that there are situations, where we can get by by < 600MB, but for cockroachdb 600MB is the max we can go if not provided a length. Correct me if I am wrong here!

marpio · 2019-01-17T22:25:17Z

@manugupt1

I honestly do not think that we should remove Minio for the very minority of the users who are on very old and limited hardware. Because by removing that, we will be actually removing a feature that is potentially useful for the other 90% of the user base.

we definitely do not have to remove minio, if we manage to reduce memory consumption considerably, otherwise it won't be a workable solution for anyone.

If you look at the minio docs, we would not ever require more than 600MB for zip if we do not provide a length

Yep, not more but 600MiB is still way more than it can/should be if we want to serve more than few requests, no?

As I said before, we can try extending storage.Version by the size of the zip inside goGetFetcher because there we don't need to read the whole stream. I'm pretty optimistic that this will help us in this regard.

manugupt1 · 2019-01-18T05:25:25Z

Let me look there! I was trying not to touch that part of the code. Maybe during this week.

arschles

@manugupt1 thanks for picking this back up! overall looks really nice, but I left some comments 👍

Also, this is really gonna help me in #1058, so I'm looking forward to this going in 🎉

pkg/download/protocol_test.go

pkg/storage/compliance/tests.go

pkg/storage/minio/cataloger.go

pkg/storage/minio/checker.go

arschles · 2019-02-20T20:29:09Z

pkg/storage/minio/deleter.go

@@ -23,17 +23,17 @@ func (v *storageImpl) Delete(ctx context.Context, module, version string) error

 	versionedPath := v.versionLocation(module, version)

-	modPath := fmt.Sprintf("%s/go.mod", versionedPath)
+	modPath := fmt.Sprintf("%s.mod", versionedPath)


same here and below regarding versionedPath?

Yes! That is why I mentioned it is a breaking change :(
The reason being: https://github.com/gomods/athens/pull/1035/files#diff-617c9c1a2f86faafdb72e42ccd20a502R46 where config.PackageVersionPath has its own function of storing it and minio will use the moduploader which uses this function.

pkg/storage/mongo/cataloger.go

pkg/storage/zip.go

marpio

@manugupt1 Just few small comments and one big question about versionLocation func.

marpio · 2019-02-21T20:53:45Z

pkg/storage/minio/minio.go

 }

 func (s *storageImpl) versionLocation(module, version string) string {
-	return fmt.Sprintf("%s/%s", module, version)
+	return fmt.Sprintf("%s/@v/%s", module, version)


Why do we want to change that? Wouldn't that make an existing deployment re-download everything again b/c Exists wouldn't find stored modules?

Yes! That is why I mentioned it is a breaking change :(
The reason being: https://github.com/gomods/athens/pull/1035/files#diff-617c9c1a2f86faafdb72e42ccd20a502R46 where config.PackageVersionPath has its own function of storing it and minio will use the moduploader which uses this function.

pkg/storage/minio/saver.go

pkg/storage/zip.go

…-upload

marwan-at-work

I'm quite surprised by how much has to change just so that a helper function works for Minio as well.

I'm not sure if a lot of these changes are necessary or not, so would love to hear some feedback. But nice work on making such changes not too crazy :)

My biggest two concerns are:

The uploader function might have to change soon anyway (or even be removed)
I can't see why we have to include "Size" and not just do -1 for all modules. What's the harm in that? It would save us a lot of complexity. And if we truly need to, maybe it's best to have the upload mechanism for Minio be untied to other storage uploads.

marwan-at-work · 2019-03-04T15:15:50Z

pkg/storage/zip.go

+// Zip represents zip file of a specific version and it's size
+type Zip struct {
+	Reader io.ReadCloser
+	Size   int64


Is the size actually required by Minio? What's the benefit of including size? I'd imagine That's something Minio should do itself once we give it a big blob. For example, I imagine S3/GCS calculate the size for you.

marwan-at-work · 2019-03-04T15:16:04Z

pkg/storage/version.go

 // Version represents a version of a module and contains .mod file, a .info file and zip file of a specific version
 type Version struct {
 	Mod    []byte
-	Zip    io.ReadCloser
+	Zip    Zip


This should probably be a pointer, because copying by value will still copy by reference the ReadCloser

marwan-at-work · 2019-03-04T15:16:32Z

pkg/storage/saver.go

 )

 // Saver saves module metadata and its source to underlying storage
 type Saver interface {
-	Save(ctx context.Context, module, version string, mod []byte, zip io.Reader, info []byte) error
+	Save(ctx context.Context, module, version string, mod []byte, zip Zip, info []byte) error


Same here, should also be a pointer to Zip so that people are aware they're "sharing" a Zip struct and not copying it.

marwan-at-work · 2019-03-04T15:20:39Z

pkg/storage/module/upload.go

@@ -14,16 +15,30 @@ import (
 const numFiles = 3

 // Uploader takes a stream and saves it to the blob store under a given path
-type Uploader func(ctx context.Context, path, contentType string, stream io.Reader) error
+type Uploader func(ctx context.Context, path, contentType string, stream Stream) error


Can all Storage save by path? Shouldn't the uploader take a module, version and then the storage itself determines how to combine them into a path, or maybe not combine them at all? This would be very necessary when we want to fix module.Checker to be able to tell us how many files a particular version has, or how many versions a particular module has.

Btw the comment above is out of this PR's scope, but just wanted to make peeps aware of potential changes to it so that we don't break everyone twice

marwan-at-work · 2019-03-04T15:22:51Z

pkg/storage/minio/saver.go

+func (s *storageImpl) upload(ctx context.Context, path, contentType string, stream moduploader.Stream) error {
+	const op errors.Op = "minio.upload"
+	if stream.Size > 600*mb {
+		stream.Size = -1


similar to my question below, why can't we just put -1 for all the things? This would guarantee the same behavior for all modules

marwan-at-work · 2019-03-04T17:42:17Z

I think it might be best to implement this solution in our pkg/storage/minio and avoid touching a lot of our codebase minio/minio-go#848 (comment)

I'm not sure yet if we need to use module.Uploader or avoid it completely. That's something to think about. But it might be worth creating a new issue for the above, and maybe a new PR. Up to you!

manugupt1 · 2019-03-28T14:43:17Z

Closing as discussed on Slack!

Start generalizing upload code for minio

9b70d55

manugupt1 requested a review from a team as a code owner January 8, 2019 06:51

marpio approved these changes Jan 12, 2019

View reviewed changes

Merge branch 'master' into minio-upload

aaf83a5

marpio requested changes Jan 12, 2019

View reviewed changes

pkg/storage/minio/saver.go Outdated Show resolved Hide resolved

Update lister and timeout

4ec33c2

manugupt1 changed the title ~~[WIP] Start generalizing upload code for minio~~ Generalizing upload code for minio Jan 13, 2019

Fix unit tests for minio

05d8047

manugupt1 force-pushed the minio-upload branch from f44e7b8 to 05d8047 Compare January 13, 2019 07:11

Check without content type

16cf34e

marpio reviewed Jan 13, 2019

View reviewed changes

pkg/storage/minio/saver.go Outdated Show resolved Hide resolved

manugupt1 added 3 commits January 13, 2019 13:46

Try without go routines on windows

96dcebf

Add a go routine

e3c22ed

gc tryout

c36ef5d

manugupt1 changed the title ~~Generalizing upload code for minio~~ [WIP] Generalizing upload code for minio Jan 15, 2019

michalpristas reviewed Jan 15, 2019

View reviewed changes

pkg/storage/module/upload.go Outdated Show resolved Hide resolved

manugupt1 added 2 commits January 15, 2019 22:15

Check by passing restricted length for streams

dfbf84c

Merge branch 'master' into minio-upload

01d14df

manugupt1 changed the title ~~[WIP] Generalizing upload code for minio~~ Generalizing upload code for minio Jan 16, 2019

Merge branch 'master' into minio-upload

36d493c

Merge branch 'master' into minio-upload

e6a17fb

manugupt1 added 7 commits February 18, 2019 21:02

Merge branch 'master' into minio-upload

ef94201

fix typo

e0c6482

Fix more tests

fa1cda3

More tests

f8d9e58

remove logs

f1c50be

Add test

ad12fd4

Fix things

6519c06

manugupt1 changed the title ~~[WIP] Generalizing upload code for minio~~ Generalizing upload code for minio Feb 19, 2019

Merge branch 'master' into minio-upload

e00dad7

arschles requested changes Feb 20, 2019

View reviewed changes

marpio requested changes Feb 21, 2019

View reviewed changes

manugupt1 added 6 commits February 21, 2019 21:25

Address a few nits

f4cfa3c

KB

d3f8cef

mb

6599a29

fmt

dff561b

Merge branch 'minio-upload' of github.com:manugupt1/athens into minio…

a4b7bcb

…-upload

renmae

25061cb

manugupt1 requested a review from arschles February 23, 2019 22:27

manugupt1 added 2 commits February 23, 2019 14:28

Merge branch 'master' into minio-upload

4445e1c

fix etcd

2dbbbd4

manugupt1 requested review from marpio and michalpristas February 23, 2019 22:39

Merge branch 'master' into minio-upload

1d81c12

arschles removed this from the v0.3.0 milestone Feb 26, 2019

manugupt1 added 2 commits February 26, 2019 23:14

Merge branch 'master' into minio-upload

e05fe55

Merge branch 'master' into minio-upload

12f2102

marwan-at-work requested changes Mar 4, 2019

View reviewed changes

manugupt1 closed this Mar 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalizing upload code for minio #1035

Generalizing upload code for minio #1035

manugupt1 commented Jan 8, 2019 •

edited

Loading

codecov-io commented Jan 13, 2019 •

edited

Loading

manugupt1 commented Jan 16, 2019

marpio commented Jan 16, 2019

manugupt1 commented Jan 16, 2019

marpio commented Jan 16, 2019

manugupt1 commented Jan 17, 2019 •

edited

Loading

manugupt1 commented Jan 17, 2019

marpio commented Jan 17, 2019

manugupt1 commented Jan 18, 2019

arschles left a comment

arschles Feb 20, 2019

manugupt1 Feb 22, 2019

marpio left a comment

marpio Feb 21, 2019

manugupt1 Feb 22, 2019 •

edited

Loading

marwan-at-work left a comment

marwan-at-work Mar 4, 2019

marwan-at-work Mar 4, 2019

marwan-at-work Mar 4, 2019

marwan-at-work Mar 4, 2019

marwan-at-work Mar 4, 2019

marwan-at-work Mar 4, 2019

marwan-at-work commented Mar 4, 2019

manugupt1 commented Mar 28, 2019

Generalizing upload code for minio #1035

Generalizing upload code for minio #1035

Conversation

manugupt1 commented Jan 8, 2019 • edited Loading

codecov-io commented Jan 13, 2019 • edited Loading

Codecov Report

manugupt1 commented Jan 16, 2019

marpio commented Jan 16, 2019

manugupt1 commented Jan 16, 2019

marpio commented Jan 16, 2019

manugupt1 commented Jan 17, 2019 • edited Loading

manugupt1 commented Jan 17, 2019

marpio commented Jan 17, 2019

manugupt1 commented Jan 18, 2019

arschles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marpio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manugupt1 Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

marwan-at-work 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

marwan-at-work commented Mar 4, 2019

manugupt1 commented Mar 28, 2019

manugupt1 commented Jan 8, 2019 •

edited

Loading

codecov-io commented Jan 13, 2019 •

edited

Loading

manugupt1 commented Jan 17, 2019 •

edited

Loading

manugupt1 Feb 22, 2019 •

edited

Loading