-
Notifications
You must be signed in to change notification settings - Fork 817
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
Adding Locust tests - initial changes for #412 #611
Conversation
Build Succeeded 👏 Build Id: b83762d3-1ae2-479b-a580-fad363844987 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Sorry - the reopening of a previously closed PR made funny things happen - so you've got a few messages. |
Build Failed 😱 Build Id: 8207771e-9203-4f7d-8e2e-91362bb98d29 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 49546ec7-2057-4434-b6f1-cfb1837abe50 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: a9ae3734-2e40-40c9-98df-5f5954eb68f6 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: a97f968f-d5c4-4584-8e2d-10d474abb45d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
To run load tests using Locust on your local machine: | ||
|
||
``` | ||
docker build -t locust-files . |
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 would add the kubectl proxy &
command
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.
Done.
test/load/README.md
Outdated
|
||
``` | ||
docker build -t locust-files . | ||
docker run --rm --network="host" -e "TARGET_HOST=http://127.0.0.1:8001" locust-files:latest |
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.
Out of curiosity are you running on linux ? Last time I checked network host on MacOS wasn't working.
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.
Good catch. Yes, I was running on Linux.
I'm thinking to add to the documentation that for macOS and Windows we can use the special DNS name host.docker.internal. Does that sound good?
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 added this to the README. Please let me know if it is sufficient.
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.
Looks good, I think it would be better with a single image.
# TAKEN FROM: https://github.com/GoogleCloudPlatform/distributed-load-testing-using-kubernetes/blob/master/docker-image/locust-tasks/run.s | ||
|
||
LOCUST="/usr/local/bin/locust" | ||
LOCUS_OPTS="-f locustfile.py --host=$TARGET_HOST" |
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 we could use a single image and with the entrypoint script your could switch from one plan to another(either via a flag or env).
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.
Do you mean a single Dockerfile for both tests?
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 changed the Dockerfile and the structure so that we have a single image. It's more flat and more readable now. Thanks for the suggestion.
Build Succeeded 👏 Build Id: 212a9e0a-d879-40ae-af40-3cf3e60071c8 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Failed 😱 Build Id: a3cc9957-4b29-47dd-b610-7b4ac6b4cf29 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
test/load/README.md
Outdated
``` | ||
|
||
The above will build a Docker container to install Locust, Grafana, and Graphite and will configure | ||
them. The test uses the HTTP proxy on the local machine to access the k8s API. |
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.
curious: is this not going to limit the perceived performance in some way (I would imagine proxy is probably not optimized for speed)?
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'm not sure, it might. I will improve that in future iterations.
RUN pip install locustio | ||
EXPOSE 8089 5557 5558 | ||
|
||
RUN mkdir /etc/service/locust |
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.
nit: /etc is typically for configuration files, not for executables
how about /opt/locust/ ?
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.
That won't work. The image that I am using in this Dockerfile (https://hub.docker.com/r/hopsoft/graphite-statsd) is built from phusion which requires specific configuration when a new daemon is added. This includes an executable that runs the daemon to the /etc/service/ directory: https://github.com/phusion/baseimage-docker#adding-additional-daemons
def scaleUpFleet(self): | ||
# Create a fleet. | ||
initial_size = 1 | ||
start_time = time.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.
nit: format 'payload' as multiline? or perhaps put those things in JSON files?
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.
Formatted as multiline for now. I will later move these to JSON files but not in this PR since it needs some parsing to build the payload.
RUN pip install locustio | ||
EXPOSE 8089 5557 5558 | ||
|
||
RUN mkdir /etc/service/locust |
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.
same here about /etc
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.
Ditto.
Build Succeeded 👏 Build Id: e65724a4-9f73-444d-9f76-d47ebccbfe2b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 734702be-07a4-4e94-bc16-1033b4212456 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.
Hello, there are some crucial and not so crucial pep8 violations. Please run pep8 util.
response = self.client.put(selfLink, data=json.dumps(payload), headers=headers) | ||
self.waitForScaling(selfLink, 0) | ||
total_time = int((time.time() - start_time) * 1000) | ||
events.request_success.fire(request_type="fleet_scaling_down", name="fleet_scaling_down", response_time=total_time, response_length=0) |
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.
@pm7h Can you please run pep8
util on your python code. For instance this lines too long:
E501 line too long (132 > 79 characters)
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.
Done.
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.
Thanks @pm7h
break | ||
if (total_time > DEADLINE): | ||
print "Fleet did not scale up in time" | ||
events.request_success.fire(request_type="fleet_scaling_timeout", name="fleet_scaling_timeout", response_time=total_time * 1000, response_length=0) |
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.
Same as above:
E501 line too long (152 > 79 characters)
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.
Done.
} | ||
headers = {'content-type': 'application/json'} | ||
response = self.client.post("/apis/stable.agones.dev/v1alpha1/namespaces/default/fleets", data=json.dumps(payload), headers=headers) | ||
response_json = response.json() |
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.
Please run pep8
. Install it using pip install pep8
.
E111 indentation is not a multiple of four
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.
Done. Thanks.
response = self.client.get(selfLink) | ||
response_json = response.json() | ||
status = response_json.get('status') | ||
if status != None: |
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.
E711 comparison to None should be 'if cond is not None:'
if status != None: | |
if status is not None: |
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.
Done.
break | ||
elif (game_server_state == "UnAllocated"): | ||
total_time = int((time.time() - start_time) * 1000) | ||
events.request_success.fire(request_type="GameServerUnAllocated", name="GameServerUnAllocated", response_time=total_time, response_length=0) |
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.
Same as in the other file. This file violates pep8 stadard. Line is too long, should be split into multiple.
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.
Done. Thanks.
Build Succeeded 👏 Build Id: d0d50340-f204-4eed-8601-5f889e5b4320 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.
Thanks @pm7h ,
Tested these two files with pycodestyle
also (pep8 util seems to be deprecated), now them are fine.
Noticed this sitting here for a while - tbh, I was waiting on either @Kuqd or @jkowalski to approve this - since they are working on performance way more than me 😄 Happy to download and take it for a spin if nobody is going to jump on this? |
Adding Locust tests. Adding command to start k8s proxy to the READMe file. Changing the Dockerfile and the directories so that we have a single image and a more flat sctucture. Small improvements to Python files for better readability. Fixed indentation. Fixed pep8 errors.
Build Succeeded 👏 Build Id: 6d376c50-d087-43ff-9e1a-5374c260e027 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 7fcbf20c-00e4-4308-ad14-54b56339b457 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Adding Locust tests. We are also working on adding load tests using the e2e framework (#573) but these tests provide a convenient way to run a quick test with very high load.