From 8768ab2bc50c719a73c9cdcbd48275b4ab27f42d Mon Sep 17 00:00:00 2001 From: Eric Fitzgerald Date: Fri, 31 Jul 2026 09:25:22 -0400 Subject: [PATCH] chore(scripts): let seeding establish its own port-forwards Seeding needs localhost:8080 and localhost:5432, but nothing established either -- they were a documented manual prerequisite. That was never workable, for a reason that is structural rather than a lapse in diligence: stop_port_forward()'s legacy reaper matches any "-n tmi-platform port-forward svc/" command, so a hand-started forward is killed as an "orphan" by the next dev-up/dev-restart/dev-down. It then resurfaces as a confusing failure inside the seed rather than as a missing prerequisite. Adds ensure_port_forward(name), modelled on tmi_common.ensure_oauth_stub: probe, return quietly if already healthy, otherwise start and wait. It is generic over the three named dev forwards; only seeding is wired to it for now. A forward whose recorded kube context is no longer the current one counts as unhealthy and is replaced rather than reused -- a live supervisor pointing localhost at another cluster is the #580 failure mode. scripts/run-dbtool.py is the single wiring point, which covers every way seeding is invoked: make cats-seed, make e2e-seed, and the cats plugin's seed hook (hence make cats-fuzz). Skipped for --oci (external ADB, no Service to forward) and when --server is not loopback. Postgres gets a first-class supervised forward, pidfile-tracked like the others so stop_port_forward() tears it down deliberately. It is deliberately NOT started by dev-up: 5432 collides with a locally installed PostgreSQL, and a developer who never runs CATS should not have to care. On a collision _preflight_port already hard-fails naming the squatter; the message now adds that a local PostgreSQL commonly holds that port, and why seeding must not silently use it. Two fixes found while doing this: - start_redis_port_forward() called the blanket stop_port_forward(), which cleared every pidfile. That forced the server forward to be started afterwards to survive -- an ordering constraint that would silently extend to any third forward -- and destroyed the postgres forward on every dev-up/dev-restart. Each starter now stops only its own pidfile. Squatters are still handled by the existing marker-scoped orphan reaping and _preflight_port. - ensure returned ~3s before the port was bound, so the contract was "started" rather than "usable" and an immediate connect could race it. It now waits for the bind, as ensure_oauth_stub does. test_server_port_forward_is_k3s_only is widened for the one new call site, the by-name dispatch inside ensure_port_forward, whose callers know the server URL is loopback rather than knowing cluster_target. Any other unguarded call site still fails. The campaign still hits the NodePort directly -- this path only ever carries low-volume seeding traffic, which is what #463/#578 actually protect. Verified against the live k3s cluster: cats-seed starts the forward and seeds from a cold start; a second call is a no-op; the forward now survives dev-restart; stop_port_forward() still tears all three down deliberately. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc --- scripts/lib/deploy.py | 138 +++++++++++++++++++++++++++++- scripts/lib/tests/test_deploy.py | 142 ++++++++++++++++++++++++++++++- scripts/run-dbtool.py | 35 ++++++++ 3 files changed, 308 insertions(+), 7 deletions(-) diff --git a/scripts/lib/deploy.py b/scripts/lib/deploy.py index a3ed387f..f21a9c72 100644 --- a/scripts/lib/deploy.py +++ b/scripts/lib/deploy.py @@ -22,7 +22,7 @@ import portfwd from tmi_common import ( check_tool, container_exists, container_is_running, get_project_root, - log_error, log_info, log_success, run_cmd, + log_error, log_info, log_success, run_cmd, wait_for_port, ) # --------------------------------------------------------------------------- @@ -57,6 +57,21 @@ # localhost:6379 — so forward the in-cluster Redis to the host as well. Redis is # low-throughput from the host (test setup only), so a port-forward is fine here. REDIS_PORT_FORWARD_PID = "/tmp/tmi-dev-redis-portforward.pid" +# Postgres is an in-cluster StatefulSet reachable only as a ClusterIP Service. +# Seeding (scripts/run-dbtool.py, which the cats plugin also calls as its `seed` +# hook) opens a DIRECT database connection using config-development.yml's +# localhost:5432, so it needs this forward on top of the server one. +# +# Unlike the redis and server forwards, this one is NOT started by dev-up: 5432 +# collides with a locally installed PostgreSQL on many machines, and a developer +# who never runs CATS should not have to care. It is established on demand by +# ensure_port_forward("postgres"). It is tracked by a pidfile like the others so +# stop_port_forward() tears it down deliberately -- a hand-started forward is +# instead matched by that function's legacy reaper (its "-n tmi-platform +# port-forward svc/" pattern) and killed as an orphan on the next dev-up / +# dev-restart / dev-down, which is exactly why seeding could not rely on one. +POSTGRES_PORT_FORWARD_PID = "/tmp/tmi-dev-postgres-portforward.pid" +POSTGRES_PORT = 5432 # Public base images the docker-desktop node would otherwise pull from cgr.dev on # first bring-up. Docker Desktop Kubernetes' containerd pulls these independently @@ -570,8 +585,9 @@ def wait_and_forward(db: str = "postgres", cluster_target: str = "docker-desktop kubectl(["-n", NS, "rollout", "status", "deploy/tmi-server", f"--timeout={server_rollout_timeout(db)}"]) start_redis_port_forward() # k3s and docker-desktop have no extraPortMappings, so preserve localhost:8080 - # with a server port-forward. Start it AFTER the redis forward, whose - # stop_port_forward() clears both pidfiles, so this one survives. + # with a server port-forward. Order no longer matters here: each starter now + # stops only its own pidfile, so these two (and the on-demand postgres + # forward) cannot clobber each other. if cluster_target in ("k3s", "docker-desktop"): start_server_port_forward() wait_for_server() @@ -627,9 +643,17 @@ def _preflight_port(port: int) -> None: time.sleep(0.5) for pid, command in portfwd.port_listeners(port): if not portfwd.is_kubectl_forward(command): + hint = "" + if port == POSTGRES_PORT: + hint = ( + f"\nA locally installed PostgreSQL commonly listens on {POSTGRES_PORT}. " + "Stop it (or free the port) and retry — seeding talks to the in-cluster " + "database, so silently using the local one would seed the wrong database." + ) log_error( f"Port {port} is held by an unrelated process (PID {pid}): {command}\n" "Refusing to start a port-forward that would not reach the dev cluster." + f"{hint}" ) sys.exit(1) @@ -683,8 +707,16 @@ def start_redis_port_forward() -> None: to whatever context is CURRENT at each respawn, so switching context (say, to a remote EKS cluster) after this forward starts would re-point "local" Redis at the wrong cluster without any visible error (#580). + + Stops only its OWN forward, via _spawn_supervised_forward's per-pidfile + stop. This used to call the blanket stop_port_forward(), which cleared + every pidfile — forcing the server forward to be started afterwards to + survive, an ordering constraint that would silently extend to any third + forward, and destroying an on-demand postgres forward (see + POSTGRES_PORT_FORWARD_PID) on every dev-up/dev-restart. Squatters are + still handled: _spawn_supervised_forward reaps marker-scoped orphans and + _preflight_port reclaims the port from any other kubectl forward. """ - stop_port_forward() ctx = current_kube_context() _spawn_supervised_forward( ["kubectl", "--context", ctx, "-n", NS, "port-forward", "svc/redis", "6379:6379"], @@ -719,6 +751,99 @@ def start_server_port_forward() -> None: ) +def start_postgres_port_forward() -> None: + """Forward the in-cluster Postgres to localhost:5432 for host-side seeding. + + Seeding is low-volume (a few hundred inserts), so the userspace forward's + throughput ceiling -- the #463/#578 problem that bans a forward for the + fuzzing campaign itself -- does not apply. The campaign still hits the + NodePort directly; this forward only ever serves seeding and prep. + + The kube context is pinned via --context for the same reason as the redis + and server forwards: the supervisor re-execs kubectl on every pod roll, and + an unpinned kubectl would resolve whatever context is CURRENT at each + respawn -- so switching context mid-run would silently re-point "localhost" + Postgres at another cluster (#580). + """ + ctx = current_kube_context() + _spawn_supervised_forward( + ["kubectl", "--context", ctx, "-n", NS, "port-forward", "svc/postgres", + f"{POSTGRES_PORT}:{POSTGRES_PORT}"], + POSTGRES_PORT_FORWARD_PID, + f"localhost:{POSTGRES_PORT} -> svc/postgres:{POSTGRES_PORT}", + port=POSTGRES_PORT, + context=ctx, + ) + + +# Named dev port-forwards that ensure_port_forward() knows how to establish. +# The starter functions own their kubectl argv; this table only maps a name to +# the pidfile/port used for the health check and to the starter itself. +_FORWARDS: dict[str, tuple[str, int]] = { + "server": (PORT_FORWARD_PID, HOST_PORT), + "redis": (REDIS_PORT_FORWARD_PID, 6379), + "postgres": (POSTGRES_PORT_FORWARD_PID, POSTGRES_PORT), +} + + +def _forward_is_healthy(name: str) -> bool: + """True when the named forward is already up, on-target, and listening. + + All three conditions matter. A live supervisor whose recorded context is + not the current one is NOT healthy: it is forwarding localhost to a + different cluster, which is the #580 failure mode, so it must be replaced + rather than reused. + """ + pid_path, port = _FORWARDS[name] + record = portfwd.read_pidfile(pid_path) + if record is None: + return False + try: + os.killpg(os.getpgid(record.pid), 0) + except (ProcessLookupError, PermissionError, OSError): + return False + if record.context and record.context != current_kube_context(): + return False + return bool(portfwd.port_listeners(port)) + + +def ensure_port_forward(name: str) -> None: + """Ensure the named dev port-forward is up, starting it only if needed. + + The routine that needs a forward should call this rather than relying on a + developer having started one by hand -- a hand-started forward does not + survive the next dev-up/dev-restart/dev-down (see POSTGRES_PORT_FORWARD_PID) + and nothing detects its absence until a confusing downstream failure. + + Modelled on tmi_common.ensure_oauth_stub: probe, return quietly if already + healthy, otherwise start and let the underlying helpers hard-fail loudly. + Idempotent by design, so callers can invoke it unconditionally without + churning forwards that dev-up already established. + + Hard-fails (via _preflight_port) if an unrelated process holds the port, + rather than binding elsewhere or proceeding against the wrong target. + """ + if name not in _FORWARDS: + log_error(f"Unknown port-forward '{name}' (known: {', '.join(sorted(_FORWARDS))})") + sys.exit(1) + if _forward_is_healthy(name): + log_info(f"Port-forward already running: {name}") + return + log_info(f"Port-forward for {name} not running, starting it...") + if name == "server": + start_server_port_forward() + elif name == "redis": + start_redis_port_forward() + else: + start_postgres_port_forward() + # The supervisor is spawned asynchronously, so the port is not bound the + # instant the starter returns -- measured ~3s for postgres. Without this + # wait the contract would be "started", not "usable", and an immediate + # connect would race it. ensure_oauth_stub does the same for the same + # reason. Callers may then connect as soon as this returns. + wait_for_port(_FORWARDS[name][1], timeout=30, label=f"{name} port-forward") + + def _stop_port_forward_pidfile(pid_path: str) -> None: record = portfwd.read_pidfile(pid_path) if record is not None: @@ -747,6 +872,11 @@ def _stop_port_forward_pidfile(pid_path: str) -> None: def stop_port_forward() -> None: _stop_port_forward_pidfile(PORT_FORWARD_PID) _stop_port_forward_pidfile(REDIS_PORT_FORWARD_PID) + # Tear the on-demand postgres forward down deliberately here, by pidfile, + # rather than leaving it to the legacy reaper below — whose pattern does + # match it, but which would report it as an "orphan" and, being a blanket + # sweep, gives no way to distinguish a tracked forward from a stray one. + _stop_port_forward_pidfile(POSTGRES_PORT_FORWARD_PID) # Legacy tier (#580): reap any kubectl port-forward supervisor for a # tmi-platform Service that predates marker-based pidfile tracking # entirely — e.g. a forward started before this module existed, or one diff --git a/scripts/lib/tests/test_deploy.py b/scripts/lib/tests/test_deploy.py index 2f75339a..7e57d3e3 100644 --- a/scripts/lib/tests/test_deploy.py +++ b/scripts/lib/tests/test_deploy.py @@ -1,3 +1,4 @@ +import os import re import sys import unittest @@ -178,17 +179,39 @@ def test_server_port_forward_is_k3s_only(self): cmd_lines = re.findall(r'port-forward".*svc/tmi-server', src) self.assertEqual(len(cmd_lines), 1, "exactly one server port-forward command (the no-own-cluster helper)") - # Every call site (excluding the def) must be immediately gated on the tuple. + # Every call site (excluding the def) must be guarded. There are two + # legitimate guard forms: + # + # 1. The deploy orchestration paths (dev-up / dev-restart), gated on + # cluster_target in ("k3s", "docker-desktop") -- the original #463 + # invariant. + # 2. The single dispatch inside ensure_port_forward(), which starts a + # forward only when a caller asked for one BY NAME. Its callers + # (currently only seeding, via scripts/run-dbtool.py) do not know + # cluster_target; they know the server URL is loopback, which is + # the equivalent signal. This path is low-volume seeding traffic, + # never the fuzzing campaign -- the campaign still hits the + # NodePort directly, which is what #463/#578 actually protect. + # + # Anything else -- a new, unguarded call site -- must still fail here. call_sites = re.findall(r"(? None: + """Establish the port-forwards seeding needs, rather than assuming them. + + Seeding talks to the API over --server and, for a Postgres target, opens a + direct database connection using the bootstrap config's localhost:5432. + Both paths reach in-cluster ClusterIP Services, so both need a forward. + + This is the single wiring point that covers every way seeding is invoked: + `make cats-seed`, `make e2e-seed`, and the cats plugin's `seed` hook (hence + `make cats-fuzz`). Requiring a developer to have started these by hand was + never workable -- a hand-started forward is killed as an orphan by the next + dev-up/dev-restart/dev-down (see deploy.POSTGRES_PORT_FORWARD_PID), so it + reliably disappears and the failure surfaces later as a confusing seed + error rather than a missing prerequisite. + + Skipped for --oci: that target is an external managed Oracle ADB reached + over the internet, with no in-cluster Service to forward. Also skipped when + --server is not loopback, which means the caller is deliberately driving a + remote deployment. + """ + if args.oci: + log_info("Oracle target: skipping port-forwards (ADB is external)") + return + + ensure_port_forward("postgres") + + if "localhost" in args.server or "127.0.0.1" in args.server: + ensure_port_forward("server") + else: + log_info(f"Server URL is not loopback ({args.server}); skipping server port-forward") + + def main() -> None: args = parse_args() apply_verbosity(args) @@ -119,6 +152,8 @@ def main() -> None: # by switching config files. config = args.config + _ensure_forwards(args) + build_dbtool(args.oci, project_root) run_dbtool(config, args.user, args.provider, args.server, args.input_file, project_root)