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

♻️ Replace aiopg in catalog service #2869

Merged
merged 41 commits into from
Mar 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
5c3e227
replace aiopg
sanderegg Mar 4, 2022
e9c9b6f
catalog uses sqlalchemy in async mode
sanderegg Mar 4, 2022
8a9a42a
only allow aiopg in tests
sanderegg Mar 4, 2022
ee2c755
asyncpg needed by sqlalchemy
sanderegg Mar 4, 2022
2613623
added utils for async sqlalchemy
sanderegg Mar 4, 2022
3c36134
removed usage of aiopg
sanderegg Mar 4, 2022
2dc8020
use async sqlalchemy in tests too
sanderegg Mar 4, 2022
fb2d97c
a test for measuring catalog listing speed
sanderegg Mar 6, 2022
afde986
refactor
sanderegg Mar 6, 2022
e1bf7e1
improve construction of no detail service
sanderegg Mar 6, 2022
de25841
no detail service improvement 20%
sanderegg Mar 6, 2022
cc77013
TEMP: testing
sanderegg Mar 6, 2022
fbc9f96
add caching
sanderegg Mar 6, 2022
c11f970
small improvement
sanderegg Mar 7, 2022
f9576c8
added async lru caching with ttl
sanderegg Mar 7, 2022
198b07d
added async-lru
sanderegg Mar 7, 2022
d1ffa03
added aiocache
sanderegg Mar 7, 2022
d6c5c45
small optimizations + refactoring
sanderegg Mar 7, 2022
f065c9c
refactor
sanderegg Mar 7, 2022
65ed601
small improvements
sanderegg Mar 7, 2022
737acdd
removed aiopg
sanderegg Mar 7, 2022
bc2823d
removed import
sanderegg Mar 7, 2022
7a19a74
set postgres username doc
sanderegg Mar 7, 2022
a3fc426
typo
sanderegg Mar 7, 2022
b72d44a
fix test env
sanderegg Mar 7, 2022
04a142b
refactor
sanderegg Mar 7, 2022
fd05b2b
list services test goes through
sanderegg Mar 7, 2022
7669e6e
fix fixtures
sanderegg Mar 7, 2022
41507f2
fix testing of services list
sanderegg Mar 7, 2022
0456d67
list services with details tested
sanderegg Mar 7, 2022
e40b23e
added pytest-benchmark
sanderegg Mar 7, 2022
1e2a083
refactor
sanderegg Mar 7, 2022
968e2bc
remove echoing
sanderegg Mar 7, 2022
4142e2a
cleanup
sanderegg Mar 8, 2022
d229e5b
@pcrespov review: avoid sql injection
sanderegg Mar 8, 2022
b269305
@pcrespov review: removed too simple function
sanderegg Mar 8, 2022
041faff
@pcrespov review: remove comment
sanderegg Mar 8, 2022
cc08de3
@pcrespov review: removed temp
sanderegg Mar 8, 2022
b5b209b
renamed locustfile towards good naming
sanderegg Mar 8, 2022
9d735ed
improve logging, increase timeout for CI
sanderegg Mar 10, 2022
66fcd7e
add pessimistic handling of disconnections
sanderegg Mar 10, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
from typing import Any, Dict

import sqlalchemy as sa
from sqlalchemy.ext.asyncio import AsyncEngine

from .utils_migration import get_current_head


async def get_pg_engine_stateinfo(engine: AsyncEngine) -> Dict[str, Any]:
return {
"current pool connections": f"{engine.pool.checkedin()=},{engine.pool.checkedout()=}",
}


class DBMigrationError(RuntimeError):
pass


async def raise_if_migration_not_ready(engine: AsyncEngine):
"""Ensures db migration is complete

:raises DBMigrationError
"""
async with engine.connect() as conn:
version_num = await conn.scalar(
sa.DDL('SELECT "version_num" FROM "alembic_version"')
)
head_version_num = get_current_head()
if version_num != head_version_num:
raise DBMigrationError(
f"Migration is incomplete, expected {head_version_num} but got {version_num}"
)
21 changes: 18 additions & 3 deletions packages/pytest-simcore/src/pytest_simcore/postgres_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import logging
from typing import AsyncIterator, Dict, Iterator, List

import aiopg.sa
import pytest
import sqlalchemy as sa
import tenacity
Expand Down Expand Up @@ -168,7 +167,7 @@ def postgres_db(
@pytest.fixture(scope="function")
async def aiopg_engine(
postgres_db: sa.engine.Engine,
) -> AsyncIterator[aiopg.sa.engine.Engine]:
) -> AsyncIterator:
"""An aiopg engine connected to an initialized database"""
from aiopg.sa import create_engine

Expand All @@ -181,6 +180,22 @@ async def aiopg_engine(
await engine.wait_closed()


@pytest.fixture(scope="function")
async def sqlalchemy_async_engine(
postgres_db: sa.engine.Engine,
) -> AsyncIterator:
# NOTE: prevent having to import this if latest sqlalchemy not installed
from sqlalchemy.ext.asyncio import create_async_engine

engine = create_async_engine(
f"{postgres_db.url}".replace("postgresql", "postgresql+asyncpg")
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this will stay in there until we fully remove aiopg. Maybe put a note.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only location where the async engine is created (pytest-simcore package). but no worries, the PostgresSettings also have a special attribute that does it for you.

)
assert engine
yield engine

await engine.dispose()


@pytest.fixture(scope="function")
def postgres_host_config(postgres_dsn: Dict[str, str], monkeypatch) -> Dict[str, str]:
monkeypatch.setenv("POSTGRES_USER", postgres_dsn["user"])
Expand All @@ -195,7 +210,7 @@ def postgres_host_config(postgres_dsn: Dict[str, str], monkeypatch) -> Dict[str,


@pytest.fixture(scope="module")
def postgres_session(postgres_db: sa.engine.Engine) -> sa.orm.session.Session:
def postgres_session(postgres_db: sa.engine.Engine) -> Iterator[sa.orm.session.Session]:
from sqlalchemy.orm.session import Session

Session_cls = sessionmaker(postgres_db)
Expand Down
12 changes: 11 additions & 1 deletion packages/settings-library/src/settings_library/postgres.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,12 @@ class PostgresSettings(BaseCustomSettings):
POSTGRES_CLIENT_NAME: Optional[str] = Field(
None,
description="Name of the application connecting the postgres database, will default to use the host hostname (hostname on linux)",
env=["HOST", "HOSTNAME", "POSTGRES_CLIENT_NAME"],
env=[
"POSTGRES_CLIENT_NAME",
# This is useful when running inside a docker container, then the hostname is set each client gets a different name
"HOST",
"HOSTNAME",
],
)

@validator("POSTGRES_MAXSIZE")
Expand All @@ -57,6 +62,11 @@ def dsn(self) -> str:
)
return dsn

@cached_property
def dsn_with_async_sqlalchemy(self) -> str:
dsn = self.dsn.replace("postgresql", "postgresql+asyncpg")
return dsn

@cached_property
def dsn_with_query(self) -> str:
"""Some clients do not support queries in the dsn"""
Expand Down
4 changes: 3 additions & 1 deletion services/catalog/requirements/_base.in
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@ fastapi[all]
pydantic[dotenv]

# database
aiopg[sa]
asyncpg
sqlalchemy[asyncio]
Copy link
Member

Choose a reason for hiding this comment

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

two things, we already introduce this "extra"

sqlalchemy[postgresql_psycopg2binary]

should we always extra with both?

sqlalchemy[asyncio,postgresql_psycopg2binary]

Copy link
Member Author

Choose a reason for hiding this comment

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

no I think once we can fully migrate, the psycopg2binary becomes obsolete. maybe just for testing where we do have a sync engine as well


# web client
httpx

# other
aiocache[redis,msgpack]
tenacity
packaging
pyyaml
27 changes: 20 additions & 7 deletions services/catalog/requirements/_base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#
# pip-compile --output-file=requirements/_base.txt --strip-extras requirements/_base.in
#
aiocache==0.11.1
# via -r requirements/_base.in
aiodebug==1.1.2
# via
# -c requirements/../../../packages/service-library/requirements/./_base.in
Expand All @@ -12,8 +14,15 @@ aiofiles==0.5.0
# via
# -c requirements/../../../packages/service-library/requirements/./_base.in
# -r requirements/../../../packages/service-library/requirements/_base.in
aiopg==1.3.3
# via -r requirements/_base.in
aioredis==1.3.1
# via
# -c requirements/../../../packages/models-library/requirements/../../../requirements/constraints.txt
# -c requirements/../../../packages/postgres-database/requirements/../../../requirements/constraints.txt
# -c requirements/../../../packages/service-library/requirements/../../../requirements/constraints.txt
# -c requirements/../../../packages/service-library/requirements/./../../../requirements/constraints.txt
# -c requirements/../../../packages/settings-library/requirements/../../../requirements/constraints.txt
# -c requirements/../../../requirements/constraints.txt
# aiocache
alembic==1.7.4
# via -r requirements/../../../packages/postgres-database/requirements/_base.in
anyio==3.5.0
Expand All @@ -23,7 +32,9 @@ anyio==3.5.0
asgiref==3.4.1
# via uvicorn
async-timeout==4.0.2
# via aiopg
# via aioredis
asyncpg==0.25.0
# via -r requirements/_base.in
certifi==2020.12.5
# via
# httpcore
Expand Down Expand Up @@ -56,6 +67,8 @@ h11==0.12.0
# via
# httpcore
# uvicorn
hiredis==2.0.0
# via aioredis
httpcore==0.14.4
# via httpx
httptools==0.2.0
Expand Down Expand Up @@ -102,6 +115,8 @@ markupsafe==1.1.1
# via
# jinja2
# mako
msgpack==1.0.3
# via aiocache
multidict==5.1.0
# via yarl
opentracing==2.4.0
Expand All @@ -113,9 +128,7 @@ orjson==3.4.8
packaging==20.9
# via -r requirements/_base.in
psycopg2-binary==2.8.6
# via
# aiopg
# sqlalchemy
# via sqlalchemy
pydantic==1.9.0
# via
# -c requirements/../../../packages/models-library/requirements/../../../requirements/constraints.txt
Expand Down Expand Up @@ -180,7 +193,7 @@ sqlalchemy==1.4.31
# -c requirements/../../../packages/settings-library/requirements/../../../requirements/constraints.txt
# -c requirements/../../../requirements/constraints.txt
# -r requirements/../../../packages/postgres-database/requirements/_base.in
# aiopg
# -r requirements/_base.in
# alembic
starlette==0.17.1
# via fastapi
Expand Down
3 changes: 2 additions & 1 deletion services/catalog/requirements/_test.in
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ jsonschema

# testing
pytest
pytest-aiohttp # incompatible with pytest-asyncio. See https://github.com/pytest-dev/pytest-asyncio/issues/76
pytest-aiohttp
pytest-benchmark
pytest-cov
pytest-mock
pytest-runner
Expand Down
5 changes: 5 additions & 0 deletions services/catalog/requirements/_test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ ptvsd==4.3.2
# via -r requirements/_test.in
py==1.11.0
# via pytest
py-cpuinfo==8.0.0
# via pytest-benchmark
pycparser==2.21
# via cffi
pylint==2.12.2
Expand All @@ -183,13 +185,16 @@ pytest==6.2.5
# -r requirements/_test.in
# pytest-aiohttp
# pytest-asyncio
# pytest-benchmark
# pytest-cov
# pytest-docker
# pytest-mock
pytest-aiohttp==1.0.4
# via -r requirements/_test.in
pytest-asyncio==0.18.1
# via pytest-aiohttp
pytest-benchmark==3.4.1
# via -r requirements/_test.in
pytest-cov==3.0.0
# via -r requirements/_test.in
pytest-docker==0.10.3
Expand Down
Original file line number Diff line number Diff line change
@@ -1,38 +1,32 @@
import logging
from typing import AsyncGenerator, Callable, Type

from aiopg.sa import Engine
from fastapi import Depends
from fastapi.requests import Request
from sqlalchemy.ext.asyncio import AsyncEngine

from ...db.repositories import BaseRepository

logger = logging.getLogger(__name__)


def _get_db_engine(request: Request) -> Engine:
def _get_db_engine(request: Request) -> AsyncEngine:
return request.app.state.engine


def get_repository(repo_type: Type[BaseRepository]) -> Callable:
async def _get_repo(
engine: Engine = Depends(_get_db_engine),
engine: AsyncEngine = Depends(_get_db_engine),
) -> AsyncGenerator[BaseRepository, None]:
# NOTE: 2 different ideas were tried here with not so good
# 1st one was acquiring a connection per repository which lead to the following issue https://github.com/ITISFoundation/osparc-simcore/pull/1966
# 2nd one was acquiring a connection per request which works but blocks the director-v2 responsiveness once
# the max amount of connections is reached
# now the current solution is to acquire connection when needed.
available_engines = engine.maxsize - (engine.size - engine.freesize)
if available_engines <= 1:
logger.warning(
"Low pg connections available in pool: pool size=%d, acquired=%d, free=%d, reserved=[%d, %d]",
engine.size,
engine.size - engine.freesize,
engine.freesize,
engine.minsize,
engine.maxsize,
)
# now the current solution is to connect connection when needed.
logger.info(
"%s",
f"current pool connections {engine.pool.checkedin()=},{engine.pool.checkedout()=}",
)
yield repo_type(db_engine=engine)

return _get_repo
Loading