feat(scaletest/notifications): reduce overhead cost of user setup via connection-pool - #28092
feat(scaletest/notifications): reduce overhead cost of user setup via connection-pool#28092cstyan wants to merge 1 commit into
Conversation
…ith reliability hardening
spikecurtis
left a comment
There was a problem hiding this comment.
This PR is still very complicated and is trying to do a lot of things at once. It's headlined as setting a connection pool for the setup, which would be a fairly simple change. But it also completely refactors the setup and adds a bunch of complex local defenses for runs that are killed and leave state behind.
In terms of reducing the number of connections for user creation, I'm not actually convinced this is a good idea. It sort of violates the ethos of our scale testing in that it's deliberately trying to make the load generator work in a way that takes fewer resources compared with what we would see if the load were real.
I understand the perspective that if we are testing notifications, we might not be interested in the load impact of creating users. But, isn't that case covered by the --reuse-users flag up the stack from here? That allows you to independently set the concurrency of user setup in a way that doesn't impact notifications.
In terms of all the defenses that this PR adds around cleaning up broken runs: they seem very ad-hoc and complicated. I'm not convinced it's worth all this complexity when you can burn down the Coder deployment and start again. As this PR shows, it's just very hard to cover all your bases in terms of getting the cluster back to a pristine state, and you're of course still left with cases like a hard kill of the process by the cluster itself.
I would much rather we invest in tools that reset the Coder deployment to a known declared state than invest in bulletproof cleanup routines load generator by load generator.
|
|
||
| // Identify everything this run creates so residue from a hard kill is | ||
| // greppable, and bound the token lifetime to a little beyond the run so | ||
| // orphans expire in hours rather than the deployment default. |
There was a problem hiding this comment.
this bit about token lifetime seems out of place. I don't see anything here about token lifetime.
|
|
||
| // A separate client because RunReturningUser turns on body logging for whatever | ||
| // client it is handed, which would follow the shared setup client into cleanup. | ||
| createClient, err := loadtestutil.DupClientConfiguringTransport(r.client, BypassHeader, boundPool(r.concurrency)) |
There was a problem hiding this comment.
This is very convoluted. You create a pooled client on line 227 called setupClient, then pass it into the scaletestRun. But then the actual setup() method which does the majority of the work setting up calls createUsers() which goes and dupes the main client, ignoring setupClient.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This PR is the first in a stack that supersedes #27717.
In this PR we're just addressing the overhead cost associated with the user setup portion of the notifications load generator.
Background
Currently the notifications load generator has two main issues.
The first is that it creates new users, promotes some portion of them to template admins, then creates and deletes a template. The goal here is to measure the delivery timing of the notifications to the template admins about the template deletion, but the first issue is that we also have written the load generator such that user creation and deletion notifications are sent into the system as well. This results in roughly an order of magnitude more notifications being processed through the system than one might expect.
A second issue stems from the fact that we do the user setup, login, and websocket connection establishment via a separate connection each per runner. This means that for a notifications scenario of 20k users we actually end up establishing 60k connections. Even though they may not all be open concurrently, this can overwhelm some portions of the scaletest cluster infrastructure just as a result of using so many ports for the load balancer, service -> pod connection establishment, etc.
This PRs Fix
The fix contained in this PR is to address the second issue by introducing a new setup connection pool, currently 10 connections, that is used for all of the user setup (creation/token establishment) and cleanup (deletion). We then only have this small pool + the configured # of users (for their websocket connections) in terms of # of connections that need to be established. The first issue will be addressed later in the stack.