Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Sep 27, 2024

What do these changes do?

As a step into migrating from aiopg into asyncpg (#4529) this PR starts isolating the different engines used in the repo, specifically in the webserver.

  • The webserver app currently initializes two engines and we will have a third one in the transition to asyncpg, specifically:
    1. aiopg engine (deprecated). SEE db._aiopg
    2. low-level asyncpg.Pool (deprecated). SEE login.storage
    3. asyncpg engine integrated via sqlalchemy.ext.asyncio (new) . SEE db._asyncpg
  • At the end of migrate code from aiopg to asyncpg #4529, engines 1 and 2 should be fully integrated into 3.
  • For more details, please see test_db.py::test_all_pg_engines_in_app

Highlights

  • ♻️ servicelib: Unifies api to setup/teardown asyncio engines
    • Splits the db_asyncio_engine implementation for servicelib.aiohttp and servicelib.fastapi.
    • This affected other fastapi libraries where we were already using the new engine
  • 🎨 The web-server's db plugin integrates an additional asyncpg engine.
    • NOTE that for now it is ONLY setup/teardown in the tests.

Related issue/s

How to test

Driving test

cd services/web/server
make install-dev
pytest -vv tests/unit/with_dbs -k test_all_pg_engines_in_app

Dev-ops checklist

NOne

@pcrespov pcrespov self-assigned this Sep 27, 2024
@codecov
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 57.28155% with 88 lines in your changes missing coverage. Please review.

Project coverage is 87.8%. Comparing base (cafbf96) to head (da1b807).
Report is 601 commits behind head on master.

Files with missing lines Patch % Lines
...ibrary/src/servicelib/aiohttp/db_asyncpg_engine.py 0.0% 33 Missing ⚠️
...service-library/src/servicelib/db_asyncpg_utils.py 0.0% 32 Missing ⚠️
...ibrary/src/servicelib/fastapi/db_asyncpg_engine.py 0.0% 16 Missing ⚠️
.../server/src/simcore_service_webserver/db/_aiopg.py 87.7% 6 Missing ⚠️
.../simcore_service_payments/services/healthchecks.py 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6466      +/-   ##
=========================================
+ Coverage    84.5%   87.8%    +3.3%     
=========================================
  Files          10    1229    +1219     
  Lines         214   53044   +52830     
  Branches       25    1043    +1018     
=========================================
+ Hits          181   46616   +46435     
- Misses         23    6242    +6219     
- Partials       10     186     +176     
Flag Coverage Δ
integrationtests 64.7% <68.6%> (?)
unittests 85.4% <56.3%> (+0.8%) ⬆️

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

Files with missing lines Coverage Δ
...library/src/servicelib/aiohttp/application_keys.py 100.0% <100.0%> (ø)
.../settings-library/src/settings_library/postgres.py 93.6% <100.0%> (ø)
...catalog/src/simcore_service_catalog/core/events.py 100.0% <100.0%> (ø)
.../src/simcore_service_payments/services/postgres.py 100.0% <100.0%> (ø)
...vice_resource_usage_tracker/modules/db/__init__.py 100.0% <100.0%> (ø)
...s/storage/src/simcore_service_storage/constants.py 100.0% <100.0%> (ø)
services/storage/src/simcore_service_storage/db.py 97.5% <100.0%> (ø)
...s/storage/src/simcore_service_storage/db_tokens.py 100.0% <100.0%> (ø)
...rage/src/simcore_service_storage/simcore_s3_dsm.py 89.8% <ø> (ø)
...server/src/simcore_service_webserver/_constants.py 100.0% <ø> (ø)
... and 16 more

... and 1213 files with indirect coverage changes

@pcrespov pcrespov added a:webserver webserver's codebase. Assigning the area is particularly useful for bugs a:services-library issues on packages/service-libs a:catalog catalog service a:payments area: payments service labels Sep 27, 2024
@pcrespov pcrespov added this to the MartinKippenberger milestone Sep 27, 2024
@pcrespov pcrespov changed the title Mai/db async engine ♻️ Preparations in webserver to integrate asyncpg engine Sep 27, 2024
@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Sep 27, 2024
@pcrespov pcrespov marked this pull request as ready for review September 27, 2024 19:02
@pcrespov pcrespov requested a review from sanderegg as a code owner September 27, 2024 19:02
Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

👍 thanks

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.

👍
I'm only worried of the extra connections to Postgres when we have 3 engines connected from webserver.

@pcrespov
Copy link
Member Author

pcrespov commented Sep 30, 2024

👍 I'm only worried of the extra connections to Postgres when we have 3 engines connected from webserver.

@GitHK Some notes on your comment:

  1. This new pool is still NOT activate. I only set it up in on test-suite
  2. We already have multiple pools when we have replicas but, it would evidently double.
  3. A way to avoid that is to do the migration from aipg to asyncpg in one go (similar to what we are doing with pydantic)

@pcrespov pcrespov enabled auto-merge (squash) September 30, 2024 09:04
@sonarqubecloud
Copy link

@pcrespov pcrespov disabled auto-merge September 30, 2024 14:43
@pcrespov pcrespov merged commit ba6829a into ITISFoundation:master Sep 30, 2024
57 checks passed
@pcrespov pcrespov deleted the mai/db-async-engine branch September 30, 2024 14:43
mrnicegyu11 pushed a commit to mrnicegyu11/osparc-simcore that referenced this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:catalog catalog service a:payments area: payments service a:services-library issues on packages/service-libs a:webserver webserver's codebase. Assigning the area is particularly useful for bugs t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants