-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add some load testing tools to the sandbox #351
Conversation
19bd3c5
to
6387293
Compare
limitador-server/sandbox/docker-compose-limitador-distributed.yaml
Outdated
Show resolved
Hide resolved
args: | ||
- CARGO_ARGS=--all-features | ||
depends_on: | ||
- envoy |
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.
envoy is not being used, is it?
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.
All the other limitador compose files have this dependency. But Yeah that seems backwards.
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, exactly.
We are pushing the current "unique" sandbox too far. In other PR, we could refactor to have different sandboxes, each one running only necessary services. Each sandbox with custom readme.md and docker-compose.yaml file.
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 fixed the dependency on this compose file and the other ones too.
I'm somewhat confused about this pull request tbh... I'm probably missing something, but what's the intent here? |
Just to be clear tho... not against it, just missing how/when I'd use this |
Cargo.toml
Outdated
@@ -1,5 +1,5 @@ | |||
[workspace] | |||
members = ["limitador", "limitador-server"] | |||
members = ["limitador", "limitador-server", "limitador-server/sandbox/loadtest"] |
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 wouldn't add this to the workspace here... There is no dependency and loadtest
is really not useful in the context of limitador work, yet that'd impose build times et al...
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.
makes sense, removed it.
046b81c
to
9b3f8bd
Compare
* support passing cargo build args to the Docker image builder, allowing us to enable all features. * add a distributed deployment option in the sandbox * update the deployments to have envoy depend on limitador instead of limitador depending on envoy. Signed-off-by: Hiram Chirino <hiram@hiramchirino.com>
9b3f8bd
to
54136f4
Compare
squashed and removed #350 |
``` | ||
|
||
The report will be saved in `report.htm` file. | ||
|
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.
@alexsnaps see above re: how to use
Signed-off-by: Hiram Chirino <hiram@hiramchirino.com>
@eguzki 3 node deployment option added. |
|
||
The `make deploy-distributed` target can be connected to other Limitador servers but requires you to set the `PEER_ID` and `PEER_URLS` environment variables when you run the target. | ||
|
||
If you have 3 servers you want to replicate between, you would run the following commands: |
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.
no need to have 3 servers, right?
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.
Well, you might still want to do this on 3 machines if you are benchmarking. Doing it all on one would not be very representative.
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 buy 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.
Looking good.
left few comments.
After this PR is being merged, I probably will refactor a bit to have multiple "sandboxes" instead a "big" one. More scalable (easier to add new ones).
@@ -4,20 +4,34 @@ | |||
|
|||
# Use bullseye as build image instead of Bookworm as ubi9 does not not have GLIBCXX_3.4.30 | |||
# https://access.redhat.com/solutions/6969351 | |||
FROM --platform=${BUILDPLATFORM} rust:1.78.0-bullseye as limitador-build |
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.
is this breaking cross platform builds?
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.
why would it? This Dockerfile works fine on my arm mac.
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.
https://docs.docker.com/reference/dockerfile/#from
The optional --platform flag can be used to specify the platform of the image in case FROM references a multi-platform image.
For example, linux/amd64, linux/arm64, or windows/amd64. By default, the target platform of the build request is used. Global build arguments can be used in the value of this flag, for example [automatic platform ARGs](https://docs.docker.com/reference/dockerfile/#automatic-platform-args-in-the-global-scope)
allow you to force a stage to native build platform (--platform=$BUILDPLATFORM), and use it to cross-compile to the target platform inside the stage.
When I need to do cross-compile, I need that rust:1.78.0-bullseye
be native to my current machine platform. When docker buildx build --platform=linux/amd64
, without that FROM --platform=${BUILDPLATFORM}
the rust:1.78.0-bullseye
will be linux/amd64
and my current machine platform might be something else (like arm). Which is not correct.
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 think it's best if the plain Dockerfile just does a container build that matches the host, and then we have additional Dockerfiles to do the cross compiles like Dockerfile.aarch64.
Your saying your on arm trying to build linux/amd64 with that Dockerfile?? Shouldn't that work if the host is running QEMU?
…er, we don’t build it. Signed-off-by: Hiram Chirino <hiram@hiramchirino.com>
Dockerfile
Outdated
COPY limitador/Cargo.toml ./limitador/ | ||
COPY limitador-server/Cargo.toml ./limitador-server/ | ||
RUN mkdir -p limitador-server/src && echo 'fn main() {}' > limitador-server/src/main.rs | ||
RUN mkdir -p limitador-server/sandbox/loadtest/src && echo 'fn main() {}' > limitador-server/sandbox/loadtest/src/main.rs |
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.
is this still 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.
yep that's not needed either..
Signed-off-by: Hiram Chirino <hiram@hiramchirino.com>
Documents how to use ghz to benchmark the gRPC interface, and adds a rust goose based load test that works against the envoy proxy which should help simulate more real world end-to-end stats.
Adds a deployment option for the distributed store.
Note: this PR stacks on PR #350 so that we can easily build a local container using docker-compose with the right feature enabled depending on the deployment.