Add rolling-safe HA for versiond routing - #1587
Open
snevolin wants to merge 118 commits into
Open
Conversation
Track B: graceful evacuation, replacement, addition and permanent decommission of a whole versiond host. - table-driven host lifecycle and full-response admission leases in versiond - one absolute, configurable shutdown budget for proxy drain, child drain, HTTP shutdown, escalation and child reap - table-driven child-generation, supervised-process and devshardd lifecycle machines - persistent router membership with immutable membership IDs and an explicit one-host-at-a-time transfer - forward-reconciling router control plane with revisioned nginx config projections, durable completion receipts and an audit outbox - local gonka-routerctl and resumable SSH-based gonka-hostctl workflows - full-stack coverage for a sticky long-running SSE request, evacuation, replacement, decommission and re-addition Replay of sn/versiond-host-evacuation (3f68fde..2064415, 40 commits) onto upgrade-v0.2.15. The original history could not be rebased: two upstream backmerges carried manual conflict resolutions that a flattening rebase drops. The resulting tree is byte-identical to the source branch.
0.2.14 is released, so its release guide is restored to the base state. Add devshard/docs/release-0.2.15-v5.md following the v4 guide structure and carry the graceful-shutdown section and rollout checklist item over verbatim.
Move the Docker host evacuation acceptance job into a dedicated workflow with path filters for Track B code, devshardd lifecycle code, the test harness, and its build inputs. Cancel superseded runs for the same pull request so unrelated testenv changes and stale pushes do not consume the 40-minute runner budget.
Stop triggering the 40-minute acceptance test for every root Makefile change. The shared Testermint workflow already exercises the devshardd-build target, while the dedicated workflow retains triggers for Track B code, its test harness, and direct build metadata.
versiond-router had no Go code and no persisted state before this work, so router state schema 1, WAL schemas 1-4 and hostctl journal schemas 1-2 only ever existed inside this branch. No deployment wrote them. Remove the migration paths and pin every persisted format at schema 1: - delete router state and operation-journal migrations; decode the current schema directly and reject any other version - delete the legacy rollback recovery policy, the pre-image journal fields it needed, and the config restore it performed - delete the audit-to-receipt-index import and the ActionAgnostic/Conflict receipt fields that only that import could set - delete the hostctl journal migration and the host_idle resume alias Torn-tail audit repair stays: it protects the append path independently of the removed import.
Host evacuation was built around nginx OSS not being able to discover that a
versiond is draining: since it cannot health-check upstreams, something had to
tell it. That something was a 12.9k-line control plane — a durable router FSM,
membership IDs, transfer ownership, a WAL, a receipt index, an audit outbox, and
two SSH-driven CLIs — whose entire job was to move an upstream to `down` before
anyone sent SIGTERM.
Move both routers to HAProxy and let them observe instead:
* membership comes from DNS. VERSIOND_POOL_HOST / EDGE_API_POOL_HOST is one
name with an A record per running instance (a Compose network alias), read
through `server-template` + `resolvers`. Starting or stopping a container is
the whole operation; no config change, no reload.
* health comes from `GET /readyz` once a second. A host that is draining,
still converging, or cut off from the chain takes no traffic and rejoins on
its own.
versiond makes that safe with a new `announcing` state: on SIGTERM it fails
/readyz while still accepting for VERSIOND_DRAIN_ANNOUNCE (5s), so the router
removes it before admission closes. Readiness moved to the traffic listener,
which drops the loopback admin listener entirely and is what a Kubernetes
readinessProbe will consume unchanged.
Readiness also had to be relaxed: `Converged` now latches once every desired
version has run. Without that, a routine same-name SHA bump — published to every
host at once — would un-converge the whole pool simultaneously and evict it.
HA storage safety gains a startup half: with GONKA_HA set, devshardd refuses to
boot on storage a sibling cannot see, instead of booting and failing every
HA-marked request. The per-request Devshard-Ha guard stays, because a partial
rollout can leave a process that started before the deployment became HA.
edge-api gets the same treatment: a cached /readyz that reports chain
reachability, so an instance that loses the chain leaves the round-robin
rotation rather than answering.
What remains of the control plane is `gonka-drain`, an 80-line shell guard over
the HAProxy Runtime API for quiescing a host without stopping it. It addresses
hosts by container name or IP and refuses to drain the last one still serving.
Failover discipline is preserved from the nginx config: retry on connect
failure, empty response, and upstream 502; never on 503, which is what a
draining host and the storage guard answer; and `disable-l7-retry` for
non-idempotent methods so an inference is never executed twice.
Net effect on the PR: +6.0k/-0.8k across 75 files, down from +18k.
Verified end-to-end against real HAProxy 3.0 containers: sticky hashing by
escrow, legacy version pinning, /devshard prefix handling, Devshard-Ha
stamping, health-based eviction and re-entry, DNS-driven join and removal,
the last-host drain guard, and edge-api round-robin with health eviction.
`make test-render` renders every supported shape and validates each result with
the HAProxy the routers ship.
Adding /readyz to edge-api put it behind an active health check without giving
it anything to announce: SIGTERM went straight into Echo.Shutdown with a
hardcoded 10s, /readyz kept answering 200 while the chain was reachable, and no
Compose file set stop_grace_period. Replacing an instance therefore cut every
query still running at ten seconds, and the router only found out afterwards.
Mirror the versiond sequence:
* BeginDrain latches /readyz to 503 with reason "draining", checked before the
chain probe so it does not wait out the readiness cache;
* keep serving for EDGE_API_DRAIN_ANNOUNCE (5s) so the router's 1s check
removes the instance before it stops accepting, with a second signal cutting
the wait short;
* then Shutdown under EDGE_API_SHUTDOWN_BUDGET (2m, matching the router's
default read timeout — the process should wait exactly as long as the hop in
front is still willing to wait), and Close with a diagnostic if it expires.
Malformed durations now fail at boot rather than silently falling back to a
default that is only wrong during an outage.
Every Compose file that runs edge-api gets stop_grace_period: 3m, including the
single-instance base. Without it Docker's 10s default would SIGKILL the process
in the middle of the drain it just learned to do.
Verified against the built binary with a chain endpoint that accepts and never
answers: /readyz reports "draining" at t+1s while /healthz still serves; a query
in flight survives past the old 10s cutoff and keeps running to t+28s (announce
plus budget); at budget expiry the process closes it and logs
"graceful shutdown did not finish" instead of waiting for SIGKILL.
Three gaps in the previous commit, from review:
The single-instance Compose never passed the settings. Compose forwards only
what a service lists, so EDGE_API_DRAIN_ANNOUNCE and EDGE_API_SHUTDOWN_BUDGET in
config.env reached nothing and the built-in defaults were always used. Both are
now declared on the base edge-api service, and the announce window is in
config.env alongside the budget it pairs with.
A second signal could only cut the announce window short. Once Shutdown started,
nothing read the signal channel — and signal.Notify had already taken SIGTERM
away from the runtime, so an operator staring at a stuck two-minute drain had
nothing left but SIGKILL. Shutdown now runs in a goroutine while the caller
selects on {finished, budget expired, another signal}, and either failure mode
closes the remaining connections.
Escalation goes to http.Server.Close rather than echo.Close, which turned out to
matter: echo.Shutdown holds startupMutex for its entire run, so escalating
through Echo deadlocks against the very drain it is meant to interrupt. The
end-to-end test below is what found that — the fix was written first and hung.
The sequence itself had no test. It is now extracted as drainAndShutdown and
covered against a real server on a real port with a blocking handler:
* an in-flight request survives the drain and completes, while /readyz already
reports "draining" — both halves of the guarantee in one test, so either one
regressing fails CI;
* budget expiry ends the wait and says why;
* a signal during Shutdown forces the close (this is the deadlock guard);
* BeginDrain is called before Shutdown, not after.
Two follow-ups from review. The budget was only enforced through the error Shutdown chose to return, so it depended on Shutdown reaching the point where it looks at its context. A Shutdown blocked before that — on a lock, say, which is exactly how the echo.Close deadlock behaved — would have run past the budget unbounded. Watch ctx.Done directly alongside the result and the signal, so the ceiling holds whether or not the thing it bounds cooperates. Shutdown also reports a failed listener close the same way it reports a deadline, and wrapping both as "shutdown budget expired" would send the next operator to tune a setting that has nothing to do with it. Only context.DeadlineExceeded is now called a budget expiry; anything else keeps its own cause. Both guards were checked against a reverted implementation: without the ctx.Done branch the blocking-Shutdown test hangs to the test timeout, and without the cause check the listener-failure test fails.
A server's position on a consistent-hash ring defaults to its numeric slot id.
With server-template the slots are filled from a DNS answer whose order nothing
guarantees, so a plain router restart could hand the same hosts different slots —
and move every session at once. That is the one failure the sticky pool exists to
prevent, and it would have happened silently on a restart nobody thought of as
risky.
Measured with four backends and the same addresses in different slot orders,
default keying:
forward order: .5 .2 .2 .2 .4 .2 .5 .3
reversed order: .2 .5 .5 .5 .3 .5 .2 .4
Not one of the eight escrows stayed put. With hash-key addr the mapping is
identical under forward, reversed and shuffled slot order. hash-key is a server
keyword and is already available in the HAProxy 3.0 the routers ship, so no image
bump is needed.
Two guards, at the two costs they are worth:
* make test-render asserts the pool's servers carry hash-key addr, so removing
it fails on every PR;
* the evacuation acceptance test now restarts the router and requires the
escrow to come back to the same host, which is the behaviour rather than the
spelling.
Moving onto this router re-homes sessions once, since the ring is not the one
nginx computed. HA sessions recover from shared Postgres, so that costs a lookup.
Two problems with the previous commit's guard, from review. The acceptance test restarted the router and then kept talking to the port it had before. The harness publishes random host ports, and Docker hands out a new one when the container comes back, so the suite failed with connection refused — reproducibly, in CI. Endpoints are re-resolved after the restart now. That test also could not prove anything: a restart does not force DNS to answer in a different order, so the very regression it was written for would have sailed through it. It stays as a smoke test, with a comment saying so, and the proof moves somewhere it can be made deterministic. `make test-hash-ring` takes the hashing directives out of the rendered config, puts the same four addresses into the server slots in forward, reversed and shuffled order, and requires every escrow to reach the same address in all three. It needs no upstreams: HAProxy logs which server it selected even when the connection is refused, and that selection is the entire question. Checked in both directions. With `hash-key addr` all three orders agree; with it removed from the template the test fails and names the sessions that moved. The first attempt at that negative check passed by accident — the extraction matched `hash-key addr` inside a comment — so it now reads only the server-template line. test-render depends on it, so CI and `make -C devshard ci-testenv-unit` pick it up with no workflow change.
The rejoin check required observing the target DOWN right after start, which is a race the test loses whenever the child comes up quickly — reproducibly, per review. Waiting for a transient is no way to check that an unready host gets no traffic; the host has to be held unready. It now comes back pointed at an oracle that refuses connections. versiond boots, appears in DNS and stays there, but never learns what to run, so /readyz keeps failing for exactly as long as the test holds the barrier — no transient to catch. Lifting the barrier restores the real oracle and the host rejoins. Verified the barrier directly against a versiond container: alive with /healthz 200 and /readyz 503 at 6s and still at 16s, reconcile failing on every poll. It is a state, not a window. The barrier is deliberately scoped to one host, which is why PatchComposeEnvKey would not do — it rewrites the key in every service, and repointing the survivor too would just empty the pool. PatchComposeServiceEnv edits one service block and returns the value it replaced, so the restore is the file's own value rather than one reconstructed from config. It has its own unit test, including that a sibling service and an unrelated block with the same key are left alone. This also pins something worth pinning: one host that cannot converge does not make the pool unready. The survivor keeps its oracle, keeps serving, and the test asserts the session stays reachable on it throughout.
/readyz gated on Conditions.Degraded, which is set by any reconcile error — including "oracle fetch failed". Every versiond reads the same oracle, so that error does not arrive on one host: it arrives on all of them, within one poll interval of each other. An unreachable oracle or one bad archive would therefore have emptied the pool, and the router would have answered 503 for every version, while every child was still running and serving normally. That is the same correlated-failure shape the Converged latch already fixes, and it contradicts what the endpoint is for: readiness answers "can this host serve now", not "did the last control-plane poll succeed". Reconcile failures keep being reported through Degraded, /healthz and the logs, which is where a deployment problem belongs. The cost is named rather than hidden, in code and in the docs: a host that has served before and then fails to install a newly approved version stays in the pool, so requests for that one version fail on it instead of moving to a host that installed it. Fixing that properly needs per-version readiness, which one balancer health check cannot express. Should a condition turn up under which accepting traffic is genuinely unsafe, it gets its own typed condition — not a ride on the generic reconcile error. The acceptance-test barrier is unaffected and was re-checked against a real container: a host pointed at a dead oracle has never converged and has no child, so it stays 503 on /readyz — alive, /healthz 200, still 503 at 18s — with Available and Converged now carrying that on their own.
Two docs said a failed reconcile is reported "through the Degraded condition, in /healthz and in the logs". Only the last of those is true: /healthz serves health.StatusEntry — name, port, status, sha256, binary_version — and nothing else. Promising a field that is not there is worse than saying nothing, because an operator can build an alert on it and only find out during the outage it was meant to catch. Corrected to what actually happens: the failure is kept in versiond's internal Degraded condition and logged at ERROR, and /healthz is deliberately unchanged because that array is a contract existing clients parse. The gap is recorded as a follow-up rather than papered over. Reconcile failures have no machine-readable exposure today; that belongs in a metric, not bolted onto the legacy JSON, and versiond has no metrics endpoint of its own yet.
A new SHA that will not download reaches Degraded through the reconcile result, not through ReportReconcileError, so the existing regression test for an unreachable oracle did not cover it. Both triggers are fleet-wide — every host reads the same oracle and the same archive — and both must leave a serving host in the pool. The test asserts what the balancer consumes after a failed install: the old child is still running so Available holds, Converged is not retracted, and the failure is still reported as Degraded. That downloadAndSwap keeps the old child is already covered by TestDownloadAndSwap_NewChildNotReadyKeepsOldServing; this pins the conditions that follow from it.
The docs said a reconcile failure is logged at ERROR. An unreachable oracle is, but an empty version list sets the same Degraded condition and logs at WARN, so anyone grepping for the promised level would miss half the cases. Say only that it is logged.
A single readiness flag per host cannot say "I am missing one of several versions", so the choice was between letting a host serve a version it does not have, and evicting it entirely. Both are wrong, and the second is worse: the cause of a missing version is a shared archive, so it is missing on every host at once and the whole pool would leave over a version most traffic does not use. So stop asking the host-level question. versiond now answers /readyz?version=<v> from the same route table the proxy uses, and the router keeps one backend per version in VERSIOND_VERSIONS, health-checked with that version's own question. Same hosts in every backend; what differs is which of them pass. A host that cannot run v5 leaves v5's ring and keeps serving v4. Approving a new version becomes safe for the same reason: until a host has v6 running it is simply not in v6's pool, so v6 traffic goes only where it can be served and v4 carries on untouched. The per-version answer needs no convergence latch and no view of the desired set, which is what makes it precise — either a running child serves that version here or it does not. The host-level /readyz stays as the fallback for versions the router was not told about, so listing a version buys precision and is never a prerequisite for serving it. All three pool backends are rendered from one pool-backend.cfg.template, so the routing policy — hashing, hash-key addr, retry discipline, header handling — cannot drift between them. test-render asserts each rendered backend carries all five policy directives, which is the check that would catch a fragment edited for one pool and not the others. Verified end to end against the shipped image with two upstreams that disagree about which versions they serve: v4 spreads over both, v5 reaches only the host that has it, an undeclared version falls back to the host-level pool, and gonka-drain can inspect any of them. make test-version-routing pins exactly that, and fails when the version dispatch is removed.
Three holes in the previous commit, from review. gonka-drain drained one backend. HAProxy server state belongs to a backend/server pair, and a host now sits in every pool, so draining it from versiond_ha_pool left it serving every declared version — the command reported success and did almost nothing. It now plans across all pools, refuses if any of them would be emptied, applies, and rolls back if one apply fails, so a host is never left half out. Verified: draining one of two hosts removes it from v4 and v5 at once, and putting it back restores both. An undeclared version fell back to the host-level pool, which is the coarse check per-version pools exist to replace: the request reached whichever host the hash picked and 404'd there if that host lacked the version. So the claim that approving a new version was automatically safe was wrong, and the docs said so. While any version is declared, an undeclared one is now refused at the router with a 503 that names the setting to fix. Non-version paths like /healthz are exempt, and an empty VERSIOND_VERSIONS disables the mechanism entirely and keeps the previous behaviour. The two-phase rollout — declare, then approve — is documented as the procedure it is. Version names had two contracts. The router accepted only [A-Za-z0-9._-] and refused to *boot* on anything else, while the chain accepts any non-empty name and versiond only bars path separators. A legal approved version could therefore take the router down. The name is now taken as governance wrote it and only the HAProxy identifier is derived from it, with a loud failure if two names derive the same one — a silent collision would merge two versions' pools. test-version-routing covers the new refusal and that /healthz survives it; the render test covers name derivation and collisions.
Six problems, from review. The acceptance test failed on the drain refusal wording. The message and the assertion now say the same thing, and the message names the backend it is protecting. The strict guard called the first path segment a version, so it refused /metrics, /stats, /devshard/healthz and the session observability routes — all of which versiond serves without a version. It now classifies against that grammar (proxy.go: isVersionlessObsPath) on the canonical path, so the prefix form is covered too. Checked against each of those paths. gonka-drain skipped versiond_legacy, so draining the legacy owner reported success while legacy traffic kept arriving. Every backend the host appears in is now included, which makes the legacy owner undrainable — its backend has one server, and emptying it would fail every pinned version. That is the honest answer, and it is now the one the command gives. A legacy backend with nothing pinned to it is skipped, since no request can reach it. The last-server check counted the backend rather than the target: a host that serves v4 but not v5 could not be drained, because v5's pool had one server — which was not the host being drained. The guard now applies only where the target is itself taking traffic. Version names went through a lossy tr, so v5+cuda and v5-cuda derived the same backend, and '+' in the health-check query decodes to a space on the versiond side, leaving the host down forever. One grammar now applies — [A-Za-z0-9][A-Za-z0-9._-]* — used verbatim as backend name, query value and map key, with anything else refused at startup. Narrowing the chain's own validation to match is recorded as a follow-up. The rollback was not one: it applied the opposite action instead of restoring what was there, and two concurrent drains could each see a live peer and then leave none. Admin states are snapshotted per server and restored exactly, and the whole plan-and-apply is under a lock. Verified: concurrent drains leave one host serving, and the loser is told why. The documented rollout said "restart the router", which does not pick up an environment change, and recreating it cut live streams. The procedure is now `up -d --force-recreate`, and the router stops on SIGUSR1 — HAProxy's soft stop — so the outgoing container finishes its streams.
…prove Seven problems, from review. The acceptance test asked the host-level /readyz while waiting on a per-version pool; those do not turn 200 at the same moment. The harness was also flattening every backend into one list, so a wait settled on whichever backend printed last. RouterSlot now carries its backend, the waits name the pool they mean, and TryVersiondReady asks the same question the router asks. Replacing the router to declare a version refused every new connection from the moment the old container was told to stop until it finished its longest stream — minutes, up to stop_grace_period, not the "short gap" the docs claimed. Added gonka-reload: render, check, then SIGUSR2, which is HAProxy's master-worker reload. The listener never closes and established streams stay on the old worker. Measured over 1200 requests at 92/s, one reload cost a single 503, a window under about 10ms. Server states are handed to the new worker first, or it would re-probe from scratch — and would also forget which hosts an operator had drained. Version names are no longer restricted to what happens to be a valid HAProxy identifier, which the chain does not promise. Each of the three uses now gets a form that suits it: the map key is the name as written, because it is matched against the path segment; the health-check query is percent-encoded, because '+' there decodes to a space; the backend identifier gets a hash appended when the name is not already one. v5+cuda routes end to end. A name that cannot appear literally in a path segment is still refused, because the path would not match it — that residue is documented rather than papered over. gonka-drain matched slot names per backend, but a slot is local to its backend: versiond1 is a different host in each. Draining by slot could leave the legacy backend serving. The target is resolved to one address first and every backend is matched on that address alone. The versionless ACL was unanchored, so /metrics-v9 and /healthz-v9 slipped past the undeclared-version guard. Each alternative is anchored now, and both are refused while /metrics and /healthz still work. Duplicate detection matched with a regex, so 'v1.2' collided with 'v1x2'. It compares the map's first field exactly. gonka-drain read the legacy map from disk, which misses a pin added through the Runtime API. It reads the live map from HAProxy.
The last review's closing point is the one that matters: gonka-reload put a second source of truth back into the router. Four of its findings are the same finding — a reload re-rendered the config from the environment and so discarded runtime map pins, left drains applied in some pools and not others, resurrected stale drains from a state file on disk, and reported success before the new worker was serving. That is a control plane growing back, one convenience at a time, in the PR that deleted one. So the reload is gone, along with the server-state file it needed. Declaring a version changes the deployment and replaces the container, as any other router config change does. To keep a governance approval from needing that at all, the join overlay now declares a window — v4 through v8 — because a pool for a version nobody runs has no healthy members and costs nothing but its checks. The README says this plainly instead of offering an in-place path that cannot be made consistent. Separately, a real routing bug: a host was selectable the moment DNS mentioned it, before its first health check had run — a window straight onto a versiond that is still starting, which is what the acceptance test kept catching. HAProxy 3.1 added `init-state fully-down` for exactly this; the routers move to 3.2, still LTS, and every pool server now starts down and has to earn its place. Measured with a host whose listener starts six seconds late: the router answers 503 until the checks pass at t+9s, and never routes to it before. Also from review: gonka-drain treated "this host is in no backend" as success, which is worst for a host that has just entered DNS and is about to join — it now fails and says so. Duplicate version detection compared with awk's ==, which is numeric when both sides look like numbers, so 1, 01 and 1.0 were one version. The rolling-update suite drained the legacy owner, which is correctly refused. It does not need legacy pinning, so it clears it, and the legacy backend then has nothing routed to it and stays out of the drain.
Readiness for a version was answered from the route table alone, which only says a child process is running. devshardd reports itself unready when its chain subscription drops, and versiond went on answering 200 for that version — the router kept the host in the pool and kept sending it work. ServesVersion now re-asks the child, cached for under a second so a per-second balancer check does not become a per-second probe of the child, and bounded so a child that has stopped answering is not a serving one. gonka-drain loses `out` and `in`. HAProxy identifies a server by its slot in a server-template and slots are reused: a drained host that leaves DNS frees its slot, and the next host to arrive inherits the drain — kept out of rotation with nothing to show why. Admin state belongs to the identity of a process, and the router has no such identity, only an address DNS lent it. Taking a host out of rotation is stopping its versiond, which is graceful by construction and cannot be inherited. `status` stays, read-only. The rolling-update suite pinned traffic by draining the other host; it stops it instead, and the old-generation check now looks only at the host that flipped, since a stopped host has no health to report. Docs caught up with the code. rolling-update.md still specified a transactional router FSM, hostctl checkpoints, membership IDs and schema migrations for tests that no longer exist, and claimed them as the implemented status of Track B. The release guide said the join overlay declares v4 when it declares v4 through v8, and said in one place that a host failing to install a version leaves that version's pool and in another that it stays. The README still demanded the old name grammar that the entrypoint no longer enforces.
The cached probe had the shape the review described. It read the child under the lock, released it, spent up to two seconds on HTTP, then wrote the answer keyed by version name — so a swap in that window let a departed generation decide for its replacement, in either direction, and concurrent callers could land their answers out of order. Nothing invalidated the cache on a route rebuild either, and the test that came with it deleted the entry by hand, so it never exercised any of that. Both that and the fan-out are the same design mistake: answering a question by doing I/O inside the answer. A monitor now runs per generation, refreshing that generation's own flag once a second, and ServesVersion is a pure read. The flag lives on the child, so it is bound to the generation by construction rather than by a check — a late answer can only be written to the child that was asked, and a swap simply ends one monitor and starts the next. A balancer asking every second cannot become a probe every second, and no number of concurrent callers fans out. The flag also carries when it was last refreshed, and an answer older than five seconds is not an answer: a monitor that has stopped must not leave the version frozen at "ready". Tests cover losing and regaining readiness through the real monitor, staleness, and that the current generation alone decides. Docs: the release guide still offered gonka-drain for quiescing and still said a host failing to install a version stays in that version's pool; rolling-update.md still credited gonka-drain with refusing to empty the pool; and the evacuation invariants skipped from 6 to 8 after the earlier renumbering.
Five findings from review, four of them seams where the monitor met the rest of the manager. The route could be seen before the flag. close(c.ready) wakes the swap path, which publishes the new generation itself, and c.serving was stored only after the unlock — so a per-version check landing in that window got 503, and with fall 1 one such answer evicts the host. A fleet-wide rolling update could blink the whole pool. The flag is now seeded before the lock is even taken, so nothing can observe the route without the flag. A crash-restart could leave two monitors on one child. The monitor's context was the generation's, and a monitor caught inside its two-second probe while the process died and came back would see running again and keep writing over the new attempt's answers. The context is now scoped to the process attempt, cancelled and awaited right after proc.Wait() — the probe is context-aware, so the await is prompt. The contract is pinned by a test: after cancel+await, a flipped child provokes no further writes. The empty-VERSIOND_VERSIONS mode never saw live readiness: the coarse /readyz gated on Available, which only says a child process exists. Conditions gains Serving — at least one running child whose vouch is current, computed by the same predicate ServesVersion uses so the two answers cannot drift — and versiondReady requires it. Deliberately "at least one", not "every": requiring every child would let one version's fleet-correlated unreadiness empty the pool and take the healthy versions with it, which is the same trap as gating on Degraded. The reviewer's single-child scenario is covered identically either way; per-version pools remain the precise tool. The legacy /healthz fallback logged a warning per probe, and the monitor probes every second. It logs once per generation now, from the callers that know the generation — which also quiets the startup poller, the other repeat offender. Docs: invariant 9 claimed an unconverged host is not routed to at all; it is not routed to through the host-level pool, while per-version pools serve what it already has, on purpose. The three descriptions of the coarse /readyz now mention the live-readiness requirement.
Serving means "at least one live-ready child", so with VERSIOND_VERSIONS empty a host whose v5 child went unready keeps answering 200 as long as v4 is healthy, and v5 requests keep landing on it hash-dependently — the exact failure the per-version pools exist to prevent, reachable through a supported configuration. Making the coarse answer stricter is not the fix; "every child ready" is the correlated-eviction trap again. The fix is not letting an HA deployment route blind: GONKA_HA with an empty VERSIOND_VERSIONS now fails at startup, with the message naming both remedies. The escape hatch is deliberate and says what it accepts — VERSIOND_ROUTER_ALLOW_COARSE_READINESS=1 — because one legitimate user exists: local-test-net mints version names dynamically inside test scenarios and cannot declare them up front. The join overlay already declares v4 through v8 and never hits the check. test-render pins refusal, override and the non-HA pass-through. Two documentation corrections from the same review. The evacuation doc promised the vouch is withdrawn "within a second"; it normally takes one probe interval, a probe may run up to its 2s timeout, and an unrefreshed answer expires after 5s — it now says that. The README still claimed a host downloading an archive or restarting a child reports 503; after first convergence the latch holds, and a restarting child leaves only its own version's pool.
The review found the override fail-open: any non-empty value, 0 and false included, unlocked coarse readiness. That is an instance of a class, and the class was worse than the instance. Every boolean env here was parsed by each consumer separately, and the two consumers of GONKA_HA disagreed: the router read non-emptiness, so false meant on; devshardd read a known-values switch whose default was off, so a typo silently disabled the storage boot guard — the one value the variable exists to enable. The same deployment setting could be simultaneously on for routing and off for safety. One grammar now, on both sides: 1/true/yes are on, empty/0/false/no are off, anything else refuses to start and names the variable. Each entrypoint parses its booleans once, at the top, into plain variables — GONKA_HA, the coarse override, RENDER_ONLY — so no use site can reinvent the parse; devshardd's HADeployment returns an error for values outside the grammar and the boot guard propagates it instead of guessing "off". Pinned from both directions. Go: every off spelling passes on sqlite, every on spelling refuses it, garbage refuses to boot naming the grammar. test-render: 0/false/NO do not unlock the override, garbage overrides and garbage GONKA_HA are refused with the grammar named, and GONKA_HA=false renders without the Devshard-Ha header — the spelling that used to mean on. Reverting the parser to non-emptiness fails the suite on the first check.
GONKA_HA=' true ' was on for devshardd and a startup failure for the router — the one grammar still had two readings at its edges. Both bool_env helpers now trim leading and trailing whitespace only, matching Go's TrimSpace: deleting all whitespace would accept 't rue', which Go rejects, and recreate the divergence in the opposite direction. The render tests pin ' true ' as on, header stamped, and ' no ' as off, header stripped; both were added before the fix and failed against the untrimmed parser.
Confirmed from review, and worse than stated. edge-api gives its chain probe two seconds, but neither router set `timeout check`, and without it `inter 1s` is HAProxy's entire check budget — connect and read. A chain answering in one to two seconds is slow, not down, yet every check against it fails, and with fall 1 every edge-api leaves the pool at once, since they all share the chain node. The part the review could not see from outside: the probe ran on the request's context, so the checker aborting at 1s cancelled the probe, and the resulting context.Canceled was cached as "chain unreachable" for the next three seconds of checks. The pool did not just blink on a slow moment; the abort kept it down. Both routers now set `timeout check 3s`, above edge-api's 2s probe budget, with the relationship written on both sides of the boundary — the constant in readiness.go names the router setting and the template comment names the constant, since no compiler spans the two. versiond-router gets the same 3s: its /readyz is a memory read, so the tolerance costs nothing, and a host that cannot answer a memory read in three seconds is genuinely wedged. The probe itself now runs on its own context with the readiness budget — the answer is about the chain, not about the caller's patience — and is singleflighted, so an expired cache under concurrent checks costs one chain query instead of one per caller. Tests pin both: eight concurrent checks share one probe, and a hanging probe is cut at its own budget, verdict cached, not at whatever deadline the checker happened to have. Render suites pin the presence of `timeout check` in both routers.
The name promised draining, which the script deliberately no longer does, and its place in /usr/local/bin presented it as an operator CLI, which it is not: it is a read-only formatter over the HAProxy Runtime API whose output the citest harness parses. It is now /usr/local/lib/versiond-router/pool-status — named for what it does, off PATH because its primary consumer is a test harness, and stripped of the out/in refusal stub: the control plane it apologised for never shipped, so nobody has muscle memory for it. Any argument is refused with a pointer to docker compose stop. The harness invokes it by full path and names it as its contract; the docs show the full path, which doubles as a statement that this is a diagnostic you look at, not a tool you operate.
The assert greps for 'timeout check' and the comment I put beside the directive names it verbatim, so sed carries the words into the rendered config and the check passed with the directive deleted — the same comment-satisfies-the-grep mistake the hash-key assert had, made again one commit after documenting the first. The pattern now requires the directive itself, at line start with its value; both suites were shown to fail against a template with the comment kept and the directive removed, then pass restored. Audited the remaining render asserts for the class: the rest either anchor already or assert phrases no comment spells out.
The updater treated an existing router-HA marker as a reason to exit, even when images, services, or the actual Compose topology no longer matched the release. A crash after cutover but before the marker write could also leave a healthy v5 deployment that the next run refused to adopt. Serialize upgrades with a deployment lock and store a fingerprint of the release, topology, ordered Compose files, project identity, rendered config, and image set. On every rerun, recover that committed topology when no explicit one was supplied, idempotently converge each application service and the router fleet, verify application and ingress state, and only then rewrite the marker. The updater now repairs drift instead of trusting the existence of a file, and child commands cannot accidentally retain the lock after signal handling. Tests cover missing services, changed fingerprints, committed custom topology, and recovery after the cutover/marker crash window.
Repeated router-HA runs replaced the public proxy without preserving the exact running generation. A bad image or routing environment could pass container startup, fail admission, and leave no automatic route back to the previously working ingress. Before either the initial v4 cutover or a later v5 update, tag the running image by digest and capture every routing environment key. Keep rollback armed while the candidate starts, while fleet routes are checked, and while edge-api admission converges. On failure or signal, recreate the previous proxy with its captured mode and settings and verify that its production routes are usable. Only a fully admitted candidate clears the traps and removes the temporary image. Shell tests cover v4 and v5 compensation, changed environments, failed admission, and interrupts so a public-router update has an explicit commit point rather than a best-effort restart.
The catalog reconciler could expose a newly approved version as soon as one backend became healthy. That made a governance addition routable without the configured replica reserve and turned the first host failure into an immediate outage during rollout. Introduce a shared activation minimum for the parent and inner catalogs. Slot assignment and health checks may prepare a version locally, but its map entry is not published until enough servers pass the per-version readiness contract. Existing version routes remain untouched while the new revision waits. Carry the reserve through Compose, fleet placement compatibility, release cutover, and documentation. Live routing tests hold one candidate below the threshold and prove that no partial route leaks before the required HA capacity is present.
Matching PGHOST, database, and user strings does not prove that two versiond replicas reached the same database. DNS, proxies, or operator overrides could send them to different clusters while every static configuration check passed, breaking session and execution consistency. Create one durable UUID in the devshard PostgreSQL schema and expose it through a loopback-only versiond diagnostic. The upgrade driver reads that identity through both supervisors and refuses to commit an HA deployment unless the values match. The endpoint returns no useful surface to remote clients and fails closed when PostgreSQL lookup is unavailable. Migration, handler, lookup, and upgrade tests cover stable identity creation, loopback access, unavailable storage, and divergent replicas. Shared storage is now verified by data-plane identity rather than inferred from connection text.
Named container tags can be moved after the release metadata is reviewed. A host could therefore run the documented upgrade command at two different times and receive different binaries under the same apparent release contract. Require every v5 release image reference to include an immutable digest and validate that form before the updater or release gate proceeds. Keep the digest pins in the central release environment so documentation and automation consume the same artifact identities. Contract tests reject mutable tags, malformed digests, and missing pins. The release source commit and every deployed image are now reproducible inputs rather than registry conventions.
A version name accepted by governance could still be impossible to represent as a URL path segment, HAProxy Runtime API key, or local binary basename. Different components had overlapping but non-identical validation, so one name could be stored on chain and then disappear or be interpreted differently in the serving path. Define the safe-basename rules once in the consensus parameter validation and align versiond's oracle client and the routing reconciler with them. Reject path separators, query and fragment delimiters, percent escapes, whitespace, backslashes, quotes, and dot path components before approval or installation. Cross-component tests cover accepted punctuation and every unsafe class. A governance-approved name can now travel unchanged through dapi, routing maps, health URLs, and binary storage.
The cutover verified that both live versiond replicas shared one database, but the completed marker did not remember which database it was. A later rerun could adopt a different PostgreSQL cluster, see two matching replicas, and silently declare the changed storage topology converged. Persist the verified database UUID in the atomic upgrade marker. Subsequent runs compare the live identity with both the peer replica and the committed value before application or ingress state may be accepted. The marker is written only after the identity has been read successfully. Upgrade tests cover marker creation, stable reruns, and replacement with a different database identity. Recovery can therefore repair containers without changing the durable state domain it previously committed.
Router slots retain version-to-backend assignments monotonically so a transient catalog contraction cannot reshuffle established routes. Governance could still remove an approved name, however, leaving routers and hosts with different ideas of whether that name was permanently retired or merely stale. Reject parameter updates that remove any previously approved devshard version. New names may be appended up to the routing capacity guaranteed by the shipped software, and changing metadata for an existing name remains possible without reusing its identity. Raise that capacity only together with a software upgrade that provides more router slots. Keeper and parameter tests cover removals, additions, corrupt legacy entries, and the capacity ceiling. The catalog revision can now grow monotonically from a consensus-enforced source rather than relying only on router convention.
The reconciler previously added version and readiness map entries one by one. A process crash or Runtime API error in the middle of a governance revision could expose only part of that revision, making requests depend on which key had been written before the failure. Build the complete projection first, reserve and enable every required backend, and verify the activation minimum before publishing any data route. Use HAProxy's map transactions to seed a full replacement and commit readiness first, then commit the request-routing map as the visibility point. Persist the accepted catalog before publication so restart can replay the whole revision. Live tests inject multi-version revisions and failed publication paths and assert that traffic never observes a partial request map. A catalog update is now one ordered commit rather than a sequence of independently visible edits.
The initial execution claim prevented two replicas from starting together but did not distinguish a crash before the ML POST from a crash after it. Reclaiming every stale claim risks duplicate side effects; never reclaiming one that was not dispatched leaves work stuck forever. Persist explicit claimed, dispatched, completed, and abandoned phases. A short lease may transfer only a pre-dispatch claim, and every transfer receives a new fence. Immediately before sending the request, the engine atomically marks the target as dispatched; dispatched work is never retried automatically, while an undispatched failure may be abandoned and safely claimed by another replica. Only the matching owner and fence may commit the result. Host, engine, memory, hybrid, and PostgreSQL tests cover lease expiry, stale owners, crash boundaries, replayed results, and forbidden post-dispatch fallback. The durable FSM makes the exact side-effect boundary explicit instead of inferring it from request errors.
A process could finish the router cutover and lose the public proxy container before writing the final marker. On rerun, topology discovery then had neither the old runtime labels nor enough explicit input and rejected the already committed deployment instead of recreating it. Treat a valid release marker as an authoritative recovery source for topology mode, ordered Compose files, project name, and project directory. Respect an explicit operator selection, but otherwise restore those values before runtime discovery and allow the committed model to recreate a missing public proxy. Still reject a project override that contradicts surviving runtime ownership. Tests cover absent proxy recovery, auto-mode restoration, custom projects, and conflicting overrides. The marker now contains enough deployment identity to rebuild the committed topology rather than merely report its release number.
The public HAProxy balanced an anonymous scaled nginx service. Compose could replace all replicas together, and the release driver had no stable identity with which to update one worker, verify admission, or restore its exact image. That made a routine policy or certificate-image update a tier-wide restart. Represent the two policy workers as fixed Compose services with the same policy contract. Capture each running image and replica count, verify that the candidate policy and public proxy advertise a compatible contract, replace one worker at a time, and wait until HAProxy admits its concrete address before moving to the next. Keep both policy and public-proxy compensation armed until the complete ingress path is healthy. Update observability and compatibility projections for the fixed slots and add tests for mixed generations, admission delay, interrupted rollout, exact-image rollback, and the absent-public-proxy bootstrap. At least one known-good policy worker remains available through every supported update order.
The release updater, router cutover, and fleet helper each protected only its own operation. A crash between application convergence, ingress convergence, and the final marker left no durable phase from which to resume, while another entry point could mutate the same deployment concurrently. Introduce one re-entrant deployment lock shared through a verified file descriptor and a phase journal containing the exact release and Compose state. Checkpoint prepared, applications-verified, and ingress-verified phases with atomic renames; restore the saved topology from an interrupted journal; and remove it only when the final committed marker is durable. Child helpers inherit the lock intentionally, so no competing mutation can enter while compensation or signal handling is still reaping them. Include the pinned PostgreSQL image in convergence and require router images to advertise a compatible persistent-catalog protocol before rolling the fleet. Failure-injection tests exercise every journal boundary, lock handoff, signal, rollback, and resume path. A host can rerun the same command after interruption and converge forward without guessing which half of the release was applied.
Prevent net/http from transparently replaying an inference POST after dispatch and cover the failure window with a raw TCP regression test. Enforce one ASCII protocol-version grammar at consensus, versiond, and router boundaries, and prohibit removing the devshard catalog through nil params. Publish data routes and per-version admission keys through one HAProxy map transaction so readiness can never lead routability. Persist versiond's full last-known-good catalog before reconciliation and reject stale revisions, in-place mutations, and version removals after restart.
Treat the public ingress cutover and the enclosing v5 release as one crash-recoverable operation instead of a sequence whose rollback state lived in shell variables. The ingress journal now records the exact previous Compose generations, rollback image references, config hashes, and every resource before it is touched. Recovery replays only those resources in reverse mutation order, survives SIGKILL and reboot, and redacts the potentially secret-bearing rollback model before the transaction is committed. The release journal now outranks an older committed marker, fences resume with transaction, base, desired-state, and rendered-Compose fingerprints, and is atomically renamed into the final marker. PostgreSQL identity is carried from the committed state and persisted as soon as the first upgraded versiond can prove it, so later supervisors and resumed runs cannot silently switch execution ledgers. Render and verify an exact v4 nginx rollback projection, including operator-owned observability environment, and require Compose 2.24.4 for reset/override semantics. Extend cutover, crash recovery, topology, and fleet tests; the maintenance rollback test now uses an explicitly route-dead candidate rather than treating a valid legacy routing declaration as invalid. Document the local PostgreSQL failure boundary and the private transaction-journal contract.
Route every consensus catalog mutation through one validation and progression contract, including the v0.2.15 upgrade handler. A fresh versiond now authenticates DAPI's first catalog against a caught-up local consensus node, while an existing last-known-good catalog continues serving through temporary API failures. Bump the router cache protocol and migrate the legacy cache atomically into a generation-specific file. Keep the old cache intact for rollback, allow one protocol generation per fleet rollout, and require both router release images to publish the new contract. Make deployment recovery safer without adding hoster steps: recover interrupted ingress before slow convergence, recheck the effective Compose model before each mutation, update the non-serving policy replica first, infer and apply the external-PostgreSQL overlay automatically, and reject ambiguous HA database configuration. Cover invalid consensus upgrades, first-catalog trust, cache durability and migration, degraded policy rollout, Compose drift, external PostgreSQL, transaction recovery, and the complete Docker fleet lifecycle.
The ML backend has no Idempotency-Key deduplication contract, so forwarding a generated key advertised protection that did not exist and made the HTTP replay behavior harder to reason about.\n\nRemove the header and key generation while retaining the real at-most-once boundary: persist dispatched before sending, never steal dispatched executions, and never replay a failed ML POST. Update the architecture and storage docs to describe only that implemented contract.
Stop exposing internal router and edge-api addresses in public response headers, and keep the graceful fleet test deterministic without a production test hook. Reduce the durable execution FSM to the state and lease data used for correctness. Keep the PostgreSQL upgrade append-only while creating a compact schema for hosts upgrading from the release base. Remove dead deployment variables and unsupported overrides for fixed router internals, with render assertions preventing diagnostic headers from returning.
Gate versiond startup traffic on a fresh consensus-verified artifact catalog while preserving same-name rolling replacements after admission. Require HA consensus endpoints, reject local overrides in HA, and constrain catalog revisions to the local consensus height. Bind ingress rollback to actual container generations and the outer Compose fingerprint so interrupted or drifting upgrades cannot recreate unchanged services or commit a different topology. Normalize the HA PostgreSQL contract by rejecting service-file overrides. Bound ambiguous ML execution recovery, disable transport-level request replay, validate legacy chain catalogs during migration, and align release smoke tests and operator documentation with the resulting contracts.
Track the SHA from each freshly accepted catalog before reconciliation and give an already-admitted old generation one non-renewable rollout lease. Failed same-name replacements now leave only the affected version pool after the deadline, while mirror URL changes with identical bytes remain admissible. Parse GONKA_HA once with the full closed boolean grammar and pass the typed value through versiond startup and session lookup. Prefer fresh consensus state before reconciling persisted LKG children, and reject DATABASE_URL already present in running HA supervisors. Require matching rollback container IDs to remain running and healthy before treating recovery as complete. Journals without generation identity now restore conservatively, with crash tests for stopped generations and absent legacy resources.
The bounded same-name rollout lease could not protect a host that missed the catalog update, did not cover direct versiond routes, and expired before the supported download budget. Remove that second eligibility model instead of extending another timeout. Consensus now permanently binds an approved version name to its SHA while allowing the download mirror to change. New artifact bytes require a new version name, so name-only routing remains correct even when hosts observe different catalog revisions. Versiond keeps its route-based serving predicate and no longer exposes or configures an artifact rollout lease. Verify DAPI catalogs against inference params queried at the catalog's exact Cosmos block height, including the returned-height proof. Governance and v0.2.15 upgrade tests pin immutable SHA progression and URL-only mirror updates. Make ingress --recover-only restore from journaled project and rollback context before sourcing forward config, fix the exact CI shellcheck failure, cover config-independent recovery, and include the external PostgreSQL overlay in release smoke triggers with a complete cold-start example.
Remove the inference execution fence, append-only governance catalog, consensus verification, and versiond artifact lease introduced while hardening the router HA rollout. These mechanisms change Gonka core behavior and require separate architectural approval.
Restore inference-chain, DAPI, chainoracle, devshard inference/storage interfaces, and versiond artifact reconciliation to the parent branch contracts. Keep the HA-specific PostgreSQL liveness signal and durable storage identity used to prove that versiond replicas share one database.
Make both router tiers consume the existing DAPI {versions:[...]} response. Monotonicity and generation tracking now belong only to the local crash-safe HAProxy projection cache, without adding a governance revision API.
Update deployment configuration, documentation, and router acceptance tests for the restored boundary.
Include the canonical router fleet specification in the release and ingress transaction fingerprints, and reject configuration drift before fleet mutation or ingress commit. Gate standalone HA activation on the live PostgreSQL identity and the router-compatible version-name catalog. Make recovery fail closed for corrupt or unknown journals while retaining them for diagnosis. Preserve complete operator Compose topologies in day-2 documentation and document the remaining additions-only routing and client-retry semantics. Add regression coverage for fleet drift, database mismatch, invalid catalog names, and recovery failures.
Reject PGOPTIONS in both rendered HA Compose models and running versiond supervisors before any replacement or fleet mutation. Treat even matching non-empty values as unsafe because libpq session parameters can select a different logical schema. Use an absolute end anchor for version catalog names so trailing line feeds cannot pass the deployment grammar gate. Compare standalone router cutovers with the PostgreSQL identity stored in an existing upgrade marker. Cover matching and divergent PGOPTIONS, terminal-LF names, and committed identity drift. Run the full router fleet Docker acceptance as a separate pull-request CI job.
snevolin
marked this pull request as ready for review
August 13, 2026 01:32
![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: ab4542bd76
ℹ️ 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".
Comment on lines
+469
to
+470
| [[ $signal_status -eq 143 ]] || fail \ | ||
| "TERM during cutover returned $signal_status instead of 143" |
There was a problem hiding this comment.
On GNU Bash 5.2.21, running deploy/join/enable-router-ha_test.sh directly—the same command required by the compose-contract job in .github/workflows/release-image-smoke.yml—deterministically reaches this assertion with signal_status=1 after the script reports that automatic ingress rollback failed, rather than returning 143. Consequently, every PR matching that workflow's paths gets a failing required job; fix the signal/rollback scenario so it preserves the expected exit status before making this test mandatory.
Useful? React with 👍 / 👎.
The live version-routing recipe used Bash's [[ =~ ]] operator even though Make executes it with /bin/sh. Ubuntu runners therefore stopped at parse time before exercising the dynamic governance route. Express the host assertion as a POSIX case statement so the same test runs under dash and Bash. Treat a TERM during ingress cutover according to the durable transaction contract rather than one exact shell status. Both signal exit 143 and rollback-failure exit 1 are valid non-success outcomes; require a recoverable active or rolled-back journal and prove that the following run commits it. This keeps the test strict about recovery while removing a timing-dependent exit-code failure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac710847d7
ℹ️ 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".
| && chown -R haproxy:haproxy /etc/haproxy /var/run/haproxy \ | ||
| /var/lib/gonka-router | ||
|
|
||
| COPY versiond-router/haproxy.cfg.template /etc/haproxy/haproxy.cfg.template |
There was a problem hiding this comment.
When the local versiond stack is built from local-test-net/docker-compose.versiond.yml, Compose uses context: ./versiond-router, so this source path resolves outside the build context and Docker fails before creating the router image (the later router-runtime/... copy is likewise unavailable). Use repository-root context for that Compose service, as the Makefile and generated testenv already do, or make the Dockerfile compatible with its directory-local context.
Useful? React with 👍 / 👎.
Model policy replacement with the nginx image's graceful SIGQUIT shutdown instead of force-removing an upstream after it may have accepted a POST. Make the connect-level redispatch assertion deterministic by selecting a hash key pinned to a fixture whose data listener is then disabled while its admin readiness remains available. Give policy rollback explicit success and failure returns so interrupted ingress recovery behaves consistently on Bash 5.2 and 5.3, with resource-specific diagnostics when restoration fails.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0342b4837
ℹ️ 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".
Comment on lines
142
to
+146
| versiond2: | ||
| <<: *versiond | ||
| container_name: versiond2 | ||
| deploy: | ||
| replicas: ${VERSIOND2_REPLICAS:-1} |
There was a problem hiding this comment.
In the new two-versiond HA overlay, an active session that exists when both devshardd children start is eagerly reconstructed and cached by both replicas (session/manager.go:424-446); after the selected replica advances that session, the standby is never refreshed because SessionServerExisting returns its cached server directly (session/manager.go:159-170). If the router later evacuates the selected host or remaps the hash, versiond2 therefore serves its old in-memory state despite sharing the current PostgreSQL rows, which can produce invalid nonces or conflicting state transitions. Recovery must be made ownership-aware or fenced/refreshed before enabling the second replica.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
GitHub requires the base branch to belong to the target repository, so this
PR is opened against
gonka-ai/gonka:upgrade-v0.2.15. Functionally it isstacked on
snevolin/gonka:sn/versiond-host-evacuation-0.2.15and should bereviewed relative to that branch.
b1f28c12c5157bc49775b5bfa689d0d67a27b28db1f28c12c5157bc49775b5bfa689d0d67a27b28d..c0342b4837f5e79af1e08908fa11f308b1796bde740ad0902a9ef7b76557bc1179a2a22e590129b1^..c0342b4837f5e79af1e08908fa11f308b1796bde(41 commits)GitHub's Files changed view also includes the prerequisite host-evacuation
series between
upgrade-v0.2.15andb1f28c12c; that series is outside thisPR's conceptual scope.
Summary
This change turns
versiond-routerfrom one process into a replicated,stateless routing tier. Three independent router slots derive the same
version-aware pools and escrow-based consistent-hash placement from Docker DNS,
active checks, and DAPI's existing
/versionscatalog. They require no sharedrouting database, leader, or replica-to-replica coordination.
The PR also supplies the operational machinery needed to use that redundancy
safely: rolling fleet reconciliation, end-to-end admission checks, automatic
route discovery, crash-recoverable host upgrade, exact rollback, release
preflight, and production acceptance coverage.
Architecture
HAProxy owns membership, health, and replica selection. nginx keeps the
existing HTTP policy. Established streams remain on the process that accepted
them during a planned drain; new requests avoid starting, draining, and
unavailable members.
What this delivers
remaining admitted slots continue receiving new traffic.
key and use address-keyed consistent hashing. Placement is stable across DNS
answer order and router restarts.
v5leaves only thev5pool andcan continue serving healthy versions such as
v4.VERSIOND_VERSIONSis a cold-start floor, not aday-2 allowlist. Newly approved names are learned from
/versions, preparedin bounded backend slots, and published only after the configured ready
reserve exists. No hoster edit or router restart is required.
atomically and restored after restart. Malformed, stale, removal, or
capacity-exhausting snapshots cannot erase working routes.
routing layer. A non-idempotent request is not replayed after it may have
reached an application.
distributor. Fleet status and Prometheus metrics cover both router levels.
/versionsbridgeinstead of access to DAPI's callback surface; Runtime API mutation remains on
container-local Unix sockets.
Fleet and upgrades
deploy/join/versiond-router-fleet.shprovides one guarded lifecycle fornetwork preparation, bootstrap, status, rolling replacement, rollback,
maintenance rollout, shutdown, and cleanup. It replaces one slot at a time,
keeps the configured ready reserve, verifies actual parent admission for every
required route, and restores the exact previous image and environment on
failure. Placement-changing configuration requires an explicit maintenance
operation rather than silently mixing hash contracts.
The v5 updater uses the same fleet lifecycle and treats the host update as one
recoverable deployment transaction:
operator overlays, observability settings, and external PostgreSQL topology;
the actual shared PostgreSQL identity observed through both supervisors;
recovery procedure, and network-update source.
This makes future router fixes part of the normal host update path instead of
an undocumented side command.
Scope
proxy-router, Docker daemon, host networking, and bundled PostgreSQL remainhost-level failure domains.
future Kubernetes topology must preserve version-aware routing.
semantics, or ML execution algorithms. Catalog monotonicity and recovery are
local properties of the HA routing projection.
explicit maintenance operation.
and is not claimed by this routing layer.
Validation
Coverage added or extended by this series includes:
stability, retries, catalogs, isolation, and PROXY protocol;
degraded-slot repair, network ownership, shutdown, and cleanup;
external-PostgreSQL topologies;
drift, PostgreSQL identity mismatch, and rollback;
existing host-evacuation acceptance against the replicated path.