-
Notifications
You must be signed in to change notification settings - Fork 88
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
service discovery initial code #32
Conversation
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 It would be awesome then to generate the apps with a users preferences and it mostly be code stuff. What do you think? |
lile.go
Outdated
} | ||
|
||
func CreateServer() *grpc.Server { | ||
zap, err := zap.NewProduction() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also because it adds another logger and we have logrus, which should be abstracted away too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
// 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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
template/cmd_main.tmpl
Outdated
@@ -23,5 +23,7 @@ func main() { | |||
Provider: fromenv.PubSubProvider(), | |||
}) | |||
|
|||
cmd.Execute() | |||
go cmd.Execute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example:
var upCmd = &cobra.Command{
Use: "up",
Short: "up runs both RPC and pubub subscribers",
Run: func(cmd *cobra.Command, args []string) {
lile.Run()
},
}
template/cmd_up.tmpl
Outdated
if err != nil { | ||
logrus.Fatalf("gRPC serve failed. Error: %v", err) | ||
} else { | ||
logrus.Infof("Application stopped.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just log from the shutdown hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
template/cmd_main.tmpl
Outdated
cmd.Execute() | ||
go cmd.Execute() | ||
|
||
lile.ShutdownHook() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call this GracefulShutdown
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed, moved.
template/client.tmpl
Outdated
conn, err := grpc.Dial("{{ .SnakeCaseName }}:80", grpc.DialOption(grpc.WithInsecure())) | ||
logrus.Infof("Creating %s gRPC client", serviceName) | ||
|
||
rc, _ := registry.NewConsulClient("localhost:8500") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People may run Consul on a different URL, so I'd check for an ENV var or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
lile.go
Outdated
rc, rcErr := GetRegistryClient() | ||
if rcErr == nil { | ||
err := rc.Register(service.ID, service.Name, service.Config.Service.Port, &api.AgentServiceCheck{ | ||
CheckID: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :)
lile.go
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the logging in the consul implementation I think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not!
registry/consul.go
Outdated
@@ -0,0 +1,55 @@ | |||
package registry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it around
registry/registry.go
Outdated
) | ||
|
||
const ( | ||
CONSUL_DEFAULT_ADDRESS = "localhost:8500" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these to their respective packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
DeRegister(string) error | ||
} | ||
|
||
func NewRegistryClient(provider, address string) (Client, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move this out into template generation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
Hey @nadilas, nice. Can we move the 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. |
@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 |
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 😄 |
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. 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 😄 |
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 |
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
|
in a second step, I'll make an interface out of the current registry implementation.