Skip to content

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

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Nov 21, 2023

This PR replaces bash with dd 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 by math/rand to avoid entropy bottlenecks.

Fixes #10795
Refs #8556

@mafredri mafredri force-pushed the mafredri/fix-scaletest-traffic-performance branch 6 times, most recently from 8e884da to 79c002e Compare November 30, 2023 14:50
@mafredri mafredri requested a review from johnstcn November 30, 2023 14:52
@mafredri mafredri marked this pull request as ready for review November 30, 2023 14:52
@mafredri mafredri force-pushed the mafredri/fix-scaletest-traffic-performance branch from 79c002e to 764b9b2 Compare November 30, 2023 15:07
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

Comment on lines +227 to +228
var allowedChars = []byte("\t !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}")

Copy link
Member

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

Comment on lines +268 to +269

return p[:n]
Copy link
Member

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)
Copy link
Member

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!

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
}
Copy link
Member

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?

Copy link
Member Author

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 🤔.

Copy link
Member Author

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.

@mafredri mafredri merged commit 9915118 into main Nov 30, 2023
@mafredri mafredri deleted the mafredri/fix-scaletest-traffic-performance branch November 30, 2023 17:30
@github-actions github-actions bot locked and limited conversation to collaborators Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce (CPU) overhead of traffic generation
2 participants