From 3f14582544232b0bbb5d173eb2ec36c98986dfa8 Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Wed, 29 Jan 2025 22:46:09 -0800 Subject: [PATCH 1/8] Timeout troubleshooting --- tests/ctl/cli/test_cli.py | 3 --- tests/ctl/conftest.py | 13 +++---------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/tests/ctl/cli/test_cli.py b/tests/ctl/cli/test_cli.py index 1d2e4938bb..8309ee6970 100644 --- a/tests/ctl/cli/test_cli.py +++ b/tests/ctl/cli/test_cli.py @@ -145,9 +145,6 @@ def test_parse(test_config_path: str, test_cli_runner: CliRunner) -> None: class TestDB: - @pytest.mark.skip( - "This test is timing out only in CI: Safe-Tests (3.10.13, ctl-not-external)" - ) @pytest.mark.integration def test_reset_db(self, test_config_path: str, test_cli_runner: CliRunner) -> None: result = test_cli_runner.invoke( diff --git a/tests/ctl/conftest.py b/tests/ctl/conftest.py index db9f33e2ef..73b01bef6b 100644 --- a/tests/ctl/conftest.py +++ b/tests/ctl/conftest.py @@ -45,18 +45,11 @@ def monkeypatch_requests(test_client, monkeysession) -> None: @pytest.fixture(scope="session", autouse=True) -@pytest.mark.usefixtures("monkeypatch_requests") -def setup_ctl_db(test_config, test_client, config): +def setup_ctl_db(api_client, config): "Sets up the database for testing." assert config.test_mode - assert ( - requests.post == test_client.post - ) # Sanity check to make sure monkeypatch_requests fixture has run - yield api.db_action( - server_url=test_config.cli.server_url, - headers=config.user.auth_header, - action="reset", - ) + assert requests.post != api_client.post + yield api_client.post(url=f"{config.cli.server_url}/v1/admin/db/reset") @pytest.fixture(scope="session") From 8d8db9d59d654f5893f8271905029e0103b1a4e4 Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Wed, 29 Jan 2025 23:16:04 -0800 Subject: [PATCH 2/8] Skipping test_reset_db Enabling test_filter_data_categories_excluded --- tests/ctl/api/test_admin.py | 2 -- tests/ctl/api/test_seed.py | 1 - tests/ctl/cli/test_cli.py | 3 +++ 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/ctl/api/test_admin.py b/tests/ctl/api/test_admin.py index 648f80afb2..ac9ca21f3b 100644 --- a/tests/ctl/api/test_admin.py +++ b/tests/ctl/api/test_admin.py @@ -6,7 +6,6 @@ from fides.config import FidesConfig -@pytest.mark.skip("Troubleshooting") def test_db_reset_dev_mode_enabled( test_config: FidesConfig, test_client: TestClient, @@ -22,7 +21,6 @@ def test_db_reset_dev_mode_enabled( } -@pytest.mark.skip("Troubleshooting") def test_db_reset_dev_mode_disabled( test_config: FidesConfig, test_config_dev_mode_disabled: FidesConfig, # temporarily switches off config.dev_mode diff --git a/tests/ctl/api/test_seed.py b/tests/ctl/api/test_seed.py index 22528ee882..454607ea1b 100644 --- a/tests/ctl/api/test_seed.py +++ b/tests/ctl/api/test_seed.py @@ -83,7 +83,6 @@ def parent_server_config_password_only(): @pytest.mark.unit class TestFilterDataCategories: - @pytest.mark.skip("this times out on CI") def test_filter_data_categories_excluded(self) -> None: """Test that the filter method works as intended""" excluded_data_categories = [ diff --git a/tests/ctl/cli/test_cli.py b/tests/ctl/cli/test_cli.py index 8309ee6970..1d2e4938bb 100644 --- a/tests/ctl/cli/test_cli.py +++ b/tests/ctl/cli/test_cli.py @@ -145,6 +145,9 @@ def test_parse(test_config_path: str, test_cli_runner: CliRunner) -> None: class TestDB: + @pytest.mark.skip( + "This test is timing out only in CI: Safe-Tests (3.10.13, ctl-not-external)" + ) @pytest.mark.integration def test_reset_db(self, test_config_path: str, test_cli_runner: CliRunner) -> None: result = test_cli_runner.invoke( From 2d71b85d61bbe4380648154622dbf9962b92c1ad Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Thu, 30 Jan 2025 16:59:32 -0800 Subject: [PATCH 3/8] Moving data seeding out of migrate_db function --- src/fides/api/api/v1/endpoints/admin.py | 9 ++++++--- src/fides/api/app_setup.py | 16 +++++++++++----- src/fides/api/db/database.py | 14 +++----------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/fides/api/api/v1/endpoints/admin.py b/src/fides/api/api/v1/endpoints/admin.py index 7027aa8f5d..e6c1fa15db 100644 --- a/src/fides/api/api/v1/endpoints/admin.py +++ b/src/fides/api/api/v1/endpoints/admin.py @@ -29,7 +29,10 @@ class DBActions(str, Enum): ], status_code=status.HTTP_200_OK, ) -async def db_action(action: DBActions, revision: Optional[str] = "head") -> Dict: +def db_action( + action: DBActions, + revision: Optional[str] = "head", +) -> Dict: """ Initiate one of the enumerated DBActions. @@ -40,7 +43,7 @@ async def db_action(action: DBActions, revision: Optional[str] = "head") -> Dict if action == DBActions.downgrade: try: - await migrate_db(database_url=CONFIG.database.sync_database_uri, revision=revision, downgrade=True) # type: ignore[arg-type] + migrate_db(database_url=CONFIG.database.sync_database_uri, revision=revision, downgrade=True) # type: ignore[arg-type] action_text = "downgrade" except Exception as e: logger.exception("Database downgrade failed") @@ -68,7 +71,7 @@ async def db_action(action: DBActions, revision: Optional[str] = "head") -> Dict try: logger.info("Database being configured...") - await configure_db(CONFIG.database.sync_database_uri, revision=revision) + configure_db(CONFIG.database.sync_database_uri, revision=revision) except Exception as e: logger.exception("Database configuration failed: {e}") raise HTTPException( diff --git a/src/fides/api/app_setup.py b/src/fides/api/app_setup.py index 75e1be0e4b..ff60080101 100644 --- a/src/fides/api/app_setup.py +++ b/src/fides/api/app_setup.py @@ -14,7 +14,7 @@ from slowapi.errors import RateLimitExceeded # type: ignore from slowapi.extension import _rate_limit_exceeded_handler # type: ignore from slowapi.middleware import SlowAPIMiddleware # type: ignore - +from fides.api.db.ctl_session import async_session import fides from fides.api.api.deps import get_api_session from fides.api.api.v1 import CTL_ROUTER @@ -25,7 +25,11 @@ from fides.api.api.v1.exception_handlers import ExceptionHandlers from fides.api.common_exceptions import RedisConnectionError, RedisNotConfigured from fides.api.db.database import configure_db -from fides.api.db.seed import create_or_update_parent_user +from fides.api.db.seed import ( + create_or_update_parent_user, + load_default_resources, + load_samples, +) from fides.api.models.application_config import ApplicationConfig from fides.api.oauth.system_manager_oauth_util import ( get_system_fides_key, @@ -164,9 +168,11 @@ async def run_database_startup(app: FastAPI) -> None: if CONFIG.database.automigrate: try: - await configure_db( - CONFIG.database.sync_database_uri, samples=CONFIG.database.load_samples - ) + configure_db(CONFIG.database.sync_database_uri) + async with async_session() as session: + await load_default_resources(session) + if CONFIG.database.load_samples: + await load_samples(session) except Exception as e: logger.error("Error occurred during database configuration: {}", str(e)) else: diff --git a/src/fides/api/db/database.py b/src/fides/api/db/database.py index 8bf4d3e699..5aa6497559 100644 --- a/src/fides/api/db/database.py +++ b/src/fides/api/db/database.py @@ -48,9 +48,8 @@ def downgrade_db(alembic_config: Config, revision: str = "head") -> None: command.downgrade(alembic_config, revision) -async def migrate_db( +def migrate_db( database_url: str, - samples: bool = False, revision: str = "head", downgrade: bool = False, ) -> None: @@ -66,11 +65,6 @@ async def migrate_db( else: upgrade_db(alembic_config, revision) - async with async_session() as session: - await load_default_resources(session) - if samples: - await load_samples(session) - def create_db_if_not_exists(database_url: str) -> None: """ @@ -121,13 +115,11 @@ def get_db_health( return ("unhealthy", None) -async def configure_db( - database_url: str, samples: bool = False, revision: Optional[str] = "head" -) -> None: +def configure_db(database_url: str, revision: Optional[str] = "head") -> None: """Set up the db to be used by the app.""" try: create_db_if_not_exists(database_url) - await migrate_db(database_url, samples=samples, revision=revision) # type: ignore[arg-type] + migrate_db(database_url, revision=revision) # type: ignore[arg-type] except InvalidCiphertextError as cipher_error: log.error( "Unable to configure database due to a decryption error! Check to ensure your `app_encryption_key` has not changed." From 1a061fb56ea31d45898e432fe7f08ce5afada152 Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Thu, 30 Jan 2025 17:09:12 -0800 Subject: [PATCH 4/8] Formatting fixes --- src/fides/api/app_setup.py | 3 ++- src/fides/api/db/database.py | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/fides/api/app_setup.py b/src/fides/api/app_setup.py index ff60080101..8cf30c89aa 100644 --- a/src/fides/api/app_setup.py +++ b/src/fides/api/app_setup.py @@ -14,7 +14,7 @@ from slowapi.errors import RateLimitExceeded # type: ignore from slowapi.extension import _rate_limit_exceeded_handler # type: ignore from slowapi.middleware import SlowAPIMiddleware # type: ignore -from fides.api.db.ctl_session import async_session + import fides from fides.api.api.deps import get_api_session from fides.api.api.v1 import CTL_ROUTER @@ -24,6 +24,7 @@ from fides.api.api.v1.endpoints.health import HEALTH_ROUTER from fides.api.api.v1.exception_handlers import ExceptionHandlers from fides.api.common_exceptions import RedisConnectionError, RedisNotConfigured +from fides.api.db.ctl_session import async_session from fides.api.db.database import configure_db from fides.api.db.seed import ( create_or_update_parent_user, diff --git a/src/fides/api/db/database.py b/src/fides/api/db/database.py index 5aa6497559..2764e98821 100644 --- a/src/fides/api/db/database.py +++ b/src/fides/api/db/database.py @@ -14,8 +14,6 @@ from sqlalchemy_utils.types.encrypted.encrypted_type import InvalidCiphertextError from fides.api.db.base import Base # type: ignore[attr-defined] -from fides.api.db.ctl_session import async_session -from fides.api.db.seed import load_default_resources, load_samples from fides.api.util.errors import get_full_exception_name from fides.core.utils import get_db_engine From 1d1eb5716acf20afe06936a8e5f593ce73105d60 Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Thu, 30 Jan 2025 20:26:49 -0800 Subject: [PATCH 5/8] Testing without async data seeding --- noxfiles/setup_tests_nox.py | 9 +++-- src/fides/api/app_setup.py | 10 ++--- src/fides/api/main.py | 74 +++++++++++++++++++++---------------- 3 files changed, 53 insertions(+), 40 deletions(-) diff --git a/noxfiles/setup_tests_nox.py b/noxfiles/setup_tests_nox.py index 7ced896694..a0f8456c67 100644 --- a/noxfiles/setup_tests_nox.py +++ b/noxfiles/setup_tests_nox.py @@ -92,15 +92,18 @@ def pytest_ctl(session: Session, mark: str, coverage_arg: str) -> None: session.run(*LOGIN, external=True) run_command = ( *EXEC, - "timeout", - "--signal=INT", - "360", + # "timeout", + # "--signal=INT", + # "30", "pytest", coverage_arg, "tests/ctl/", "-m", mark, "--full-trace", + "-s", + # "-x", + # "--pdb", ) session.run(*run_command, external=True) diff --git a/src/fides/api/app_setup.py b/src/fides/api/app_setup.py index 8cf30c89aa..1a851fb70d 100644 --- a/src/fides/api/app_setup.py +++ b/src/fides/api/app_setup.py @@ -158,7 +158,7 @@ def log_startup() -> None: CONFIG.log_all_config_values() -async def run_database_startup(app: FastAPI) -> None: +def run_database_startup(app: FastAPI) -> None: """ Perform all relevant database startup activities/configuration for the application webserver. @@ -170,10 +170,10 @@ async def run_database_startup(app: FastAPI) -> None: if CONFIG.database.automigrate: try: configure_db(CONFIG.database.sync_database_uri) - async with async_session() as session: - await load_default_resources(session) - if CONFIG.database.load_samples: - await load_samples(session) + # async with async_session() as session: + # await load_default_resources(session) + # if CONFIG.database.load_samples: + # await load_samples(session) except Exception as e: logger.error("Error occurred during database configuration: {}", str(e)) else: diff --git a/src/fides/api/main.py b/src/fides/api/main.py index ef7e50ce39..93c8770083 100644 --- a/src/fides/api/main.py +++ b/src/fides/api/main.py @@ -69,51 +69,61 @@ async def lifespan(wrapped_app: FastAPI) -> AsyncGenerator[None, None]: and must be maintained. """ start_time = perf_counter() - logger.info("Starting server setup...") + try: + logger.info("Starting server setup...") - if not CONFIG.dev_mode: - sys.tracebacklimit = 0 + if not CONFIG.dev_mode: + sys.tracebacklimit = 0 - log_startup() + log_startup() - await run_database_startup(wrapped_app) + logger.debug("Starting database startup...") + run_database_startup(wrapped_app) + logger.debug("Database startup complete") - check_redis() + logger.debug("Checking Redis...") + check_redis() + logger.debug("Redis check complete") - if not scheduler.running: - scheduler.start() - if not async_scheduler.running: - async_scheduler.start() + if not scheduler.running: + logger.debug("Starting scheduler...") + scheduler.start() + if not async_scheduler.running: + logger.debug("Starting async scheduler...") + async_scheduler.start() - # generate and/or cache the identity salt - get_identity_salt() + # generate and/or cache the identity salt + get_identity_salt() - initiate_scheduled_batch_email_send() - initiate_poll_for_exited_privacy_request_tasks() - initiate_scheduled_dsr_data_removal() - initiate_bcrypt_migration_task() + initiate_scheduled_batch_email_send() + initiate_poll_for_exited_privacy_request_tasks() + initiate_scheduled_dsr_data_removal() + initiate_bcrypt_migration_task() - logger.debug("Sending startup analytics events...") - # Avoid circular imports - from fides.api.analytics import in_docker_container, send_analytics_event + logger.debug("Sending startup analytics events...") + # Avoid circular imports + from fides.api.analytics import in_docker_container, send_analytics_event - await send_analytics_event( - AnalyticsEvent( - docker=in_docker_container(), - event=Event.server_start.value, - event_created_at=datetime.now(tz=timezone.utc), + await send_analytics_event( + AnalyticsEvent( + docker=in_docker_container(), + event=Event.server_start.value, + event_created_at=datetime.now(tz=timezone.utc), + ) ) - ) - # It's just a random bunch of strings when serialized - if not CONFIG.logging.serialization: - logger.info(FIDES_ASCII_ART) + # It's just a random bunch of strings when serialized + if not CONFIG.logging.serialization: + logger.info(FIDES_ASCII_ART) - warn_root_user_enabled() + warn_root_user_enabled() - logger.info("Fides startup complete! v{}", VERSION) - startup_time = round(perf_counter() - start_time, 3) - logger.info("Server setup completed in {} seconds", startup_time) + logger.info("Fides startup complete! v{}", VERSION) + startup_time = round(perf_counter() - start_time, 3) + logger.info("Server setup completed in {} seconds", startup_time) + except Exception as e: + logger.error(f"Startup failed: {str(e)}") + raise yield # All of this happens before the webserver comes up From f009f84d4b87211dbef8a456ae24ec85803f31db Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Thu, 30 Jan 2025 22:14:38 -0800 Subject: [PATCH 6/8] Using Python version from matrix --- .github/workflows/backend_checks.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/backend_checks.yml b/.github/workflows/backend_checks.yml index 5d3705ae3a..229f4395e0 100644 --- a/.github/workflows/backend_checks.yml +++ b/.github/workflows/backend_checks.yml @@ -235,7 +235,7 @@ jobs: - name: Set Up Python uses: actions/setup-python@v5 with: - python-version: ${{ env.DEFAULT_PYTHON_VERSION }} + python-version: ${{ matrix.python_version }} cache: "pip" - name: Install Nox @@ -287,7 +287,7 @@ jobs: - name: Set Up Python uses: actions/setup-python@v5 with: - python-version: ${{ env.DEFAULT_PYTHON_VERSION }} + python-version: ${{ matrix.python_version }} cache: "pip" - name: Install Nox @@ -349,7 +349,7 @@ jobs: - name: Set Up Python uses: actions/setup-python@v5 with: - python-version: ${{ env.DEFAULT_PYTHON_VERSION }} + python-version: ${{ matrix.python_version }} cache: "pip" - name: Install Nox @@ -444,7 +444,7 @@ jobs: - name: Set Up Python uses: actions/setup-python@v5 with: - python-version: ${{ env.DEFAULT_PYTHON_VERSION }} + python-version: ${{ matrix.python_version }} cache: "pip" - name: Install Nox From 88973dbd13c6825922ccb0ab248ce3f8de84b601 Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Thu, 30 Jan 2025 22:42:52 -0800 Subject: [PATCH 7/8] Using bind kwarg for drop_all --- src/fides/api/db/database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fides/api/db/database.py b/src/fides/api/db/database.py index 2764e98821..db3f5742dd 100644 --- a/src/fides/api/db/database.py +++ b/src/fides/api/db/database.py @@ -80,7 +80,7 @@ def reset_db(database_url: str) -> None: engine = get_db_engine(database_url) with engine.connect() as connection: log.info("Dropping tables...") - Base.metadata.drop_all(connection) + Base.metadata.drop_all(bind=connection) log.info("Dropping Alembic table...") migration_context = migration.MigrationContext.configure(connection) From ffa20b7fe148ad7e68206ab67ca5453662c1e22f Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Thu, 30 Jan 2025 23:29:22 -0800 Subject: [PATCH 8/8] Engine dispose --- src/fides/api/db/database.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/fides/api/db/database.py b/src/fides/api/db/database.py index db3f5742dd..3b5de1f86d 100644 --- a/src/fides/api/db/database.py +++ b/src/fides/api/db/database.py @@ -78,6 +78,7 @@ def reset_db(database_url: str) -> None: """ log.info("Resetting database...") engine = get_db_engine(database_url) + engine.dispose() with engine.connect() as connection: log.info("Dropping tables...") Base.metadata.drop_all(bind=connection)