Refactor Gunicorn worker calculation and tests

#11
02b249e
fd60470
https://docs.python.org/3/reference/expressions.html#boolean-operations

The `gunicorn_conf.calculate_workers()` method returns a desired number
of Gunicorn workers, based on the arguments passed in. The arguments are
read from environment variables. The calculation is deceptively complex,
and a variety of edge cases emerged over time. Commit fd60470 fixed the
web concurrency calculation, but not the max workers calculation. There
should have been an `else use_max_workers if max_workers_str` condition.
The tests were correspondingly complex and convoluted, testing various
interrelated conditions, and becoming less and less readable.

This commit will refactor the Gunicorn worker calculation and tests. The
result is more readable (and correct) code.

Refactor the `gunicorn_conf.calculate_workers()` method arguments:

- Remove `_str` from function arguments: arguments are type-annotated,
  and it is redundant to add `_str` to arguments annotated as strings.
- Rename the `web_concurrency_str` function argument to `total_workers`:
  the "web concurrency" name is a legacy Gunicorn environment variable,
  and doesn't really convey what the setting does. "Web concurrency" is
  a total number of workers, so just call it `total_workers`.
- Make the `workers_per_core` argument a keyword argument (kwarg): the
  corresponding environment variable `WORKERS_PER_CORE` has a default,
  so set the same default on the kwarg.
- Move the cpu cores calculation into the function body: not necessary
  to set a number of cores, so just use `multiprocessing.cpu_count()`.
- Keep same order of arguments and argument type-annotations to avoid
  breaking existing functionality and to ensure backwards-compatibility.

Refactor the `gunicorn_conf.calculate_workers()` method logic, by using
`if` expressions to more clearly evaluate each condition, then returning
the correct value by using `or` to evaluate Boolean conditions in order.

Improve test parametrization to more thoroughly test use cases, and
refactor tests so that each one tests an isolated use case:

- Check defaults
- Set maximum number of workers
- Set total number of workers ("web concurrency")
- Set desired number of workers per core
- Set both maximum number of workers and total number of workers

inboard/gunicorn_conf.py

@@ -6,24 +6,17 @@
 
 
 def calculate_workers(
-    max_workers_str: Optional[str],
-    web_concurrency_str: Optional[str],
-    workers_per_core_str: str,
-    cores: int = multiprocessing.cpu_count(),
+    max_workers: Optional[str] = None,
+    total_workers: Optional[str] = None,
+    workers_per_core: str = "1",
 ) -> int:
     """Calculate the number of Gunicorn worker processes."""
-    use_default_workers = max(int(float(workers_per_core_str) * cores), 2)
-    if max_workers_str and int(max_workers_str) > 0:
-        use_max_workers = int(max_workers_str)
-    if web_concurrency_str and int(web_concurrency_str) > 0:
-        use_web_concurrency = int(web_concurrency_str)
-    return (
-        min(use_max_workers, use_web_concurrency)
-        if max_workers_str and web_concurrency_str
-        else use_web_concurrency
-        if web_concurrency_str
-        else use_default_workers
-    )
+    cores = multiprocessing.cpu_count()
+    use_default = max(int(float(workers_per_core) * cores), 2)
+    use_max = m if max_workers and (m := int(max_workers)) > 0 else False
+    use_total = t if total_workers and (t := int(total_workers)) > 0 else False
+    use_least = min(use_max, use_total) if use_max and use_total else False
+    return use_least or use_max or use_total or use_default
 
 
 # Gunicorn setup

tests/test_gunicorn_conf.py

@@ -1,5 +1,4 @@
 import multiprocessing
-import os
 from typing import Optional
 
 import pytest
@@ -12,74 +11,42 @@ class TestCalculateWorkers:
     ---
     """
 
-    def test_gunicorn_conf_workers_default(self) -> None:
+    def test_calculate_workers_default(self) -> None:
         """Test default number of Gunicorn worker processes."""
+        cores = multiprocessing.cpu_count()
         assert gunicorn_conf.workers >= 2
-        assert gunicorn_conf.workers == multiprocessing.cpu_count()
-
-    def test_gunicorn_conf_workers_custom_max(
-        self, monkeypatch: pytest.MonkeyPatch
-    ) -> None:
-        """Test custom Gunicorn worker process calculation."""
-        monkeypatch.setenv("MAX_WORKERS", "1")
-        monkeypatch.setenv("WEB_CONCURRENCY", "4")
-        monkeypatch.setenv("WORKERS_PER_CORE", "0.5")
-        assert os.getenv("MAX_WORKERS") == "1"
-        assert os.getenv("WEB_CONCURRENCY") == "4"
-        assert os.getenv("WORKERS_PER_CORE") == "0.5"
-        assert (
-            gunicorn_conf.calculate_workers(
-                str(os.getenv("MAX_WORKERS")),
-                str(os.getenv("WEB_CONCURRENCY")),
-                str(os.getenv("WORKERS_PER_CORE")),
-            )
-            == 1
-        )
-
-    @pytest.mark.parametrize("number_of_workers", ["1", "2", "4"])
-    def test_gunicorn_conf_workers_custom_concurrency(
-        self, monkeypatch: pytest.MonkeyPatch, number_of_workers: str
-    ) -> None:
-        """Test custom Gunicorn worker process calculation."""
-        monkeypatch.setenv("WEB_CONCURRENCY", number_of_workers)
-        monkeypatch.setenv("WORKERS_PER_CORE", "0.5")
-        assert os.getenv("WEB_CONCURRENCY") == number_of_workers
-        assert os.getenv("WORKERS_PER_CORE") == "0.5"
-        assert (
-            gunicorn_conf.calculate_workers(
-                None,
-                str(os.getenv("WEB_CONCURRENCY")),
-                str(os.getenv("WORKERS_PER_CORE")),
-            )
-            == int(number_of_workers)
-        )
-
-    @pytest.mark.parametrize("concurrency", [None, "10"])
-    def test_gunicorn_conf_workers_custom_cores(
-        self, monkeypatch: pytest.MonkeyPatch, concurrency: Optional[str]
+        assert gunicorn_conf.workers == max(cores, 2)
+
+    @pytest.mark.parametrize("max_workers", (None, "1", "2", "5", "10"))
+    def test_calculate_workers_max(self, max_workers: Optional[str]) -> None:
+        """Test Gunicorn worker process calculation with custom maximum."""
+        cores = multiprocessing.cpu_count()
+        result = gunicorn_conf.calculate_workers(max_workers, None)
+        assert result == int(max_workers) if max_workers else max(cores, 2)
+
+    @pytest.mark.parametrize("total_workers", (None, "1", "2", "5", "10"))
+    def test_calculate_workers_total(self, total_workers: Optional[str]) -> None:
+        """Test Gunicorn worker process calculation with custom total."""
+        cores = multiprocessing.cpu_count()
+        result = gunicorn_conf.calculate_workers(None, total_workers)
+        assert result == int(total_workers) if total_workers else max(cores, 2)
+
+    @pytest.mark.parametrize("workers_per_core", ("0.5", "1.5", "10"))
+    def test_calculate_workers_per_core(self, workers_per_core: str) -> None:
+        """Test Gunicorn worker process calculation with custom workers per core.
+        Worker number should be the greater of 2 or the workers per core setting.
+        """
+        cores = multiprocessing.cpu_count()
+        result = gunicorn_conf.calculate_workers(workers_per_core=workers_per_core)
+        assert result == max(int(float(workers_per_core) * cores), 2)
+
+    @pytest.mark.parametrize("max_workers", ("1", "2", "5", "10"))
+    @pytest.mark.parametrize("total_workers", ("1", "2", "5", "10"))
+    def test_calculate_workers_both_max_and_total(
+        self, max_workers: str, total_workers: str
     ) -> None:
-        """Test custom Gunicorn worker process calculation.
-        - Assert that number of workers equals `WORKERS_PER_CORE`, and is at least 2.
-        - Assert that setting `WEB_CONCURRENCY` overrides `WORKERS_PER_CORE`.
+        """Test Gunicorn worker process calculation if max workers and total workers
+        (web concurrency) are both set. Worker number should be the lesser of the two.
         """
-        monkeypatch.setenv("WORKERS_PER_CORE", "0.5")
-        workers_per_core = os.getenv("WORKERS_PER_CORE")
-        assert workers_per_core == "0.5"
-        cores: int = multiprocessing.cpu_count()
-        assert gunicorn_conf.calculate_workers(
-            None, None, workers_per_core, cores=cores
-        ) == max(int(cores * float(workers_per_core)), 2)
-        assert (
-            gunicorn_conf.calculate_workers(None, "10", workers_per_core, cores=cores)
-            == 10
-        )
-        monkeypatch.setenv("WEB_CONCURRENCY", concurrency) if concurrency else None
-        assert os.getenv("WEB_CONCURRENCY") == concurrency
-        assert (
-            gunicorn_conf.calculate_workers(
-                None, concurrency, workers_per_core, cores=cores
-            )
-            == 10
-            if concurrency
-            else max(int(cores * float(workers_per_core)), 2)
-        )
+        result = gunicorn_conf.calculate_workers(max_workers, total_workers)
+        assert result == min(int(max_workers), int(total_workers))