Implementing an external storage driver #1551

arschles · 2020-02-21T19:25:16Z

What is the problem I am trying to address?

Over time, requests have come in for a storage driver for "X". Over time, the Athens codebase and config has grown as we've added more drivers

How is the fix applied?

I've added a new "external" storage driver that uses a gRPC API to communicate with any external server for storage. This is so that anybody can generate gRPC stubs for their language of choice, build their own storage driver, and then configure Athens to use it. This can be done without compiling additional code into Athens.

Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.

We have had considerable discussion and PRs on this topic (external storage drivers) over time, and I am hoping that this PR can move us toward a solution 😄

Closes #1353
Fixes #1110
Fixes #1130
Fixes #1131

arschles · 2020-02-21T19:27:17Z

Ref #1539, where the discussion of storage drivers came up most recently.

cc/ @marwan-at-work @wburningham since you've been in discussion on this issue
cc/ @Kunde21 @marpio @tylerchr since you've all been in discussions about this topic sometime in the past

This is a draft PR currently with just the gRPC definitions. I'd like some feedback on how the API looks before I go implement anything more. Thanks all! 😄

xytan0056 · 2020-02-26T09:50:46Z

proto/external_storage.proto

+
+service ProxyStorage {
+
+    rpc GetModule (ModuleMetadata) returns (ModuleData) {}


Not an expert in proto. But does this mean @info or @mod will download zip_file too?

Yes, that matched the interface, at the time (before #406 landed).
This service definition would need to be changed to match the storage interface.

I think that Athens never downloads module@version metadata without also downloading the zip file. That said, does it make more sense to split this int GetModuleZip and GetModuleMetadata?

xytan0056 · 2020-02-26T09:52:55Z

proto/external_storage.proto

+message ModuleData {
+    ModuleMetadata metadata = 1;
+    string mod_file = 2;
+    string info = 3;


Should info be a json?

message ModuleInfo { string version = 1; string timstamp =2; }

Indeed it should. Thanks @xytan0056 !

instead of a raw string

arschles · 2020-02-27T00:09:25Z

@Kunde21 @xytan0056 I updated the interface according to your comments. Do you mind taking another look?

geototti21

looks good, left you a comment for CI diff check

geototti21 · 2020-02-27T21:33:36Z

pkg/storage/external/external_storage.pb.go

@@ -0,0 +1,555 @@
+// Code generated by protoc-gen-go. DO NOT EDIT.


💡 Should we add a CI check that diffs the commited auto-generated with the one that is getting generated by the proto file on CI. So this way we know that auto-generate code that we commit is up to date with what is expected?

@geototti21 good idea, yes I think we should

marwan-at-work · 2020-02-27T22:04:09Z

@arschles I see this is still in draft, lemme know if you want me to give this a review now or later :)

arschles · 2020-02-28T01:50:32Z

@marwan-at-work yes please! I'd really like your input on the interface

marwan-at-work

Couple of design questions off the bat.

But I have a few thoughts (that I'm not sure about them myself just yet):

Why gRPC in particular? The only reason I can think of is that we would then be able to provide a new "library" for them to import and implement the interface it exposes...I kinda like that, but I wonder if we can do the same thing without having to go with gRPC. This way people don't have to stick to gRPC which is http/2 only...people can implement a storage in good old http, which will work nicely behind aws/gcp load balancers and whatnot.
What about "extending" the download protocol? The download protocol already gives us 4/6 of these operations, so why not extend it with POST equivalents to persist things: POST /{module}/@v/{version}.info and so on and so forth.

Anyway, I'm just brainstorming as I write, but nice work so far!

marwan-at-work · 2020-02-28T02:12:36Z

proto/external_storage.proto

+
+// ModuleZip is the zip file of a module's source code
+message ModuleZip {
+    bytes zip_file = 1;


Wouldn't this be a big performance problem? Go by default loads 10 dependencies concurrently as far as I recall, and if you have multiple go clients, this can quickly become a problem.

Would be interesting to test it against a couple of concurrent go mod downloads and see how it performs.

The only option I see is to use the stream clause

@marwan-at-work if the problem you're talking about is how many concurrent calls to GetZip Athens is making, I think we can use GoGetWorkers and the accompanying machinery to limit the number of times we hit that endpoint. The other side could also decide how many concurrent calls that it wants to serve concurrently.

I don't think we get any worse from a memory usage perspective if we limit concurrent calls to the GetZip like we do with go mod download. Streaming definitely doesn't hurt us though.

if the problem you're talking about is how many concurrent calls to GetZip Athens is making

That's just one of the problems :) I think the biggest one is the fact that we'd have to completely load the module zip in memory on its way in and on its way out. Even if we limit save/get operations, then the Go command will time out because it would simply take too long to try and process less than 10 gets at a time.

proto/external_storage.proto

And removing redundant exists field. Just using the error to indicate existance

arschles · 2020-03-07T01:13:28Z

@marwan-at-work I have some answers 😄

For (1), I chose gRPC because its code generation (for servers) & versioning come for free. If we end up streaming per #1551 (comment), then that will be another reason (we wouldn't need to roll a websocket solution ourselves). There are other code generation options, but gRPC is the most widely used and supported that I know of. Second to gRPC would be Swagger/OpenAPI, which Kubernetes uses and, from my understanding, has had to hack a bit to get it to work for its purposes.

I haven't seen any major cloud load balancers that don't support HTTP/2. If someone has on-prem LB's that don't, we can include instructions in our docs on how to use grpc-web with their server.

For (2), the major reason I didn't roll a custom solution is because gRPC more widely supported and understood than our homegrown HTTP client/server would be. And at the risk of sounding redundant, the code generation will end up saving us a lot of time when people start building servers 😄

marwan-at-work · 2020-03-07T14:59:56Z

@arschles

we wouldn't need to roll a websocket solution ourselves

We wouldn't need to do websockets for streaming. You can still stream with pure http in the same way that the Go Download Protocol does for zips, and we can use multi-part for sending files up.

Definitely don't want you to change this PR though to completely get rid of gRPC, we can potentially jam on both ideas and see which one provides a better user experience :)

My hope is that users wouldn't have to learn something completely new to them just in order to implement a storage for Athens. So what we can also do, whether we go gRPC or http, is provide some helper functions where we'd hide away the transport itself.

arschles · 2020-03-11T01:04:48Z

Long response ahead, sorry in advance 😄

You can still stream with pure http in the same way that the Go Download Protocol does for zips

Oh, I misunderstood your suggestion. I'd be cool with doing the the as-is HTTP chunked streaming as we do now, yes!

I'm also cool with streaming server -> client with gRPC (the docs for client streaming show roughly how we could do it) if we go this route.

My hope is that users wouldn't have to learn something completely new to them just in order to implement a storage for Athens

I'm with you there. I'll ditch gRPC in an instant if it ends up being hard on server authors (I am going to be one of those authors right after this gets merged 😉)

I just auto-generated the server stub and checked it into ./pkg/storage/external_storage package. At the moment, you'd have to follow the "how to write a gRPC server" docs to run your server. They're pretty well written but verbose. We could mostly replace those docs with a few paragraphs on our site and some helpers. Writing a server would then boil down to implementing an interface and then calling a function.

The downside of all of this is that gRPC is fairly easy to learn and the tooling ecosystem is decent, but at the end of the day, it's still new underlying technology for most people. Obviously I think the tradeoff is worth it.

@Kunde21 @tylerchr @marpio @xytan0056 @geototti21 you have all either built alternative backends for Athens, or are involved with this topic in some other way. Would any of you be up for giving feedback on how comfortable you'd be writing and running your own gRPC-based Athens backend?

we can potentially jam on both ideas and see which one provides a better user experience :)

I'm cool with that. Would you be up with PRing something small showing what you're thinking for the HTTP protocol?

I appreciate you all talking this through with me! 💚

marwan-at-work · 2020-03-11T04:29:48Z

@arschles here ya go: #1570

arschles added 3 commits February 21, 2020 11:04

Adding gRPC definition for external storage

6813b83

Adding gRPC/protobuf files for external storage

0588866

Generating gRPC code

961c48f

arschles added the storage work to do on one or more of our storage systems label Feb 21, 2020

arschles and others added 2 commits February 21, 2020 11:58

Updating the gRPC how-to instructions

9aace7d

Merge branch 'master' into ext-storage

f077680

arschles mentioned this pull request Feb 26, 2020

[question] What is use of @Delete method in Storage interface? #1547

Closed

xytan0056 reviewed Feb 26, 2020

View reviewed changes

arschles and others added 5 commits February 26, 2020 15:55

Merge branch 'master' into ext-storage

e92f5e2

Splitting module get calls up into mod, info and zip

07f261f

Returning ModuleInfo as the info field in ModuleData

18fdc90

instead of a raw string

fixing syntax errors in proto file

785f539

updating generated code

9b104fa

geototti21 reviewed Feb 27, 2020

View reviewed changes

Merge branch 'master' into ext-storage

ffb4718

marwan-at-work reviewed Feb 28, 2020

View reviewed changes

arschles and others added 4 commits March 6, 2020 16:46

Merge branch 'master' into ext-storage

ebcde93

Removing redundant names

4d7c9a6

And removing redundant exists field. Just using the error to indicate existance

updating generated code

73fab5a

updating the language guide link to proto3

6b04ce2

adding grpc plugin output

1aec9d7

arschles mentioned this pull request Mar 11, 2020

Add a configuration for Athens to proxy to upstream proxies #1488

Closed

arschles mentioned this pull request Mar 20, 2020

pkg/storage: add External implementation #1587

Merged

marwan-at-work closed this in #1587 Mar 27, 2020

arschles deleted the ext-storage branch April 1, 2020 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing an external storage driver #1551

Implementing an external storage driver #1551

arschles commented Feb 21, 2020

arschles commented Feb 21, 2020 •

edited

Loading

xytan0056 Feb 26, 2020

Kunde21 Feb 26, 2020

arschles Feb 26, 2020

xytan0056 Feb 26, 2020

arschles Feb 26, 2020

arschles commented Feb 27, 2020

geototti21 left a comment

geototti21 Feb 27, 2020

arschles Feb 28, 2020

marwan-at-work commented Feb 27, 2020

arschles commented Feb 28, 2020

marwan-at-work left a comment

marwan-at-work Feb 28, 2020 •

edited

Loading

arschles Mar 7, 2020

marwan-at-work Mar 7, 2020 •

edited

Loading

arschles commented Mar 7, 2020

marwan-at-work commented Mar 7, 2020

arschles commented Mar 11, 2020

marwan-at-work commented Mar 11, 2020 •

edited

Loading


		service ProxyStorage {

		rpc GetModule (ModuleMetadata) returns (ModuleData) {}

		@@ -0,0 +1,555 @@
		// Code generated by protoc-gen-go. DO NOT EDIT.

Implementing an external storage driver #1551

Implementing an external storage driver #1551

Conversation

arschles commented Feb 21, 2020

arschles commented Feb 21, 2020 • edited Loading

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

arschles commented Feb 27, 2020

geototti21 left a 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 Feb 27, 2020

arschles commented Feb 28, 2020

marwan-at-work left a comment

Choose a reason for hiding this comment

marwan-at-work Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marwan-at-work Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

arschles commented Mar 7, 2020

marwan-at-work commented Mar 7, 2020

arschles commented Mar 11, 2020

marwan-at-work commented Mar 11, 2020 • edited Loading

arschles commented Feb 21, 2020 •

edited

Loading

marwan-at-work Feb 28, 2020 •

edited

Loading

marwan-at-work Mar 7, 2020 •

edited

Loading

marwan-at-work commented Mar 11, 2020 •

edited

Loading