feat(autobahn): deploy native multi-host AWS clusters - #4090
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4090 +/- ##
==========================================
- Coverage 61.30% 60.25% -1.06%
==========================================
Files 2178 2070 -108
Lines 190826 178519 -12307
==========================================
- Hits 116990 107568 -9422
+ Misses 62807 60939 -1868
+ Partials 11029 10012 -1017
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview The deployer provisions four instances ( New CLI knobs include Supporting changes add native build/install scripts, systemd/sysctl templates, small localnode script hooks ( Reviewed by Cursor Bugbot for commit b194a2d. Bugbot is set up for automated code reviews on this repo. Configure here. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-tested conversion of the autobahn-e2e AWS target from a single Docker host to four native systemd validators; the config-generation flow (step1 → peer rewrite → step2 genesis on node 0 → step4 per node → per-node tarball) is correct and the localnode script changes (ADD_VALIDATOR_SCRIPT, NODE_IP, anchored peer grep) are backward compatible with the Docker path. No blockers; the notes are about failure-path robustness (instance leak on tag failure, undiagnosed remote build failures) and remote-host cleanup.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
TestAWSDeployCreatesManagedResourcesAndReadyStateasserts thatinstall_native_node.shandprepare_native_cluster.share invoked, but nothing asserts thatnode-N.tgzis uploaded to the instance withNodeIndex == N. A mis-mapping there hands two hosts the samepriv_validator_key.jsonand produces a cluster that boots but double-signs, with no test to catch it. Worth an assertion that eachscpupload pairsnode-<i>.tgzwithinstances[i].PublicIP. - [suggestion]
inspectAWSCluster's new native branch (cmd/autobahn-e2e/list.go:161-208) duplicates ~40 lines of report assembly frominspectLegacyAWSCluster; the only real differences are batched-vs-singledescribe-instancesand the systemd-vs-docker status/height probes. Extracting the shared per-nodenodeReportconstruction and passing the two probe commands in would keep the legacy path from drifting from the native one. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| return fail(fmt.Errorf("AWS returned %d instances, expected %d", len(instanceIDs), awsClusterSize)) | ||
| } | ||
| state.AWS.Instances = make([]awsInstanceState, len(instanceIDs)) | ||
| for nodeIndex, instanceID := range instanceIDs { |
There was a problem hiding this comment.
[suggestion] Instance IDs are only persisted after the whole tagging loop finishes (save at line 248). If create-tags fails for nodeIndex == 1, fail(err) saves a state whose Instances slice holds real IDs for 0 and 1 and zero values for 2 and 3, so:
teardowncollects onlyi-100/i-101, leaks the other two instances, and then fails to delete the security group because those instances still reference it, andlistbreaks on the retainedfailedstate:awsInstanceForNodematches onNodeIndex, the zero-valued entries all reportNodeIndex: 0, so node-2 falls through toAWS instance for node-2 is missingand the wholelisterrors out — the recovery path the README documents ("Runlistto inspect it andteardownto remove the instances").
The single-instance code this replaces avoided exactly this by writing state.AWS.InstanceID immediately after run-instances. Suggest filling every Instances entry and calling a.store().save(state) before entering the tagging loop.
| } | ||
| } | ||
| for _, instance := range aws.Instances { | ||
| if err := a.waitForRemoteCommand(ctx, state, instance, "test -f "+shellQuote(filepath.Join(aws.RemoteDir, "build", "autobahn-native-build.ready"))); err != nil { |
There was a problem hiding this comment.
[suggestion] build_native_node.sh is backgrounded with nohup and only ever signals success (touch build/autobahn-native-build.ready). A build failure — a compile error on the selected --ref, a missing libwasmvm artifact, an OOM during make build-linux — produces no marker, so this poll spins until --timeout (default 20m) and the operator gets wait for native build on node-0: context deadline exceeded with no hint that build/autobahn-native-build.log exists on the host. The previous make docker-cluster-start was streamed, so failures surfaced immediately.
A cheap fix: have build_native_node.sh trap 'touch build/autobahn-native-build.failed' ERR (or write the marker in an if ! ...; then wrapper), and make the wait loop check for the failure marker too and tail the log into the returned error. Note also that 20m now has to cover apt/Go bootstrap plus a cold make build-linux on every host, so the default may want raising.
| if [[ -e "$service_home/.sei" ]]; then | ||
| mv "$service_home/.sei" "$service_home/.sei.autobahn-backup-$(date +%s)" | ||
| fi | ||
| tar -C "$service_home" -xzf "$archive" |
There was a problem hiding this comment.
[suggestion] Nothing reclaims either of two things this script leaves behind on the host: the uploaded archive at /tmp/<cluster>-node-N.tgz (created by scp with default 0644, and it contains .sei/config/priv_validator_key.json), and the $service_home/.sei.autobahn-backup-<epoch> directory from line 16. Redeploying the same cluster name accumulates one full .sei copy per run, so the root volume grows monotonically across redeploys. Adding rm -f "$archive" after the tar and pruning or overwriting the previous backup instead of timestamping a new one would keep repeat deploys bounded.
There was a problem hiding this comment.
The native multi-host AWS topology is coherent end to end — SG rules, private-IP peer/autobahn wiring, genesis distribution, systemd unit flags, and the legacy single-host compatibility paths all check out, with reasonable unit coverage. No blockers; the notes are about failure diagnosability of the backgrounded remote build and a hardcoded 4-node arity in the new shell script.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
install_native_node.shmoves a pre-existing~/.seito.sei.autobahn-backup-<epoch>and never prunes it, and nothing removes the uploaded/tmp/<cluster>-node-N.tgzafterwards. On a re-install onto a host that already ran (the path theif [[ -e ... ]]guard exists for), a full node data directory plus archive accumulates against the 100 GiB default root volume. Consider pruning older backups or dropping the archive once extracted. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
docker/localnode/scripts/step1_configure_init.shruns undershwith noset -eand silencesseid init/gentx/keys addoutput, so it exits 0 even when node initialization fails (only the twovalidator_pubkey.txt/node_pubkey.txtcopies guard with|| exit 1).prepare_native_cluster.shdoes useset -euo pipefail, but cannot detect a partially-failed node init and will go on to tar and ship a broken node home.
| } | ||
| } | ||
| for _, instance := range aws.Instances { | ||
| if err := a.waitForRemoteCommand(ctx, state, instance, "test -f "+shellQuote(filepath.Join(aws.RemoteDir, "build", "autobahn-native-build.ready"))); err != nil { |
There was a problem hiding this comment.
[suggestion] A failed remote build is not diagnosable. build_native_node.sh runs under set -euo pipefail and only touches build/autobahn-native-build.ready on success, so any failure (compile error, a moved libwasmvm artifact path, a sudo install problem) leaves no marker and this poll spins until readyCtx expires — surfacing as wait for native build on node-N: context deadline exceeded after the full --timeout, with build/autobahn-native-build.log left unread on the host. The previous flow streamed make docker-cluster-start, so a build failure returned immediately with output.
Suggest having the build script trap failure and write a build/autobahn-native-build.failed marker, then poll for either marker and, on failure, tail the log into the returned error.
Related: the --timeout default of 20m (aws.go:281) now has to cover apt/Go bootstrap plus a cold cgo build of the whole tree plus genesis generation plus node start. That is plausibly tight on the default 8-vCPU c7g.2xlarge; worth either raising the default for the AWS target or calling the requirement out in the README.
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| if [[ $# -ne 5 ]]; then |
There was a problem hiding this comment.
[suggestion] The 4-node cluster size is hardcoded in four places here ($# -ne 5, both for node_index in 0 1 2 3 loops, and CLUSTER_SIZE=4) while the Go side owns it as awsClusterSize. Changing awsClusterSize would make deploy fail on this usage guard with exit 2 rather than scaling. Since the count is already implied by the argument list, deriving it keeps the two sides from drifting:
if [[ $# -lt 2 ]]; then
echo "usage: $0 FINAL_HOME NODE0_IP [NODE1_IP ...]" >&2
exit 2
fi
final_home=$1
shift
private_ips=("$@")
node_count=${#private_ips[@]}then loop over $(seq 0 $((node_count - 1))) and pass CLUSTER_SIZE=$node_count to step4_config_override.sh.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 74581a7. Configure here.
| "--tags", fmt.Sprintf("Key=Name,Value=sei-autobahn-e2e-%s-node-%d", options.name, nodeIndex), | ||
| ); err != nil { | ||
| return fail(err) | ||
| } |
There was a problem hiding this comment.
Partial instance IDs leak on failure
Medium Severity
run-instances can succeed before any instance IDs are persisted. A count mismatch calls fail() with an empty Instances list, and a mid-loop create-tags error saves only the IDs assigned so far. teardown then has no record of the remaining hosts, so those EC2 instances stay running.
Reviewed by Cursor Bugbot for commit 74581a7. Configure here.
There was a problem hiding this comment.
Replaces the single-host Docker AWS topology with four native systemd validators, and the Go/state/teardown/list/forward changes are internally consistent, backward compatible with legacy single-host state, and covered by updated tests. No blocking defects found; the main gaps are a silent-hang failure mode when the remote build never launches and the fact that a full end-to-end provision has not yet been run.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
--instance-typeand--architectureare validated independently, and the mismatch is only caught byrun-instances— which happens after the security group and (for a managed key) the key pair have already been created, leaving afailedcluster the operator must tear down. Deploying--instance-type c8i.48xlargewithout--architecture amd64(the README example passes both, but nothing enforces it) hits this. Consider resolving the instance type's supported architectures up front, or defaulting--architecturefrom the instance-type family. - [suggestion] The PR description notes a fresh end-to-end provision is still pending (expired AWS sessions); validation so far is unit tests plus a configuration-generation smoke test. The pieces with no automated coverage are exactly the new ones —
build_native_node.sh,prepare_native_cluster.sh,install_native_node.sh, and the systemd unit — so a fulldeploy/list/forward/teardowncycle before merge would be worth waiting for. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| "git checkout --detach FETCH_HEAD", | ||
| "mkdir -p " + shellQuote(buildDir), | ||
| "rm -f " + shellQuote(readyMarker) + " " + shellQuote(failedMarker), | ||
| "nohup integration_test/autobahn/scripts/build_native_node.sh " + shellQuote(aws.RemoteDir) + " </dev/null >" + shellQuote(buildLog) + " 2>&1 &", |
There was a problem hiding this comment.
[suggestion] If the build script never starts, this deploy hangs for the full --timeout (now 45 min) with no diagnostic. The && chain succeeds as soon as nohup ... & is backgrounded, so a failure to exec the script — most plausibly deploying a --ref that predates integration_test/autobahn/scripts/build_native_node.sh, which is a normal use of this tool for revision comparisons — leaves neither marker on disk: the .failed marker is only written by the trap inside the script. waitForNativeBuilds then polls forever and finally returns wait for native builds: context deadline exceeded, discarding the build log that would explain it.
Two cheap fixes, either works: add test -x integration_test/autobahn/scripts/build_native_node.sh to the && chain so the launch fails fast with a clear error, and/or tail buildLog on the ctx.Done() path in waitForNativeBuilds the same way the failed case already does.
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
We will need to rip most of this out and standardise on K8S.
For now approving to have parallel work on loadtesting progress.
Cc @philipsu522
2 participants
Summary
seidrevision natively on every instance and run it through systemdGOMAXPROCSandGOGCThis branch is based directly on
mainand intentionally contains no executor or throughput changes from #4087.Validation
go test -race ./cmd/autobahn-e2e -count=1go vet ./cmd/autobahn-e2ebash -n integration_test/autobahn/scripts/*.shgo build -o /tmp/autobahn-e2e-native ./cmd/autobahn-e2egolangci-lint run --timeout 10m0sgofmt,goimports, andgit diff --checkc8i.48xlarge: four validator keys, gentxs, node archives, a four-validator Autobahn file, and three persistent peers per node were generated successfullyAWS end-to-end deployment
Validated commit
b194a2d0don 2026-09-08 with a fresh cluster inus-east-2:c8i.48xlargeAMD64 instances and completed cold native builds on every host/opt/seid/bin/seiddirectly through systemd withGOMAXPROCS=24andGOGC=200seid.servicewas active on all four instanceslist --jsonreported all validators active713715; transaction0x49f05f9d598ae6b193a874f0571cff19e5f72b71ea6a250949d9f306cf134056was accepted and all four validators advanced to executed height 1-32601teardownand verified all four instances were terminated, the managed security group and EC2 key pair were deleted, and the local state, private key, and SSH forward were removed