-
Notifications
You must be signed in to change notification settings - Fork 937
feat(scaletest): replace bash with dd in ssh/rpty traffic and use pseudorandomness #10821
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
Conversation
8e884da
to
79c002e
Compare
79c002e
to
764b9b2
Compare
// 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) { |
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.
🙏
var allowedChars = []byte("\t !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}") | ||
|
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.
oh wow, this must have been causing so many flakes
|
||
return p[:n] |
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.
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice, the per-request overhead is gone now!
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we hold the lock while we do this?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reorganized the struct to clarify why mu
is present.
This PR replaces
bash
withdd
and 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
dd
command).The use of
crypto/rand
was also replaced bymath/rand
to avoid entropy bottlenecks.Fixes #10795
Refs #8556