feat(pusher): harden native RollingUpdate (SCA-39 long-term rollout, DRAFT)#10280
feat(pusher): harden native RollingUpdate (SCA-39 long-term rollout, DRAFT)#10280Git-on-my-level wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5dc3f5562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not ReadinessGate.is_serving(): | ||
| logger.info(f'Rejecting new WS for {uid}: pusher is draining') | ||
| await websocket.close(code=1001) |
There was a problem hiding this comment.
When a new connection reaches a draining pod before the NEG/ALB has stopped routing to it, this closes the WebSocket before accept(). Starlette/Uvicorn turn a pre-accept close into a failed HTTP upgrade rather than a 1001 close frame, so backend-listen's connect_to_trigger_pusher() follows its exception path and records a circuit-breaker failure for each retry (backend/utils/pusher.py:153-159); enough normal sessions during a rollout can open the backend pod's pusher circuit even though other pusher pods are healthy. If the intent is to signal a graceful reconnect, accept then close with 1001, or teach the client not to count this drain response as a pusher failure.
Useful? React with 👍 / 👎.
| Stdlib-only: runs in the shared pre-push lane before backend dependencies are | ||
| installed. |
There was a problem hiding this comment.
This new quality gate says it runs in the shared pre-push lane, but rg verify_pusher_rollout_gate only finds the script, tests, and docs; it is not invoked from .github/checks-manifest.yaml, backend/testing/workflow_contracts.json, the pusher deploy workflows, or scripts/pre-push. That leaves the fail-closed telemetry/probe checks as an on-demand script, so rollouts can still merge/deploy with green required checks while this gate is never executed; please register it alongside verify_pusher_rollout_budget.py.
AGENTS.md reference: AGENTS.md:L143-L144
Useful? React with 👍 / 👎.
SCA-39 long-term rollout strategy (DRAFT, architecture review only): - split /ready (readiness) from /health (liveness) + loopback-only drain endpoint + reject new WS during drain (in-flight sessions keep draining) - chart readinessProbe->/ready, BackendConfig connectionDraining (60s <= grace), preStop flips readiness before the NEG-convergence sleep - fail-closed rollout preflight + read-only rollback contract; fail-closed on missing rollout-blocking telemetry - operator/dev runbook + N/N-1 compatibility checklist + dev qualification plan Defers build-once digest promotion to SCA-40 (owns digest/chart-only mode/ REDIS_DB_HOST preflight/rollback evidence); no deploy, no mutation, no CRDs.
Absorbs the durable long-term pieces from the superseded incident recovery (#10278) into the hardened RollingUpdate contract — no Argo/Flagger/mesh/Gateway. Exact-digest build-once promotion contract (incident-class artifact fix): - chart renders repository@sha256:<hex> when image.digest is set; rejects malformed/ambiguous identity, and a digest + mutable tag, at render time - rollout gate admits a digest only with tag dropped and pullPolicy IfNotPresent (the digest is the content address; Always only adds a registry round-trip); tag mode (chart delegates the tag to the deploy workflow) is unchanged - rollback contract restore is digest-aware; stale SCA-40 ownership notes removed Cross-resource shared-config migration guard (2026-07-22 incident root fix): - new verify_shared_config_migration.py fails CLOSED before any mutation when a serving workload still references a ConfigMap/Secret source or key the proposed state removes or reclassifies — the exact partial transition that dropped prod Pusher to zero healthy pods while /health stayed green - reads object/key NAMES only; never reads, prints, or stores values - transition-aware: a key moving between objects is allowed only once no stale old-source reference remains Discards the incident-only chart-only recovery CLI surface and its broad normalize() (which could hide real chart policy drift), per SCA-39 objective. Coverage: 4 new gate digest tests + 9 migration-guard fixtures incl. the historical partial transition and fail-closed drift. End-to-end verified against the real rendered dev Pusher chart (22 refs resolve; a depleted REDIS_DB_HOST inventory is caught).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5dc3f5562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f"kubectl get deployment {release} " f"-o jsonpath='{{.spec.template.spec.containers[0].image}}'" | ||
| ), | ||
| restore_command=( | ||
| f"helm upgrade --install {release} ./backend/charts/pusher " | ||
| f"--set image.tag=<prior-immutable-tag> " |
There was a problem hiding this comment.
In rollback mode, the capture command reads the container .image field, which Kubernetes documents as the full image name, but the restore command only substitutes that captured value into image.tag. If an operator follows this contract during an incident, they either have to do undocumented parsing or can render an invalid image like gcr.io/.../pusher:gcr.io/.../pusher:<tag>; make the command capture just the tag/digest or have the restore command consume the full captured image, and use Helm's --set-string when setting tags as the deploy workflows already do.
Useful? React with 👍 / 👎.
c5dc3f5 to
4dfdb5b
Compare
…ce digest pullPolicy Independent review (reviewer + adversarial refuter) converged on one MAJOR finding: the shared-config migration guard scanned only explicit env.valueFrom refs and was blind to envFrom whole-object bulk loads. Pusher bulk-loads ALL ConfigMap keys via envFrom, so a key removed from that ConfigMap (the exact 2026-07-22 outage class) went undetected when it lacked an explicit ref. - verify_shared_config_migration.py now scans envFrom sources: a bulk-loaded object must still exist, and (given a previous inventory) no key may be removed from it. - Generalizes to every pod-template workload (Deployment/StatefulSet/DaemonSet/Job/CronJob/ReplicaSet) and scans initContainers. - Chart render-enforces image.pullPolicy=IfNotPresent for a digest-pinned release (defense-in-depth matching the gate). Coverage: +4 migration-guard tests (envFrom object/key removal, envFrom clean pass, StatefulSet+initContainer). 81 affected tests pass; real rendered dev Pusher now reports 22 per-key refs + 1 envFrom source.
1 participant
SCA-39 — Harden pusher native RollingUpdate (long-term rollout strategy)
DRAFT — architecture review only. Implements the accepted PRIMARY from the
SCA-39 Phase-0 decision: harden the existing native Kubernetes RollingUpdate
for the pusher GKE workload. No Argo / Flagger / service mesh / Gateway API, and
no two-color (A2) experiment — those remain documented variants only.
Dependency / branch
origin/main(241f1c4c4dc6). Not stacked on SCA-40 because SCA-40(
c0590c68) was still in_progress with no published branch at start; this PRimplements only the non-overlapping long-term pieces and must be rebased
onto SCA-40's head if/when it lands (digest promotion / chart-only mode /
REDIS_DB_HOST preflight / rollback evidence are SCA-40's and are not
duplicated here). The quality-gate script explicitly asserts
image.digestis absent and will need a lockstep update alongside SCA-40's digest field.
Changed surfaces
Runtime (Lane A)
backend/utils/readiness.py(new): reusable, dependency-freeReadinessGate(SERVING/DRAINING), thread-safe, idempotent
begin_drain(); designed forpusher (wired) and the llm-gateway (planned).
backend/pusher/main.py:/health(liveness, 200 while alive), new/ready(503 when draining), new loopback-only
POST /__internal/drain; lifespanshutdown calls
begin_drain()first then drains background tasks.backend/routers/pusher.py: rejects new WS (close 1001 before accept)during drain; in-flight sessions keep draining via their own bounded logic.
backend/utils/metrics.py: two low-cardinality gaugespusher_ready,pusher_drain_in_progress.Helm/GKE contract (Lane B)
backend/charts/pusher/{dev,prod}_omi_pusher_values.yaml:readinessProbe→/ready(liveness/startup stay/health);preStopnowcurl -sf -m 5 -X POST localhost:8080/__internal/drain || true; sleep 15(flip readiness early + NEG-convergence sleep); added
backendConfig.connectionDraining.drainingTimeoutSec: 60.backend/charts/pusher/templates/backendconfig.yaml: addedconnectionDraining.drainingTimeoutSec; healthCheck stays on/health(liveness semantics — routing it to
/readywould defeat connectionDraining).Quality gates + rollback contract (Lane D)
backend/scripts/verify_pusher_rollout_gate.py(new): fail-closedpreflight(capacity headroom, image-identity, probe split, telemetry-present) + a
read-only
rollbackcontract (emits JSON; never executes). Missing telemetry⇒ fail, not green.
backend/scripts/verify_pusher_rollout_budget.py: extended to also enforce theprobe split, connectionDraining,
drainingTimeoutSec ≤ grace, and theBackendConfig healthCheck invariant (the highest-value guard added from the
refuter pass).
Docs (Lane E)
backend/docs/pusher_rolling_update_operations.md(new): honest availabilitycontract, termination sequence, operator runbook, fail-closed gates, N/N-1
compatibility checklist, dev qualification plan, SCA-40 relationship.
Honest availability contract (no overclaim)
In-flight WebSocket sessions are not migrated across a cutover over the
proxied ALB+NEG path. "Graceful drain" = zero new-connection rejection + a
bounded ~1–60s reconnect gap per affected session (backend-listen reconnects);
connectionDrainingbounds how long the LB waits before cutting. No sub-secondor zero-session-impact cutover is claimed.
Tests
test_pusher_readiness_drain.py(6): serving/drain, loopback-only, idempotent,non-loopback 403, reset, real-route wiring + shutdown-drain.
test_verify_pusher_rollout_budget.py(+6) andtest_verify_pusher_rollout_gate.py(6):good-chart passes; fail-closed regressions for probe routing, missing
connectionDraining/healthCheck invariant, drain>grace, missing telemetry.
make setup;helm templatedev+prod (liveness/health,readiness
/ready, BackendConfig healthCheck/health,connectionDraining60 ≤ grace 120); 81 affected unit tests pass with the worktree venv;
scripts/pr-preflight --suggestclean.Known gaps / remaining
draft; the automated check is a static definition-presence scan only (tracked).
deferred (wiring it could itself rebuild); both scripts are runnable standalone.
Boundary (DRAFT)
No dev/prod deployment, no controller/CRD install, no production mutation, no
workflow dispatch. Production adoption is blocked on a healthy Pusher
baseline (SCA-36/SCA-40 repair) plus explicit approval — this PR is for
review only.