diff --git a/eventing/shared/pidfile.py b/eventing/shared/pidfile.py index 9b6dc949..796c26ce 100644 --- a/eventing/shared/pidfile.py +++ b/eventing/shared/pidfile.py @@ -2,8 +2,8 @@ `PidFile(name)` writes `${TMPDIR}/rossoctl-keda1/.pid` on __enter__, removes it on __exit__. Refuses to start if the file exists and points at a live process -(unless RUN_FORCE=1 is set in the env). Stale PID files (process gone) are -silently reclaimed. +(unless RUN_FORCE=1 is set in the env). Stale PID files (process gone, or naming +our own PID) are reclaimed, with a line on stdout saying so. """ from __future__ import annotations @@ -44,7 +44,15 @@ def __enter__(self) -> "PidFile": existing = int(self._path.read_text().strip() or "0") except ValueError: existing = 0 - if existing and _pid_alive(existing): + if existing == os.getpid(): + # We have not written this file yet, so our own PID in it means the + # previous process died abruptly and the restarted one was handed the + # same PID. In a container that is every restart: the process is PID 1, + # `_pid_alive(1)` is True because PID 1 is the caller itself, and the + # guard below would refuse to start forever ("already running as pid 1"). + print(f"[{self._name}] reclaimed stale pidfile naming our own pid" + f" {existing}: {self._path}") + elif existing and _pid_alive(existing): if os.environ.get("RUN_FORCE") != "1": raise SystemExit( f"[{self._name}] already running as pid {existing}" diff --git a/eventing/tests/test_pidfile.py b/eventing/tests/test_pidfile.py index f526a5b9..0b2a8f34 100644 --- a/eventing/tests/test_pidfile.py +++ b/eventing/tests/test_pidfile.py @@ -1,12 +1,31 @@ """PidFile: writes on enter, removes on exit, guards against double-start.""" import os import pathlib +import subprocess +import sys import pytest from shared.pidfile import PidFile, _pid_alive +@pytest.fixture +def live_other_pid(): + """A process that is alive, is not us, and has a non-zero pid in any namespace. + + `os.getppid()` was the first stand-in, but it degrades to 0 when the suite itself + runs as PID 1 in a container (`docker run ... pytest`): the pidfile would then hold + 0, `__enter__` reads a falsy `existing`, takes the stale branch and never refuses. + A spawned child has no ambient-process-tree dependency. + """ + p = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"]) + try: + yield p.pid + finally: + p.terminate() + p.wait() + + def test_writes_and_removes(tmp_path: pathlib.Path): with PidFile("eb-test", directory=tmp_path) as pf: assert pf.path.exists() @@ -21,23 +40,49 @@ def test_stale_pidfile_is_reclaimed(tmp_path: pathlib.Path): assert pf.path.read_text().strip() == str(os.getpid()) -def test_live_pidfile_refuses_start(tmp_path: pathlib.Path, monkeypatch): - # Use *our own* pid — guaranteed to be alive - (tmp_path / "eb-test.pid").write_text(f"{os.getpid()}\n") +def test_live_pidfile_refuses_start(tmp_path: pathlib.Path, monkeypatch, live_other_pid): + # The fixture's child: alive, and -- unlike our own pid -- not us. A pidfile naming + # our own pid is deliberately reclaimable (see the PID 1 test below), so it cannot + # stand in for "another process is running" here. + (tmp_path / "eb-test.pid").write_text(f"{live_other_pid}\n") monkeypatch.delenv("RUN_FORCE", raising=False) with pytest.raises(SystemExit) as exc: with PidFile("eb-test", directory=tmp_path): pass assert "already running" in str(exc.value) # Pidfile must remain untouched so the running process's cleanup still works - assert (tmp_path / "eb-test.pid").read_text().strip() == str(os.getpid()) + assert (tmp_path / "eb-test.pid").read_text().strip() == str(live_other_pid) -def test_run_force_overrides(tmp_path: pathlib.Path, monkeypatch): - (tmp_path / "eb-test.pid").write_text(f"{os.getpid()}\n") +def test_run_force_overrides(tmp_path: pathlib.Path, monkeypatch, live_other_pid, capsys): + (tmp_path / "eb-test.pid").write_text(f"{live_other_pid}\n") monkeypatch.setenv("RUN_FORCE", "1") + with PidFile("eb-test", directory=tmp_path): + pass + # The override branch announces itself; the stale-reclaim branch does not. Without + # this, the pidfile content alone cannot tell the two apart (both end in write_text). + out = capsys.readouterr().out + assert "WARNING: overwriting live pidfile" in out + + +def test_a_pidfile_naming_our_own_pid_is_reclaimed_not_refused( + tmp_path: pathlib.Path, monkeypatch): + """The container crash-loop: in a pod the restarted process is PID 1 again. + + An abrupt exit (OOMKill at the 512Mi limit, SIGKILL, node failure) leaves a pidfile + naming 1 on the mounted volume -- both images set TMPDIR=/data -- and on restart + `_pid_alive(1)` is True because PID 1 is the caller itself. The guard then refused + to start forever, and on the demo overlay's PVC deleting the pod did not help. + + Our own PID stands in for PID 1: the property that matters is that the pidfile names + the process doing the check, which cannot be a different live process. + Reported by @huang195 during review of rossoctl/rossoctl#2609. + """ + (tmp_path / "eb-test.pid").write_text(f"{os.getpid()}\n") + monkeypatch.delenv("RUN_FORCE", raising=False) with PidFile("eb-test", directory=tmp_path) as pf: assert pf.path.read_text().strip() == str(os.getpid()) + assert not pf.path.exists(), "and normal cleanup still applies afterwards" def test_pid_alive_zero_and_negative_are_dead():