Sei-load: configurable load generators - #3917
Conversation
PR SummaryMedium Risk Overview Targets no longer hard-code arctic/atlantic: Adds Docker ( Reviewed by Cursor Bugbot for commit 183de2d. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3917 +/- ##
==========================================
- Coverage 59.51% 58.50% -1.02%
==========================================
Files 2325 2229 -96
Lines 198686 188054 -10632
==========================================
- Hits 118251 110016 -8235
+ Misses 69217 67661 -1556
+ Partials 11218 10377 -841
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large but well-structured addition of configurable generated load modes (defi/tokenops/nativetransfers) plus a Docker image and Helm chart for the standalone load generator; the code is defensive (EXECUTE gating, safety ceilings, manifest validation, exact-decimal funding) and adds real unit coverage. No blockers, but a few operational sharp edges: reusing a RUN_ID makes ERC-721 mints collide, the Helm runner can start against a stale fixture manifest, and the switch from process.exit(1) to process.exitCode = 1 can hang failing entry points.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
package.jsonaddsoverrides: { "protobufjs": "$protobufjs" }, which silently forces@confio/ics23(a deprecated package whose static codec was generated against protobufjs 6) from^6.8.8up to 8.7.1, and drops all@protobufjs/*andlong@4entries from the lockfile.@cosmjs/stargateimports ics23 eagerly, so an incompatibility would surface at module load on every command. Worth a sentence in the PR body or a smoke assertion recording why the override is needed (presumably the protobufjs 6.11 advisory) and that ics23 still loads.LoadMetrics.listeninstalls anasyncrequest handler and doesresponse.end(await this.registry.metrics()); ifregistry.metrics()rejects (a default-metrics collector failing to read/proc, for example) the rejection is unhandled and, under Node's default policy, terminates a long-running load run on a scrape. Wrapping the handler body in try/catch and returning 500 keeps a metrics hiccup from killing the run.- Helm object names are not length-guarded: the runner
Deployment/Servicenames (fullname-<runner>) are built with notrunc, andsei-load-generator.fixtureSetupNametruncates to 63 after appending-<revision>. With a long release or fixture-set name the former is rejected by the API server, and the latter can truncate the revision suffix away so the new setup Job collides with the previous revision's (immutable) Job and the upgrade fails. Truncate-then-suffix, or append a short hash. - 4 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] No GitHub workflow runs the
integration_test/load_generatorsuites (npm test/npm run test:fixtures/npm run typecheck) —.github/workflows/integration-test.ymlonly coversrpc_tests. The ~260 lines of new mocha specs and the new hardhattokenOps.spec.tsthis PR adds are therefore not exercised on CI, so regressions in the load generator land silently.
| evmOperation('erc20_burn', 8, token, ERC20, 'burn', GAS_LIMIT, () => [AMOUNT / 20n]), | ||
| evmOperation('erc721_mint', 14, nft, ERC721, 'safeMint', GAS_LIMIT, (worker, sequence) => [ | ||
| worker.evmAddress, | ||
| uniqueTokenId(context.runId, worker, sequence), |
There was a problem hiding this comment.
[suggestion] erc721_mint derives its token ID from (runId, workerAddress, sequence), and chooseOperation is driven by seededRandom(config.runId), so two runs with the same RUN_ID replay an identical operation stream and mint the identical set of token IDs. loadConfig only requires RUN_ID to be non-empty, never unique, so:
- The README's own example, run twice (
EXECUTE=1 RUN_ID=tokens-50 npm run load -- run --type tokenops --tps 50), makes everyerc721_mint(weight 14 of ~102) revert withalready mintedfor the whole second run. - Under the Helm chart
--run-idis$(POD_NAME), which is stable across container restarts within a pod whilesequenceresets to 0 — so any crash/restart of atokenopsrunner permanently degrades that share of its load intoincluded_failed.
Mixing a per-process component into the ID (process start time, or the value recorded in run.json) would make reruns and restarts collision-free without giving up the determinism the new test asserts.
| command: | ||
| - sh | ||
| - -ec | ||
| - timeout 1800 sh -c 'until test -s /fixtures/deployment.json; do echo "waiting for fixtures"; sleep 2; done' |
There was a problem hiding this comment.
[suggestion] wait-for-fixtures treats any non-empty /fixtures/deployment.json as ready, but the fixture PVC outlives Helm revisions while the setup Job is recreated per revision and runs concurrently with this Deployment. On upgrade the runner therefore starts against whatever manifest the previous revision left behind, before the new setup Job has validated or replaced it.
It is not silently wrong — validateManifests in runSynthetic rejects a mismatched schemaVersion/network/chainId — but the observable behaviour is a crash-loop plus a wasted provision-users funding pass on every restart, and it never converges if the setup Job itself fails (e.g. verifyExisting refusing a manifest from a different network, which then needs a manual FORCE_DEPLOY=1). Gating on a marker file that setup writes only after success (schema- or revision-stamped) makes the ordering an invariant instead of a race.
| if (require.main === module) { | ||
| runReplayMain().catch(error => { | ||
| console.error('Fatal:', error instanceof Error ? error.message : error); | ||
| process.exitCode = 1; |
There was a problem hiding this comment.
[suggestion] Switching from process.exit(1) to process.exitCode = 1 means the process only exits once the event loop drains, and this entry point creates handles outside the try/finally that owns cleanup: liveMetrics.listen (line 187) and the JsonRpcProvider are established before the try at line 217, while liveMetrics.close() / provider.destroy() live in that block's finally. A failure in between — e.g. bucketAudit.initialize() on a non-writable replay dir — leaves the metrics server listening and the process alive indefinitely, with /healthz still answering 200 so a Kubernetes liveness probe won't recycle it.
The same pattern is now in runLoad.ts, runBuffered.ts (which may still hold a child that ignored SIGTERM), deployFixtures.ts (provider.destroy() only on the two success paths, and the setup Job has no activeDeadlineSeconds), and provisionUsers.ts. Either move handle creation inside the guarded region or keep process.exit(1) after logging.
| emit Transfer(address(0), to, value); | ||
| } | ||
|
|
||
| function burn(uint256 value) external { |
There was a problem hiding this comment.
[nit] This copy of TestERC20 now diverges from integration_test/rpc_tests/contracts/TestERC20.sol (byte-identical before this PR), while the file's own header still says it is "the canonical contract across the RPC suite" and is "[d]eployed identically on Sei and the geth reference so contract-touching parity specs see the same layout on both chains." Either mirror burn into the rpc_tests copy or drop that claim from this one, so the next reader doesn't rely on a guarantee the two files no longer honour.
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
Love that AI has made @kollegian into a SUPER USER! awesome end-to-end work. the only blocker for me is to simply move pieces of this work into different repos and leave the code here to be purely about the implementation of the load generation itself
We also need to figure out where the containers for this are being published and pulled from.
Super thanks to @bdchatham for helping out on this one 🙌
| @@ -0,0 +1,42 @@ | |||
| {{- define "sei-load-generator.name" -}} | |||
There was a problem hiding this comment.
I would just move all of these to platform repo (and probably use kustomize) to save the hassle of having to set up helm repository and build process.
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 183de2d. Configure here.
| { | ||
| startedAt, | ||
| completedAt: completedAt.toISOString(), | ||
| durationSeconds: (completedAt.getTime() - Date.parse(startedAt)) / 1_000, |
There was a problem hiding this comment.
Summary TPS includes setup time
Medium Severity
reportRun computes durationSeconds from the timestamp taken before RPC checks, worker creation, and fixture preparation through post-run drain. summarizeRun uses that span as the TPS denominator, so achieved rate is diluted by setup and teardown rather than the scheduled load window.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 183de2d. Configure here.
2 participants
Describe your changes and provide context
Integrates configurable load runners for sei load.
Testing performed to validate your change
No source code change.