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

🔨 Updates settings and removes config files: storage service #2369

Merged
merged 19 commits into from
Jun 9, 2021

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 4, 2021

What do these changes do?

The Problem

Currently the design of service configs have some issues:

  • two different store models for config: aiohttp-based services in this repo get the configuration from a json file AND env vars.
    • store config in env vars is preferred (see 12factor app) (docker secrets will also be supported)
    • Env vars allows to easily change service configs on each deployment environment (e.g. in portainer it is easy to change env vars)
  • number of variables in a config is growing, undocumented, redundant and sparse in the code base. Where is my variable? what does it mean? Do I still need it or is deprecated?
  • changes in the configs (e.g. new/delete/update env vars) are difficult to incorporate in the envfiles in each deployment environment (i.e. envfile in osparc-ops )

Refactored Solution

This PR proposes a new design for the service configs that address the problems above. I have created a simplified sample service that implements this approach to facilitate the review.

In this new approach:

  • All config is in one place, namely in pydantic Settings class under {app_package_name}.settings.py
    • Composition: As any pydantic model, It is composed of a list of fields
      • main config are added as single fields
      • Each app sub module can have a sub-model e.g postgres, s3, etc
      • Naming convention: All fields are capitalized as the env vars they represent for clarity (having multiple env associated to a field is highly discouraged)
    • Scope: Settings instance is app-scoped
    • Construction: It capture ALL the app config from env vars when app starts and remains frozen
    • Can use common settings (that live in models_library.settings) either as a subsection of inheriting
    • All Settings-like classes inherit from models_library.settings.base.BaseCustomSettings
  • app CLI can display settings in multiple formats. This shall facilitate the maintenance of envfiles for the different deployments
$ simcore-service-storage settings --help          
Usage: simcore-service-storage settings [OPTIONS]

  Resolves settings and prints envfile

Options:
  --as-json / --no-as-json        [default: False]
  --as-json-schema / --no-as-json-schema
                                  [default: False]
  --compact / --no-compact        Print compact form  [default: False]
  --verbose / --no-verbose        [default: False]
  --help                          Show this message and exit.

Here an example (notice that it even captures the information in the pydantic fields if needed)

$ simcore-service-storage settings --compact --verbose 

STORAGE_HOST=0.0.0.0
STORAGE_PORT=8080
STORAGE_LOG_LEVEL=INFO
# Number of workers for the thead executor pool
STORAGE_MAX_WORKERS=8
STORAGE_MONITORING_ENABLED=False
STORAGE_DISABLE_SERVICES=[]
# Flag to enable some fakes for testing purposes
STORAGE_TESTING=False
# Blackfynn API key ONLY for testing purposes
BF_API_KEY=none
# Blackfynn API secret ONLY for testing purposes
BF_API_SECRET=none
STORAGE_POSTGRES='{"host": "postgres", "port": 5432, "user": "me", "password": "secret", "db": "simcoredb", "minsize": 1, "maxsize": 50, "dsn": "postgresql://scu:adminadmin@postgres:5432/simcoredb"}'
STORAGE_S3='{"endpoint": "172.17.0.1:9001", "access_key": "me", "secret_key": "secret", "bucket_name": "simcore", "secure": false}'
STORAGE_TRACING='{"enabled": true, "zipkin_endpoint": "http://jaeger:9411"}'

This PR

The refactoring will be implemented in three steps:

  • this PR only addresses storage.
  • next will address the webserver (which will also remove a dependency with trafaret)
  • finally another will address all the common settings under models_library.settings

Related issue/s

This is part of the overall refactoring planned for storage in #2276

How to test

$ make devenv
$ source .venv/bin/activate
$ cd services/storage
$ make install-dev
$ simcore-service-storage --help

Export .env and create a envfile, a json or a json-schema

$ set -o allexport; source .env
$ simcore-service-storage settings --verbose
 ...
$ set +o allexport

Checklist

  • Did you change any service's API? Then make sure to bundle document and upgrade version (make openapi-specs, git commit ... and then make version-*)
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@pcrespov pcrespov changed the title Updates settings and removes config files WIP: Updates settings and removes config files Jun 4, 2021
@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #2369 (57868ef) into master (dc4c691) will increase coverage by 0.4%.
The diff coverage is 68.9%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2369     +/-   ##
========================================
+ Coverage    74.3%   74.8%   +0.4%     
========================================
  Files         395     516    +121     
  Lines       16941   20029   +3088     
  Branches     1781    1971    +190     
========================================
+ Hits        12599   14990   +2391     
- Misses       3870    4525    +655     
- Partials      472     514     +42     
Flag Coverage Δ
integrationtests 67.4% <50.0%> (+8.0%) ⬆️
unittests 68.3% <68.9%> (-1.7%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...models-library/src/models_library/settings/base.py 0.0% <0.0%> (ø)
...ages/service-library/src/servicelib/application.py 0.0% <ø> (ø)
packages/service-library/src/servicelib/tracing.py 0.0% <ø> (ø)
...es/storage/src/simcore_service_storage/__main__.py 0.0% <ø> (ø)
...s/storage/src/simcore_service_storage/resources.py 100.0% <ø> (ø)
...src/simcore_service_webserver/computation_utils.py 100.0% <ø> (ø)
...storage/src/simcore_service_storage/application.py 44.1% <45.4%> (ø)
...ervices/storage/src/simcore_service_storage/cli.py 60.0% <56.0%> (-17.4%) ⬇️
services/storage/src/simcore_service_storage/db.py 70.2% <62.5%> (ø)
...src/simcore_service_webserver/activity/handlers.py 90.0% <66.6%> (ø)
... and 179 more

@pcrespov pcrespov self-assigned this Jun 4, 2021
@pcrespov pcrespov added a:storage issue related to storage service t:maintenance Some planned maintenance work labels Jun 4, 2021
@pcrespov pcrespov changed the title WIP: Updates settings and removes config files WIP: ♲: Updates settings and removes config files Jun 4, 2021
@pcrespov pcrespov changed the title WIP: ♲: Updates settings and removes config files WIP: ♻️: Updates settings and removes config files Jun 4, 2021
@pcrespov pcrespov changed the title WIP: ♻️: Updates settings and removes config files WIP: ♻️ Updates settings and removes config files Jun 4, 2021
@pcrespov pcrespov changed the title WIP: ♻️ Updates settings and removes config files WIP: 🔨 Updates settings and removes config files Jun 4, 2021
@pcrespov pcrespov changed the title WIP: 🔨 Updates settings and removes config files WIP: 🔨 refactoring: Updates settings and removes config files Jun 5, 2021
@pcrespov pcrespov changed the title WIP: 🔨 refactoring: Updates settings and removes config files WIP: 🔨 Updates settings and removes config files Jun 5, 2021
@pcrespov pcrespov force-pushed the refactor/storage-settings branch from cb24bde to a8e98f1 Compare June 7, 2021 14:18
@pcrespov pcrespov added this to the Chinchilla milestone Jun 7, 2021
@pcrespov pcrespov changed the title WIP: 🔨 Updates settings and removes config files 🔨 Updates settings and removes config files Jun 7, 2021
@pcrespov pcrespov marked this pull request as ready for review June 7, 2021 15:57
@pcrespov pcrespov changed the title 🔨 Updates settings and removes config files 🔨 Updates settings and removes config files: storage service Jun 7, 2021
@pcrespov pcrespov requested a review from Surfict June 7, 2021 16:16
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

great! this will simplify things a good way. maybe just check my minor comments...

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Very nice overall. I like overall simplification of the settings management system.

Pleas find below some questions and proposals.

.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
services/storage/src/simcore_service_storage/s3.py Outdated Show resolved Hide resolved
services/storage/tests/test_resources.py Outdated Show resolved Hide resolved
@pcrespov pcrespov force-pushed the refactor/storage-settings branch from fb75e78 to 0fa6fcf Compare June 8, 2021 14:06
@pcrespov pcrespov requested a review from GitHK June 8, 2021 14:17
@pcrespov pcrespov force-pushed the refactor/storage-settings branch from 3979902 to 57868ef Compare June 8, 2021 16:38
@pcrespov pcrespov merged commit 1ad88d2 into ITISFoundation:master Jun 9, 2021
@pcrespov pcrespov deleted the refactor/storage-settings branch June 9, 2021 13:08
@sanderegg sanderegg modified the milestones: Chinchilla, Marmoset Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:storage issue related to storage service t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants