Skip to content

fix(spurctld): stop serving when the raft core is dead - #810

Open
pre wants to merge 7 commits into
ROCm:mainfrom
silogen:pr/health-endpoints
Open

fix(spurctld): stop serving when the raft core is dead#810
pre wants to merge 7 commits into
ROCm:mainfrom
silogen:pr/health-endpoints

Conversation

@pre

@pre pre commented Sep 2, 2026

Copy link
Copy Markdown

Required merge order: main < #806 < #810 < #843 < #844

Relates to issue:

Related pull requests:

Motivation

A spurctld whose RaftCore has stopped keeps running and keeps taking work.

RaftCore is a task of its own. When it panics, that task ends and every other
task survives, the gRPC listener included. The process therefore goes on accepting
connections and serving reads from a state machine nothing can replicate to, while
every write fails. The readiness probe is tcpSocket: 6817, which reaches the
listener and not Raft, so Kubernetes sees a healthy Pod: 1/1 Running, 0 restarts,
and the Service keeps sending clients to it.

The goal is that a controller which cannot replicate stops taking traffic, and then
stops.

Technical Details

Leaving when the core dies. openraft owns the metrics watch channel from
inside RaftCore, so the channel closes exactly when that task ends, whatever
ended it. RaftHandle::core_stopped awaits that close; a supervisor task in
main.rs logs the reason and exits with code 70, so a supervisor restarts a whole
controller instead of leaving a half dead one in service.

A probe that reads Raft. /livez reports whether the core runs. /readyz
adds a known leader, because a replica that knows no leader cannot answer a read
that means anything.

A listener of its own for the probes. The first version of this change put the two
routes on the metrics HTTP server. That was wrong: metrics.bind defaults to
loopback and metrics.enabled can turn that server off, so with the defaults a
kubelet probe reaches nothing and every replica stays unready for good. A new
[probes] section, enabled by default and bound to every interface on port 6823,
carries them instead. It is named [probes] rather than [health] so that it does not collide with the [health] node check that #836 adds. The two routes answer with a status code and one word, so
they carry nothing worth hiding, unlike the metrics they used to share a port with.
They stay mounted on the metrics port as well, for a local check.

Note for reviewers: this opens a new listener on an existing deployment after
upgrade. It is documented in docs/deployment/upgrading.rst and
docs/deployment/native-host.rst, and probes.enabled = false switches it off.
If you would rather it defaulted to loopback, say so, but be aware that then a
deployer who forgets to override it gets every replica permanently unready, which
is the failure this section exists to prevent.

Two Services. The replicas need peer DNS before a leader exists, and
readiness waits for a leader, so the headless Service must set
publishNotReadyAddresses or the two conditions deadlock and the cluster never
starts. That flag also publishes an unready replica, which would make the readiness
probe pointless for clients. A second Service, spurctld-client, has no such flag
and therefore drops a replica while /readyz fails. The operator uses it.

Related:

Test Plan

  1. Cold start three replicas and confirm the readiness gate and peer DNS do not
    deadlock.
  2. Put a controller into a state with no quorum and confirm it is held out of
    service, and that the old TCP probe would have passed it.
  3. Confirm the headless Service still resolves peers while that replica is unready,
    and that spurctld-client does not.
  4. Run with a config that has no [probes] section, to prove the default works
    without the deployer knowing about it, and that metrics stay on loopback.
  5. Run a real job through spurctld-client, and confirm accounting records it.
  6. Kill the leader and sample the ready endpoint count, to find out whether the
    readiness gate causes an outage of its own during an election.

Test Result

Environment: three node RKE2 cluster on cloud VMs, 8 vCPU and 96 GiB each, no GPU,
with PostgreSQL accounting.

  • No deadlock. Three replicas cold started to 1/1 Running on all three in 40 s.
  • A controller with no quorum is held out. It stayed 0/1 Running. Its
    container was Started, so the old tcpSocket: 6817 probe would have passed it,
    while the kubelet logged Readiness probe failed: HTTP probe failed with statuscode: 503 25 times.
  • The two Services behave differently, as intended. For that replica the
    headless Service still published the address for peer DNS
    (ready:true, serving:false), and spurctld-client published nothing, so no
    client could reach it.
  • The default works with no configuration. The controllers ran with a config
    carrying no [probes] section. The log reads metrics ... bound=127.0.0.1:6822
    beside probe HTTP server listening bound=[::]:6823, and all three replicas were Ready. Metrics
    stayed on loopback and the probes still answered, which is the whole point of the
    separate listener.
  • Real work goes through the client Service. A job submitted through
    spurctld-client completed, and sacct shows it.
  • No outage during a failover. The leader Pod was deleted while the ready
    endpoint count of spurctld-client was sampled every 0.45 s. It went 3, then 2,
    then back to 3 after 43 s. It never reached 0, so the readiness gate costs nothing
    during an election. A job submitted afterwards completed on the new leader.
  • cargo clippy --workspace --exclude spur-ffi --all-targets --locked reports
    nothing; cargo test --locked passes 3525 tests, including new ones for the two
    endpoints and for the probe defaults. sphinx-build -W --keep-going succeeds.

Not verified: the exit(70) path has never been seen to fire on a cluster.
RaftCore panicked reliably only through the scale-up defect, and the change in
the related bootstrap PR removes that route, so the condition can no longer be
produced on a real cluster. It is covered by unit tests only, and this is stated
rather than glossed over.

Submission Checklist

@pre

pre commented Sep 2, 2026

Copy link
Copy Markdown
Author

Verified on a cluster: exit(70) fires.

The description says the exit(70) path had never been seen on a real cluster, because #806 removes the only route that reliably kills RaftCore. That gap is now closed. I built an image from this branch with #806 reverted, which makes the scale-up defect reachable again, and ran the transition.

Method. Three node RKE2 cluster, in a namespace of its own so the healthy deployment stayed untouched.

  1. replicas: 1 with a single Raft peer, then 55 jobs submitted to fill the log.
  2. replicas: 0, which freezes the store at term 1 with membership {1}.
  3. Three Raft peers in the config, then replicas: 3. The two new replicas get empty volumes and bootstrap a second cluster, which is what fix(spurctld)!: refuse to bootstrap a second raft cluster #806 prevents.

Result on spurctld-0:

raft store recovered from disk log_entries=57 vote=Some(Vote { leader_id: LeaderId { term: 1, node_id: 1 }, committed: true })
thread 'tokio-rt-worker' (22) panicked at library/alloc/src/collections/btree/search.rs:121:21:
range start is greater than range end in BTreeMap
ERROR spurctld: RaftCore has stopped; this controller can no longer replicate. Exiting so the supervisor restarts it.

Container lastState.terminated: exitCode: 70, reason: Error, started 12:51:26Z, finished 12:51:38Z, and restartCount went to 1.

Before this change the same panic left the Pod 1/1 Running with 0 restarts. The process now ends 12 s after start and the kubelet replaces it. The restart is not a crash loop: the new process joins the cluster that won and becomes Ready after about 30 s. The 55 jobs of the old cluster are gone, which is the data loss #806 exists to prevent.

One note for anyone reproducing it: the panic needs a genuinely stale store. A first attempt without the replicas: 0 step did not panic, because the new replicas sent vote requests to spurctld-0 while it was still running and carried it to term 8, so it recovered a store that was no longer divergent. Scaling to 0 first is what keeps the store at term 1.

@codecov-commenter

codecov-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.00000% with 56 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
+ Coverage   80.40%   80.43%   +0.02%     
==========================================
  Files         187      187              
  Lines       92752    93094     +342     
==========================================
+ Hits        74577    74875     +298     
- Misses      18175    18219      +44     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pre added 3 commits September 10, 2026 15:48
BREAKING CHANGE: a spurctld with an empty Raft store now refuses to start
when a configured peer already runs a cluster that does not list it. This
turns a silent corruption into a clear startup failure, and it makes the
replica count of a running controller fixed.

A replica added to a running cluster starts with an empty store, exactly
like a first start, so it bootstrapped a NEW cluster. On Kubernetes a
StatefulSet rolling update starts at the highest ordinal and gives each
new ordinal an empty volume, so raising replicas from 1 to 3 made the two
new empty replicas form a cluster and out-vote the one holding the data.
Measured results: the replica with the data panicked in openraft
("range start is greater than range end in BTreeMap"), the job id counter
restarted at 1 and collided with ids already in accounting, and the
replicas answered sinfo differently behind one headless Service.

An empty store now asks each peer over the new ClusterProbe RPC whether it
belongs to a cluster. The probe reports the whole membership, voters and
learners. A peer whose membership contains this node, in either role,
means a member lost its volume, so it starts and catches up. A peer whose
membership does not contain it means bootstrapping would split the
cluster, so startup fails with a message that says the replica count
cannot be raised. A peer that predates the RPC answers UNIMPLEMENTED,
which is read as "no cluster", so mixed versions behave as before.

ClusterProbe is added to raft_internal.proto, which is controller-to-
controller plumbing only. No field tag changes, and slurm.proto is
untouched.

Signed-off-by: Petrus Repo <petrus.repo@amd.com>
The deployment page told the reader to apply spurd.yaml AND operator.yaml.
Both register the same Kubernetes nodes, so a reader who followed it got
two agents claiming every node. They are alternatives: the page now gives
a table of the two topologies and says that Pod mode launches only a
SpurJob, so sbatch and spur submit accept a job that never gets a Pod.

Add a warning that the spurctld replica count is fixed at the first apply,
which is what the controller now enforces.

Example manifest: spurd.yaml selected spur.amd.com/compute, but the
operator default and every other reference is spur.amd.com/managed. A
reader who labelled nodes for one got nothing scheduled by the other.

Signed-off-by: Petrus Repo <petrus.repo@amd.com>
@pre
pre force-pushed the pr/health-endpoints branch from 8735661 to 1eb7594 Compare September 10, 2026 12:54
pre added 4 commits September 10, 2026 16:00
A panic inside RaftCore ended that task alone. Every other task kept
running, so the process stayed up, the gRPC listener kept accepting
connections, and the replica served stale reads while every write failed.
Kubernetes could not see it either: the probes were tcpSocket on 6817,
which the listener answers whatever Raft is doing. Observed as a Pod that
stayed 1/1 Running with 0 restarts while it replicated nothing.

Two changes, because one alone is not enough. openraft moves the metrics
sender into RaftCore and leaves only the receiver on the handle, so the
channel closing is an exact signal that the task has ended. spurctld now
watches it and exits 70, and the supervisor restarts a whole process.

The metrics server also serves /livez, which reports whether the core
runs, and /readyz, which adds a known leader so a replica that cannot
answer a read leaves the Service. The example manifests probe these
instead of the TCP port, and set metrics bind = "all" so the kubelet can
reach them.

The headless Service gains publishNotReadyAddresses: true. Peer DNS has to
resolve before a pod is ready, because the replicas need each other to
elect the leader that readiness waits for; without it the two conditions
deadlock at bootstrap and no cluster ever forms.
The headless spurctld Service sets publishNotReadyAddresses, because the
replicas need peer DNS before they can elect a leader. That flag also
publishes a replica whose Raft is dead, so the readiness probe changed
nothing for a client.

Add spurctld-client, a normal Service without the flag. It drops a
replica while /readyz fails. The operator now uses it.
/livez and /readyz lived on the metrics server. metrics.bind defaults
to loopback and metrics.enabled can turn the server off, so a kubelet
probe against that port reaches nothing and every replica stays unready
for good. Behind a Service that honours readiness this is a full outage,
caused by a default.

Add [probes], enabled and bound to all interfaces by default, on port
6823. The two routes answer with a status code and one word, so they
carry nothing worth hiding. They stay mounted on the metrics port too,
for a local check.

The example Pod probes now use 6823, and the example config no longer
has to publish metrics to the Pod network to make its probes work.
@pre
pre force-pushed the pr/health-endpoints branch from 1eb7594 to 4965d8b Compare September 10, 2026 13:01
@pre

pre commented Sep 10, 2026

Copy link
Copy Markdown
Author

This PR is stacked on PR 806. Merge order: 806, 810, 843, 844.

The branch now sits on top of the 806 branch, so the diff shown here includes 806's commits until 806 merges. After that it shrinks to this PR's own four commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants