feat(scaletest): replace bash with dd in ssh/rpty traffic and use pseudorandomness #10821
Conversation
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
mafredri
force-pushed
the
mafredri/fix-scaletest-traffic-performance
branch
6 times, most recently
from
8e884da to
79c002e
Compare
mafredri
force-pushed
the
mafredri/fix-scaletest-traffic-performance
branch
from
79c002e to
764b9b2
Compare
johnstcn
approved these changes
Nov 30, 2023
Comment on lines
+249
to
+251
| // On macOS, ENOTTY is returned when calling sync on /dev/stdout. We | ||
| // can safely ignore this error. | ||
| if err != nil && !xerrors.Is(err, syscall.EINVAL) && !xerrors.Is(err, syscall.ENOTTY) { |
Comment on lines
+227
to
+228
| var allowedChars = []byte("\t !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}") | ||
|
|
There was a problem hiding this comment.
oh wow, this must have been causing so many flakes
Comment on lines
+268
to
+269
|
|
||
| return p[:n] |
There was a problem hiding this comment.
<3 for removing the load-bearing octothorpe
| @@ -139,7 +139,7 @@ func TestRun(t *testing.T) { | |||
| t.Logf("bytes written total: %.0f\n", writeMetrics.Total()) | |||
|
|
|||
| // We want to ensure the metrics are somewhat accurate. | |||
| assert.InDelta(t, bytesPerTick+fudgeWrite, writeMetrics.Total(), 0.1) | |||
| assert.InDelta(t, bytesPerTick, writeMetrics.Total(), 0.1) | |||
There was a problem hiding this comment.
oh nice, the per-request overhead is gone now!
johnstcn
reviewed
Nov 30, 2023
Comment on lines
+125
to
+138
| // Send Ctrl+C to interrupt the command. | ||
| _, err = c.writeNoLock([]byte("\u0003")) | ||
| if err != nil { | ||
| return xerrors.Errorf("write ctrl+c: %w", err) | ||
| } | ||
| select { | ||
| case <-time.After(connCloseTimeout): | ||
| return xerrors.Errorf("timeout waiting for read to finish") | ||
| case err = <-c.readErr: | ||
| if errors.Is(err, io.EOF) { | ||
| return nil | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
should we hold the lock while we do this?
There was a problem hiding this comment.
I don't think we need to, I opted to use the closed boolean to allow Write to exit as soon as we hit Close(). If we held the lock throughout, we wouldn't need the boolean but it might give the wrong idea up the stack about why the write routine didn't exit in time 🤔.
There was a problem hiding this comment.
I reorganized the struct to clarify why mu is present.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR replaces
bashwithddand implements a more graceful shutdown for both rpty and ssh connections to ensure the command on the other side is terminated.The wrapper for rpty conns was also improved to more accurately track reads and writes. This new wrapper also enforces a maximum payload size to avoid breaking the websocket connection.
Finally, we also allow disabling PTYs for ssh connections so that we can reduce CPU usage even further (this relies on turning on echo for the
ddcommand).The use of
crypto/randwas also replaced bymath/randto avoid entropy bottlenecks.Fixes #10795
Refs #8556