fix: eliminate rickshaw-settings.json.xz race condition when multiple engines start simultaneously - #871
Conversation
1b19737 to
9b47720
Compare
…ts and kube endpoints When multiple engine containers start simultaneously and share a common directory, they all execute bootstrap scripts that SCP rickshaw-settings.json.xz from the controller to the shared directory at the same time. The concurrent SCP writes corrupt the file — one container's partial write is overwritten mid-stream by another container's SCP. The "SCP succeeded" message reports the SCP's own exit code, not whether the file on disk is intact. The concurrent writes can corrupt the file between the SCP completing and the engine reading it. **Root causes:** - **remotehosts**: Multiple containers on the same host share /shared-engines-dir via bind mount - **kube**: Multiple containers in the same pod share an emptyDir volume This fix addresses both endpoints: **remotehosts.py**: - Add copy_rickshaw_settings_worker_thread() and copy_rickshaw_settings_to_remotes() to copy the settings file to each remote's data directory before container launch - Call copy_rickshaw_settings_to_remotes() in main() after create_remote_dirs() and before remotes_pull_images() **kube.py**: - Add an initContainer to each pod that downloads rickshaw-settings.json.xz once before main containers start - The initContainer uses Fabric/SSH to fetch the file from the controller and writes it to the shared emptyDir - Eliminates the race between multiple containers in the same pod **bootstrap.py**: - Check if rickshaw-settings.json.xz already exists in /shared-engines-dir - If present (endpoint pre-copied it), use it and log success - If missing, fall back to SCP with a warning (maintains backwards compatibility) This approach: - Eliminates the race condition for remotehosts and kube - Maintains backwards compatibility (SCP fallback) - Scales to any number of engines per host/pod - No coordination or locking required Testing: - 4-engine remotehosts fio test: ✅ File copied to all 4 remotes before launch, no SCP from bootstrap - kube endpoint: initContainer runs before main containers, eliminating race Closes #870 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
9b47720 to
2d6b02d
Compare
InitContainer Lifecycle ExplanationFor reviewers unfamiliar with Kubernetes initContainers, here's how they solve the race condition: Pod Startup SequenceInitContainer Execution (Sequential, Blocking)Our Specific FlowWhat happens in our pod: Key Properties of InitContainersSequential Execution:
Blocking:
Failure Handling: Shared Storage:
Why This Eliminates the RaceBEFORE (broken): AFTER (fixed): Evidence from TestingInitContainer status (from endpoint logs): {
"containerID": "cri-o://8b9836c10021...",
"exitCode": 0,
"reason": "Completed",
"startedAt": "2026-08-20T23:14:52Z",
"finishedAt": "2026-08-20T23:14:52Z"
}Main container bootstrap log: Notice the timestamps:
The main container literally couldn't start until the initContainer succeeded! SummaryInitContainers are Kubernetes' way of saying:
This guarantees our settings file is present before any engine containers try to use it, completely eliminating the race condition. |
PR Review: rickshaw#871 — fix: eliminate rickshaw-settings.json.xz race condition when multiple engines start simultaneouslySummary: Eliminates a race condition that corrupts Issues
File Coverage
Limitations
VerdictApprove with comments — The PR is incredibly well-designed, extremely clean, backwards-compatible, and resolves a highly critical race condition without needing complex coordination or locking. The ignored return code in |
The return value was previously discarded in main(), so a failed settings-file copy to a remote would fail silently at the endpoint layer. Now log an error when it happens; not fatal since bootstrap.py still falls back to SCP from the controller as a backwards-compatible safety net. Addresses PR review feedback on #871.
|
Addressed the ignored-return-value finding: The missing unit test coverage note was left as-is per the review's own observation that it's consistent with other pre-existing endpoint functions. Commit: 6af45cb |
atheurer
left a comment
There was a problem hiding this comment.
LGTM! The pre-copy strategy via Kubernetes initContainers and remotehosts thread pool cleanly eliminates the race condition while preserving the SCP fallback in bootstrap.py.
Problem
When multiple engine containers start simultaneously and share a common directory, they all execute bootstrap scripts that SCP
rickshaw-settings.json.xzfrom the controller to the shared directory at the same time. The concurrent SCP writes corrupt the file — one container's partial write is overwritten mid-stream by another container's SCP.From issue #870 logs:
Both SCPs overlap. The "SCP succeeded" message reports the SCP's own exit code — it does not mean the file on disk is intact, only that the bytes were sent. The other container's concurrent write can corrupt the file between the SCP completing and the engine reading it.
Root causes:
/shared-engines-dirvia bind mount/shared-engines-dir8-stream tests succeed because fewer concurrent SCPs make the race unlikely. 16-stream tests hit it consistently because 16 concurrent SCPs almost guarantee an overlap.
Solution
Move the
rickshaw-settings.json.xzcopy operation from bootstrap (runs in every container) to the endpoint scripts (run once before containers/pods start).remotehosts endpoint fix
Copy the file once per remote host before any containers are launched using the existing SSH infrastructure.
From the issue recommendation:
kube endpoint fix
Use a Kubernetes initContainer that downloads the file once before the main containers in each pod start. The initContainer:
printf '%b'to properly handle SSH key newlinesThis is the Kubernetes-native way to prepare shared data for pods.
bootstrap.py fallback
Bootstrap now checks if the file already exists:
This maintains compatibility with any endpoints that haven't been updated yet.
Changes
endpoints/remotehosts/remotehosts.py:copy_rickshaw_settings_worker_thread()- worker thread to copy settings file to a remotecopy_rickshaw_settings_to_remotes()- orchestrates copying to all remotes in parallelmain()aftercreate_remote_dirs()and beforeremotes_pull_images()endpoints/kube/kube.py:printf '%b'to properly convert\nsequences to real newlines for SSH key/shared-engines-dir/engine/bootstrap.py:Testing
remotehosts endpoint ✅
Ran a 4-engine fio test across 4 remote hosts:
Endpoint log excerpt:
Engine log excerpt:
kube endpoint ✅
Ran a fio test on Kubernetes cluster:
exitCode: 0,reason: "Completed")Endpoint log excerpt:
{ "exitCode": 0, "finishedAt": "2026-08-20T23:14:52Z", "reason": "Completed", "startedAt": "2026-08-20T23:14:52Z" }Engine log excerpt:
Why This Fix is Correct
Closes #870
🤖 Generated with Claude Code