feat: in-process provisionerd connection #1568
Conversation
Signed-off-by: Spike Curtis <spike@coder.com>
There was a problem hiding this comment.
We'll probably need IncludeProvisionerD to be ProvisionerDaemonCount instead. Right now, we have a CLI option on coder server to customize the amount, which defaults to 3.
With just one, it's possible to experience looong build times from even just a few users.
|
instead of You can always add more provisionerds if you want |
|
Lint is complaining about me passing the |
|
By the way, the reason builds take a long time with a few users is because provisionerd currently will only run one job at a time. We'll need to fix it up so that it can run multiple jobs in parallel (as long as they are for different workspaces). |
|
Ahh my bad, I misread. I dislike that you cannot disable the provisioned daemon halfway through, I'm pretty sure we used that for some tests to ensure jobs would stay in progress, but I suppose maybe not. @spikecurtis why do you feel daemons should run multiple jobs in parallel? We almost want to promote the idea of run once and exit style runners, especially because they perform arbitrary code. |
No, you still can, and some tests do. You just have to use the more verbose form: |
Signed-off-by: Spike Curtis <spike@coder.com>
|
@spikecurtis how are you planning to handle that test-case when the HTTP endpoint goes away? |
All tests are moved to in-process communication with this PR. You can start and stop provisionerds at will. They just have to be in the same process as coderd. There was just one test case where we weren't doing it, testing the |
…_provisionerd_comms
…_provisionerd_comms
…_provisionerd_comms
| ID: uuid.New(), | ||
| CreatedAt: database.Now(), | ||
| Name: namesgenerator.GetRandomName(1), | ||
| Provisioners: []database.ProvisionerType{database.ProvisionerTypeEcho, database.ProvisionerTypeTerraform}, |
There was a problem hiding this comment.
Maybe this isn't a new issue since this function is just a copy of the existing code in provisionerDaemonsListen, but should this list be derived from the keys in daemon.Provisioners? As is, when we're running in production mode, the echo provisioner won't be included in the map of provisioner implementations, but it will still be in this list.
No idea if that actually causes problems, but it seems sketchy.
There was a problem hiding this comment.
This is copied from the existing code, and you've stumbled on a weakness in the design here: which is that there is no direct link between the provisioners that are actually started by a provisionerd and the provisioners that are registered.
On the coderd side, it's just hardcoded that there are echo and terraform provisioners, and on the provisionerd side we start one or other or both depending on the circumstances.
Really, we should have the provisionerd register the provisioners it is actually running when it connects. I'll create an issue to track #1605
…_provisionerd_comms
* in-process provisionerd connection Signed-off-by: Spike Curtis <spike@coder.com> * disable lint for server.go/newProvisionerDaemon Signed-off-by: Spike Curtis <spike@coder.com>
Switches provisionerd to use an in-process communication, rather than a remote-HTTP endpoint for provisionerds that are started in the same process as coderd.
First part of #1375
I'm not actually disabling the remote endpoint in this PR because we squash commits and I want to make it easy to revert later when we're ready to include auth.
Note there was one instance, in testing, where we did run a provisionerd in a separate process from coderd -- testing the
coder servercommand. I've modifiedcoder server --devto start upechoprovisioners for this testing to sidestep the need to start an echo provisioner in a separate process for now.