diff --git a/.github/workflows/deploy-gcp.yml b/.github/workflows/deploy-gcp.yml index 727c03a8d..8a29f9d90 100644 --- a/.github/workflows/deploy-gcp.yml +++ b/.github/workflows/deploy-gcp.yml @@ -272,49 +272,39 @@ jobs: echo "Waiting 30 seconds for Cloud Run to be ready..." sleep 30 - - name: Test health endpoint - run: | - URL="${{ steps.get-url.outputs.url }}" - echo "Testing health endpoint: $URL/health" - - for i in {1..5}; do - # /health reports but never triggers the lazy DB init on the HTTP - # server; any /api/ request does. /health below is the gate. - curl -s -o /dev/null --max-time 60 "$URL/api/auth/check-admin" || true - # /health returns HTTP 200 even when degraded; the body is the verdict. - if RESPONSE=$(curl -fsS --max-time 10 "$URL/health"); then - echo "Response: $RESPONSE" - if jq -e '.status == "healthy"' > /dev/null <<< "$RESPONSE"; then - echo "Health check passed!" - exit 0 - fi - fi - echo "Attempt $i failed, retrying in 10 seconds..." - sleep 10 - done - - echo "Health check failed after 5 attempts" - exit 1 - + # Needs the repo for scripts/wait-for-healthy.sh, used by both gates below. + - name: Checkout code + uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 + with: + persist-credentials: false + sparse-checkout: scripts/wait-for-healthy.sh + + # Fast fail gate: the server initializes its database in the background + # at startup, so no warm-up request is needed to make progress (#488). + # /ready answers 503 while the instance is still initializing, which the + # script counts as a failed attempt. The old inline gate warmed up + # /api/auth/check-admin and then parsed /health, and the two requests + # could land on different instances (#648). + - name: Test readiness endpoint + env: + APP_URL: ${{ steps.get-url.outputs.url }} + HEALTH_REQUIRED_STREAK: "1" + HEALTH_MAX_ATTEMPTS: "5" + HEALTH_INTERVAL_SECONDS: "10" + run: bash scripts/wait-for-healthy.sh "$APP_URL" + + # Rollout race (#513): a probe can land on an instance of the new + # revision that is still initializing, so one not-ready response must + # not fail the step. The script requires HEALTH_REQUIRED_STREAK + # consecutive ready responses within HEALTH_MAX_ATTEMPTS rounds and fails + # with the last body otherwise. - name: Run smoke tests - run: | - URL="${{ steps.get-url.outputs.url }}" - - echo "Running basic smoke tests..." - - # Test health endpoint multiple times - for i in {1..3}; do - if RESPONSE=$(curl -fsS --max-time 10 "$URL/health") && - jq -e '.status == "healthy"' > /dev/null <<< "$RESPONSE"; then - echo "Health check $i: passed" - else - echo "Health check $i: failed: ${RESPONSE:-no response}" - exit 1 - fi - sleep 2 - done - - echo "All smoke tests passed!" + env: + APP_URL: ${{ steps.get-url.outputs.url }} + HEALTH_REQUIRED_STREAK: "3" + HEALTH_MAX_ATTEMPTS: "30" + HEALTH_INTERVAL_SECONDS: "5" + run: bash scripts/wait-for-healthy.sh "$APP_URL" # Post deployment summary summary: diff --git a/terraform/modules/compute/gcp/cloud-run/main.tf b/terraform/modules/compute/gcp/cloud-run/main.tf index 76e115953..ff0ffc621 100644 --- a/terraform/modules/compute/gcp/cloud-run/main.tf +++ b/terraform/modules/compute/gcp/cloud-run/main.tf @@ -182,16 +182,24 @@ resource "google_cloud_run_v2_service" "main" { } } - # Startup probe + # Startup probe. Cloud Run admits an instance to traffic once its + # startup probe passes, so this is the readiness gate: /ready answers + # 503 until the instance has completed database initialization, so a + # cold instance is never admitted (#648, #488). A startup probe that + # never passes shuts the container down, so the failure budget must + # outlast a slow-but-recovering init: 10 s delay + 24 x 10 s = 250 s, + # above the 120 s default migration timeout (CUDLY_MIGRATION_TIMEOUT) + # plus database connect retries, and inside Cloud Run's 600 s cap on + # failure_threshold x period_seconds. Liveness below stays on /health. startup_probe { http_get { - path = "/health" + path = "/ready" port = 8080 } initial_delay_seconds = 10 timeout_seconds = 3 period_seconds = 10 - failure_threshold = 3 + failure_threshold = 24 } # Liveness probe diff --git a/terraform/modules/compute/gcp/gke/main.tf b/terraform/modules/compute/gcp/gke/main.tf index 4857b89e6..518852832 100644 --- a/terraform/modules/compute/gcp/gke/main.tf +++ b/terraform/modules/compute/gcp/gke/main.tf @@ -595,9 +595,16 @@ resource "kubernetes_deployment" "app" { failure_threshold = 3 } + # Readiness is the traffic-admission signal: /ready answers 503 + # until this replica has completed database initialization, so a + # cold pod stays out of the Service endpoints (and out of the GCE + # Ingress NEG, whose health check GKE infers from this probe when + # the Ingress is created) (#648, #488). Liveness above stays on + # /health, which answers 200 while the process is alive, so a slow + # initialization never gets the pod killed. readiness_probe { http_get { - path = var.health_check_path + path = var.readiness_probe_path port = var.health_check_port } initial_delay_seconds = var.readiness_probe_initial_delay diff --git a/terraform/modules/compute/gcp/gke/variables.tf b/terraform/modules/compute/gcp/gke/variables.tf index 28e34d56c..b0ea9c480 100644 --- a/terraform/modules/compute/gcp/gke/variables.tf +++ b/terraform/modules/compute/gcp/gke/variables.tf @@ -216,11 +216,22 @@ variable "deploy_kubernetes_resources" { # ============================================== variable "health_check_path" { - description = "HTTP path for health checks" + description = "HTTP path for the liveness probe. /health answers 200 while the process is alive, even before initialization completes, so a slow init does not get the pod restarted." type = string default = "/health" } +variable "readiness_probe_path" { + description = "HTTP path for the readiness probe (traffic admission). /ready answers 503 until the replica has completed initialization, keeping cold pods out of the Service endpoints (#648)." + type = string + default = "/ready" + + validation { + condition = startswith(var.readiness_probe_path, "/") + error_message = "readiness_probe_path must be an absolute HTTP path starting with \"/\"." + } +} + variable "health_check_port" { description = "Port for health checks" type = number diff --git a/terraform/modules/compute/gcp/readiness_probe_test.go b/terraform/modules/compute/gcp/readiness_probe_test.go new file mode 100644 index 000000000..88964fa48 --- /dev/null +++ b/terraform/modules/compute/gcp/readiness_probe_test.go @@ -0,0 +1,128 @@ +// Regression test for issue #648: after #488 added /ready (503 until the +// replica has completed initialization), the GKE readiness probe and the +// Cloud Run startup probe still gated traffic on /health, which answers 200 +// even while the replica is cold, so uninitialized replicas were admitted to +// traffic. Liveness must stay on /health so a slow init never gets the +// container killed. +package gcp_test + +import ( + "os" + "regexp" + "strconv" + "testing" +) + +func readTF(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + return string(data) +} + +// probeBlock returns the body of the first ` { ... }` block, matching +// braces so nested http_get blocks are included. +func probeBlock(t *testing.T, content, name string) string { + t.Helper() + loc := regexp.MustCompile(`(?m)^\s*` + name + `\s*\{`).FindStringIndex(content) + if loc == nil { + t.Fatalf("no %s block found", name) + } + depth := 0 + for i := loc[1] - 1; i < len(content); i++ { + switch content[i] { + case '{': + depth++ + case '}': + depth-- + if depth == 0 { + return content[loc[1]:i] + } + } + } + t.Fatalf("unterminated %s block", name) + return "" +} + +func blockPath(block string) string { + m := regexp.MustCompile(`path\s*=\s*(\S+)`).FindStringSubmatch(block) + if m == nil { + return "" + } + return m[1] +} + +func blockInt(t *testing.T, block, attr string) int { + t.Helper() + m := regexp.MustCompile(attr + `\s*=\s*(\d+)`).FindStringSubmatch(block) + if m == nil { + t.Fatalf("probe block has no numeric %s", attr) + } + n, err := strconv.Atoi(m[1]) + if err != nil { + t.Fatalf("parsing %s: %v", attr, err) + } + return n +} + +func variableDefault(t *testing.T, vars, name string) string { + t.Helper() + block := regexp.MustCompile(`(?s)variable\s+"` + name + `"\s*\{.*?\n\}`).FindString(vars) + if block == "" { + t.Fatalf("variable %q is not declared", name) + } + m := regexp.MustCompile(`default\s*=\s*"([^"]*)"`).FindStringSubmatch(block) + if m == nil { + t.Fatalf("variable %q has no string default", name) + } + return m[1] +} + +func TestGKEReadinessProbeGatesOnReady(t *testing.T) { + main := readTF(t, "gke/main.tf") + vars := readTF(t, "gke/variables.tf") + + if got := blockPath(probeBlock(t, main, "readiness_probe")); got != "var.readiness_probe_path" { + t.Errorf("GKE readiness_probe path = %s, want var.readiness_probe_path (#648): readiness is the traffic-admission signal and must not share the liveness path", got) + } + if got := variableDefault(t, vars, "readiness_probe_path"); got != "/ready" { + t.Errorf(`var.readiness_probe_path default = %q, want "/ready" (#648); /health answers 200 on a cold replica`, got) + } + + if got := blockPath(probeBlock(t, main, "liveness_probe")); got != "var.health_check_path" { + t.Errorf("GKE liveness_probe path = %s, want var.health_check_path: liveness must stay on /health so a slow init does not restart the pod", got) + } + if got := variableDefault(t, vars, "health_check_path"); got != "/health" { + t.Errorf(`var.health_check_path default = %q, want "/health": pointing liveness at /ready would kill pods whose init is slow`, got) + } +} + +func TestCloudRunStartupProbeGatesOnReady(t *testing.T) { + main := readTF(t, "cloud-run/main.tf") + + startup := probeBlock(t, main, "startup_probe") + if got := blockPath(startup); got != `"/ready"` { + t.Errorf(`Cloud Run startup_probe path = %s, want "/ready" (#648): Cloud Run admits an instance once its startup probe passes`, got) + } + + // A startup probe that never passes shuts the container down, so the + // window must outlast the 120 s default migration timeout plus connect + // retries, and stay inside Cloud Run's 600 s cap on + // failure_threshold x period_seconds. + window := blockInt(t, startup, "failure_threshold") * blockInt(t, startup, "period_seconds") + if window < 180 { + t.Errorf("Cloud Run startup probe window = %d s, want >= 180 s so a slow-but-recovering init does not restart-loop the container", window) + } + if window > 600 { + t.Errorf("Cloud Run startup probe window = %d s, exceeds Cloud Run's 600 s limit on failure_threshold x period_seconds", window) + } + if timeout, period := blockInt(t, startup, "timeout_seconds"), blockInt(t, startup, "period_seconds"); timeout > period { + t.Errorf("Cloud Run startup probe timeout_seconds (%d) must not exceed period_seconds (%d)", timeout, period) + } + + if got := blockPath(probeBlock(t, main, "liveness_probe")); got != `"/health"` { + t.Errorf(`Cloud Run liveness_probe path = %s, want "/health": liveness must not depend on initialization`, got) + } +}