-
Notifications
You must be signed in to change notification settings - Fork 13
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
Stop services when core stop #790
Conversation
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.
lgtm, but we got to delete previous service stopping part from the commands provider #792
not really necessary, services will anyway be stopped but that simplifies the cli for sure |
@mesg-foundation/core can you please review |
0656331
to
acd03bb
Compare
server, err := initGRPCServer(c) | ||
if err != nil { | ||
// init system services. | ||
if err := deployCoreServices(dep.config, dep.api); err != nil { |
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 it makes more sense to start the gRPC server before deploying the Core services:
The services will fail (and restart) until the gRPC server is up and running.
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.
just kept the same behavior here as before just refactors it but the behavior is the same.
I agree it's not perfect, optimal solution would be:
- deploy service
- start service api
- start deployed services
- start core api
But this is not the purpose of this PR
if err != nil { | ||
return err | ||
} | ||
for _, s := range services { |
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.
Could all the services be stop at the same time in parallel?
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.
It could I just wanted to make it simple for now. This can be done in another PR
Modif of https://github.com/mesg-foundation/core/pull/792/files are not in this PR anymore. Maybe because of a force push. |
0926c24
to
220295e
Compare
modifications from #792 in the PR again. |
Manual tests are OK. I see a weird bug sometimes(we fixed this before), ethwallet service seems running but it's Docker service is actually not exists. I'll open an issue, if I can reproduce again. |
This pull request has been mentioned on MESG Community. There might be relevant details there: |
Right now we rely on the CLI to stop all service before stoping the core. This is not really nice and create issues if we stop the core with another CLI (
docker service rm
for exemple).When the Core stops, it will stop all the running services, close the api and then exit.