Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions eventing/shared/pidfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

`PidFile(name)` writes `${TMPDIR}/rossoctl-keda1/<name>.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

Expand Down Expand Up @@ -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}"
Expand Down
57 changes: 51 additions & 6 deletions eventing/tests/test_pidfile.py
Original file line number Diff line number Diff line change
@@ -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()
Expand All @@ -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():
Expand Down
Loading