Add unit tests for reactive.runner_manager

src-docs/runner_manager.md

@@ -13,6 +13,7 @@ Runner Manager manages the runners on LXD and GitHub.
 - **REACTIVE_RUNNER_SCRIPT_FILE**
 - **REACTIVE_RUNNER_TIMEOUT_STR**
 - **PYTHON_BIN**
+- **PS_COMMAND_LINE_LIST**
 - **TIMEOUT_COMMAND**
 - **UBUNTU_USER**
 

src/reactive/runner.py

@@ -3,7 +3,7 @@
 import argparse
 import logging
 
-from reactive.job import Job, MessageQueueConnectionInfo, JobError
+from reactive.job import Job, JobError, MessageQueueConnectionInfo
 
 logger = logging.getLogger(__name__)
 

src/reactive/runner_manager.py

@@ -1,7 +1,6 @@
 #  Copyright 2024 Canonical Ltd.
 #  See LICENSE file for licensing details.
 import logging
-import os
 import shutil
 import subprocess
 from pathlib import Path
@@ -15,6 +14,7 @@
 REACTIVE_RUNNER_SCRIPT_FILE = "scripts/reactive_runner.py"
 REACTIVE_RUNNER_TIMEOUT_STR = "1h"
 PYTHON_BIN = "/usr/bin/python3"
+PS_COMMAND_LINE_LIST = ["ps", "axo", "cmd"]
 TIMEOUT_COMMAND = "/usr/bin/timeout"
 UBUNTU_USER = "ubuntu"
 
@@ -40,23 +40,19 @@ def reconcile(self, quantity: int) -> int:
             ReactiveRunnerError: If the runner fails to spawn.
         """
 
-        result = secure_run_subprocess(cmd=["ps", "axo", "cmd"])
-        if result.returncode != 0:
-            raise ReactiveRunnerError("Failed to get list of processes")
-
-        commands = result.stdout.decode().rstrip().split("\n")[1:]
-        logger.info(commands)
-        actual_quantity = 0
-        for command in commands:
-            if command.startswith(f"{PYTHON_BIN} {REACTIVE_RUNNER_SCRIPT_FILE}"):
-                actual_quantity += 1
+        actual_quantity = self._determine_current_quantity()
         logger.info("Actual quantity of ReactiveRunner processes: %s", actual_quantity)
         delta = quantity - actual_quantity
+        actual_delta = delta
         if delta > 0:
             logger.info("Will spawn %d new ReactiveRunner processes", delta)
             self._setup_log_file()
             for _ in range(delta):
-                self._spawn_runner()
+                try:
+                    self._spawn_runner()
+                except ReactiveRunnerError:
+                    logger.exception("Failed to spawn a new ReactiveRunner process")
+                    actual_delta -= 1
         elif delta < 0:
             logger.info(
                 "%d ReactiveRunner processes are running. Will skip spawning. Additional processes should terminate after %s.",
@@ -66,8 +62,27 @@ def reconcile(self, quantity: int) -> int:
         else:
             logger.info("No changes to number of ReactiveRunners needed.")
 
-        return max(delta, 0)
+        return max(actual_delta, 0)
+
+    def _determine_current_quantity(self):
+        """Determine the current quantity of reactive runners.
+
+        Returns:
+            The number of reactive runners.
 
+        Raises:
+            ReactiveRunnerError: If the number of reactive runners cannot be determined
+        """
+        result = secure_run_subprocess(cmd=PS_COMMAND_LINE_LIST)
+        if result.returncode != 0:
+            raise ReactiveRunnerError("Failed to get list of processes")
+        commands = result.stdout.decode().rstrip().split("\n")[1:]
+        logger.debug(commands)
+        actual_quantity = 0
+        for command in commands:
+            if command.startswith(f"{PYTHON_BIN} {REACTIVE_RUNNER_SCRIPT_FILE}"):
+                actual_quantity += 1
+        return actual_quantity
 
     def _setup_log_file(self) -> None:
         """Set up the log file."""
@@ -76,7 +91,6 @@ def _setup_log_file(self) -> None:
             logfile.touch()
             shutil.chown(logfile, user=UBUNTU_USER, group=UBUNTU_USER)
 
-
     def _spawn_runner(self) -> None:
         """Spawn a runner."""
         env = {"PYTHONPATH": "src:lib:venv"}
@@ -89,11 +103,11 @@ def _spawn_runner(self) -> None:
                 self._reactive_config.mq_uri,
                 self._queue_name,
             ],
-            # TODO: we are using devnull because there are hanging issues if we reuse the same fd as the parent
-            # maybe add a check if the process is alive
             stdout=subprocess.DEVNULL,
             stderr=subprocess.DEVNULL,
             env=env,
             user=UBUNTU_USER,
         )
         logger.debug("Spawned a new ReactiveRunner process with pid %s", process.pid)
+        if process.returncode not in (0, None):
+            raise ReactiveRunnerError("Failed to spawn a new ReactiveRunner process")

tests/unit/reactive/test_runner.py

@@ -6,10 +6,10 @@
 from pathlib import Path
 from unittest.mock import MagicMock
 
-from pytest import MonkeyPatch, LogCaptureFixture
 import pytest
+from pytest import LogCaptureFixture, MonkeyPatch
 
-from reactive.job import Job, JobDetails, MessageQueueConnectionInfo, JobError
+from reactive.job import Job, JobDetails, JobError, MessageQueueConnectionInfo
 from reactive.runner import reactive_runner
 
 

tests/unit/reactive/test_runner_manager.py

@@ -0,0 +1,164 @@
+#  Copyright 2024 Canonical Ltd.
+#  See LICENSE file for licensing details.
+import random
+import subprocess
+from pathlib import Path
+from subprocess import CompletedProcess
+from unittest.mock import MagicMock
+
+import pytest
+
+from charm_state import ReactiveConfig
+from reactive.runner_manager import (
+    PS_COMMAND_LINE_LIST,
+    PYTHON_BIN,
+    REACTIVE_RUNNER_SCRIPT_FILE,
+    ReactiveRunnerError,
+    ReactiveRunnerManager,
+)
+from utilities import secure_run_subprocess
+
+
+@pytest.fixture(name="log_file_path", autouse=True)
+def log_file_path_fixture(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path:
+    """Return the path to the log file."""
+    log_file_path = tmp_path / "log.txt"
+    monkeypatch.setattr("reactive.runner_manager.REACTIVE_RUNNER_LOG_FILE", log_file_path)
+    monkeypatch.setattr("shutil.chown", lambda *args, **kwargs: None)
+    return log_file_path
+
+
+@pytest.fixture(name="secure_run_subprocess_mock")
+def secure_run_subprocess_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock:
+    """Mock the ps command."""
+    secure_run_subprocess_mock = MagicMock(spec=secure_run_subprocess)
+    monkeypatch.setattr(
+        "reactive.runner_manager.secure_run_subprocess", secure_run_subprocess_mock
+    )
+    return secure_run_subprocess_mock
+
+
+@pytest.fixture(name="subprocess_popen_mock")
+def subprocess_popen_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock:
+    """Mock the subprocess.Popen function."""
+    subprocess_popen_mock = MagicMock(
+        spec=subprocess.Popen,
+        return_value=MagicMock(pid=1234, returncode=random.choice([None, 0])),
+    )
+    monkeypatch.setattr("subprocess.Popen", subprocess_popen_mock)
+    return subprocess_popen_mock
+
+
+def test_reconcile_spawns_runners(
+    secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, log_file_path: Path
+):
+    """
+    arrange: Mock that two reactive runner processes are active.
+    act: Call reconcile with a quantity of 5.
+    assert: Three runners are spawned. Log file is setup.
+    """
+    reactive_config = ReactiveConfig(mq_uri="http://example.com")
+    _arrange_reactive_processes(secure_run_subprocess_mock, count=2)
+
+    manager = ReactiveRunnerManager(reactive_config, "test-queue")
+    delta = manager.reconcile(5)
+
+    assert delta == 3
+    assert subprocess_popen_mock.call_count == 3
+    assert log_file_path.exists()
+
+
+def _arrange_reactive_processes(secure_run_subprocess_mock: MagicMock, count: int):
+    """Mock reactive runner processes are active.
+
+    Args:
+        secure_run_subprocess_mock: The mock to use for the ps command.
+        count: The number of processes
+    """
+    process_cmds = "\n".join([f"{PYTHON_BIN} {REACTIVE_RUNNER_SCRIPT_FILE}" for _ in range(count)])
+    secure_run_subprocess_mock.return_value = CompletedProcess(
+        args=PS_COMMAND_LINE_LIST,
+        returncode=0,
+        stdout=f"CMD\n{process_cmds}".encode("utf-8"),
+        stderr=b"",
+    )
+
+
+def test_reconcile_does_not_spawn_runners(
+    secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock
+):
+    """
+    arrange: Mock that two reactive runner processes are active.
+    act: Call reconcile with a quantity of 2.
+    assert: No runners are spawned.
+    """
+    reactive_config = ReactiveConfig(mq_uri="http://example.com")
+    _arrange_reactive_processes(secure_run_subprocess_mock, count=2)
+
+    manager = ReactiveRunnerManager(reactive_config, "test-queue")
+    delta = manager.reconcile(2)
+
+    assert delta == 0
+    assert subprocess_popen_mock.call_count == 0
+
+
+def test_reconcile_does_not_spawn_runners_for_too_many_processes(
+    secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock
+):
+    """
+    arrange: Mock that two reactive runner processes are active.
+    act: Call reconcile with a quantity of 1.
+    assert: No runners are spawned and delta is 0.
+    """
+    reactive_config = ReactiveConfig(mq_uri="http://example.com")
+    _arrange_reactive_processes(secure_run_subprocess_mock, count=2)
+    manager = ReactiveRunnerManager(reactive_config, "test-queue")
+    delta = manager.reconcile(1)
+
+    assert delta == 0
+    assert subprocess_popen_mock.call_count == 0
+
+
+def test_reconcile_raises_reactive_runner_error_on_ps_failure(
+    secure_run_subprocess_mock: MagicMock,
+):
+    """
+    arrange: Mock that the ps command fails.
+    act: Call reconcile with a quantity of 1.
+    assert: A ReactiveRunnerError is raised.
+    """
+    secure_run_subprocess_mock.return_value = CompletedProcess(
+        args=PS_COMMAND_LINE_LIST,
+        returncode=1,
+        stdout=b"",
+        stderr=b"error",
+    )
+
+    reactive_config = ReactiveConfig(mq_uri="http://example.com")
+    manager = ReactiveRunnerManager(reactive_config, "test-queue")
+    with pytest.raises(ReactiveRunnerError) as err:
+        manager.reconcile(1)
+
+    assert "Failed to get list of processes" in str(err.value)
+
+
+def test_reconcile_spawn_runner_failed(
+    secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock
+):
+    """
+    arrange: Mock that one reactive runner spawn fails.
+    act: Call reconcile with a quantity of 3.
+    assert: The delta is 2.
+    """
+    subprocess_popen_mock.side_effect = [
+        MagicMock(returncode=0),
+        MagicMock(return_code=1),
+        MagicMock(returncode=0),
+    ]
+    _arrange_reactive_processes(secure_run_subprocess_mock, count=0)
+
+    reactive_config = ReactiveConfig(mq_uri="http://example.com")
+    manager = ReactiveRunnerManager(reactive_config, "test-queue")
+    delta = manager.reconcile(3)
+
+    assert delta == 2