Skip to content
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

Serverless services #3824

Merged
merged 12 commits into from
May 5, 2023
Merged

Conversation

javfg
Copy link
Contributor

@javfg javfg commented Apr 27, 2023

This PR adds Serverless services to Reva. It is the first step before the big PR for the Notifications Service.

This adds a new type of service which doesn't have a listener (neither http or grp). As the Notifications service is communicating via NATS only with other backend services, it doesn't need to be exposed.

It could later on be used to implement other services which don't need http/grp endpoints, like metrics.

@update-docs
Copy link

update-docs bot commented Apr 27, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@javfg javfg changed the title Serverless services cs3org Serverless services Apr 27, 2023
@labkode
Copy link
Member

labkode commented Apr 27, 2023

@javfg can you add an example of how the config would look like for an example service?

@gmgigi96
Copy link
Member

This is awesome, we could use the "serverless" services for running some internal reva procedures!

Regarding the code, I have a comment regarding the management of the services. In the HTTP and GRPC services, revad is handling properly the graceful shutdown when a signal is received (check https://github.com/cs3org/reva/blob/master/cmd/revad/internal/grace/grace.go#L258), so every service does not have to implement its own signal handler.

Also, how do you deal with errors in a single service? We don't have any way of propagating this to the Serverless struct. If we have a way then, do we need to restart the single service? Do we need to panic the entire reva?

@javfg
Copy link
Contributor Author

javfg commented Apr 27, 2023

This is awesome, we could use the "serverless" services for running some internal reva procedures!

Regarding the code, I have a comment regarding the management of the services. In the HTTP and GRPC services, revad is handling properly the graceful shutdown when a signal is received (check https://github.com/cs3org/reva/blob/master/cmd/revad/internal/grace/grace.go#L258), so every service does not have to implement its own signal handler.

I looked into this initially and thought about adding some way to deal with the shutdown for serverless in grace.go too, but in the end I did not because I'm not sure if there is a way to deal with them in a unified way.

Maybe we could discuss this during next week? I thought perhaps using context (which serverless services don't have right now), and maybe using context.WithCancel to start the services?

Also, how do you deal with errors in a single service? We don't have any way of propagating this to the Serverless struct. If we have a way then, do we need to restart the single service? Do we need to panic the entire reva?

Not dealing with them at all (a broken service crashes the whole Reva instance), and indeed this is something that is missing too. I'm not sure about how we handle them in rhttp/rgrpc. Another thing to discuss, I could use some input.

@javfg
Copy link
Contributor Author

javfg commented Apr 27, 2023

@javfg can you add an example of how the config would look like for an example service?

Sure! Here is the config for the notifications service:

[serverless.services.notifications]
nats_address = ""
nats_token = "TOKEN"
nats_template_subject = "reva-notifications-template"
nats_notification_subject = "reva-notifications-notification"
nats_trigger_subject = "reva-notifications-trigger"
storage_driver = "sql"
grouping_interval = 5
grouping_maxsize = 5

@javfg javfg requested a review from a team as a code owner April 27, 2023 16:26
@labkode
Copy link
Member

labkode commented Apr 27, 2023

@javfg thanks!

@gmgigi96 has a good point. Even if they are server less, I expect that these services will create network connections to upstream services and maybe open local files. These cases should be handled gracefully if possible.

Adding the following methods shouldn't be difficult. We can split the interface into two: Gracer and Listener

Stop() error
GracefulStop() error

@glpatcern
Copy link
Member

@javfg can you add an example of how the config would look like for an example service?

Sure! Here is the config for the notifications service:

Thanks! I'd push this in the example folder, the PRs tend to get lost...

pkg/rserverless/rserverless.go Outdated Show resolved Hide resolved
pkg/rserverless/rserverless.go Outdated Show resolved Hide resolved
pkg/rserverless/rserverless.go Outdated Show resolved Hide resolved
pkg/rserverless/rserverless.go Show resolved Hide resolved
cmd/revad/runtime/runtime.go Show resolved Hide resolved
@javfg javfg force-pushed the serverless-services-cs3org branch from d6eb4c0 to b10ff76 Compare May 3, 2023 07:20
javfg and others added 5 commits May 3, 2023 09:21
Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>
Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>
Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>
@javfg javfg force-pushed the serverless-services-cs3org branch from b10ff76 to 6cae6d5 Compare May 3, 2023 07:21
pkg/rserverless/rserverless.go Outdated Show resolved Hide resolved
pkg/rserverless/rserverless.go Outdated Show resolved Hide resolved
pkg/rserverless/rserverless.go Outdated Show resolved Hide resolved
pkg/rserverless/rserverless.go Outdated Show resolved Hide resolved
@gmgigi96 gmgigi96 merged commit 1c681a3 into cs3org:master May 5, 2023
gmgigi96 added a commit to gmgigi96/reva that referenced this pull request Jun 5, 2023
* Add serverless services

* Load serverless services

* Start serverless services on launch

* Codacy changes

* Changelog

* Example serverless config

* Add example serverless service

* Add service name to logger

Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>

* Simplify function call

Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>

* Exit with errors if initserverless fails

Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>

* Add signal handling to serverless services

* Use context to pass timeout on service stop

---------

Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>
gmgigi96 added a commit to gmgigi96/reva that referenced this pull request Jun 28, 2023
* Add serverless services

* Load serverless services

* Start serverless services on launch

* Codacy changes

* Changelog

* Example serverless config

* Add example serverless service

* Add service name to logger

Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>

* Simplify function call

Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>

* Exit with errors if initserverless fails

Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>

* Add signal handling to serverless services

* Use context to pass timeout on service stop

---------

Co-authored-by: Gianmaria Del Monte <g.macmount@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants