Add killSonic and healDb scenario actions using docker exec - #271
Open
facuMH wants to merge 8 commits into
Open
Conversation
LuisPH3
previously approved these changes
Jul 31, 2026
The build root lookup walked up from the working directory until it found a Dockerfile next to a Makefile. The vendored sonic/ sub-tree has both, so the search stopped there whenever it started inside it and docker build ran against the wrong project -- the very case the marker was meant to exclude. Key the lookup on a Dockerfile next to a go.mod declaring the norma module instead, which no sub-tree can satisfy, and cover it with a test that plants a nested module carrying its own Dockerfile and Makefile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A restarted client replays blocks it has already reported, which surfaces as out-of-order appends. These were silenced by making shouldSuppressAppendConflict accept every out-of-order error and latch the node as syncing from then on. That suppressed the replay, but it also disabled out-of-order detection for every node for the rest of the run, hiding the inconsistencies this monitoring exists to find. The information the sources were missing is whether a node restarted, so provide it: NodeLogDispatcher tracks the nodes it has seen and, when one reappears, calls OnNodeRestart on the listeners that opt in via the new NodeRestartListener interface. syncingTracker implements it and marks that one node as syncing again, so its replayed blocks are excused while every other node stays under scrutiny. With the signal in place, restore shouldSuppressAppendConflict to excusing only nodes already known to be syncing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The in-place restart path always started the client as an observer, so a validator that was killed and healed came back without validator flags and stopped participating in consensus for the rest of the scenario -- while the step reported success and the scenario yml still said "type: validator". Failures to determine the sync target or to reach the network's block height were logged as warnings on the same path, so a node that never caught up also passed. Start the client in the role the step asks for, and return the errors. Extract the whole path into restartNodeInPlace, which also moves the peer reconnect and the resume notification into a readable order, and lift healDbTimeout up to the other file-level constants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the client as a docker exec rather than the container entrypoint
left its output outside the container's stdout, which is what StreamLog
serves and what the block and throughput parsers consume. It was routed
back by appending "| tee /proc/1/fd/1" to the command, and that workaround
caused the problems fixed here:
- The shell pipeline reported tee's exit status, not the client's, and
ExecBackground never inspected the exec at all. A client that died on
startup was indistinguishable from a healthy one until WaitForSync gave
up with a generic timeout.
- Exec instances ran with Tty: true, which merges stdout and stderr into
an undemultiplexable stream and makes the client emit ANSI colour
escapes. The log parser had grown a regex to strip them and
prom_log_provider had dropped parse failures to Debug to quiet the
resulting noise.
- Passing the argument vector through "sh -c" re-split it, so any
argument containing a space was mangled and scenario-supplied
extraArguments reached a shell unquoted.
Fan the output out in the driver instead. The new logBroadcaster demuxes the
exec stream with stdcopy, splits it into lines and delivers them to both a
host-side log file and any StreamLog subscriber. It belongs to the Container
and outlives a single exec, so subscribers keep working across a client
restart, and it replays the recent tail so a consumer attaching after
startup does not miss the early lines. A subscriber that falls behind drops
lines rather than stalling the stream, and one that only wants a snapshot
bounds its read with a context -- StreamLog follows a running container and
would otherwise never reach EOF, which printLog and dumpNodeLogs both rely
on. ContainerLogs is demultiplexed on the remaining paths too; it never was,
so every frame header had been landing inline in the log text.
With the output no longer going through a shell, the client runs in exec
form, its real exit status is recorded on ExecHandle and consulted by
WaitForSync, Tty is off, and the two parser workarounds are reverted.
Also folded in, because they touch the same call sites:
- ExecHandle reports the log path, so StreamExecLog reads it directly
instead of globbing and sorting the logs directory.
- Failing to create a log file is an error at start, rather than a warning
followed by a nil file handle that silently discarded the whole stream.
- OperaNode held a network.Host and a *docker.Container aliasing the same
object, with either used at random. Keeping only the container lets
ExecWithEnv, whose logName argument is a docker log-file detail, come
back out of the backend-agnostic Host interface; docker-specific options
move to docker.ExecOptions.
- Node actions claim a transitional state under the lock before doing any
work, so concurrent callers cannot interleave, and restore the previous
state when they fail. A failed heal used to strand the node in Healing
with no way to retry. ForceStopSonicd deliberately stays in Killed: once
a kill is attempted the database must be assumed dirty.
- Stop no longer demands the Running state. It is also the teardown path,
and refusing to release the container because the client was already
gone failed otherwise-successful scenarios.
- Client logs go to <outputDir>/client-logs so they outlive the run. They
were written to a temp directory that Cleanup deleted, discarding them
exactly when a failed run needed them.
- config.toml and password.txt are written to absolute paths under the
data directory via environment variables, instead of relying on the
working directory an exec happens to inherit and interpolating content
into a shell command.
- NewOperaNode delegates genesis, keystore and log directory preparation
to helpers that report any directory they created even when they fail,
so a partially prepared directory is still cleaned up.
- sonictool heal asks for a named 1024 MB cache instead of 12522 MB, which
only provoked GC pressure against the container's 1GiB GOMEMLIMIT.
- The client environment is declared once in the image rather than also in
the container config and again per exec.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The container no longer runs a startup script: everything the script did -- creating the data directory, importing genesis, writing the config and password files, applying the tc netem latency and launching the client -- is now performed as individual exec steps from driver/node/node_actions.go, so the client's lifecycle can be driven independently of the container's. The Dockerfile stopped copying the script and nothing else referenced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The state machine gated command sequences but never observed the process it
described, so the two could disagree without anything noticing:
- A client that crashed while the node was Syncing or Running left the
state saying the node was up. The exit handle knew, but it was only
consulted inside WaitForSync's retry loop, so a crash after startup went
unnoticed until some later action happened to touch the node.
- waitForSonicdExit returned nothing and treated context expiry the same
as the process exiting, so an interrupted StopSonicd still transitioned
to Ready. The invariant "Ready means no client is running" was not
enforced, it just was not being violated yet.
- The signal script ended in `; true`, so signalSonicd could not tell
signalling a live client from finding nothing to signal.
- sonicd was written outside stateMutex while the exported ExecDone and
StreamExecLog read it.
Close the loop in three places. A watcher goroutine per start waits on the
exec handle and, if the exit was not requested, moves the node to Killed and
logs the exit status: a crash is now a transition rather than a discrepancy.
Starts carry a generation so the previous process's watcher cannot be
attributed to the current client, and the generation is claimed together
with the Ready->Syncing transition, before the new process exists, which is
what makes the retirement airtight. waitForSonicdExit reports context expiry
as an error and StopSonicd keeps the node in Stopping when the process did
not exit, since it may still hold the data directory; ForceStopSonicd is
accepted from Stopping so that is not a dead end. The /proc scan reports how
many processes it matched, letting a stop that found nothing conclude the
client had already died and the database needs healing; the same scan
confirms no client is alive before a start or a heal, so neither relies on
the recorded state being right.
An unexpected exit reuses NodeStateKilled rather than adding a state,
because the recovery path is identical -- the client went away without
flushing, so heal is the only way back to Ready -- and duplicating it would
double every transition mentioning it. ClientExitWasUnexpected keeps the two
causes distinguishable for diagnostics.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
simonlechner
force-pushed
the
facundo/use_docker_exec
branch
from
August 4, 2026 14:54
46eaa9d to
c816189
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds scenario-level support for simulating an unclean sonicd shutdown and in-place database recovery on validator nodes by controlling the sonicd process via docker exec (instead of destroying/recreating containers). It also introduces an explicit node lifecycle state machine and updates monitoring and network plumbing to handle node restarts cleanly.
Changes:
- Add new scenario steps
killSonicandhealDb, plus an example scenario demonstrating kill → heal → restart → resync. - Refactor Opera Docker node startup to keep containers idle (
sleep infinity) and managesonicdviadocker execwith state-guarded actions and restart handling. - Add exec-log capture/broadcasting and restart notifications so monitoring can suppress expected “replayed blocks” inconsistencies after restarts.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run_sonic.sh | Removes the old entrypoint-based startup script (exec-driven lifecycle replaces it). |
| scenarios/examples/db_heal.yml | New example scenario exercising kill/heal/restart flow and block height convergence checks. |
| driver/parser/scenario.go | Registers killSonic/healDb step functions and parsing rules. |
| driver/parser/check.go | Validates killSonic/healDb steps require a node identifier. |
| driver/node/opera.go | Refactors node to own a docker container directly and adds lifecycle/state helpers plus exec log reading. |
| driver/node/opera_test.go | Updates node tests to stop via new lifecycle and to validate graceful shutdown via exec log output. |
| driver/node/node_state.go | New node lifecycle state enum and documentation of allowed transitions. |
| driver/node/node_state_test.go | New tests for state stringification and transition correctness/exclusivity. |
| driver/node/node_actions.go | New exec-based node actions: init/start/wait/stop/kill/heal plus /proc-based client scanning. |
| driver/node/node_actions_test.go | New unit tests covering action state guards, exit watcher behavior, and command construction helpers. |
| driver/network/validator.go | Minor logging formatting adjustment. |
| driver/network/local/local.go | Adds reconnect + suspend/resume hooks, and wires node logs to the run output directory. |
| driver/network/local/local_test.go | Updates shutdown/log assertions and checksum extraction to use exec/log-file behavior. |
| driver/network.go | Extends Network interface with ReconnectNode and Suspend/Resume hooks. |
| driver/network_mock.go | Updates gomock Network mock to include new interface methods. |
| driver/monitoring/node/transactions_throughput_test.go | Adds coverage for suppressing out-of-order appends after restart notifications. |
| driver/monitoring/node/syncing_tracker.go | Adds OnNodeRestart to mark nodes syncing again after restart. |
| driver/monitoring/node/block_metrics_test.go | Adds coverage for restart-triggered suppression logic in block metric sources. |
| driver/monitoring/node_log_provider.go | Adds restart listener interface and restart detection/notification in the log dispatcher. |
| driver/executor/run.go | Wires killSonic/healDb into executor; supports in-place restart and reconnection. |
| driver/docker/logs.go | Adds exec log broadcaster with replay + bounded subscriber buffers. |
| driver/docker/logs_test.go | Adds unit tests for broadcaster/subscription semantics and line splitting behavior. |
| driver/docker/images.go | Tightens build-root detection to avoid nested Dockerfiles by validating module root via go.mod. |
| driver/docker/images_test.go | Updates tests for new build-root detection markers and nested module behavior. |
| driver/docker/docker.go | Adds exec-with-env/background exec + log capture, demuxes container logs, and updates SaveLogTo semantics. |
| Dockerfile | Makes the image idle by default and relies on docker exec to run sonicd/sonictool. |
Files not reviewed (1)
- driver/network_mock.go: Generated file
Comment on lines
+135
to
+139
| numValidators := n.config.NetworkConfig.Validators.GetNumValidators() | ||
| dsProtection := "5000000000" | ||
| if numValidators == 1 && n.config.ValidatorId != nil && *n.config.ValidatorId == 1 { | ||
| dsProtection = "0" | ||
| } |
Comment on lines
+149
to
+153
| tcCmd := fmt.Sprintf( | ||
| "tc qdisc add dev eth0 root netem delay %v"+ | ||
| " && (ip link show eth1 2>/dev/null"+ | ||
| " && tc qdisc add dev eth1 root netem delay %v || true)", | ||
| latency, latency) |
Comment on lines
+335
to
+339
| for line := range strings.Lines(string(content)) { | ||
| if strings.TrimSpace(line) == normaModulePath { | ||
| return true | ||
| } | ||
| } |
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
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.
Introduces two new scenario steps —
killSonicandhealDb— that allow test scenarios to simulate a dirty shutdown (SIGKILL) and database recovery cycle on individual validator nodes without tearing down their containers.Motivation
Norma previously could only stop and restart nodes by destroying and recreating containers. This made it impossible to test recovery from unclean shutdowns (e.g., power loss), which is a critical validator scenario. The new actions keep the container alive and operate on the sonicd process directly via docker exec, enabling realistic crash-and-recover test flows.
Changes
ExecWithEnv,ExecBackground, andExecHandleto run commands inside existing containers via docker exec instead of docker run, including optional log capture.Initialize,StartSonicdAsObserver,WaitForSync,StopSonicd,ForceStopSonicd, andHealDbas state-guarded operations.killSonicandhealDbsteps into the scenario executor, with in-place restart support for startNode on nodes that were killed and healed.SuspendNode/ResumeNode/ReconnectNodeto pause monitoring and re-establish peer connections after node recovery.