Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Integrating vegeta #44

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Integrating vegeta #44

wants to merge 6 commits into from

Conversation

rsevilla87
Copy link
Member

@rsevilla87 rsevilla87 commented Sep 28, 2023 •
edited
Loading

Type of change

  • Refactor
  • New feature
  • Bug fix
  • Optimization
  • Documentation Update

Description

Adding vegeta to ingress-perf can be useful in the future. Have to perform more exhaustive tests with this tool.
Some of its benefits:

  • Native JSON output.
  • Support for not keepalived connections
  • Contant throughput support. This is quite useful for latency focused benchmarks.
  • It returns a summary of the returned status codes
  • Possibility of integrating the plot functionality for debugging purposes in the future.

Downsides

  • More resource intensive as compared to wrk, it requires more clients to reach a similar performance, this shouldn't be a problem, in most of our scenarios there're free CPU resources in the worker nodes
  • Needs more testing if we want to transition to this tool

Switching wrk <--> vegeta should be transparent as the indexed documents follow the same format and structure.

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please describe the System Under Test.
  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

@rsevilla87 rsevilla87 force-pushed the vegeta branch 3 times, most recently from 2cfeb97 to 1a9d2d9 Compare September 28, 2023 10:07
@rsevilla87 rsevilla87 added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 28, 2023
@jtaleric
Copy link
Member

Can we numerate the issues with wrk?

Maybe why we should possibly switch from wrk -> vegeta?

If this is to allow more options, great! But you mention switching to vegeta.

krishvoor
krishvoor reviewed Sep 28, 2023
View reviewed changes
containers/Containerfile
RUN dnf install -y iproute procps-ng
RUN curl -sS -L https://github.com/tsenart/vegeta/releases/download/v${VEGETA_VERSION}/vegeta_${VEGETA_VERSION}_linux_amd64.tar.gz | tar xz -C /usr/bin/
Copy link
Member

Choose a reason for hiding this comment

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

Multi-arch container might break here, as we are always pulling linux-amd64

Copy link
Member

Choose a reason for hiding this comment

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

By the looks of is vegeta is available for amd64/arm64 and not for other archs.

Copy link
Member Author

@rsevilla87 rsevilla87 Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, added a note about the supported archs. Only amd64 for now, we can work to integrate others in the future

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, added a note about the supported archs. Only amd64 for now, we can work to integrate others in the future

Ack, I missed the README.md.

I do not want us to publish/ship multi-architecture images with a linux_amd64 binary,
can we please update this?
https://github.com/cloud-bulldozer/ingress-perf/blob/main/Makefile#L38

krishvoor
krishvoor reviewed Sep 28, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
containers/Containerfile
RUN dnf install -y iproute procps-ng
RUN curl -sS -L https://github.com/tsenart/vegeta/releases/download/v${VEGETA_VERSION}/vegeta_${VEGETA_VERSION}_linux_amd64.tar.gz | tar xz -C /usr/bin/
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, added a note about the supported archs. Only amd64 for now, we can work to integrate others in the future

Ack, I missed the README.md.

I do not want us to publish/ship multi-architecture images with a linux_amd64 binary,
can we please update this?
https://github.com/cloud-bulldozer/ingress-perf/blob/main/Makefile#L38

@rsevilla87
Copy link
Member Author

rsevilla87 commented Sep 28, 2023 •
edited
Loading

Can we numerate the issues with wrk?

Maybe why we should possibly switch from wrk -> vegeta?

If this is to allow more options, great! But you mention switching to vegeta.

One of the main reasons is to consolidate into a single tool. At the moment, wrk is great to measure throughput on HAProxy, however it lacks of some features like constant throughput configuration (available in wrk2 though), this feature turned out to be useful to perform latency focused, we need this kind of constant throughput to measure latency with more accuracy.

Other nice features vegeta brings is:

  • Non keepalive connections support. This will allow us to define more benchmark scenarios
  • More debugging options, the plot feature can be useful to perform debugging of regressions.

@jtaleric
Copy link
Member

jtaleric commented Oct 2, 2023

Can we numerate the issues with wrk?
Maybe why we should possibly switch from wrk -> vegeta?
If this is to allow more options, great! But you mention switching to vegeta.

One of the main reasons is to consolidate into a single tool. At the moment, wrk is great to measure throughput on HAProxy, however it lacks of some features like constant throughput configuration (available in wrk2 though), this feature turned out to be useful to perform latency focused, we need this kind of constant throughput to measure latency with more accuracy.

Other nice features vegeta brings is:

  • Non keepalive connections support. This will allow us to define more benchmark scenarios
  • More debugging options, the plot feature can be useful to perform debugging of regressions.

From the above it begs the question, why not just use wrk2? My main beef with wrk2 is that it seems to be a stale project - however if it gives us the performance and functionality, why switch to Vegeta?

I don't have an issue w/ Vegeta - but I am concerned if we switch, teams using our tooling will report a false flag bug due to the workload changing, and the harness not able to produce the same throughput.

PR looks fine BTW :)

@rsevilla87
Copy link
Member Author

the harness not able to produce the same throughput.

I'm working on a config towards getting similar throughput using vegeta, so we can make a soft transition.

@rsevilla87
Copy link
Member Author

I'm hitting some sporadic issues while validating vegeta, reported at tsenart/vegeta#651

@rsevilla87
Copy link
Member Author

I'm hitting some sporadic issues while validating vegeta, reported at tsenart/vegeta#651

Rolling back to 12.9.0 fixed the issue

@rsevilla87 rsevilla87 force-pushed the vegeta branch 2 times, most recently from f55eebe to 8ef48d8 Compare October 4, 2023 08:35
@rsevilla87
Copy link
Member Author

rsevilla87 commented Oct 4, 2023 •
edited
Loading

I can't achieve a good performance on the passthrough termination, it seems like the connection is not using keepalive (I have seen a high number of sockets in TIME-WAIT state) for some reason.

@rsevilla87
Vegeta support
0406dde 
Signed-off-by: Raul Sevilla <[email protected]>
@rsevilla87
Typo
f1e73cd 
Signed-off-by: Raul Sevilla <[email protected]>
@rsevilla87
Missing license
e9cd122 
Signed-off-by: Raul Sevilla <[email protected]>
@rsevilla87
Not write results in disk, use max-body=0
7f70196 
Signed-off-by: Raul Sevilla <[email protected]>
@rsevilla87
Bumping to 12.11.1
94b9667 
Signed-off-by: Raul Sevilla <[email protected]>
@rsevilla87
Fix concurrent map writes panic
9bb84c1 
Signed-off-by: Raul Sevilla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krishvoor krishvoor krishvoor left review comments

@jtaleric jtaleric Awaiting requested review from jtaleric

@smalleni smalleni Awaiting requested review from smalleni

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rsevilla87 @jtaleric @krishvoor