service discovery initial code #32

nadilas · 2018-03-19T18:50:53Z

in a second step, I'll make an interface out of the current registry implementation.

arbarlow · 2018-03-19T20:12:20Z

Continuing the discussion here from the issue!

I don't think we need a new registry repo, we can add the interface to this main repo and then much like how the google package works in pubsub there can be a subpackage that implements that interface that can be brought in an used?

It would be awesome then to generate the apps with a users preferences and it mostly be code stuff.

What do you think?

arbarlow · 2018-03-19T20:02:40Z

lile.go

 }

 func CreateServer() *grpc.Server {
+	zap, err := zap.NewProduction()


I think we should probably leave this out, we found logging to be far too verbose for production (we use metrics instead), I am actually thinking that would extract all this into a DefaultMiddleware option and then users can opt out and do whatever

Also because it adds another logger and we have logrus, which should be abstracted away too

arbarlow · 2018-03-19T20:02:55Z

lile.go

 	// Default interceptors, [prometheus, opentracing]
 	AddUnaryInterceptor(grpc_prometheus.UnaryServerInterceptor)
 	AddStreamInterceptor(grpc_prometheus.StreamServerInterceptor)
 	AddUnaryInterceptor(otgrpc.OpenTracingServerInterceptor(
 		fromenv.Tracer(service.Name)))

-	gs := grpc.NewServer(
+	// add recovery later to avoid panics within handlers
+	AddStreamInterceptor(grpc_recovery.StreamServerInterceptor())


This is nice!

arbarlow · 2018-03-19T20:06:47Z

template/cmd_main.tmpl

@@ -23,5 +23,7 @@ func main() {
 		Provider:    fromenv.PubSubProvider(),
 	})

-	cmd.Execute()
+	go cmd.Execute()


This probably isn't what you want, as non server cmd's won't exit until you hit the shutdown hook, I would move the shootdown hook just into the run cmd

Correct. I simplified the complete basecommand ending up in a single lile.Run() for the service command. ;) I probably looked at the code too much at the time.

Example:

var upCmd = &cobra.Command{ Use: "up", Short: "up runs both RPC and pubub subscribers", Run: func(cmd *cobra.Command, args []string) { lile.Run() }, }

arbarlow · 2018-03-19T20:08:06Z

template/cmd_up.tmpl

+            if err != nil {
+                logrus.Fatalf("gRPC serve failed. Error: %v", err)
+            } else {
+                logrus.Infof("Application stopped.")


I think just log from the shutdown hook

arbarlow · 2018-03-19T20:08:31Z

template/cmd_main.tmpl

-	cmd.Execute()
+	go cmd.Execute()
+
+	lile.ShutdownHook()


Maybe call this GracefulShutdown?

Renamed, moved.

arbarlow · 2018-03-19T20:08:53Z

template/client.tmpl

-	conn, err := grpc.Dial("{{ .SnakeCaseName }}:80", grpc.DialOption(grpc.WithInsecure()))
+	logrus.Infof("Creating %s gRPC client", serviceName)
+
+    rc, _ := registry.NewConsulClient("localhost:8500")


People may run Consul on a different URL, so I'd check for an ENV var or similar

true, somehow my edit got lost? it was supposed to look like this

logrus.Infof("Creating %s gRPC client", serviceName) registryAddress := os.Getenv("REGISTRY_ADDRESS") if registryAddress == "" { registryAddress = "localhost:8500" } rc, _ := registry.NewConsulClient(registryAddress) entries, _, err := rc.Service(serviceName, "", nil) if err != nil { logrus.Fatalf("Failed to find service '%s'. error: %v", serviceName, err) os.Exit(1) } srv := entries[0] s := fmt.Sprintf("%s:%d", srv.Service.Address, srv.Service.Port) conn, err := grpc.Dial(s, grpc.DialOption(grpc.WithInsecure()))

Yet again, after the abstracted registry, this will also have to change

fix client template simplify cmd_main template simplify cmd_up template extract models

nadilas · 2018-03-20T05:27:23Z

I understand what you mean. I hid consul behind the interface, I still need to wrap some consul specific structs (api.AgentServiceCheck, QueryOptions, ServiceEntry, QueryMeta) to fully abstract it away, but it looks good.

Take a look.

arbarlow · 2018-03-21T16:00:24Z

lile.go

+		rc, rcErr := GetRegistryClient()
+		if rcErr == nil {
+			err := rc.Register(service.ID, service.Name, service.Config.Service.Port, &api.AgentServiceCheck{
+				CheckID:                        "",


We could abastract this out and just pass the lile service type or similar.

Also by default in Go, everything is it's empty type, so you don't need to the fill the struct in 😸

I don't understand what you mean? Hide it in another file? :-)

I know about the default values, I just wanted to leave it there for now, so I can play around with it. This struct cannot stay as-is, because of consul's abstraction. :)

arbarlow · 2018-03-21T16:00:51Z

lile.go

+			if err != nil {
+				logrus.Errorf("Failed to register service at '%s'. error: %v", service.Config.RegistryAddress, err)
+			} else {
+				logrus.Infof("Regsitered service '%s' at consul.", service.ID)


Do the logging in the consul implementation I think!

Sure, why not!

arbarlow · 2018-03-21T16:01:55Z

registry/consul.go

@@ -0,0 +1,55 @@
+package registry


So if we make this a consul package, inside lile then I think that's a boon and we add some others, like zookeeper etc

I moved it around

arbarlow · 2018-03-21T16:02:09Z

registry/registry.go

+)
+
+const (
+	CONSUL_DEFAULT_ADDRESS    = "localhost:8500"


Move these to their respective packages

arbarlow · 2018-03-21T16:02:21Z

registry/registry.go

+	DeRegister(string) error
+}
+
+func NewRegistryClient(provider, address string) (Client, error) {


We can move this out into template generation

What do you mean?

arbarlow · 2018-03-21T16:03:27Z

Hey @nadilas, nice.

Can we move the models stuff just into the lile package, we've had trouble with types only packages before and I like having them as top level citizens.

Can you run gofmt too? 😄

All in all, good stuff. I think I'll pull this in to a new version branch and do some things I've been wanted to do for a while that will allow more customisation generally.

nadilas · 2018-03-22T04:30:40Z

@arbarlow no problem.

I also like models to be first level, so they live now in the project root. :)

I did! 😆 Although I'm not sure how Goland handles that. I ran it on the whole project 👍
Obviously it's not even close being finished, I'll have more time in a couple weeks, but feel free to dive in.

arbarlow · 2018-05-30T22:28:43Z

Hi @nadilas, I haven't heard anything for a while but I'm going to close this PR as we have lile v2 and that has Registry now, which is just await some implementations.

If you're keen for Consul I'd be happy to take a PR 😄

nadilas · 2018-05-31T17:51:06Z

Hi @arbarlow, sorry about that. I was trying to reach you on the linkerd.io Slack. 😆 Didn't work...

I had quite a sprint using lile for a project I had in April and May. I had to figure out how to go about service discovery and went with a slightly different setup, but still using lile. I ended up using Nomad for scheduling and Consul (from within Nomad) for service discovery.
Also I needed to a SPA as the UI for the user facing part of the application where I had to integrate grpc-web into lile. I will be reviewing lile v2 and my changes to see, how they go together.

On top of that I'm now implementing linkerd.io, where I found your issue http://github.com/linkerd/linkerd/issues/1301 and would appreciate your input on that.

It's live now, but once I have linkerd implemented, I will have to reiterate, but the final stack will probably end up like: lile-services, grpc-web, mist, consul, nomad, linkerd.

Sorry for going MIA 😄

arbarlow · 2018-05-31T19:59:41Z

Hi @nadilas, no worries!

I'm glad you're using lile.

We have a similar setup with an SPA, grpc-web and others. But we just use Kubernetes DNS for service discovery.

I've added linkerd support in the latest release of lile actually, https://github.com/lileio/lile/blob/master/lile.go#L145

You can now point your apps as accounts:80 and set SERVICE_HOST_OVERRIDE to your linkerd instance and then you can set your dtab as needed. Also, by passing the header around, you can do deep service re-routing!

arbarlow · 2018-05-31T20:05:30Z

We publish our typescript repos to a private NPM which allows us to just import those in our front end repos, we have something like this in our makefile

proto:
	@go get github.com/golang/protobuf/protoc-gen-go
	@npm install -g ts-protoc-gen
	@mkdir -p clients/js
	@protoc -I . accounts.proto --lile-server_out=. \
	  --js_out=import_style=commonjs,binary:clients/js \
	  --go_out=plugins=grpc:$$GOPATH/src \
	  --ts_out=service=true:clients/js

service discovery initial code

901350e

nadilas mentioned this pull request Mar 19, 2018

Service Discovery integration #31

Closed

arbarlow requested changes Mar 19, 2018

View reviewed changes

step towards consul abstraction

9d628eb

fix client template simplify cmd_main template simplify cmd_up template extract models

arbarlow requested changes Mar 21, 2018

View reviewed changes

cleanup & reorg

f11992b

arbarlow closed this May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service discovery initial code #32

service discovery initial code #32

nadilas commented Mar 19, 2018

arbarlow commented Mar 19, 2018

arbarlow Mar 19, 2018

arbarlow Mar 19, 2018

nadilas Mar 20, 2018

arbarlow Mar 19, 2018

nadilas Mar 20, 2018

arbarlow Mar 19, 2018

nadilas Mar 20, 2018

nadilas Mar 20, 2018

arbarlow Mar 19, 2018

nadilas Mar 20, 2018

arbarlow Mar 19, 2018

nadilas Mar 20, 2018

arbarlow Mar 19, 2018

nadilas Mar 20, 2018

nadilas Mar 20, 2018

nadilas commented Mar 20, 2018

arbarlow Mar 21, 2018

nadilas Mar 22, 2018

arbarlow Mar 21, 2018

nadilas Mar 22, 2018

arbarlow Mar 21, 2018

nadilas Mar 22, 2018

arbarlow Mar 21, 2018

nadilas Mar 22, 2018

arbarlow Mar 21, 2018

nadilas Mar 22, 2018

arbarlow commented Mar 21, 2018

nadilas commented Mar 22, 2018

arbarlow commented May 30, 2018

nadilas commented May 31, 2018 •

edited

arbarlow commented May 31, 2018

arbarlow commented May 31, 2018

service discovery initial code #32

service discovery initial code #32

Conversation

nadilas commented Mar 19, 2018

arbarlow commented Mar 19, 2018

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

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

Choose a reason for hiding this comment

nadilas commented Mar 20, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbarlow commented Mar 21, 2018

nadilas commented Mar 22, 2018

arbarlow commented May 30, 2018

nadilas commented May 31, 2018 • edited

arbarlow commented May 31, 2018

arbarlow commented May 31, 2018

nadilas commented May 31, 2018 •

edited