From ad39efe4198522856279f07a133f877f5cd9c9ac Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sun, 4 Apr 2021 18:54:40 -0400 Subject: [PATCH] Organize Gunicorn and Uvicorn server settings br3ndonland/inboard#2 br3ndonland/inboard#3 br3ndonland/inboard@ff9155a The core logic for running the Uvicorn and Gunicorn+Uvicorn servers is located in start.py. The `start.start_server()` method is what actually starts the servers. Uvicorn and Gunicorn are configured differently, so and the `start.start_server()` method ended up getting quite complex in order to handle these differences. This commit will refactor the configuration options passed to Gunicorn and Uvicorn into separate functions. The result is a start script with the same programming API and almost exactly the same line length, but improved readability and separation of concerns. - Refactor Gunicorn and Uvicorn options into separate functions, `start.set_gunicorn_options()` and `start.set_uvicorn_options()`. - Remove `start.set_conf_path()`: this function was written in a general way to find either Gunicorn or logging configuration files. Starting with ff9155a, it became used only for Gunicorn configuration files. Now that the configuration options for Gunicorn are in a separate function, the logic from `set_conf_path()` can be moved there. - Simplify logger type annotations: simply `import logging` and annotate functions with `logging.Logger` instead of having a separate import for `from logging import Logger`. - Reorganize test_start.py to more clearly reflect order of start.py --- inboard/start.py | 107 +++++++++++----------- tests/test_start.py | 214 ++++++++++++++++++++++++++++---------------- 2 files changed, 189 insertions(+), 132 deletions(-) diff --git a/inboard/start.py b/inboard/start.py index 51d6a26..fd6b67e 100644 --- a/inboard/start.py +++ b/inboard/start.py @@ -4,27 +4,14 @@ import logging.config import os import subprocess -from logging import Logger from pathlib import Path from typing import Optional import uvicorn # type: ignore -def set_conf_path(module_stem: str) -> str: - """Set the path to a configuration file.""" - conf_var = str( - os.getenv(f"{module_stem.upper()}_CONF", f"/app/inboard/{module_stem}_conf.py") - ) - if conf_var and Path(conf_var).is_file(): - conf_path = conf_var - else: - raise FileNotFoundError(f"Unable to find {conf_var}") - return conf_path - - def configure_logging( - logger: Logger = logging.getLogger(), + logger: logging.Logger = logging.getLogger(), logging_conf: str = os.getenv("LOGGING_CONF", "inboard.logging_conf"), ) -> dict: """Configure Python logging based on a path to a logging module or file.""" @@ -52,20 +39,7 @@ def configure_logging( raise -def set_app_module(logger: Logger = logging.getLogger()) -> str: - """Set the name of the Python module with the app instance to run.""" - try: - app_module = str(os.getenv("APP_MODULE")) - if not importlib.util.find_spec((module := app_module.split(sep=":")[0])): - raise ImportError(f"Unable to find or import {module}") - logger.debug(f"App module set to {app_module}.") - return app_module - except Exception as e: - logger.error(f"Error when setting app module: {e.__class__.__name__} {e}.") - raise - - -def run_pre_start_script(logger: Logger = logging.getLogger()) -> str: +def run_pre_start_script(logger: logging.Logger = logging.getLogger()) -> str: """Run a pre-start script at the provided path.""" logger.debug("Checking for pre-start script.") pre_start_path = os.getenv("PRE_START_PATH", "/app/inboard/app/prestart.py") @@ -81,42 +55,67 @@ def run_pre_start_script(logger: Logger = logging.getLogger()) -> str: return message +def set_app_module(logger: logging.Logger = logging.getLogger()) -> str: + """Set the name of the Python module with the app instance to run.""" + try: + app_module = str(os.getenv("APP_MODULE")) + if not importlib.util.find_spec((module := app_module.split(sep=":")[0])): + raise ImportError(f"Unable to find or import {module}") + logger.debug(f"App module set to {app_module}.") + return app_module + except Exception as e: + logger.error(f"Error when setting app module: {e.__class__.__name__} {e}.") + raise + + +def set_gunicorn_options() -> list: + """Set options for running the Gunicorn server.""" + gunicorn_conf_path = os.getenv("GUNICORN_CONF", "/app/inboard/gunicorn_conf.py") + worker_class = os.getenv("WORKER_CLASS", "uvicorn.workers.UvicornWorker") + if not Path(gunicorn_conf_path).is_file(): + raise FileNotFoundError(f"Unable to find {gunicorn_conf_path}") + return ["gunicorn", "-k", worker_class, "-c", gunicorn_conf_path] + + +def set_uvicorn_options(log_config: Optional[dict] = None) -> dict: + """Set options for running the Uvicorn server.""" + with_reload = ( + True + if (value := os.getenv("WITH_RELOAD")) and value.lower() == "true" + else False + ) + reload_dirs = ( + [d.lstrip() for d in str(os.getenv("RELOAD_DIRS")).split(sep=",")] + if os.getenv("RELOAD_DIRS") + else None + ) + return dict( + host=os.getenv("HOST", "0.0.0.0"), + port=int(os.getenv("PORT", "80")), + log_config=log_config, + log_level=os.getenv("LOG_LEVEL", "info"), + reload=with_reload, + reload_dirs=reload_dirs, + ) + + def start_server( process_manager: str, - app_module: str = str(os.getenv("APP_MODULE", "inboard.app.main_base:app")), - logger: Logger = logging.getLogger(), + app_module: str, + logger: logging.Logger = logging.getLogger(), logging_conf_dict: Optional[dict] = None, - worker_class: str = str(os.getenv("WORKER_CLASS", "uvicorn.workers.UvicornWorker")), ) -> None: """Start the Uvicorn or Gunicorn server.""" try: if process_manager == "gunicorn": logger.debug("Running Uvicorn with Gunicorn.") - gunicorn_conf_path = set_conf_path("gunicorn") - subprocess.run( - ["gunicorn", "-k", worker_class, "-c", gunicorn_conf_path, app_module] - ) + gunicorn_options: list = set_gunicorn_options() + gunicorn_options.append(app_module) + subprocess.run(gunicorn_options) elif process_manager == "uvicorn": - with_reload = ( - True - if (value := os.getenv("WITH_RELOAD")) and value.lower() == "true" - else False - ) - reload_dirs = ( - [d.lstrip() for d in str(os.getenv("RELOAD_DIRS")).split(sep=",")] - if os.getenv("RELOAD_DIRS") - else None - ) logger.debug("Running Uvicorn without Gunicorn.") - uvicorn.run( - app_module, - host=os.getenv("HOST", "0.0.0.0"), - port=int(os.getenv("PORT", "80")), - log_config=logging_conf_dict, - log_level=os.getenv("LOG_LEVEL", "info"), - reload=with_reload, - reload_dirs=reload_dirs, - ) + uvicorn_options: dict = set_uvicorn_options(log_config=logging_conf_dict) + uvicorn.run(app_module, **uvicorn_options) else: raise NameError("Process manager needs to be either uvicorn or gunicorn") except Exception as e: diff --git a/tests/test_start.py b/tests/test_start.py index aef78a3..1f13226 100644 --- a/tests/test_start.py +++ b/tests/test_start.py @@ -9,37 +9,6 @@ from inboard import gunicorn_conf, start -class TestConfPaths: - """Test paths to configuration files. - --- - """ - - def test_set_default_conf_path_gunicorn(self, gunicorn_conf_path: Path) -> None: - """Test default Gunicorn configuration file path (different without Docker).""" - assert "inboard/gunicorn_conf.py" in str(gunicorn_conf_path) - assert "logging" not in str(gunicorn_conf_path) - assert start.set_conf_path("gunicorn") == str(gunicorn_conf_path) - - def test_set_custom_conf_path_gunicorn( - self, - gunicorn_conf_tmp_file_path: Path, - monkeypatch: pytest.MonkeyPatch, - tmp_path: Path, - ) -> None: - """Set path to custom temporary Gunicorn configuration file.""" - monkeypatch.setenv("GUNICORN_CONF", str(gunicorn_conf_tmp_file_path)) - assert os.getenv("GUNICORN_CONF") == str(gunicorn_conf_tmp_file_path) - assert "/gunicorn_conf.py" in str(gunicorn_conf_tmp_file_path) - assert "logging" not in str(gunicorn_conf_tmp_file_path) - assert start.set_conf_path("gunicorn") == str(gunicorn_conf_tmp_file_path) - - def test_set_incorrect_conf_path(self, monkeypatch: pytest.MonkeyPatch) -> None: - """Set path to non-existent file and raise an error.""" - with pytest.raises(FileNotFoundError): - monkeypatch.setenv("GUNICORN_CONF", "/no/file/here") - start.set_conf_path("gunicorn") - - class TestConfigureGunicorn: """Test Gunicorn configuration independently of Gunicorn server. --- @@ -255,6 +224,66 @@ def test_configure_logging_tmp_module_no_dict( ) +class TestRunPreStartScript: + """Run pre-start scripts using the method in `start.py`. + --- + """ + + def test_run_pre_start_script_py( + self, + mocker: MockerFixture, + monkeypatch: pytest.MonkeyPatch, + pre_start_script_tmp_py: Path, + ) -> None: + """Test `start.run_pre_start_script` using temporary Python pre-start script.""" + mock_logger = mocker.patch.object(start.logging, "root", autospec=True) + monkeypatch.setenv("PRE_START_PATH", str(pre_start_script_tmp_py)) + pre_start_path = os.getenv("PRE_START_PATH") + start.run_pre_start_script(logger=mock_logger) + mock_logger.debug.assert_has_calls( + calls=[ + mocker.call("Checking for pre-start script."), + mocker.call(f"Running pre-start script with python {pre_start_path}."), + mocker.call(f"Ran pre-start script with python {pre_start_path}."), + ] + ) + + def test_run_pre_start_script_sh( + self, + mocker: MockerFixture, + monkeypatch: pytest.MonkeyPatch, + pre_start_script_tmp_sh: Path, + ) -> None: + """Test `start.run_pre_start_script` using temporary pre-start shell script.""" + mock_logger = mocker.patch.object(start.logging, "root", autospec=True) + monkeypatch.setenv("PRE_START_PATH", str(pre_start_script_tmp_sh)) + pre_start_path = os.getenv("PRE_START_PATH") + start.run_pre_start_script(logger=mock_logger) + mock_logger.debug.assert_has_calls( + calls=[ + mocker.call("Checking for pre-start script."), + mocker.call(f"Running pre-start script with sh {pre_start_path}."), + mocker.call(f"Ran pre-start script with sh {pre_start_path}."), + ] + ) + + def test_run_pre_start_script_no_file( + self, + mocker: MockerFixture, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Test `start.run_pre_start_script` with an incorrect file path.""" + mock_logger = mocker.patch.object(start.logging, "root", autospec=True) + monkeypatch.setenv("PRE_START_PATH", "/no/file/here") + start.run_pre_start_script(logger=mock_logger) + mock_logger.debug.assert_has_calls( + calls=[ + mocker.call("Checking for pre-start script."), + mocker.call("No pre-start script found."), + ] + ) + + class TestSetAppModule: """Set app module string using the method in `start.py`. --- @@ -355,63 +384,92 @@ def test_set_app_module_incorrect( ) -class TestRunPreStartScript: - """Run pre-start scripts using the method in `start.py`. +class TestSetGunicornOptions: + """Test Gunicorn configuration options method. --- """ - def test_run_pre_start_script_py( - self, - mocker: MockerFixture, - monkeypatch: pytest.MonkeyPatch, - pre_start_script_tmp_py: Path, + def test_set_gunicorn_options_default( + self, gunicorn_conf_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Test `start.run_pre_start_script` using temporary Python pre-start script.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - monkeypatch.setenv("PRE_START_PATH", str(pre_start_script_tmp_py)) - pre_start_path = os.getenv("PRE_START_PATH") - start.run_pre_start_script(logger=mock_logger) - mock_logger.debug.assert_has_calls( - calls=[ - mocker.call("Checking for pre-start script."), - mocker.call(f"Running pre-start script with python {pre_start_path}."), - mocker.call(f"Ran pre-start script with python {pre_start_path}."), - ] - ) - - def test_run_pre_start_script_sh( + """Test default Gunicorn server options.""" + monkeypatch.setenv("GUNICORN_CONF", str(gunicorn_conf_path)) + result = start.set_gunicorn_options() + assert os.getenv("GUNICORN_CONF") == str(gunicorn_conf_path) + assert "/gunicorn_conf.py" in str(gunicorn_conf_path) + assert "logging" not in str(gunicorn_conf_path) + assert isinstance(result, list) + assert result == [ + "gunicorn", + "-k", + "uvicorn.workers.UvicornWorker", + "-c", + str(gunicorn_conf_path), + ] + + def test_set_gunicorn_options_custom( self, - mocker: MockerFixture, + gunicorn_conf_tmp_file_path: Path, monkeypatch: pytest.MonkeyPatch, - pre_start_script_tmp_sh: Path, + tmp_path: Path, ) -> None: - """Test `start.run_pre_start_script` using temporary pre-start shell script.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - monkeypatch.setenv("PRE_START_PATH", str(pre_start_script_tmp_sh)) - pre_start_path = os.getenv("PRE_START_PATH") - start.run_pre_start_script(logger=mock_logger) - mock_logger.debug.assert_has_calls( - calls=[ - mocker.call("Checking for pre-start script."), - mocker.call(f"Running pre-start script with sh {pre_start_path}."), - mocker.call(f"Ran pre-start script with sh {pre_start_path}."), - ] + """Test custom Gunicorn server options with temporary configuration file.""" + monkeypatch.setenv("GUNICORN_CONF", str(gunicorn_conf_tmp_file_path)) + result = start.set_gunicorn_options() + assert os.getenv("GUNICORN_CONF") == str(gunicorn_conf_tmp_file_path) + assert "/gunicorn_conf.py" in str(gunicorn_conf_tmp_file_path) + assert "logging" not in str(gunicorn_conf_tmp_file_path) + assert isinstance(result, list) + assert result == [ + "gunicorn", + "-k", + "uvicorn.workers.UvicornWorker", + "-c", + str(gunicorn_conf_tmp_file_path), + ] + + def test_set_incorrect_conf_path(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Set path to non-existent file and raise an error.""" + monkeypatch.setenv("GUNICORN_CONF", "/no/file/here") + with pytest.raises(FileNotFoundError): + start.set_gunicorn_options() + + +class TestSetUvicornOptions: + """Test Uvicorn configuration options method. + --- + """ + + def test_set_uvicorn_options_default(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Test default Uvicorn server options.""" + monkeypatch.setenv("WITH_RELOAD", "false") + result = start.set_uvicorn_options() + assert isinstance(result, dict) + assert result == dict( + host="0.0.0.0", + port=80, + log_config=None, + log_level="info", + reload=False, + reload_dirs=None, ) - def test_run_pre_start_script_no_file( - self, - mocker: MockerFixture, - monkeypatch: pytest.MonkeyPatch, + def test_set_uvicorn_options_custom( + self, logging_conf_dict: dict, monkeypatch: pytest.MonkeyPatch ) -> None: - """Test `start.run_pre_start_script` with an incorrect file path.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - monkeypatch.setenv("PRE_START_PATH", "/no/file/here") - start.run_pre_start_script(logger=mock_logger) - mock_logger.debug.assert_has_calls( - calls=[ - mocker.call("Checking for pre-start script."), - mocker.call("No pre-start script found."), - ] + """Test custom Uvicorn server options.""" + monkeypatch.setenv("LOG_LEVEL", "debug") + monkeypatch.setenv("WITH_RELOAD", "true") + monkeypatch.setenv("RELOAD_DIRS", "inboard, tests") + result = start.set_uvicorn_options(log_config=logging_conf_dict) + assert isinstance(result, dict) + assert result == dict( + host="0.0.0.0", + port=80, + log_config=logging_conf_dict, + log_level="debug", + reload=True, + reload_dirs=["inboard", "tests"], )