diff --git a/AGENTS.md b/AGENTS.md index 41202321..c5a0443f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -42,7 +42,11 @@ src/panopticon/ # workflows; the spawner routes on it, skipping the image + the clone unless the # workflow opts in via clone_repo); images.py = ADR-0005 composed images # (base→workflow→repo); provisioner.py = host-side provisioning - # (ADR 0011: branch the per-task clone on slug, record it back); clones.py = + # (ADR 0011: branch the per-task clone on slug, record it back); priority.py = + # host-side resource priority (env → argv: the deprioritizing `docker run` flags + # every task container gets — cpu/blkio weight floors + a raised OOM score — the + # agent pane's own oom_score_adj, `nice` for shell tasks, and the cgroup-flag + # strip the runner degrades through on a daemon that refuses them); clones.py = # per-repo clone cache; spawn.py = spawn-prep (clone --local the per-task # checkout, mounted rw at /workspace; point origin at the forge, then init any # submodules — in that order, since relative .gitmodules URLs resolve against @@ -251,6 +255,13 @@ on every PR (the same commands the Makefile wraps). unresumable-transcript recognizer: the SDK marker shapes captured off the two tasks this broke, the newest-first resume order, and the regression that an SDK-only project now yields a first-run argv instead of the `--continue` that killed the pane. +- `tests/sessionservice/test_priority.py` — resource priority (env → argv): the shipped defaults + every task container is spawned with, each per-host override, the `off` switch that returns the + argv to its pre-priority form, the clamps (a negative OOM adjustment refused), a bad value falling + back rather than failing a spawn, and the cgroup-flag strip. `test_local_runner.py` covers the + wiring — the flags on `docker run`, the pane wrapper that raises the exec'd agent's OOM score + (`docker exec` doesn't inherit the container's), and the retry-without-cgroup-flags fallback plus + its latch; `test_shell_runner.py` covers the `nice` prefix on a shell task's host session. - `tests/test_prefill.py` — the input-box prefill poller: unit tests drive `prefill_pane` with a fake tmux runner + injected `sleep`/raw-log — pin the `pipe-pane`/`load-buffer`/`paste-buffer -p` commands when the box becomes ready, and every best-effort give-up (empty prompt, timeout, diff --git a/docs/container.md b/docs/container.md index 318ac77a..0b2a3e1c 100644 --- a/docs/container.md +++ b/docs/container.md @@ -116,6 +116,73 @@ For what a slug, branch, clone, and `provisioned` mean as task concepts, see [ta - **Auth.** The agent authenticates from a `CLAUDE_CODE_OAUTH_TOKEN` injected from the repo's env-file — see [auth](auth.md). +## Resource priority: tasks yield to you + +A task container is where unbounded work happens — an agent running a repo's whole test suite, a +`docker build` inside a dind task, six tasks at once — on the same machine as your editor, your +shell, and panopticon's own control plane. So every container is spawned **deprioritized**: it +loses CPU and disk races against normally-weighted processes, and under real memory pressure the +kernel kills it before anything of yours. + +This is *priority*, **not a cap**. Nothing limits how much CPU or RAM a task may use when nobody +else wants it, so on an idle host tasks run at full speed. + +Concretely, each container gets `--cpu-shares 2` (docker's floor — cgroup v2 `cpu.weight` 1), +`--blkio-weight 10` (the floor; disk contention is what actually makes a desktop stutter) and +`--oom-score-adj 500`. The agent pane raises its own OOM score too: `oom_score_adj` is +per-process and inherited across *fork*, and the pane is started with `docker exec` (forked from +the Docker daemon, not from the container's PID 1), so the flag on `docker run` would otherwise +miss the one process that actually eats memory. The cgroup levers need no such trick — an exec'd +process joins the container's cgroup. The workspace-cleanup sweep runs deprioritized as well. + +If a task does get OOM-killed you'll see it: the runner reads `OOMKilled` off `docker inspect` +before the exit code, so the task shows `failed` with an out-of-memory detail rather than a +mystery. + +### Tuning it per host + +The knobs are read from the **runner host's** environment, so a big build box and a laptop can +differ. Set any of them to `off` (or empty) to drop that flag entirely — the argv is then exactly +what panopticon emitted before any of this existed. + +| Variable | Default | What it sets | +|---|---|---| +| `PANOPTICON_CONTAINER_CPU_SHARES` | `2` | CPU weight (`--cpu-shares`) | +| `PANOPTICON_CONTAINER_BLKIO_WEIGHT` | `10` | block-IO weight (`--blkio-weight`) | +| `PANOPTICON_CONTAINER_OOM_SCORE_ADJ` | `500` | OOM-killer preference, for the container *and* the agent pane | +| `PANOPTICON_CONTAINER_CGROUP_PARENT` | unset | parent cgroup (`--cgroup-parent`) — see below | +| `PANOPTICON_HOST_NICE` | `19` | `nice` for a shell task's host session (no container to weight) | + +A bad value warns and falls back to the default instead of failing the spawn, and values are +clamped to what docker and the kernel accept (a *negative* OOM adjustment — shielding a task at +the host's expense — is refused). + +**Hosts that refuse the flags.** Some daemons reject `--cpu-shares`/`--blkio-weight` outright: a +nested Docker daemon whose cgroup is in threaded mode answers any of them with `unable to apply +cgroup configuration`. A spawn is never lost to that — the runner retries once without the +cgroup-backed flags (keeping `--oom-score-adj`, which needs no controller), logs one warning, and +remembers the answer for the rest of its life. + +**Going further with a slice.** With docker's systemd cgroup driver, containers land in +`system.slice/docker-.scope` while your own processes sit in `user.slice`, and cgroup weights +are compared **between siblings** — so weight 1 on a container's scope deprioritizes it *within* +`system.slice` but doesn't by itself make it lose to `user.slice`. If you want that enforced at the +hierarchy level, create a low-weight slice and point panopticon at it: + +```ini +# /etc/systemd/system/panopticon.slice +[Slice] +CPUWeight=1 +IOWeight=1 +``` + +```sh +sudo systemctl daemon-reload +export PANOPTICON_CONTAINER_CGROUP_PARENT=panopticon.slice # on the runner host +``` + +Installing a unit is your call, so per-container weights stay the zero-setup default. + ## When it goes wrong A container can disappear out from under a live task — an OOM kill, a host reboot, a `docker rm`. diff --git a/src/panopticon/sessionservice/local_runner.py b/src/panopticon/sessionservice/local_runner.py index 3e0bb6fb..b94abdbe 100644 --- a/src/panopticon/sessionservice/local_runner.py +++ b/src/panopticon/sessionservice/local_runner.py @@ -11,6 +11,7 @@ from __future__ import annotations +import logging import os import shlex import subprocess @@ -21,8 +22,17 @@ from panopticon.core.dirs import credential_dir_path, secrets_file_path from panopticon.core.features import CODEX_FLAG, codex_enabled from panopticon.core.models import DEFAULT_AGENT_CLI, LifecyclePhase +from panopticon.sessionservice.priority import ( + BLKIO_WEIGHT_VAR, + CPU_SHARES_VAR, + container_oom_score_adj, + container_priority_flags, + strip_cgroup_flags, +) from panopticon.sessionservice.runner import Runner +_log = logging.getLogger(__name__) + #: The container home the per-CLI config dir lives under (the base image's ``panopticon`` user). CONTAINER_HOME = "/home/panopticon" @@ -130,6 +140,29 @@ def _invoking_user() -> str: return f"{os.getuid()}:{os.getgid()}" +def _deprioritized(command: Sequence[str]) -> list[str]: + """``command``, wrapped so it raises its own OOM score before exec'ing (or unchanged when off). + + The agent pane is started with ``docker exec``, and ``oom_score_adj`` is a **per-process** + attribute inherited across fork — the pane forks from the Docker daemon, not from the + container's PID 1, so ``docker run --oom-score-adj`` does *not* reach it. That leaves the one + process that actually eats memory (the agent CLI and its children) at the host's default score, + which defeats the point. ``docker exec`` has no flag for it, so the command writes + ``/proc/self/oom_score_adj`` itself — raising your own score needs no privilege (only + *lowering* it does) — and then ``exec``s, keeping the process tree and signal behaviour + identical to running the command directly. Best-effort: a host that refuses the write is + tolerated (``|| true``) rather than losing the agent. The cgroup-based flags need no such + treatment — an exec'd process joins the container's cgroup like any other.""" + adj = container_oom_score_adj() + if adj is None: + return list(command) + return [ + "sh", + "-c", + f"echo {adj} > /proc/self/oom_score_adj 2>/dev/null || true; exec {shlex.join(command)}", + ] + + class LocalRunner(Runner): """Runs task containers + host tmux on the local Docker daemon (one host).""" @@ -161,12 +194,54 @@ def __init__( self._agent_command = list(agent_command) self._tmux_socket = tmux_socket # isolate panopticon's tmux server when set (-L) self._extra_env = dict(extra_env or {}) + # Set once a `docker run` proves this daemon can't apply the cgroup priority flags, so the + # flagless retry in `_run_container` is paid at most once per process, not per spawn. + self._cgroup_flags_unsupported = False self._run = run def _tmux(self, *args: str) -> list[str]: prefix = ["tmux", *(["-L", self._tmux_socket] if self._tmux_socket else [])] return [*prefix, *args] + def _priority_flags(self) -> list[str]: + """The deprioritizing ``docker run`` flags for a task container (see + :mod:`panopticon.sessionservice.priority`), minus the cgroup ones once this daemon has + proved it can't apply them.""" + flags = container_priority_flags() + return strip_cgroup_flags(flags) if self._cgroup_flags_unsupported else flags + + def _run_container(self, docker_run: Sequence[str], container: str | None = None) -> None: + """``docker run``, degrading rather than failing on a daemon that refuses the cgroup + priority flags. + + Some daemons reject ``--cpu-shares``/``--blkio-weight`` outright — a nested daemon whose + cgroup is in threaded mode answers with ``unable to apply cgroup configuration`` — and no + work may be lost to a host that merely can't deprioritize. So a failed flagged run is + retried once without those flags; when the run named a container (a task spawn), the + half-created one is cleared first, since the name is taken. The "this daemon can't do it" + latch is set **only if the retry succeeds**: an ordinary failure (a bad image, say) then + propagates as before, with later runs still asking for full priority control.""" + stripped = strip_cgroup_flags(docker_run) + if list(stripped) == list(docker_run): # nothing to fall back to + self._run(docker_run) + return + try: + self._run(docker_run) + return + except Exception as exc: # any runner failure is worth one flagless retry + first_error = exc + if container is not None: + self._run(["docker", "rm", "--force", container], check=False) + self._run(stripped) # still failing => a real error, raised as it would have been anyway + self._cgroup_flags_unsupported = True + _log.warning( + "docker refused the cgroup priority flags (%s) — running task containers without " + "them; they keep --oom-score-adj. Set %s/%s to `off` to silence this.", + first_error, + CPU_SHARES_VAR, + BLKIO_WEIGHT_VAR, + ) + def spawn( self, task_id: str, @@ -252,6 +327,11 @@ def _report(phase: LifecyclePhase) -> None: f"panopticon.task={task_id}", "--add-host", HOST_GATEWAY, + # Run the task at the lowest priority we can: it yields CPU and disk to the operator's + # own processes and is the kernel's first OOM pick, so a runaway agent or a heavy + # in-task build can't destabilize the host (see `priority`). Not a cap — an idle host + # still runs the task at full speed. + *self._priority_flags(), ] if ( docker_in_docker @@ -288,7 +368,7 @@ def _report(phase: LifecyclePhase) -> None: self._run(self._tmux("kill-session", "-t", container), check=False) self._run(["docker", "rm", "--force", container], check=False) _report(LifecyclePhase.STARTING) # docker run + the tmux session coming up - self._run(docker_run) + self._run_container(docker_run, container) # The pane is a host shell command: wait (bounded) for the entrypoint's READY_MARKER — the # remap-complete signal — then exec in as the unprivileged `panopticon` user, so `tmux # attach` and the agent's `whoami` see that named user, not root (and never the pre-remap @@ -303,7 +383,7 @@ def _report(phase: LifecyclePhase) -> None: "--user", CONTAINER_USER, container, - *self._agent_command, + *_deprioritized(self._agent_command), ] ) pane = ( @@ -383,7 +463,7 @@ def delete_workspace_contents(self, path: str) -> None: it, so the daemon can then ``rmtree`` the now-empty directory. Overrides the panopticon entrypoint (which would remap uid) so the container runs as root and can reach files it created. Raises on nonzero docker exit.""" - self._run( + self._run_container( [ "docker", "run", @@ -392,6 +472,11 @@ def delete_workspace_contents(self, path: str) -> None: "/bin/sh", "--volume", f"{path}:/cleanup", + # An unbounded delete over a whole checkout is exactly the IO storm the priority + # flags exist for, so the cleanup sweep yields to the host like a task does — and it + # degrades the same way on a daemon that won't take them (a spawn may not have + # discovered that yet). The container is `--rm` and unnamed, so no name to clear. + *self._priority_flags(), self._image, "-c", "find /cleanup -mindepth 1 -delete", diff --git a/src/panopticon/sessionservice/priority.py b/src/panopticon/sessionservice/priority.py new file mode 100644 index 00000000..c11959da --- /dev/null +++ b/src/panopticon/sessionservice/priority.py @@ -0,0 +1,182 @@ +"""Host-side resource **priority** for task work: run it at the lowest priority we can. + +A task container is the one place where unbounded work happens — an agent running a repo's full +test suite, a `docker build` inside a dind task, six tasks at once — and it shares a machine with +the operator's editor, their shell, and panopticon's own control plane. So every container is +spawned *deprioritized*: it loses every CPU and disk race against a normally-weighted process, and +under real memory pressure the kernel picks it before anything of the operator's. It still uses an +idle host fully — this is priority, **not** a cap: nothing here limits how much CPU or RAM a task +may use when nobody else wants it, so a quiet host runs tasks at full speed. + +The knobs (each read from the **runner host's** env, so a big build box and a laptop can differ): + +========================================= ================================================== +``PANOPTICON_CONTAINER_CPU_SHARES`` CPU weight (default 2 = docker's floor, cgroup v2 + ``cpu.weight`` 1) +``PANOPTICON_CONTAINER_BLKIO_WEIGHT`` block-IO weight (default 10 = the floor; disk is what + actually makes a desktop stutter) +``PANOPTICON_CONTAINER_OOM_SCORE_ADJ`` OOM-killer preference (default 500 — kill the task, + not the operator's editor) +``PANOPTICON_CONTAINER_CGROUP_PARENT`` opt-in parent cgroup (unset; see below) +``PANOPTICON_HOST_NICE`` ``nice`` for host-side task work with no cgroup + around it — a shell task's session (default 19) +========================================= ================================================== + +Set any of them to ``off`` (or empty) to drop that flag entirely; the emitted argv is then exactly +what it was before this module existed. An unparseable value warns and falls back to the default +rather than failing a spawn, and values are clamped to what docker/the kernel accept — including +refusing a *negative* OOM adjustment, which would shield a task at the host's expense (the opposite +of this module's job). + +**Why a cgroup-parent knob.** With docker's systemd cgroup driver, containers land in +``system.slice/docker-.scope`` while the operator's processes sit in ``user.slice``, and cgroup +weights are compared **between siblings**. A weight of 1 on the container's own scope therefore +deprioritizes it *within* ``system.slice`` but does not by itself make it lose to ``user.slice``. +Operators who want that enforced at the hierarchy level can create a low-weight slice +(``CPUWeight=1``, ``IOWeight=1``) and point :data:`CGROUP_PARENT_VAR` at it; installing a systemd +unit is the operator's call, so the per-container weights remain the zero-setup default. + +Pure and LLM-free: env in, argv out. :mod:`panopticon.sessionservice.local_runner` and +:mod:`panopticon.sessionservice.shell_runner` are the callers. +""" + +from __future__ import annotations + +import logging +import os +from collections.abc import Mapping, Sequence + +_log = logging.getLogger(__name__) + +#: CPU weight for a task container (``docker run --cpu-shares``). +CPU_SHARES_VAR = "PANOPTICON_CONTAINER_CPU_SHARES" +#: Block-IO weight for a task container (``docker run --blkio-weight``). +BLKIO_WEIGHT_VAR = "PANOPTICON_CONTAINER_BLKIO_WEIGHT" +#: OOM-killer preference for a task container (``docker run --oom-score-adj``). +OOM_SCORE_ADJ_VAR = "PANOPTICON_CONTAINER_OOM_SCORE_ADJ" +#: Opt-in parent cgroup for task containers (``docker run --cgroup-parent``); unset by default. +CGROUP_PARENT_VAR = "PANOPTICON_CONTAINER_CGROUP_PARENT" +#: ``nice`` increment for host-side task work outside any container (a shell task's session). +HOST_NICE_VAR = "PANOPTICON_HOST_NICE" + +#: Docker's floor for ``--cpu-shares`` (cgroup v2 turns it into ``cpu.weight`` 1) — the least CPU +#: priority a container can be given while still running when the host is idle. +DEFAULT_CPU_SHARES = 2 +#: The floor for ``--blkio-weight`` (default 500), so a task's IO yields to the operator's. +DEFAULT_BLKIO_WEIGHT = 10 +#: Positive OOM adjustment: under memory pressure the kernel scores task containers well above the +#: host's own processes, so it kills one of ours first. The runner already explains such a death — +#: ``LocalRunner.exit_reason`` reads ``OOMKilled`` off ``docker inspect`` before the exit code. +DEFAULT_OOM_SCORE_ADJ = 500 +#: The maximum ``nice`` increment (lowest scheduling priority) for host-side task work. +DEFAULT_HOST_NICE = 19 + +#: Values that switch a knob **off** (the flag is omitted). Note ``0`` is *not* one of them: it's a +#: legitimate setting for these flags (docker's default shares, normal niceness), not an opt-out. +_OFF_VALUES = frozenset({"", "off", "none"}) + +#: The flags that need a working cgroup controller to apply. A daemon whose cgroup is in an odd +#: state (nested docker, some CI images) fails ``docker run`` outright when these are present — +#: :func:`strip_cgroup_flags` is how the runner degrades instead of losing the spawn. +_CGROUP_FLAGS = ("--cpu-shares", "--blkio-weight", "--cgroup-parent") + + +def _env(env: Mapping[str, str] | None) -> Mapping[str, str]: + return env if env is not None else os.environ + + +def _int_setting( + env: Mapping[str, str] | None, var: str, default: int, *, minimum: int, maximum: int +) -> int | None: + """The integer value of ``var``, clamped to ``minimum..maximum``; ``None`` when switched off. + + Unset → ``default``. Unparseable → ``default`` with a warning: a typo in one env var must not + take out every spawn on the host.""" + raw = _env(env).get(var) + if raw is None: + return default + value = raw.strip() + if value.lower() in _OFF_VALUES: + return None + try: + parsed = int(value) + except ValueError: + _log.warning("%s=%r is not an integer — using the default %d", var, raw, default) + return default + clamped = max(minimum, min(maximum, parsed)) + if clamped != parsed: + _log.warning("%s=%d is outside %d..%d — using %d", var, parsed, minimum, maximum, clamped) + return clamped + + +def container_cgroup_parent(env: Mapping[str, str] | None = None) -> str | None: + """The opt-in parent cgroup for task containers, or ``None`` when unset (the default).""" + value = (_env(env).get(CGROUP_PARENT_VAR) or "").strip() + return value if value.lower() not in _OFF_VALUES else None + + +def container_priority_flags(env: Mapping[str, str] | None = None) -> list[str]: + """The ``docker run`` flags that deprioritize a task container, in long form. + + Defaults to ``--cpu-shares 2 --blkio-weight 10 --oom-score-adj 500`` (plus + ``--cgroup-parent`` when configured). Each flag is omitted when its knob is off, so an + operator can hand a dedicated build host the exact argv panopticon emitted before this + existed.""" + flags: list[str] = [] + if ( + shares := _int_setting(env, CPU_SHARES_VAR, DEFAULT_CPU_SHARES, minimum=2, maximum=262144) + ) is not None: + flags += ["--cpu-shares", str(shares)] + if ( + weight := _int_setting( + env, BLKIO_WEIGHT_VAR, DEFAULT_BLKIO_WEIGHT, minimum=10, maximum=1000 + ) + ) is not None: + flags += ["--blkio-weight", str(weight)] + if (adj := container_oom_score_adj(env)) is not None: + flags += ["--oom-score-adj", str(adj)] + if (parent := container_cgroup_parent(env)) is not None: + flags += ["--cgroup-parent", parent] + return flags + + +def container_oom_score_adj(env: Mapping[str, str] | None = None) -> int | None: + """The OOM-killer adjustment a task's processes should carry, or ``None`` when switched off. + + Both the container's own ``docker run --oom-score-adj`` *and* the agent pane use this: an + exec'd process does **not** inherit the container's score (``oom_score_adj`` is per-process, + inherited across fork, and ``docker exec`` forks from the daemon, not from the container's + PID 1), so the pane raises it itself — see ``LocalRunner.spawn``. Clamped to ``0..1000``: + negative values would make the kernel prefer host processes over a task, which is backwards, + and would need ``CAP_SYS_RESOURCE`` for the pane to set anyway.""" + return _int_setting(env, OOM_SCORE_ADJ_VAR, DEFAULT_OOM_SCORE_ADJ, minimum=0, maximum=1000) + + +def host_nice_prefix(env: Mapping[str, str] | None = None) -> list[str]: + """``["nice", "-n", ""]`` to prefix host-side task work with, or ``[]`` when switched off. + + For work that runs on the host with no container cgroup around it — a shell workflow's session + (``runner_type="shell"``). ``-n`` is spelled short because ``nice`` has no long option in the + BSD userland on macOS hosts (AGENTS.md's long-options convention).""" + nice = _int_setting(env, HOST_NICE_VAR, DEFAULT_HOST_NICE, minimum=0, maximum=19) + return [] if nice is None else ["nice", "-n", str(nice)] + + +def strip_cgroup_flags(argv: Sequence[str]) -> list[str]: + """``argv`` without the flags that need a cgroup controller (:data:`_CGROUP_FLAGS`) + values. + + The degradation path for a Docker daemon that refuses them outright — e.g. a nested daemon + whose cgroup is in threaded mode answers any of them with ``unable to apply cgroup + configuration``. ``--oom-score-adj`` is deliberately **kept**: it needs no controller, works on + such hosts, and is the flag that protects the host's memory.""" + kept: list[str] = [] + skip_value = False + for arg in argv: + if skip_value: + skip_value = False + continue + if arg in _CGROUP_FLAGS: + skip_value = True + continue + kept.append(arg) + return kept diff --git a/src/panopticon/sessionservice/shell_runner.py b/src/panopticon/sessionservice/shell_runner.py index 45da8a46..6a815fce 100644 --- a/src/panopticon/sessionservice/shell_runner.py +++ b/src/panopticon/sessionservice/shell_runner.py @@ -30,6 +30,7 @@ _subprocess_run, session_name, ) +from panopticon.sessionservice.priority import host_nice_prefix from panopticon.sessionservice.runner import Runner #: The panopticon shell lib (``task_lib.sh``): functions a shell workflow's script uses to drive its @@ -153,8 +154,23 @@ def _report(phase: LifecyclePhase) -> None: self._run(self._tmux("kill-session", "-t", session), check=False) _report(LifecyclePhase.STARTING) # -c sets the pane's start directory (the task's own dir) so the script runs in a known place. + # A shell task's script runs on the **host** with no container cgroup around it, so `nice` + # is the only lever that keeps it from competing with the operator's own processes (see + # `priority`). tmux execs a multi-argument shell-command directly, so the assembled script + # still reaches `sh -c` verbatim. self._run( - self._tmux("new-session", "-d", "-s", session, "-c", start_dir, "sh", "-c", command) + self._tmux( + "new-session", + "-d", + "-s", + session, + "-c", + start_dir, + *host_nice_prefix(), + "sh", + "-c", + command, + ) ) _report(LifecyclePhase.AWAITING) return session diff --git a/tests/conftest.py b/tests/conftest.py index d394929e..43261ae1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,6 +5,10 @@ ``PANOPTICON_ENABLE_CODEX`` for every test (matching the shipped default, so the "codex is off" assertions can't pass vacuously on one machine and fail on another), and the ``enable_codex`` fixture turns it on for the tests that exercise codex itself. + +The container-priority knobs (:mod:`panopticon.sessionservice.priority`) are pinned the same way +and for the same reason: an operator who exported ``PANOPTICON_CONTAINER_CPU_SHARES`` on their box +must not change what the runner's argv assertions see. """ from __future__ import annotations @@ -12,6 +16,13 @@ import pytest from panopticon.core.features import CODEX_FLAG +from panopticon.sessionservice.priority import ( + BLKIO_WEIGHT_VAR, + CGROUP_PARENT_VAR, + CPU_SHARES_VAR, + HOST_NICE_VAR, + OOM_SCORE_ADJ_VAR, +) @pytest.fixture(autouse=True) @@ -24,3 +35,16 @@ def _codex_flag_off(monkeypatch: pytest.MonkeyPatch) -> None: def enable_codex(monkeypatch: pytest.MonkeyPatch) -> None: """Turn the codex feature flag on for a test that exercises codex support.""" monkeypatch.setenv(CODEX_FLAG, "1") + + +@pytest.fixture(autouse=True) +def _priority_env_default(monkeypatch: pytest.MonkeyPatch) -> None: + """Every test sees the shipped resource-priority defaults, not the developer's exports.""" + for var in ( + CPU_SHARES_VAR, + BLKIO_WEIGHT_VAR, + OOM_SCORE_ADJ_VAR, + CGROUP_PARENT_VAR, + HOST_NICE_VAR, + ): + monkeypatch.delenv(var, raising=False) diff --git a/tests/sessionservice/test_local_runner.py b/tests/sessionservice/test_local_runner.py index a5fa265c..e0783169 100644 --- a/tests/sessionservice/test_local_runner.py +++ b/tests/sessionservice/test_local_runner.py @@ -80,9 +80,13 @@ def test_spawn_runs_detached_container_then_tmux_pane_execing_in() -> None: assert ( "[ $i -ge 150 ] && break; sleep 0.2" in pane ) # bounded — a pre-marker image still launches + # `docker exec` doesn't inherit the container's OOM score (it forks from the daemon, not from + # PID 1), so the exec'd command raises its own before `exec`ing the launcher — otherwise the one + # process that eats memory sits at the host's default score. `exec` keeps the process tree flat. assert pane.endswith( - "exec docker exec --interactive --tty --user panopticon panopticon-t1" - " python -m panopticon.container.agent" + "exec docker exec --interactive --tty --user panopticon panopticon-t1 sh -c" + " 'echo 500 > /proc/self/oom_score_adj 2>/dev/null || true;" + " exec python -m panopticon.container.agent'" ) @@ -211,6 +215,152 @@ def test_spawn_with_docker_in_docker_runs_privileged_and_flags_the_entrypoint() assert "PANOPTICON_DOCKER_IN_DOCKER=1" in docker_run # entrypoint starts dockerd +class _CgroupRefusingRunner(_Recorder): + """A daemon that rejects the cgroup priority flags — what a nested Docker daemon whose cgroup + is in threaded mode does: ``docker run`` fails with ``unable to apply cgroup configuration`` + whenever ``--cpu-shares``/``--blkio-weight`` are present.""" + + def __init__(self, *, refusing: bool = True) -> None: + super().__init__() + self.refusing = refusing + + def __call__( + self, + args: Sequence[str], + *, + check: bool = True, + interactive: bool = False, + verbose: bool = False, + ) -> str: + super().__call__(args, check=check, interactive=interactive, verbose=verbose) + if self.refusing and "--cpu-shares" in args: + raise subprocess.CalledProcessError( + 125, list(args), stderr="unable to apply cgroup configuration" + ) + return "" + + +class _RunFailingRunner(_Recorder): + """A daemon where ``docker run`` itself fails (a bad image, say) — flags or no flags. ``failing`` + is flipped to model the daemon recovering.""" + + def __init__(self) -> None: + super().__init__() + self.failing = True + + def __call__( + self, + args: Sequence[str], + *, + check: bool = True, + interactive: bool = False, + verbose: bool = False, + ) -> str: + super().__call__(args, check=check, interactive=interactive, verbose=verbose) + if self.failing and list(args[:2]) == ["docker", "run"]: + raise subprocess.CalledProcessError(125, list(args), stderr="no such image") + return "" + + +def test_spawn_runs_the_container_at_the_lowest_priority() -> None: + # A task container shares the host with the operator's editor and the control plane, so it + # yields CPU + disk and is the kernel's first OOM pick (priority, not a cap — an idle host + # still runs it at full speed). + rec = _Recorder() + LocalRunner("http://svc", image="img:1", run=rec).spawn("t1") + docker_run = rec.calls[2][0] + assert docker_run[docker_run.index("--cpu-shares") + 1] == "2" + assert docker_run[docker_run.index("--blkio-weight") + 1] == "10" + assert docker_run[docker_run.index("--oom-score-adj") + 1] == "500" + assert docker_run[-1] == "img:1" # still the final positional — flags precede the image + + +def test_spawn_priority_is_configurable_per_host(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("PANOPTICON_CONTAINER_CPU_SHARES", "512") + monkeypatch.setenv("PANOPTICON_CONTAINER_CGROUP_PARENT", "panopticon.slice") + rec = _Recorder() + LocalRunner("http://svc", run=rec).spawn("t1") + docker_run = rec.calls[2][0] + assert docker_run[docker_run.index("--cpu-shares") + 1] == "512" + assert docker_run[docker_run.index("--cgroup-parent") + 1] == "panopticon.slice" + + +def test_spawn_with_priority_switched_off_emits_the_unconstrained_argv( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # An operator on a dedicated build host opts out entirely: no flags, and the pane's exec is the + # bare launcher again (no oom_score_adj wrapper) — byte-identical to the pre-priority argv. + for var in ( + "PANOPTICON_CONTAINER_CPU_SHARES", + "PANOPTICON_CONTAINER_BLKIO_WEIGHT", + "PANOPTICON_CONTAINER_OOM_SCORE_ADJ", + ): + monkeypatch.setenv(var, "off") + rec = _Recorder() + LocalRunner("http://svc", run=rec).spawn("t1") + docker_run, pane = rec.calls[2][0], rec.calls[3][0][-1] + assert not [arg for arg in docker_run if arg.startswith(("--cpu-", "--blkio-", "--oom-"))] + assert pane.endswith( + "exec docker exec --interactive --tty --user panopticon panopticon-t1" + " python -m panopticon.container.agent" + ) + + +def test_spawn_retries_without_the_cgroup_flags_when_the_daemon_refuses_them() -> None: + # A host that merely can't deprioritize must not lose the spawn (this is reproducible on a + # nested Docker daemon). The retry drops the controller-backed flags, keeps --oom-score-adj + # (which needs no controller), and clears the half-created container first — its name is taken. + rec = _CgroupRefusingRunner() + LocalRunner("http://svc", image="img:1", run=rec).spawn("t1") + runs = [args for args, _ in rec.calls if args[:2] == ["docker", "run"]] + assert len(runs) == 2 + assert "--cpu-shares" in runs[0] + assert "--cpu-shares" not in runs[1] and "--blkio-weight" not in runs[1] + assert runs[1][runs[1].index("--oom-score-adj") + 1] == "500" + # the failed container is removed before the retry, so `docker run --name` doesn't collide + removes = [ + i for i, (args, _) in enumerate(rec.calls) if args[:3] == ["docker", "rm", "--force"] + ] + retry = [i for i, (args, _) in enumerate(rec.calls) if args[:2] == ["docker", "run"]][1] + assert max(removes) < retry + # and the tmux pane still comes up, so the task is genuinely spawned + assert rec.calls[-1][0][:4] == ["tmux", "-L", "panopticon", "new-session"] + + +def test_the_cleanup_sweep_degrades_on_a_refusing_daemon_too() -> None: + # Cleanup can run before any spawn in a process, so it can't rely on the spawn path having + # discovered the daemon's answer. The retry needs no `docker rm` — the sweep is --rm + unnamed. + rec = _CgroupRefusingRunner() + LocalRunner("http://svc", image="panopticon-base", run=rec).delete_workspace_contents("/w") + runs = [args for args, _ in rec.calls if args[:2] == ["docker", "run"]] + assert len(runs) == 2 and "--cpu-shares" not in runs[1] + assert not [args for args, _ in rec.calls if args[:3] == ["docker", "rm", "--force"]] + + +def test_a_refusing_daemon_is_only_discovered_once() -> None: + # The fallback costs an extra `docker run` per spawn if we keep asking, so the answer is latched. + rec = _CgroupRefusingRunner() + runner = LocalRunner("http://svc", run=rec) + runner.spawn("t1") + rec.calls.clear() + runner.spawn("t2") + runs = [args for args, _ in rec.calls if args[:2] == ["docker", "run"]] + assert len(runs) == 1 and "--cpu-shares" not in runs[0] + + +def test_an_ordinary_run_failure_is_raised_and_does_not_disable_the_flags() -> None: + # A bad image fails both attempts: the error surfaces (the spawner reports it), and the daemon + # is *not* written off as unable to deprioritize — the next spawn asks for the flags again. + rec = _RunFailingRunner() + runner = LocalRunner("http://svc", run=rec) + with pytest.raises(subprocess.CalledProcessError): + runner.spawn("t1") + rec.failing = False # the daemon recovers + rec.calls.clear() + runner.spawn("t1") + assert "--cpu-shares" in rec.calls[2][0] + + def test_extra_env_is_forwarded() -> None: rec = _Recorder() LocalRunner("http://svc", extra_env={"PANOPTICON_RECONNECT_BACKOFF": "0.5"}, run=rec).spawn( @@ -392,6 +542,13 @@ def test_delete_workspace_contents_runs_root_container_to_empty_directory() -> N "/bin/sh", "--volume", "/tasks/t1:/cleanup", + # deleting a whole checkout is an IO storm too — it yields to the host like a task + "--cpu-shares", + "2", + "--blkio-weight", + "10", + "--oom-score-adj", + "500", "panopticon-base", "-c", "find /cleanup -mindepth 1 -delete", diff --git a/tests/sessionservice/test_priority.py b/tests/sessionservice/test_priority.py new file mode 100644 index 00000000..30faf759 --- /dev/null +++ b/tests/sessionservice/test_priority.py @@ -0,0 +1,135 @@ +"""Resource priority (:mod:`panopticon.sessionservice.priority`): the env → argv layer. + +Pure unit tests — the defaults every task container is spawned with, each per-host override, the +off switch that returns the argv to exactly what it was before priority existed, the clamps, and +the cgroup-flag strip the runner degrades through on a daemon that refuses them. +""" + +from __future__ import annotations + +import logging + +import pytest + +from panopticon.sessionservice.priority import ( + BLKIO_WEIGHT_VAR, + CGROUP_PARENT_VAR, + CPU_SHARES_VAR, + HOST_NICE_VAR, + OOM_SCORE_ADJ_VAR, + container_cgroup_parent, + container_oom_score_adj, + container_priority_flags, + host_nice_prefix, + strip_cgroup_flags, +) + + +def test_defaults_are_the_lowest_priority_docker_can_express() -> None: + # cpu-shares 2 and blkio-weight 10 are docker's floors (cgroup v2 cpu.weight 1); a positive + # oom-score-adj makes the kernel pick a task container over the operator's own processes. + assert container_priority_flags({}) == [ + "--cpu-shares", + "2", + "--blkio-weight", + "10", + "--oom-score-adj", + "500", + ] + + +def test_each_knob_is_overridable_per_host() -> None: + env = {CPU_SHARES_VAR: "512", BLKIO_WEIGHT_VAR: "250", OOM_SCORE_ADJ_VAR: "100"} + assert container_priority_flags(env) == [ + "--cpu-shares", + "512", + "--blkio-weight", + "250", + "--oom-score-adj", + "100", + ] + + +@pytest.mark.parametrize("off", ["off", "OFF", "none", "", " "]) +def test_a_knob_switched_off_drops_its_flag_entirely(off: str) -> None: + # An operator on a dedicated build host opts out and gets the argv panopticon emitted before + # this module existed — not a flag with a permissive value. + flags = container_priority_flags( + {CPU_SHARES_VAR: off, BLKIO_WEIGHT_VAR: off, OOM_SCORE_ADJ_VAR: off} + ) + assert flags == [] + + +def test_zero_is_a_value_not_an_off_switch() -> None: + # 0 is legitimate for these flags (docker's default shares / normal niceness), so it must not + # be read as "unset" — that would silently ignore a deliberate setting. + assert "--oom-score-adj" in container_priority_flags({OOM_SCORE_ADJ_VAR: "0"}) + assert host_nice_prefix({HOST_NICE_VAR: "0"}) == ["nice", "-n", "0"] + + +def test_cgroup_parent_is_opt_in() -> None: + assert container_cgroup_parent({}) is None + assert "--cgroup-parent" not in container_priority_flags({}) + flags = container_priority_flags({CGROUP_PARENT_VAR: "panopticon.slice"}) + assert flags[flags.index("--cgroup-parent") + 1] == "panopticon.slice" + + +def test_values_are_clamped_to_what_docker_and_the_kernel_accept( + caplog: pytest.LogCaptureFixture, +) -> None: + with caplog.at_level(logging.WARNING): + assert container_priority_flags({CPU_SHARES_VAR: "1"})[:2] == ["--cpu-shares", "2"] + assert container_priority_flags({BLKIO_WEIGHT_VAR: "1"})[2:4] == ["--blkio-weight", "10"] + assert container_oom_score_adj({OOM_SCORE_ADJ_VAR: "5000"}) == 1000 + assert host_nice_prefix({HOST_NICE_VAR: "40"}) == ["nice", "-n", "19"] + assert caplog.records # a clamped value is surfaced, not silently rewritten + + +def test_a_negative_oom_adjustment_is_refused() -> None: + # Negative would make the kernel prefer host processes over a task — the opposite of the point + # (and the pane couldn't set it without CAP_SYS_RESOURCE anyway). + assert container_oom_score_adj({OOM_SCORE_ADJ_VAR: "-500"}) == 0 + + +def test_an_unparseable_value_falls_back_to_the_default(caplog: pytest.LogCaptureFixture) -> None: + # A typo in one env var must not take out every spawn on the host. + with caplog.at_level(logging.WARNING): + assert container_priority_flags({CPU_SHARES_VAR: "lowest"})[:2] == ["--cpu-shares", "2"] + assert "lowest" in caplog.text + + +def test_host_nice_prefix_default_and_off() -> None: + assert host_nice_prefix({}) == ["nice", "-n", "19"] # `-n` is short: BSD nice has no long form + assert host_nice_prefix({HOST_NICE_VAR: "off"}) == [] + + +def test_strip_cgroup_flags_drops_the_controller_backed_flags_and_keeps_the_rest() -> None: + argv = [ + "docker", + "run", + "--detach", + "--cpu-shares", + "2", + "--blkio-weight", + "10", + "--oom-score-adj", + "500", + "--cgroup-parent", + "panopticon.slice", + "image", + ] + # --oom-score-adj survives: it needs no cgroup controller, works on the hosts that refuse the + # others, and is the flag that protects the host's memory. + assert strip_cgroup_flags(argv) == [ + "docker", + "run", + "--detach", + "--oom-score-adj", + "500", + "image", + ] + + +def test_strip_cgroup_flags_is_a_no_op_when_there_is_nothing_to_strip() -> None: + argv = ["docker", "run", "--detach", "--oom-score-adj", "500", "image"] + assert strip_cgroup_flags(argv) == argv diff --git a/tests/sessionservice/test_shell_runner.py b/tests/sessionservice/test_shell_runner.py index 682eb7c4..b0513b04 100644 --- a/tests/sessionservice/test_shell_runner.py +++ b/tests/sessionservice/test_shell_runner.py @@ -52,7 +52,22 @@ def test_spawn_kills_stale_session_then_starts_the_script_in_the_task_dir() -> N assert new_session[:6] == ["tmux", "-L", "panopticon", "new-session", "-d", "-s"] assert new_session[6] == "panopticon-t1" assert new_session[7:9] == ["-c", "/tasks/t1"] # the pane starts in the task's own directory - assert new_session[9:11] == ["sh", "-c"] # the pane runs the assembled script under sh -c + # A shell task's script runs on the host with no cgroup around it, so `nice` is the only lever + # keeping it off the operator's back; the script still reaches `sh -c` verbatim behind it. + assert new_session[9:12] == ["nice", "-n", "19"] + assert new_session[12:14] == ["sh", "-c"] # the pane runs the assembled script under sh -c + + +def test_spawn_without_host_nice_runs_the_script_unprefixed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # Switching the knob off returns the pane to exactly the argv it had before priority existed. + monkeypatch.setenv("PANOPTICON_HOST_NICE", "off") + rec = _Recorder() + ShellRunner("http://svc:8000", run=rec).spawn("t1", script="true", workdir="/tasks/t1") + new_session = rec.calls[1] + assert new_session[9:11] == ["sh", "-c"] + assert "nice" not in new_session def test_spawn_falls_back_to_the_operator_home_without_a_workdir() -> None: