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

[WIP] feat: use runners to startup the services #9048

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jvillafanez
Copy link
Member

Description

Replace oklog library with customized runners. This is intended to simplify and homogenize command's behavior.
Some of the os.Exit calls will also be removed.

Note: It's expected that all the services will follow the same approach. The only notable exception (for now) is the antivirus service, which can't be stopped. This will need further analysis.

Related Issue

  • Fixes <issue_link>

Motivation and Context

Runner's behaviour is expected to be the same across all the commands. The code is also simplified because the behavior is set within the runner package, so the commands just need to provide the parameters.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@jvillafanez jvillafanez self-assigned this May 2, 2024
Copy link

update-docs bot commented May 2, 2024

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.

"time"
)

var (
StopSignals = []os.Signal{syscall.SIGTERM, syscall.SIGINT, syscall.SIGQUIT, syscall.SIGKILL}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SIGKILL may not be trapped https://pkg.go.dev/os/signal

@jvillafanez
Copy link
Member Author

jvillafanez commented May 3, 2024

We'll need at least an additional factory method for the reva runtime services. This will affect the following services:

  • users
  • storage-*
  • sharing
  • ocm
  • groups
  • gateway
  • frontend
  • auth-*
  • app-registry
  • app-provider

This will need some investigation about how to properly stop the runner for those services.

In addition, there are other services that might need special handling:

  • postprocessing: we could create an ad-hoc runner for this. Solution shouldn't differ greatly of what we have at the moment, maybe just adding another way out in case the stopper is called. (Stopping postprocessing service #9096)
  • policies
  • ocdav
  • notifications
  • nats
  • idm
  • clientlog
  • audit
  • antivirus: investigation is needed because it seems we can't stop the service. We might need changes in the code

@jvillafanez jvillafanez force-pushed the service_startup_replacement branch 2 times, most recently from aec7c35 to 868a07b Compare May 7, 2024 07:55
@jvillafanez jvillafanez force-pushed the service_startup_replacement branch 4 times, most recently from 4619f75 to 14b2554 Compare May 21, 2024 07:09
@jvillafanez
Copy link
Member Author

It seems I'm stuck with the reva-related services. I think we need heavy changes in the reva runtime in order to keep going.

The reva runtime has multiple problems:

  • No clear Stop method that could be use to stop the service. The method seems to be missing, so it doesn't seem possible to request the service to stop.
  • Handling OS signals by itself. As a service, reva shouldn't care about the OS signal. It should provide the means to manage the service and let the app (oCIS in this case) to control the signals the way it wants in order to provide the oCIS' desired behavior.
    For example, oCIS could intercept a "quit" signal in order to provide a controlled and predictable shutdown process. This could involve, for example, saving important data in the storage, or letting upload and download requests to finish (which involves saving data in the storage). However, currently, reva will shutdown anyway, and could break ongoing uploads or downloads.
  • There are multiple os.Exit calls which will cause the whole app to close. A graceful shutdown is impossible.

As a possible alternative, we could try to spawn a new process for each service. However, the solution also have some problems.

  • We'd need the location of the reva executable (or figure out where it is) in order to start a new process.
  • There are quite a bunch of reva services that would need to be launched. This means, a lot of processes.
  • We'd need to manage several processes. This might become problematic, and we'll likely need to implement additional safety measures in case something goes wrong (a process could not be stopped and relaunching it after restarting oCIS could cause problems)

It seems a better idea to wait for changes in reva so it's easier to handle its services.

Copy link

sonarcloud bot commented May 22, 2024

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.

2 participants