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

Update Healthcheck logic #3884

Merged
merged 6 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ The types of changes are:
### Changed

- Simplified the file structure for HTML DSR packages [#3848](https://github.com/ethyca/fides/pull/3848)
- Simplified the database health check to improve `/health` performance [#3884](https://github.com/ethyca/fides/pull/3884)
- Changed max width of form components in "system information" form tab [#3864](https://github.com/ethyca/fides/pull/3864)
- Remove manual system selection screen [#3865](https://github.com/ethyca/fides/pull/3865)
- System and integration identifiers are now auto-generated [#3868](https://github.com/ethyca/fides/pull/3868)
Expand Down
2 changes: 1 addition & 1 deletion noxfiles/ci_nox.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
from functools import partial
from typing import Callable, Dict

import nox
from nox.command import CommandFailed

import nox
from constants_nox import (
CONTAINER_NAME,
IMAGE_NAME,
Expand Down
1 change: 1 addition & 0 deletions noxfiles/utils_nox.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from pathlib import Path

import nox

from constants_nox import COMPOSE_FILE_LIST
from run_infrastructure import run_infrastructure

Expand Down
7 changes: 3 additions & 4 deletions src/fides/api/api/v1/endpoints/health.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,9 @@ def get_cache_health() -> str:
},
},
)
async def health(
db: Session = Depends(get_db),
) -> Dict: # Intentionally injecting the ops get_db
async def health() -> Dict:
"""Confirm that the API is running and healthy."""
database_health = get_db_health(CONFIG.database.sync_database_uri, db=db)
database_health = get_db_health(CONFIG.database.sync_database_uri)
cache_health = get_cache_health()
response = CoreHealthCheck(
webserver="healthy",
Expand All @@ -97,6 +95,7 @@ async def health(
fides_is_using_workers = not celery_app.conf["task_always_eager"]
if fides_is_using_workers:
response["workers_enabled"] = True
# Figure out a way to make this faster
response["workers"] = get_worker_ids()

for _, value in response.items():
Expand Down
22 changes: 7 additions & 15 deletions src/fides/api/db/database.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,10 @@
from os import path
from typing import Literal

from alembic import command, script
from alembic import command
from alembic.config import Config
from alembic.runtime import migration
from loguru import logger as log
from sqlalchemy.orm import Session
from sqlalchemy_utils.functions import create_database, database_exists
from sqlalchemy_utils.types.encrypted.encrypted_type import InvalidCiphertextError

Expand All @@ -18,7 +17,7 @@
from fides.api.util.errors import get_full_exception_name
from fides.core.utils import get_db_engine

DatabaseHealth = Literal["healthy", "unhealthy", "needs migration"]
DatabaseHealth = Literal["healthy", "unhealthy"]


def get_alembic_config(database_url: str) -> Config:
Expand Down Expand Up @@ -82,23 +81,16 @@ def reset_db(database_url: str) -> None:
log.info("Reset complete.")


def get_db_health(database_url: str, db: Session) -> DatabaseHealth:
"""Checks if the db is reachable and up to date in alembic migrations"""
def get_db_health(database_url: str) -> DatabaseHealth:
"""Checks if the db is reachable."""
try:
alembic_config = get_alembic_config(database_url)
alembic_script_directory = script.ScriptDirectory.from_config(alembic_config)
context = migration.MigrationContext.configure(db.connection())

if (
context.get_current_revision()
!= alembic_script_directory.get_current_head()
):
return "needs migration"
return "healthy"
get_db_engine(database_url)
except Exception as error: # pylint: disable=broad-except
error_type = get_full_exception_name(error)
log.error("Unable to reach the database: {}: {}", error_type, error)
return "unhealthy"
else:
return "healthy"


async def configure_db(database_url: str, samples: bool = False) -> None:
Expand Down
2 changes: 1 addition & 1 deletion src/fides/api/tasks/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def _create_celery(config: FidesConfig = CONFIG) -> Celery:

def get_worker_ids() -> List[Optional[str]]:
"""
Returns a list of the connected heahtly worker UUIDs.
Returns a list of the connected healthy worker UUIDs.
"""
try:
connected_workers = [
Expand Down
4 changes: 2 additions & 2 deletions src/fides/api/util/connection_util.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import List, Optional
from typing import List, Optional, Union

from fastapi import Depends, HTTPException
from fideslang.validation import FidesKey
Expand Down Expand Up @@ -325,7 +325,7 @@ def connection_status(

connector = get_connector(connection_config)
try:
status: ConnectionTestStatus | None = connector.test_connection()
status: Union[ConnectionTestStatus, None] = connector.test_connection()

except (ConnectionException, ClientUnsuccessfulException) as exc:
logger.warning(
Expand Down
4 changes: 2 additions & 2 deletions tests/ctl/core/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2009,7 +2009,7 @@ def test_404_on_api_routes(test_config: FidesConfig) -> None:
@pytest.mark.integration
@pytest.mark.parametrize(
"database_health, expected_status_code",
[("healthy", 200), ("needs migration", 200), ("unhealthy", 503)],
[("healthy", 200), ("unhealthy", 503)],
)
def test_api_ping(
test_config: FidesConfig,
Expand All @@ -2018,7 +2018,7 @@ def test_api_ping(
monkeypatch: MonkeyPatch,
test_client: TestClient,
) -> None:
def mock_get_db_health(url: str, db) -> str:
def mock_get_db_health(url: str) -> str:
return database_health

monkeypatch.setattr(health, "get_db_health", mock_get_db_health)
Expand Down
Loading