[GPII-3188]: First crack at smoke testing for GCP infra with Locust - #93
Conversation
|
Typical "failed run": |
|
|
||
| echo | ||
| echo $SESSION_STATS | ||
| SESSION_SUCCEEDED=1 |
There was a problem hiding this comment.
Do you think that using error codes could be more useful in a future? it's just a suggestion.
…for ready workers before starting the swarm
mrtyler
left a comment
There was a problem hiding this comment.
This is neat.
I realize it's still early in this work, so just a couple high-level observations below.
Remember that preferences server is supposed to stop being publicly available someday, so you might start thinking about whether/how to expose preferences server for testing, and/or how to do OAuth with flowmanager so we can test preferences through flowmanager.
|
|
||
| provisioner "local-exec" { | ||
| command = <<EOF | ||
| mkdir -p ${var.charts_dir}/locust/tasks |
There was a problem hiding this comment.
FYI this will create files as root inside a bind mount, which is likely to cause trouble.
There was a problem hiding this comment.
Alternative would be to keep locust scripts (which are app-specific) in common chart dir. It is not possible to modify this behavior, since Helm does not give file system access beyond chart dir...
It works well locally, with your changes from #92 merged. I also added cleanup script to remove those links after chart creation to eliminate possible issues with permissions on the host.
Do you think we'll still have troubles on CI workers with this?
There was a problem hiding this comment.
Did you test it on Linux, where the root ownership problem occurs?
Another alternative would be to mount a Docker Volume into the expected place inside the container, the same strategy we use to solve the problem for other locations that need to be written from inside the container. You can see examples of how this works in #92.
There was a problem hiding this comment.
Nope, I did not test this on Linux yet. Having separate Docker Volume just for Locust tasks is probably okay, but I would be happy to avoid this. Let's see if cleaning up links from inside the container works.
| command = <<EOF | ||
| if [ "${var.locust_swarm}" == "" ]; then | ||
| echo "Looks like TF_VAR_locust_swarm is unset, terminating!" | ||
| exit |
There was a problem hiding this comment.
Should we exit non-zero in this case?
There was a problem hiding this comment.
Idea behind this is that there may be a situation, when we want to deploy the Locust, but don't actually want to start the swarm. I probably overcomplicate things as always.
|
|
||
| RETRIES=10 | ||
| RETRY_COUNT=1 | ||
| while [ "$WORKERS_READY" != "${var.locust_workers}" ]; do |
There was a problem hiding this comment.
This is a lot of in-line code.
There was a problem hiding this comment.
Automation cost.
There was a problem hiding this comment.
I don't understand this response.
There was a problem hiding this comment.
Probably because I did not understand the question :) What did you mean by "a lot of in-line code"?
There was a problem hiding this comment.
This is a little clearer after you talked about this PR a bit in standup today.
-
This is a lot of code to be in-line. Maybe move it to separate file.
-
This seems like kind of a lot of (custom) code to run as part of a deployment, but I guess we need some glue to run Locust, collect stats, and assert that various stats were within certain ranges.
-
This seems like a lot of shell code. It's pretty simple right now, but perhaps a proper test framework that knows how to assert that stats are within a range, run subsets, generate reports, etc. is better than writing our own.
Relatedly, maybe it's worth moving this code to a more featureful language than shell. Python might be a good fit since Locust tasks are in Python. Ruby might be a good fit since gpii-infra is in Ruby.
There was a problem hiding this comment.
- It is already in a separate file.
- Yes.
- I was also thinking on this question. There are lots of data in Locust stats. But, while idea to have assertions with customizable thresholds per Locust task, seems to be useful in theory, I still have not decided if this would give us any practical benefit. Most meaningful assertions (on median / max response times, RPS, failure counts) are already in the code. Separate task sets can have separate thresholds too, in case we need them. Locust reports are present in logs as well, we can send them into Bigtable or something, to collect historical data. I think this topic worth a discussion with other Ops (and maybe some Devs as well?)
There was a problem hiding this comment.
- Usually with this much "in-line code", I would expect it to live in a dedicated file
run_swarm_test.shor something. But you're right, I see what you mean.
\3. Definitely worth a larger discussion.
| RETRY_COUNT=1 | ||
| while [ "$WORKERS_READY" != "${var.locust_workers}" ]; do | ||
| echo "[Try $RETRY_COUNT of $RETRIES] Waiting for all Locust workers to join the master..." | ||
| WORKERS_READY=$(kubectl -n locust logs deployment/locust-master --tail 1 | grep -oE "Currently \d+ clients" | grep -oE "\d+") |
There was a problem hiding this comment.
Does locust-master produce any other logs that might prevent us from seeing the "Currently \d+ clients" line?
There was a problem hiding this comment.
It produces some other logs, but last messages are always about workers joining the master. I tried different scenarios, changing locust_workers up and down (which scales workers deployment) and target_host (which recreates master pod). It works.
| while [ "$WORKERS_READY" != "${var.locust_workers}" ]; do | ||
| echo "[Try $RETRY_COUNT of $RETRIES] Waiting for all Locust workers to join the master..." | ||
| WORKERS_READY=$(kubectl -n locust logs deployment/locust-master --tail 1 | grep -oE "Currently \d+ clients" | grep -oE "\d+") | ||
| if [ "$WORKERS_READY" != "" ]; then |
There was a problem hiding this comment.
I would refactor this to:
if [ "$WORKERS_READY" == "" ]; then
WORKERS_READY=0
fi
echo "Number of ready workers: $WORKERS_READY out of ${var.locust_workers}!"
There was a problem hiding this comment.
Looks better, thanks!
There was a problem hiding this comment.
EDIT: Never mind, I was looking at the wrong set of commits. Sorry for the noise.
Bump
|
|
||
| RETRIES=10 | ||
| RETRY_COUNT=1 | ||
| while [ "$WORKERS_READY" != "${var.locust_workers}" ]; do |
There was a problem hiding this comment.
This is a little clearer after you talked about this PR a bit in standup today.
-
This is a lot of code to be in-line. Maybe move it to separate file.
-
This seems like kind of a lot of (custom) code to run as part of a deployment, but I guess we need some glue to run Locust, collect stats, and assert that various stats were within certain ranges.
-
This seems like a lot of shell code. It's pretty simple right now, but perhaps a proper test framework that knows how to assert that stats are within a range, run subsets, generate reports, etc. is better than writing our own.
Relatedly, maybe it's worth moving this code to a more featureful language than shell. Python might be a good fit since Locust tasks are in Python. Ruby might be a good fit since gpii-infra is in Ruby.
| echo | ||
| echo "Resetting stats..." | ||
| curl -s $LOCUST_URL/stats/reset | ||
| kill $(pgrep kubectl) |
There was a problem hiding this comment.
This seems potentially dangerous, especially with terragrunt's parallelism.
There was a problem hiding this comment.
I don't think we ever want to run this in parallel with other modules as part of the deployment. I would rather think of this as a separate build step that only deploys locust module and runs the swarm. But yeah, kill $(pgrep -f "kubectl -n locust port-forward").
There was a problem hiding this comment.
EDIT: Never mind, I was looking at the wrong set of commits. Sorry for the noise.
Bump
That revised pgrep looks good.
|
What is the end game for this - deploy locust into dev clusters, and run it against local GPII deployment every time TF runs? |
|
@stepanstipl I am thinking of this as of separate step in CI pipeline (instead of current "smoke tests" that we have for AWS). We could start with deploying locust into the same cluster. Run nightly benchmarks for @mrtyler As long as Locust deployed into the same cluster, it can swarm |
I like this. I think it would come after stg (and could potentially use stg as the cluster-under-test).
I'm not sure (these) Locust tests are a replacement for smoke tests. Something lightweight that runs as part of deployment gives us quick feedback on the health of that deployment. A sort of compromise might be lightweight Locust tests that run during deployment (so we don't have to write and maintain scripts that run I think this is a good addition to the larger discussion of testing we've been trying to have for a few days :).
I like doing it from dev in the short-term -- a nice environment that is destroyed and re-created regularly. We can add variables to the nightly build and use them to only run the benchmark step during nightly builds (not during triggered CI builds). Longer term I like a "utility environment", probably the same environment where the CI worker lives.
Cool. Hitting the NodePort/ClusterIP directly is a little different from going through the public IP, Google LB, nginx-ingress, etc. I guess it depends on exactly what we're trying to test.
That example code looks good. |
|
So, I think things look a bit better after refactoring. To start preferences test: To start flowmanager test: I configured default thresholds so preferences test can pass without need to change anything. Interestingly, flowmanager shows much worse performance, so thresholds needs to be adjusted accordingly. |
|
Added tests to CI config. |
| LOCUST_URL=http://127.0.0.1:8089 | ||
| kubectl -n locust port-forward deployment/locust-master 8089:8089 </dev/null &>/dev/null & | ||
| LOCUST_URL="http://127.0.0.1:8089" | ||
| KUBECTL_CMD="kubectl -n locust port-forward deployment/locust-master 8089:8089" |
There was a problem hiding this comment.
I would call this PORT_FORWARD_CMD.
|
|
||
| @task | ||
| def prefByKey(self): | ||
| def getPrefByKey(self): |
There was a problem hiding this comment.
Generally in Python, methods/functions should use snake_case. But maybe Locust dictates this name?
There was a problem hiding this comment.
Nope, Locust dictates nothing here, the only requirement is clear connection between task name and requests it is making, because task names are not present in stats. Let's switch to snake_case if you think it better follows Python naming style.
|
|
||
| variable "locust_swarm_duration" { | ||
| default = 60 | ||
| default = 50 |
There was a problem hiding this comment.
Right, 50 means 60 because we add 10 to it later.
It's fine the way you've implemented it but you don't think this is weird? Do you think it warrants a comment?
There was a problem hiding this comment.
Ok, let's just exit if locust_swarm_duration is unset.
| - rake configure_serviceaccount_ci_restore | ||
| - rake | ||
| # Run smoke tests for Preferences | ||
| - rake sh['sh -c " |
There was a problem hiding this comment.
This command is pretty complex, and is something that we might want to run ourselves. Can you make two new rake tasks, :smoke_test_preferences and :smoke_test_flowmanager (or something), and call them from here?
mrtyler
left a comment
There was a problem hiding this comment.
Couple more little things.
| task :test_preferences => [:set_vars] do | ||
| sh "#{@exekube_cmd} rake xk[' \ | ||
| xk down live/dev/locust && \ | ||
| sleep 30 && \ |
There was a problem hiding this comment.
Can you add echo Sleeping 30s... here so the user doesn't think the process is stuck?
There was a problem hiding this comment.
See next thread.
| task :test_flowmanager => [:set_vars] do | ||
| sh "#{@exekube_cmd} rake xk[' \ | ||
| xk down live/dev/locust && \ | ||
| sleep 30 && \ |
There was a problem hiding this comment.
Can you add echo Sleeping 30s... here so the user doesn't think the process is stuck?
There was a problem hiding this comment.
See next thread.
| sleep 30 && \ | ||
| TF_VAR_locust_target_host=http://preferences.$TF_VAR_domain_name \ | ||
| TF_VAR_locust_script=preferences.py \ | ||
| xk up live/dev/locust',skip_infra,skip_secret_mgmt]" |
There was a problem hiding this comment.
Do we want to leave Locust running after the test completes? I think it would be better to clean up, especially since we're going to tear everything down before the next test run (which we should continue to do).
There was a problem hiding this comment.
It turns out we don't need to destroy Locust module to reconfigure it. Terraform detects template changes and modifies Helm release if we provide TF_VARs with different values. I could add destroy_module command to CI pipeline, but module will be destroyed with the cluster anyway (it does not leave any leftover resources behind).
There was a problem hiding this comment.
That's good news about reconfiguration.
Some responses / additional things this topic has made me think about:
- I think some smoke tests -- meaning, at least for now, these
:test_*tasks -- should be part of the regularrake/rake deployworkflow, as they are on the AWS side. They would provide rapid feedback about the health of a new cluster. - Failing that, I think these
:test_*tasks should run as part of CI in stg and prd. Unless you do 1., can you add the relevant calls to the (disabled) gcp-stg and gcp-prd jobs? - I don't think we should leave Locust running in prd. I'm less concerned about leaving it running in stg, but we want stg to be as much like prd as possible. Depending on how you handle the above, how about adding
destroy_moduleto at least the (disabled) gcp-prd job?
There was a problem hiding this comment.
Nevermind the good news, there is a pretty good chance that Locust master configuration becomes messed up if we do not destroy module before.
- I don't have strong opinion on this. If we include tests to
rake/rake deployworkflow, it would add around 5 minutes of run time, which may be not very convenient fordevenvironment, if you just want to quickly spin everything up to debug your module. We can probably just copy tests from AWS side, and include them, but it will requirewait_forcode and maybe something else. - Added.
- Agree, added.
There was a problem hiding this comment.
I agree 5 minutes is a long time.
I think we can table this for now. gcp-* jobs will run a superset of smoke tests as part of CI. We can discuss other options later.
|
LGTM! |
| sleep 45 && \ | ||
| TF_VAR_locust_target_host=http://preferences.$TF_VAR_domain_name \ | ||
| TF_VAR_locust_script=preferences.py \ | ||
| TF_VAR_locust_desired_max_response_time=4000 \ |
There was a problem hiding this comment.
@seg10 @mrtyler This is related to GPII-3250.
Surprisingly, I did not see any failed requests in Locust stats, so instead of timing out, first request to Preferences just takes a lot of time to complete, affecting max_response_time stat. Once GPII-3250 issue is solved, this line can be removed. I'll add comment.
There was a problem hiding this comment.
@natarajaya - based on my observations of the application on GCP, it looks like couchdb just needs some time to settle down upon initial deployment. When requests are made prematurely before the system is up and running, the preferences server takes a while to complete its request. Recommend that we initially put in a wait for some amount of time before running the smoketests.
There was a problem hiding this comment.
Strike the above, this is more related to the indices for queries not being warmed up.
PR for the above now available at: gpii-ops/gpii-dataloader#7
|
Conceptually I'm not quite happy about mixing the provisioning of the locust swarm and actually running the tests. I would think that provisioning should be one thing, idempotent, handled with TF. and actually running the perf. test another. This woudl probably prevent, if I understand correctly above code, things like teraing down the cluster every time before running the test or non-idempotent TF runs. |
|
@stepanstipl Thanks for review! As the title of this PR states – it is just a first attempt to introduce Locust into our infra. I agree that there is definitely room for improvement! Maybe, once common code is merged, we can have common "utility" multi-purpose cluster to run smoke tests from. This would've resolved issue from your previous comment and allowed us to have better test results, "unstained" by Locust activity, always benchmark from different zone, etc. |
To deploy Locust:
rake sh['xk up live/dev/locust']To start the swarm:
rake sh['sh -c "TF_VAR_locust_swarm=1 xk up live/dev/locust"']I don't like the way how chart handles task scripts. There is also an issue if you want to work with multiple hosts (i.e.
preferences.andflowmanager.) within the same task set. But otherwise this approach is pretty flexible.