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

Shift role initialization from accounts to settings #1696

Merged
merged 14 commits into from
Feb 24, 2021

Conversation

refs
Copy link
Member

@refs refs commented Feb 22, 2021

Description

Service initialization shouldn't block. When the accounts service starts, it notifies the settings service of a set of default roles and permissions, this piece is essential for the accounts to work but that by no means implies that its logic has to live within its business logic, since the storage layer resides in the settings service.

We want to invert the responsibility.

Changelog

Bugfix: Fix accounts initialization

Originally the accounts service relies on both the `settings` and `storage-metadata` to be up and running at the moment it starts. This is an antipattern as it will cause the entire service to panic if the dependants are not present.

We inverted this dependency and moved the default initialization data (i.e: creating roles, permissions, settings bundles) and instead of notifying the settings service that the account has to provide with such options, the settings is instead initialized with the options the accounts rely on. Essentially saving bandwith as there is no longer a gRPC call to the settings service.

For the `storage-metadata` a retry mechanism was added that retries by default 20 times to fetch the `com.owncloud.storage.metadata` from the service registry every `500` miliseconds. If this retry expires the accounts panics, as its dependency on the `storage-metadata` service cannot be resolved.

https://github.com/owncloud/ocis/pull/1696

@refs refs requested a review from butonic February 22, 2021 11:26
@update-docs
Copy link

update-docs bot commented Feb 22, 2021

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.

@refs refs requested a review from LukasHirt as a code owner February 23, 2021 10:56
@refs refs force-pushed the initialization-responsibility branch from 0a18cc2 to 9a507a4 Compare February 23, 2021 12:42
@refs refs changed the title shift role initialization from accounts to settings Shift role initialization from accounts to settings Feb 23, 2021
Copy link
Member

@butonic butonic left a comment

Choose a reason for hiding this comment

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

We should document the protocol change to vanilla grpc and add the circuit breaker to the changelog.

I am also not sure the settings service should have the default roles hardcoded. But changing that is a different topic.

@refs
Copy link
Member Author

refs commented Feb 24, 2021

I am also not sure the settings service should have the default roles hardcoded. But changing that is a different topic.

Agree, the default data has to live somewhere, since we don't do storage, what would be the best candidate in your eyes? Since the settings service is responsible to roles, permissions and settings, it only makes sense that the required minimum data to startup and do minimal operations stays inside the service boundary. How it was before, the accounts NEEDED the roles and permissions to work, and that is assuming too much. Services should be self contained.

@refs refs requested a review from butonic February 24, 2021 08:59
@sonarcloud
Copy link

sonarcloud bot commented Feb 24, 2021

Please retry analysis of this Pull-Request directly on SonarCloud.

@refs refs merged commit 71ec5b2 into master Feb 24, 2021
@refs refs deleted the initialization-responsibility branch February 24, 2021 13:10
ownclouders pushed a commit that referenced this pull request Feb 24, 2021
Merge: 3328f9a 7b4ed39
Author: Alex Unger <6905948+refs@users.noreply.github.com>
Date:   Wed Feb 24 14:10:57 2021 +0100

    Merge pull request #1696 from owncloud/initialization-responsibility
@micbar micbar mentioned this pull request Mar 9, 2021
16 tasks
@refs refs mentioned this pull request Apr 1, 2021
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