diff --git a/infra/firecracker/integration-test.sh b/infra/firecracker/integration-test.sh index ad70ab63..df950758 100755 --- a/infra/firecracker/integration-test.sh +++ b/infra/firecracker/integration-test.sh @@ -82,6 +82,10 @@ command -v firecracker >/dev/null 2>&1 || [ -n "${FC_BINARY_PATH:-}" ] \ [ -s "${FC_ROOTFS_PATH:-}" ] || fail "FC_ROOTFS_PATH unset/empty — run build-image.sh first" WORK="$(mktemp -d)" +# This script's own process group — cleanup refuses to signal it (see cleanup). +SELF_PGID="$(ps -o pgid= -p $$ 2>/dev/null | tr -dc '0-9')" +# Where the manager wrapper publishes its process group id (see the launch). +MANAGER_PGID_FILE="${WORK}/manager.pgid" MANAGER_ADDR="127.0.0.1:50054" # The guest reaches the manager over its point-to-point TAP link (172.16.x.1), # never loopback, so the manager has to bind all interfaces. grpcurl still @@ -94,13 +98,129 @@ SECRET_VALUE="itest-secret-value-do-not-log" # manager rejects an untenanted spec outright. ITEST_TENANT_ID="00000000-0000-0000-0000-000000000001" +# Is something listening on the manager's gRPC port? +# +# Defined up here because cleanup uses it too: the port — not the pid — is what +# the NEXT run actually needs back. No `nc` dependency; bash /dev/tcp is enough. +port_open() { (exec 3<>/dev/tcp/127.0.0.1/50054) 2>/dev/null && exec 3>&- 3<&-; } + +# Reap the manager AND everything it spawned. +# +# `kill "${MANAGER_PID}"` was not enough, in two ways that compound: the manager +# does not reliably exit on SIGTERM, and its firecracker children are reparented +# rather than killed when it does. A failed run therefore left a live manager +# holding :50054, and every LATER run then failed to bind and reported +# "Firecracker backend did not report available" — a symptom that points at the +# backend, or at whatever was last edited, rather than at the leaked process. +# That cost three runs and briefly looked like a code regression. +# +# The manager is started in its own process group (see `setsid` below) so the +# whole tree can be signalled by group id. Deliberately NOT a host-wide +# `pkill -f firecracker`: that would kill unrelated VMs, and on the usual dev +# host — a Lima VM named `firecracker-dev` — the pattern also matches the +# caller's own shell command line and kills the session running this script. cleanup() { - [ -n "${MANAGER_PID:-}" ] && kill "${MANAGER_PID}" 2>/dev/null || true - # Best-effort teardown of any leaked TAPs / sockets from a failed boot. + # 1. Stop the VM through the manager FIRST, while the manager is still alive. + # + # Killing the manager's process group does NOT reap its VMs: the backend + # launches each firecracker with `pre_exec(libc::setsid())` (see + # services/runtime-manager/src/backends/firecracker.rs), so every VM leaves + # the manager's session before exec and survives a group kill. A run that + # failed after Spawn but before the explicit Stop would leave a live VM — and + # since the manager itself does die, the port check below would come back + # clean and report success while the VM leaked. + # + # Stop is also the only path that releases the VM's TAP, so this is a clean + # shutdown rather than a kill. Best-effort: before the certs exist, or with + # the manager already gone, grpcurl simply fails and the kill below covers it. + if [ -n "${VM_ID:-}" ] && [ -s "${WORK}/client.crt" ] && port_open; then + grpcurl -cacert "${WORK}/ca.crt" -cert "${WORK}/client.crt" -key "${WORK}/client.key" \ + -servername localhost -import-path "${PROTO_DIR}" -proto "${PROTO_FILE}" \ + -d "$(jq -n --arg id "${VM_ID}" '{vm_id:$id, reason:"integration-test cleanup"}')" \ + "${MANAGER_ADDR}" lantern.v1.RuntimeManager/Stop >/dev/null 2>&1 || true + fi + + # The pgid may have been published after the handshake loop gave up, so read + # the file again rather than trusting only the variable. Without this, a + # handshake that times out while the manager IS running leaks it — the very + # thing `MANAGER_PID=$!` used to cover. + if [ -z "${MANAGER_PGID:-}" ] && [ -s "${MANAGER_PGID_FILE:-/nonexistent}" ]; then + MANAGER_PGID="$(tr -dc '0-9' <"${MANAGER_PGID_FILE}" 2>/dev/null)" + fi + + # Never signal our own process group, whatever went wrong upstream: that would + # kill this script (and, under CI, the runner step) instead of the manager. + # The handshake below should make this unreachable; it is here because the + # cost of being wrong is losing the run and its diagnostics. + if [ -n "${MANAGER_PGID:-}" ] && [ "${MANAGER_PGID}" = "${SELF_PGID:-}" ]; then + printf '\033[1;33m[integration] WARN:\033[0m %s\n' \ + "refusing to signal process group ${MANAGER_PGID} — it is this script's own" >&2 + MANAGER_PGID="" + fi + if [ -n "${MANAGER_PGID:-}" ]; then + kill -TERM -- "-${MANAGER_PGID}" 2>/dev/null || true + # Give a graceful shutdown a bounded chance (~6s), then stop asking. + for _ in $(seq 1 30); do + port_open || break + sleep 0.2 + done + kill -KILL -- "-${MANAGER_PGID}" 2>/dev/null || true + elif [ -n "${MANAGER_LAUNCH_PID:-}" ]; then + # No usable group id — the wrapper never published one. Fall back to the pid + # `&` gave us, which is what the pre-handshake code always used. It is the + # manager itself when setsid exec'd, and a already-exited wrapper when setsid + # forked, so this is safe either way and strictly better than leaking. + kill -TERM "${MANAGER_LAUNCH_PID}" 2>/dev/null || true + sleep 1 + kill -KILL "${MANAGER_LAUNCH_PID}" 2>/dev/null || true + fi + + # Backstop for any VM that outlived the manager (each firecracker setsid()s + # into its own session, so nothing above reaches it). Matched on THIS run's + # vm_id in the api socket path — a uuid, so it cannot match another run's VM, + # an unrelated hypervisor, or this script's own command line. That last part + # matters: a broad `pkill -f firecracker` also matches the usual dev host's + # Lima VM name and the shell running this script. + # Matched on the vm_id ANYWHERE in a firecracker process's argv, not on the + # bare `/run/firecracker/.sock` path: under jailer mode the socket lives + # inside the jail root instead, so a path match would miss it and leak the VM. + # The id is a uuid minted for this run, and only processes actually named + # `firecracker` are considered, so this cannot match another run, an unrelated + # hypervisor, or this script. + if [ -n "${VM_ID:-}" ]; then + for pid in $(pgrep -x firecracker 2>/dev/null); do + [ "${pid}" = "$$" ] && continue + if tr '\0' ' ' <"/proc/${pid}/cmdline" 2>/dev/null | grep -qF "${VM_ID}"; then + kill -KILL "${pid}" 2>/dev/null || true + fi + done + fi + + # Confirm the port came back. If it did not, say so HERE — the next run would + # otherwise fail with the misleading message described above. + for _ in $(seq 1 25); do + port_open || break + sleep 0.2 + done + if port_open; then + printf '\033[1;33m[integration] WARN:\033[0m %s\n' \ + "port 50054 still held after teardown; the next run will fail to bind. \ +Find it with: sudo ss -lntp | grep 50054" >&2 + fi + rm -rf "${WORK}" 2>/dev/null || true } trap cleanup EXIT +# Fail fast, and for the RIGHT reason, if the port is already taken — by a +# manager leaked from an earlier run, or by anything else. Without this the +# script starts a manager that cannot bind and reports a backend problem. +if port_open; then + fail "something is already listening on 127.0.0.1:50054 (likely a runtime-manager + leaked by an earlier run). This script cannot start its own manager until that + is gone. Find and stop it with: sudo ss -lntp | grep 50054" +fi + # --------------------------------------------------------------------------- # 1. Mint a manager mTLS CA + server cert. The Firecracker backend issues a # per-VM client cert (CN=vm_id) signed by this CA at spawn; the harness @@ -160,7 +280,29 @@ fi [ -x "${MANAGER_BIN}" ] || fail "manager binary not found at ${MANAGER_BIN}" log "Starting runtime-manager (RUNTIME_BACKEND=firecracker) on ${MANAGER_ADDR}" -env \ +# `setsid` puts the manager in its own process group so cleanup can reap it and +# every firecracker VM it spawns as one unit, without signalling anything else +# on the host. +# +# The wrapper publishes the group id ITSELF rather than letting us infer it, +# because `setsid` behaves two different ways and neither is safe to guess at: +# +# - caller is NOT already a group leader (a script under CI, job control off): +# setsid execs in place, so the leader keeps the pid `$!` gave us — but only +# AFTER setsid() has run. Reading the pgid right after `&` is a race, and +# losing it returns the SCRIPT'S OWN pgid. Cleanup would then `kill` the +# test script's process group: the same self-kill this file already warns +# about for `pkill -f firecracker`, arrived at from the other direction. +# - caller IS a group leader (sourced from an interactive shell): setsid +# forks, `$!` is the short-lived wrapper, and the pid we captured exits at +# once — so the health check below would declare the manager dead, and a +# pgid derived from it would be wrong. +# +# Inside the wrapper, `$$` is the session leader in BOTH cases, so it is the one +# value that is always right. It writes that, then execs the manager, which +# inherits the pid and stays the leader. +setsid bash -c 'echo $$ >"$1"; shift; exec "$@"' _ "${MANAGER_PGID_FILE}" \ + env \ RUNTIME_BACKEND=firecracker \ LANTERN_RUNTIME_BACKEND=firecracker \ LISTEN_ADDR="${MANAGER_LISTEN_ADDR}" \ @@ -175,11 +317,27 @@ env \ LANTERN_VM_SIGNING_CA_KEY="${WORK}/ca.key" \ "${SECRET_ENV_KEY}=${SECRET_VALUE}" \ "${MANAGER_BIN}" >"${MANAGER_LOG}" 2>&1 & -MANAGER_PID=$! +# Recorded BEFORE the handshake so an early failure still has something to kill: +# every `fail` from here on runs cleanup, and until the pgid is published this is +# the only handle on the manager. +MANAGER_LAUNCH_PID=$! + +# Wait for the wrapper to publish its pid — this is a handshake, not a guess, so +# there is no race left to lose. +for _ in $(seq 1 50); do + [ -s "${MANAGER_PGID_FILE}" ] && break + sleep 0.1 +done +MANAGER_PGID="$(tr -dc '0-9' <"${MANAGER_PGID_FILE}" 2>/dev/null)" +[ -n "${MANAGER_PGID}" ] || fail "manager wrapper never published its process group id" +# The wrapper exec'd into the manager, so the leader's pid IS the manager's pid. +# Use it for liveness rather than `$!`, which is the setsid wrapper when setsid +# forks and would look dead immediately. +MANAGER_PID="${MANAGER_PGID}" # Wait for the gRPC port to accept connections. The manager has no gRPC -# reflection, so probe the raw TCP port via bash /dev/tcp (no nc dependency). -port_open() { (exec 3<>/dev/tcp/127.0.0.1/50054) 2>/dev/null && exec 3>&- 3<&-; } +# reflection, so probe the raw TCP port (port_open is defined near cleanup, +# which needs it too). for _ in $(seq 1 100); do port_open && break kill -0 "${MANAGER_PID}" 2>/dev/null || fail "manager exited early; log:\n$(cat "${MANAGER_LOG}")"