-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
e2e: test runner generates loadtime formatted transactions. #9779
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work
test/e2e/networks/simple.toml
Outdated
@@ -3,3 +3,5 @@ | |||
[node.validator03] | |||
[node.validator04] | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Minor] Needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought this was removed. Fixed.
test/e2e/runner/load.go
Outdated
return | ||
} | ||
} | ||
t.Reset(time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this, but this interval was configurable in tm-load-test
. Do we want to do something similar here?
(I'm imagining we may want to increase it to test big TX bursts)
If my comment makes sense but the answer is "not needed now, can do later", I'm OK with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, "not needed now, can do later" was ultimately what I was thinking. The current e2e test only uses 1 second as the interval for sending load and the load testing done for v0.37 also only used 1 second. If we want to expand this capability in the future we certainly can but I'm not sure it's necessary to add until we're sure we're going to use it since it would ultimately be very simple to add.
case chTx <- tx: | ||
time.Sleep(time.Second / time.Duration(multiplier)) | ||
|
||
case <-t.C: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want here a mechanism similar to this one in tm-load-test
, to avoid the possibility that the batches pile up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be preferable to replace the load generation mechanism in the E2E tests with the loadtime tool directly?
I don't believe so. It would be more complicated than preferred. It would either require pulling the code in directly or shelling out to the binary. In the case of pulling the code in, the |
Technically the CLI scaffolding provided by tm-load-test just calls This is the extent of the wiring required to call the tm-load-test machinery programmatically: https://github.com/informalsystems/tm-load-test/blob/a517a0ac52f5c26e0b1f6a480ff871ee70edba03/pkg/loadtest/cli.go#L41-L49 The only thing I can think of that would make using tm-load-test a bit of a pain here is the fact that it doesn't allow you to override its logging right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments re: tm-load-test aren't critical for this PR. We could consider perhaps reusing it as a library at some point in future.
(cherry picked from commit 21b2801)
(cherry picked from commit 21b2801) # Conflicts: # test/e2e/pkg/manifest.go # test/e2e/pkg/testnet.go
…9779) (#9800) * e2e: test runner generates loadtime formatted transactions. (#9779) (cherry picked from commit 21b2801) # Conflicts: # test/e2e/pkg/manifest.go # test/e2e/pkg/testnet.go * fixup merge conflicts Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: William Banfield <wbanfield@gmail.com>
This pull request updates the e2e test to use the
loadtime
transaction format and updates the configuration to allow for parity with theloadtime
load generation.Added configuration parameters
load_tx_size_bytes
: This size, in bytes, of each generated transaction. Defaults to 1024 bytes.load_tx_batch_size
: How many transactions to generate every 1 second.load_tx_connections
: How many connections to make to each node.node.no_send_load
: Whether or not to send load to this node.Difference with
loadtime
toolNote that there is a slight difference between this configuration scheme and the scheme used in the
loadtime
tool. Theloadtime
tool generatesbatch_size
per connection per second. This code generates a total ofload_tx_batch_size
total every second. To mimic the behavior of theloadtime
tool simply multiply by the total number of connections. This was done to maintain the legacy behavior of the tool which generated a total network-wide connection load instead of a per-connection load.Replicating the v0.37.x load test.
The v0.37.x load tests, as outlined in methods.md, opened a set of connections to a single node instead of to all nodes on the network. The e2e test previously opened a set of connections to all nodes on the network and this default behavior was maintained by this change. To replicate the the v0.37.x load test, set all nodes but one to have a
no_send_load
parameter oftrue
, settingno_send_load = true
on the one node that should receive load. Connection number can be configured exactly as was done in v0.37.x, and batch size should be set to the product of the number of connections with the original batch size. For example, if v0.37.x's load test had a batch size of 200 and a connection number of 4, the batch size should instead be set to 800.closes: #9778
PR checklist
CHANGELOG_PENDING.md
updated, or no changelog entry neededdocs/
) and code comments, or nodocumentation updates needed