Add scale down to HistoryPubnetParallelCatchupV2 by Jonathan-Eid · Pull Request #433 · stellar/supercluster

Jonathan-Eid · 2026-08-27T17:25:20Z

Purpose

Currently, the CatchupV2 mission does not scale down workers when the amount of remaining/in-progress jobs becomes less than the amount of parallel workers.

Implementation

Before each parallel worker was a replica of the same statefulset, which prevented picking out an idle worker to scale down.

In this change, each worker is its own statefulset, so once the amount of jobs goes below the amount of parallel workers, we can pick out the idle workers, mark them as decommissioned by adding their entry in Redis in one pass. Then the next pass of the same loop ensures they are not busy before collecting its logs and deleting its statefulset.

Busy workers are determined by another redis check to check which workers are job_owners.

All redis checks are performed by the F# driver execing into a ready worker pod and running a redis-cli command.

Caveats

Up to 64 workers can have their logs collected and scaled down in one pass. Keeping the log collection synchronous and blocking the loop. Keeps the code cleaner for not much affect on the loop time and avoids managing threads.

A run holds its full fleet for its whole duration, including the tail where almost nothing is left: 37.7% of prod worker-hours are idle, and at the extreme 1020 workers wait 4.5 hours on 4 remaining jobs. Each worker becomes its own single-replica StatefulSet, so any idle worker can be removed without regard to position; scaling one shared StatefulSet only ever deletes a suffix of its ordinals, which a single long range on a high ordinal can pin for hours. Each poll marks up to `ready - outstanding` idle workers into a Redis set that worker.sh checks before claiming, and deletes the ones marked on an earlier pass that are still idle. The one-pass gap is the safety property: a marked worker stops claiming within its 10s loop, so by the next poll it provably takes no new work. Busy workers come from reading job_owners directly rather than from the status snapshot, which the monitor only republishes after serially pinging every owner and so can be older than the mark. Capacity is Running pods, not the replica count: a Pending pod owns no job and so reads as idle capacity it cannot supply. Logs are collected before each deletion, since /data is emptyDir, and a failed collection skips the removal rather than losing them.

A pod being deleted still reports phase Running, so readyPods counted pods on their way out. That inflated capacity, marked more workers than the queue could spare, and re-selected the pods just deleted -- whose StatefulSets were already gone, so the delete threw NotFound and aborted the whole pass. A run lost a pass after every successful wave. The ready set also has to drop the workers removed earlier in the same pass, or the marking step reads a fleet size that is one wave stale and re-marks what it just deleted.

Both reads ran on every poll of a multi-hour run even while the queue outran the fleet and nothing was marked, and the pod list at 1024 workers is not cheap. They are now skipped unless something is marked or the queue has dropped below the fleet, which also stops the ramp logging a skipped pass every poll: with no pod Running yet, taking the head of an empty ready set threw and the handler reported it as a real failure. The guard reads livePods rather than the ready set, since consulting the ready set is the cost being avoided. livePods is always the larger of the two, so the guard errs toward doing the work rather than skipping it. redisIn now takes the ready set and picks a host itself, returning nothing when there is none, so callers stop reaching for Seq.head.

Log collection is serial and runs inside the poll loop, so an unbounded batch blocks it for as long as the batch takes. A 400-worker run retired 213 in one pass and spent 103 seconds collecting, and that was with tiny archives: each worker had run only 2 ranges. A prod worker churns dozens over hours, so per-pod cost is seconds rather than half a second, and an uncapped pass late in a 1024-worker run would block for tens of minutes with no timeout on any single exec. Capped at 32 per pass, so the worst case is bounded by the cap rather than by the fleet size. Later passes drain the rest.

helm was left to use the kubeconfig's current namespace while every other call in the mission honours context.namespaceProperty. A run launched with --namespace ssc-config-test therefore installed its chart into stellar-supercluster: 1024 workers and a monitor landed in the production namespace, while the driver polled and scaled an empty one, so scale-down silently did nothing for the whole run. Both uninstall paths get it too. They were previously consistent with the install only by accident -- both defaulted to the same current namespace -- which would have broken the moment the install was fixed alone.

Once every worker is marked retiring, a job the monitor puts back on the queue can never be claimed: worker.sh refuses to claim while marked, and there is no way to unmark. A single recoverable orphan then hangs the run. Observed on a 1024-worker run: all 4009 ranges were accounted for, one job lost its owner upstream, the monitor correctly requeued it, and nothing was left willing to take it. The run had to be aborted. Two changes, both in the mark step. The reserve is now counted against unmarked workers rather than `ready`, which includes marked pods that have not been deleted yet and so let the reserve erode away over successive passes. And the reserve never drops below minUnmarkedWorkers, so a run whose queue empties still has somewhere to put returned work. Three rather than one so the reserve survives a worker restarting or its node being disrupted, and so several returned jobs drain in parallel. The cost is three idle workers, about two nodes, against a tail that otherwise idles the whole fleet.

RunRemoteCommandAndCaptureOutput returned void and wrote the k8s status channel to the console, so neither caller could tell a failed exec from a command that produced no output. Two fail-open paths followed. `redisIn` reads job_owners to decide which workers are idle. A failed exec left it with zero lines, which reads as "no worker is busy" and makes every worker look retirable -- the exact check that is supposed to stop a busy worker being retired. Two such passes in a row would mark a worker and then delete it mid-range. `collectLogsFromPods` treats an exception as the only failure, but tar exiting non-zero raises nothing, so a failed archive counted as collected and the worker was deleted with its logs. The exit code was already available: channel 3 carries a V1Status whose causes hold it, and Kubernetes.GetExitCodeOrThrow already parses exactly that for RunRemoteCommand in the same file. Renamed the locals and fixed the comments too -- a variable called `stderr` holding the status channel is what made this invisible. Both callers now fail closed: redisIn raises into the poll loop's handler, which skips the pass with no state mutated, and a failed tar puts the pod on the failed list so retirement is skipped for it.

…ds per pass

Copilot

Pull request overview

Adds dynamic worker scale-down to parallel catchup by assigning each worker its own StatefulSet.

Changes:

Introduces worker retirement through Redis.
Collects logs before deleting idle workers.
Returns remote command exit codes and scopes Helm commands to the namespace.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`catchup_workers.yaml`	Creates one StatefulSet per worker.
`worker.sh`	Prevents retiring workers from claiming jobs.
`MissionHistoryPubnetParallelCatchupV2.fs`	Implements worker selection, log collection, and retirement.
`RemoteCommandRunner.cs`	Returns remote command exit codes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The reserve was subtracted from the idle pool alone, but `outstanding` counts in-progress jobs too, and those already have a worker each. With 1000 workers, 700 busy and 700 jobs outstanding, the surplus came out as 300 - 700 and nothing was marked at all: 300 idle workers were held to cover work that was already being served. Counting the surplus against every unmarked worker gives 1000 - 700 = 300, and those 300 idle workers are marked. Only idle workers are ever marked, and the reserve still keeps `max outstanding minUnmarkedWorkers` workers unmarked, so a requeued job still has somewhere to go -- an unmarked busy worker becomes claimable as soon as it finishes. Run stellar#192 never sat in that state: in-progress fell from 1022 to 80 in three minutes, so the largest instantaneous gap was about 80 workers and retirement was capped at 32 a pass throughout. A run whose ranges vary enough for in-progress to decay gradually would sit there and mark nothing.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:255

This updated signature takes explicit pod names, but the function documentation still says it automatically derives them from pubnetParallelCatchupNumWorkers. Update the comment so callers are not given an obsolete contract.

// Returns the pods whose collection raised; an empty archive is success.

src/MissionParallelCatchup/parallel_catchup_helm/templates/catchup_workers.yaml:28

The new StatefulSet name makes every pod end in -0 (...-stellar-core-<worker-index>-0), but worker.sh derives core_id from the final hyphen-separated segment and stores it in each Redis metric. Consequently, every worker is now reported as core 0. Pass the worker index to the container or update the script to extract the penultimate segment.

  name: {{ $.Release.Name }}-stellar-core-{{ $i }}

A worker that never claimed a job has an empty /data, so the tar glob matches nothing and tar exits 2. Checking that exit code put those workers on the failed list, which blocks retirement for the whole batch and never clears, because the batch is rebuilt the same way every pass. It lands on precisely the workers scale-down exists to remove: a surplus worker that never got work. Seen on ssc-test with 12 workers and 8 ranges, where the 4 that never claimed blocked retirement indefinitely. redisIn keeps its exit-code check. There a failed exec yields an empty busy set, which reads as "no worker is busy" and makes every worker look retirable, so failing closed there is worth having.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:305

Running is not equivalent to a ready pod: for example, a pod in CrashLoopBackOff normally retains phase Running. Because redisIn always selects the first member of this set, one such lower-sorted pod can make every Redis exec fail and disable scale-down indefinitely even when other workers are healthy. Filter on the pod's Ready=True condition as well.

    |> Seq.filter (fun pod -> pod.Status.Phase = "Running" && isNull (box pod.Metadata.DeletionTimestamp))

src/MissionParallelCatchup/parallel_catchup_helm/templates/catchup_workers.yaml:28

This new StatefulSet name makes each pod end in -<worker-index>-0. The worker still derives core_id from the final segment (worker.sh:88), so every worker now reports ID 0 in the raw catchup metrics instead of its actual worker index. Update that extraction (or pass the worker-index label through the downward API) alongside this naming change.

  name: {{ $.Release.Name }}-stellar-core-{{ $i }}

Per-worker StatefulSets renamed pods to <release>-stellar-core-<i>-0, so the last hyphen-separated segment is the StatefulSet's own -0 suffix rather than the worker index. Every metrics record in run stellar#192 reported core_id 0. Nothing consumes the field -- the monitor discards it and keeps only the two durations -- but a value that is always 0 reads as real data, so pass the index the chart already has.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:535

If a later chunk fails, earlier SADD calls have already committed in Redis, but marked is updated only after every chunk succeeds. Those workers stop claiming while the driver still treats them as unmarked; if the outstanding count then rises, they may never be selected again for tracking/removal. Record each chunk in marked immediately after its successful SADD.

                    for chunk in List.chunkBySize 30 toMark do
                        let names = chunk |> List.map (sprintf "'%s'") |> String.concat " "

                        redisIn context ready (sprintf "SADD \"%s-retiring\" %s" helmReleaseName names)

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:256

The function no longer derives pod names from pubnetParallelCatchupNumWorkers, so the preceding numbered contract is stale and can mislead callers. Document that the caller supplies the pod list.

// Returns the pods whose collection raised; an empty archive is success.
let collectLogsFromPods (context: MissionContext) (podNames: string list) : string list =

The sweep helm-uninstalls a PCv2 release before deleting resources individually, so the release secret does not end up dangling at deleted workloads. It found the release by matching a StatefulSet name ending in -stellar-core, which no longer happens: per-worker StatefulSets end in -stellar-core-<index>. Abandoned runs therefore had their workloads deleted one by one with the release left behind. Matching on the -stellar-core segment instead, and collapsing the names to a set, so a 1024-worker release is uninstalled once rather than 1024 times.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:305

Running does not imply that the stellar-core container is Ready; CrashLoopBackOff pods commonly retain phase Running. Because redisIn always chooses Seq.tryHead, one low-index crash-looping pod can be selected every pass and prevent scale-down even while other workers are healthy. Restrict this set to pods whose worker container reports Ready (and ideally retry another member if an exec races with termination).

    pods.Items
    |> Seq.filter (fun pod -> pod.Status.Phase = "Running" && isNull (box pod.Metadata.DeletionTimestamp))

src/MissionParallelCatchup/parallel_catchup_helm/files/worker.sh:30

The retirement check is separate from both LMOVE and the ownership HSET, leaving a deletion race: a worker can observe “not retiring,” be paused, then get marked; on the next poll the driver sees no owner and deletes it just as it claims a range. Make the membership check, claim, and ownership registration one atomic Redis operation (for example, a Lua script) so every claimed range is visible before a marked worker can be considered removable.

# Stop claiming once the driver marks us, so it can remove us without interrupting a range.
if [ "$(redis-cli -h "$REDIS_HOST" -p "$REDIS_PORT" SISMEMBER "$RELEASE_NAME-retiring" "$POD_NAME")" = "1" ]; then
    echo "$(date) $POD_NAME is retiring; not claiming."
    sleep $SLEEP_INTERVAL
    continue
fi

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/FSLibrary/StellarOrphanSweep.fs:79

A valid user tag may itself contain -stellar-core (MakeNetworkNonce permits any [a-z0-9-]+ tag). Using the first occurrence then derives the wrong release—for example, --tag=stellar-core produces a worker StatefulSet whose release is truncated before the tag—so the orphan sweep leaves the real Helm release installed. Split at the final suffix occurrence instead.

                   |> Seq.map (fun name -> name.Substring(0, name.IndexOf("-stellar-core")))

redis.job_owners is a chart value plumbed to workers and the monitor as JOB_OWNERS, and worker.sh uses it for both HSET and HDEL. The driver hardcoded the default name instead, so setting that value would leave the busy set permanently empty: every worker reads as idle, gets marked, and is deleted a pass later while running a range. The same command already expands $REDIS_HOST and $REDIS_PORT inside the pod, so the key name resolves the same way.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:305

A pod in Running phase is not necessarily ready; CrashLoopBackOff workers remain Running. Because redisIn always selects the first set member, one low-index crashing pod can make every exec fail and indefinitely disable scale-down, while also being counted as usable capacity. Filter on the Pod Ready=True condition before selecting exec hosts or counting workers.

    |> Seq.filter (fun pod -> pod.Status.Phase = "Running" && isNull (box pod.Metadata.DeletionTimestamp))

src/FSLibrary/StellarOrphanSweep.fs:79

Using IndexOf truncates valid release names whose optional tag itself contains -stellar-core (tags such as stellar-core-test are allowed). The sweeper then runs helm uninstall against a nonexistent prefix and leaves the actual orphaned release behind. Split at the final worker suffix instead.

                   |> Seq.map (fun name -> name.Substring(0, name.IndexOf("-stellar-core")))

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:534

marked is updated only after every SADD chunk succeeds. If an early chunk succeeds and a later exec fails, those workers stop claiming jobs in Redis but remain absent from the local set, so subsequent passes never consider them removable. Record each chunk immediately after its successful SADD so transient partial failure cannot strand workers.

                        marked <- Set.union marked (Set.ofList toMark)

src/MissionParallelCatchup/parallel_catchup_helm/files/worker.sh:87

This value now comes from WORKER_INDEX, but the validation error still reports a failure to extract it from POD_NAME. That points operators at the wrong input when the environment variable is missing; update the message to identify WORKER_INDEX.

    core_id="$WORKER_INDEX"

…arks per chunk --tag is user-supplied and validated only as [a-z0-9-]+, so a tag may contain -stellar-core and the release name inherits it. Splitting at the first occurrence then yields a prefix that is not a release at all: the sweep uninstalls something nonexistent and leaves the real orphan installed. Marks are recorded per chunk instead of once at the end. Above 30 retirements the SADD is chunked, and a failure after an early chunk left those workers in the retiring set but absent from the driver's own set. They then never claim and never retire, and once the properly marked workers are gone they are all that remains: the reserve is made up entirely of workers that cannot claim, so a requeued job has nowhere to go, which is the stall this branch already fixed once.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:305

A pod in CrashLoopBackOff still has phase Running, so this set can include a worker whose container cannot accept an exec. Since redisIn deterministically chooses the first set entry, one low-index broken worker can make every Redis check fail and disable scale-down even while other workers are healthy. Require the stellar-core container to be Ready (or retry another host).

    |> Seq.filter (fun pod -> pod.Status.Phase = "Running" && isNull (box pod.Metadata.DeletionTimestamp))

src/FSLibrary/MissionHistoryPubnetParallelCatchupV2.fs:256

This function no longer determines pod names from pubnetParallelCatchupNumWorkers; callers now supply the pod list. Update the function header so it does not document the removed behavior.

let collectLogsFromPods (context: MissionContext) (podNames: string list) : string list =

src/MissionParallelCatchup/parallel_catchup_helm/files/worker.sh:87

core_id now comes from WORKER_INDEX, but the validation error still says extraction from POD_NAME failed. This will point operators at the wrong input if the environment variable is missing.

    core_id="$WORKER_INDEX"

Jonathan-Eid added 10 commits August 26, 2026 13:21

feat(gitignore): add docs/plans and logs-smoke to ignore list

473acdb

fix(gitignore): remove unnecessary entries for docs/plans and logs-smoke

e29f2f2

fix(catchup): update comments for clarity and increase max retired po…

520696c

…ds per pass

Copilot AI balanced review requested due to automatic review settings August 27, 2026 17:25

Copilot started reviewing on behalf of Jonathan-Eid August 27, 2026 17:25 View session