diff --git a/src/scheduler/scheduler.rs b/src/scheduler/scheduler.rs index babbc84..4d6cb74 100644 --- a/src/scheduler/scheduler.rs +++ b/src/scheduler/scheduler.rs @@ -509,6 +509,23 @@ async fn cleanup_deleted(pool: &SqlitePool, deleted: Vec) { /// across readiness checks so the most-cautious wins. const DEFAULT_MIN_HEALTHY_TIME: Duration = Duration::from_secs(10); +/// Whether a deployment currently has a live container for the purpose of the +/// anti-flap window that refills `restart_count`. +/// +/// A worker that reached `Running` obviously qualifies. So does one still in +/// `Creating` that already has an instance: a host-network worker can never pass +/// Ring's readiness checks (its container has no resolvable address), so it stays +/// `Creating` for life even though its container runs fine. Requiring `Running` +/// here left such a worker's `restart_count` monotonic, so its retry backoff +/// climbed to the 60s cap and never came back down — every redeploy got slower. +fn has_live_container(status: &DeploymentStatus, instances_empty: bool) -> bool { + match status { + DeploymentStatus::Running => true, + DeploymentStatus::Creating => !instances_empty, + _ => false, + } +} + /// Resolve the anti-flap window for a deployment: take the max of the /// per-HC `min_healthy_time` (parsed via `HealthCheck::parse_duration`) /// across readiness checks. Falls back to `DEFAULT_MIN_HEALTHY_TIME` when @@ -1514,7 +1531,7 @@ pub(crate) async fn schedule( if result.kind == "worker" && healthy_window.observe( &result.id, - result.status == DeploymentStatus::Running, + has_live_container(&result.status, result.instances.is_empty()), result.restart_count, min_healthy_time_for(&result), ) @@ -2129,4 +2146,37 @@ mod tests { log_running_transition(&pool, &DeploymentStatus::Running, &d).await; assert_eq!(count_state_transition_events(&pool, "trans-3").await, 0); } + + #[test] + fn live_container_running_is_healthy() { + // A worker that reached Running always counts as healthy-running. + assert!(has_live_container(&DeploymentStatus::Running, false)); + assert!(has_live_container(&DeploymentStatus::Running, true)); + } + + #[test] + fn live_container_creating_with_instance_is_healthy() { + // The host-network case: stuck in Creating (readiness can't pass) but the + // container is actually up. This must count as healthy so restart_count + // can be forgiven and the retry backoff stops climbing. + assert!(has_live_container(&DeploymentStatus::Creating, false)); + } + + #[test] + fn live_container_creating_without_instance_is_not_healthy() { + // Creating with no instance yet: genuinely not running, no reset. + assert!(!has_live_container(&DeploymentStatus::Creating, true)); + } + + #[test] + fn live_container_other_states_are_not_healthy() { + for status in [ + DeploymentStatus::Pending, + DeploymentStatus::Failed, + DeploymentStatus::Deleted, + ] { + assert!(!has_live_container(&status, false)); + assert!(!has_live_container(&status, true)); + } + } } diff --git a/tests/e2e/docker/t43_windowed_reset_stuck_creating.sh b/tests/e2e/docker/t43_windowed_reset_stuck_creating.sh new file mode 100755 index 0000000..6cfbbbf --- /dev/null +++ b/tests/e2e/docker/t43_windowed_reset_stuck_creating.sh @@ -0,0 +1,115 @@ +#!/usr/bin/env bash +# T43: windowed restart_count reset for a worker stuck in Creating with a live +# container (the host-network regression fixed by has_live_container). +# +# A host-network worker never passes Ring's readiness checks — its container has +# no resolvable address — so it stays in `Creating` for life even though the +# container runs fine. The old anti-flap gate required status == Running, so such +# a worker's restart_count stayed monotonic: its retry backoff climbed to the 60s +# cap and never came back down. The fix counts a Creating worker that already has +# a live instance as healthy, so the accrued count is forgiven. +# +# This reproduces that exact state end-to-end. Docker re-runs the SAME command on +# every (re)spawn, so the "crash a few times then stay up" behaviour is driven by +# a counter on a host bind mount: the first 3 starts exit 1 (each a crash Ring +# counts toward restart_count); the 4th stays up (sleep 3600). The worker uses +# host networking and a readiness probe that never turns green plus a long +# start_period, so once the container is alive it sits in `creating` — never +# `running`, never `failed` — with a live instance. That is the precise state +# where the old gate left restart_count monotonic. +# +# Invariants: +# 1. The crash phase registers: restart_count climbs to 3 (< MAX = 5), so a +# *windowed* reset is exercised rather than CrashLoopBackOff. +# 2. The worker then settles in `creating` (readiness can't pass on host net) +# and STAYS there — it must never reach `running` or `failed`. +# 3. After the anti-flap window elapses the scheduler forgives the accrued +# count: restart_count is reset to 0 while the status is still `creating`. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=../lib.sh +source "$SCRIPT_DIR/../lib.sh" + +log "== T43: restart_count is forgiven for a live worker stuck in Creating ==" + +NS="ring-e2e" +NAME="crash-then-stuck-creating" + +# Host-side state directory the container increments a counter in. Start clean so +# the counter begins at 0 even across re-runs of this test. +STATE_DIR="/tmp/ring-e2e-t43" +rm -rf "$STATE_DIR" +mkdir -p "$STATE_DIR" + +start_ring +ring_login + +"$RING_BIN" apply --file "$SCRIPT_DIR/../fixtures/crash-then-stuck-creating.yaml" + +# Invariant 1: the container crashes 3 times. Wait for restart_count to reach 3, +# which proves the crash phase registered. It must stop at 3 (the 4th start stays +# up), well short of MAX_RESTART_COUNT (5) — otherwise we couldn't test a +# *windowed* reset, only a CrashLoopBackOff. +log "waiting for the crash phase to accrue restart_count=3..." +REACHED=0 +for _ in $(seq 1 60); do + RC=$(get_restart_count "$NS" "$NAME") + if [ "${RC:-0}" -ge 3 ]; then + REACHED=1 + break + fi + sleep 1 +done +if [ "$REACHED" -ne 1 ]; then + fail "restart_count never reached 3 during the crash phase (last: ${RC:-0})" +fi +log "crash phase registered: restart_count=$RC" + +if [ "${RC:-0}" -ge 5 ]; then + fail "restart_count reached the CrashLoopBackOff bound ($RC); cannot test the windowed reset" +fi + +# Invariant 2: the 4th container stays up but can never pass readiness on host +# networking, so it settles in `creating`. Wait for that, then confirm it STAYS +# there past the anti-flap window (DEFAULT_MIN_HEALTHY_TIME = 10s) — it must +# never be promoted to `running` nor failed by the rollout deadline (deferred by +# the 300s start_period). +wait_deployment_status "$NS" "$NAME" "creating" 60 + +log "worker is 'creating' with a live container; watching 40s that it stays there..." +for i in $(seq 1 40); do + STATUS=$("$RING_BIN" deployment list --output json 2>/dev/null \ + | jq -r --arg ns "$NS" --arg n "$NAME" \ + '.[] | select(.namespace==$ns and .name==$n) | .status' | head -n1) + case "$STATUS" in + creating) ;; + running) fail "worker reached 'running' at second $i — host-network readiness should never pass" ;; + failed) fail "worker reached 'failed' at second $i — start_period should defer the rollout deadline" ;; + *) fail "worker in unexpected status '$STATUS' at second $i" ;; + esac + sleep 1 +done +log "Invariant 2: PASS (worker held 'creating' with a live container past the window)" + +# Invariant 3: the crash budget must have refilled even though the worker never +# reached `running`. This is the regression: with status == Running gate, the +# count stayed at 3 forever; with has_live_container it is forgiven to 0. +RESTART_AFTER_WINDOW=$(get_restart_count "$NS" "$NAME") +STATUS=$("$RING_BIN" deployment list --output json \ + | jq -r --arg ns "$NS" --arg n "$NAME" \ + '.[] | select(.namespace==$ns and .name==$n) | .status' \ + | head -n1) + +log "observed: restart_count_after_window=$RESTART_AFTER_WINDOW status=$STATUS" + +if [ "$STATUS" != "creating" ]; then + fail "expected status still 'creating' after the window, got '$STATUS'" +fi + +if [ "${RESTART_AFTER_WINDOW:-99}" -ne 0 ]; then + fail "expected restart_count reset to 0 after the healthy window, got $RESTART_AFTER_WINDOW (has_live_container did not forgive the Creating worker)" +fi + +log "== T43: PASS ==" diff --git a/tests/e2e/fixtures/crash-then-stuck-creating.yaml b/tests/e2e/fixtures/crash-then-stuck-creating.yaml new file mode 100644 index 0000000..8c87621 --- /dev/null +++ b/tests/e2e/fixtures/crash-then-stuck-creating.yaml @@ -0,0 +1,43 @@ +deployments: + crash-then-stuck-creating: + name: crash-then-stuck-creating + namespace: ring-e2e + runtime: docker + # Host-network windowed restart_count reset (the regression fixed by + # has_live_container). Docker re-runs the SAME command on every (re)spawn, so + # the "crash a few times then stay up" behaviour is driven by state on a host + # bind mount: each start increments a counter in /state/count. While count + # <= 3 the container exits 1 (a crash Ring counts toward restart_count); on + # the 4th start it stops crashing and sleeps forever. + # + # Unlike crash-then-heal, this worker uses host networking and a readiness + # probe that never turns green (the file it checks for is never created). A + # host-network container has no resolvable address, so it can never pass + # readiness and stays in `Creating` for life even though its container is + # alive. A long start_period keeps the rollout deadline from failing it, so + # it sits in `Creating` with a live instance — the exact state where the old + # `status == Running` gate left restart_count monotonic. With the fix the + # scheduler recognises the live container and forgives the accrued count. + network: + mode: host + image: alpine:3.19 + command: + - "sh" + - "-c" + - 'n=$(cat /state/count 2>/dev/null || echo 0); n=$((n+1)); echo $n > /state/count; if [ "$n" -le 3 ]; then echo "crash #$n" >&2; exit 1; fi; echo "healthy after $n starts" >&2; exec sleep 3600' + replicas: 1 + health_checks: + - type: command + command: test -f /var/run/kemeter/never-ready + interval: 2s + timeout: 1s + threshold: 3 + on_failure: alert + readiness: true + start_period: 300s + volumes: + - type: bind + source: /tmp/ring-e2e-t43 + destination: /state + driver: local + permission: rw