Add an endpoint to restart Grist with a new config #1016
Conversation
jordigh
force-pushed
the
jordigh/restart
branch
2 times, most recently
from
1e9049a to
a4a86df
Compare
app/server/lib/FlexServer.ts
Outdated
| process.send({ newEnv }); | ||
| } | ||
| }); | ||
| return resp.status(200).send(); |
There was a problem hiding this comment.
Is it worth sending an error status code if we've got no process.send available (i.e - process isn't running under the runner script)
There was a problem hiding this comment.
On a multiserver setup, I'd expect the implementation to be to publish an event on a channel in redis that all servers (including ourselves) are subscribed to, and then each server restarting as described here. Seems like a natural later step to add, so doesn't need doing here.
There was a problem hiding this comment.
Suggest sending something like { action: "restart", newEnv } just so the nature of the message would be clear in debugging.
There was a problem hiding this comment.
Is there a risk we send sensitive environment variables over Redis in that situation? And if so, is that okay? I'm not sure what kind of security guarantees redis provides.
There was a problem hiding this comment.
Having talked to Jordi a bit, I'm not very convinced by the idea of implementing Grist configuration changes from the UI directly via changing environment variables. Specifically, there's no concrete proposal yet for how those changes will be persisted (@jordigh mentioned @SleepyLeslie's config PR for Grist Desktop as interesting). If there were, that might help answer the question of how configuration is kept consistent across servers/workers.
Pub/sub in redis seems an obvious choice for triggering a global restart. It may or may not be a reasonable way to propagate configuration, I haven't thought about that. Redis is already used for security sensitive purposes related to authentication and authorization.
Until there is a persistence story: what state do we actually need to persist? If I'm reading the code right, if a user provides an activation key, there's a path to activations.key getting set in the db:
https://github.com/gristlabs/grist-ee/blob/e86b290a4c2c3cc3aea201ae4787ecb8dab4e3c0/ext/app/server/lib/ActivationReader.ts#L87-L90
What I'm poking at here is if we need env var persistence this month.
There was a problem hiding this comment.
I've changed the JSON to add here. It should be straightforward to one day use Redis for more generalised IPC.
There was a problem hiding this comment.
Okay, did these changes.
There was a problem hiding this comment.
I've stubbed out config changes, which I think we should incorporate with this PR once it lands.
docker-runner.mjs
Outdated
| let grist; | ||
|
|
||
| function startGrist(newEnv) { | ||
| const env = newEnv ? {...process.env, ...newEnv} : process.env; |
There was a problem hiding this comment.
Could you save yourself a ternary here by setting newEnv = {} in the parameters, and just having env = { ...process.env, ...newEnv}?
There was a problem hiding this comment.
Oh, makes sense, of course, thanks.
There was a problem hiding this comment.
Oh, right, thanks, that's cleaner.
docker-runner.mjs
Outdated
| const env = newEnv ? {...process.env, ...newEnv} : process.env; | ||
|
|
||
| // H/T https://stackoverflow.com/a/36995148/11352427 | ||
| grist = spawn('./sandbox/run.sh', { |
There was a problem hiding this comment.
Does this get killed correctly when parent is killed? I'd think so, but just want to double check 🙂
There was a problem hiding this comment.
Yes, it gets a sigint, which works fine, as it's exec'ed itself into the main server.js process that knows how to handle sigals. The event handlers then do the work correctly.
There was a problem hiding this comment.
Yep! It works! That's because the run.sh process execs into node, so this spawn effectively ends up directly controlling a node process.
app/server/lib/FlexServer.ts
Outdated
| process.send({ newEnv }); | ||
| } | ||
| }); | ||
| return resp.status(200).send(); |
app/server/lib/FlexServer.ts
Outdated
| process.send({ newEnv }); | ||
| } | ||
| }); | ||
| return resp.status(200).send(); |
There was a problem hiding this comment.
On a multiserver setup, I'd expect the implementation to be to publish an event on a channel in redis that all servers (including ourselves) are subscribed to, and then each server restarting as described here. Seems like a natural later step to add, so doesn't need doing here.
app/server/lib/FlexServer.ts
Outdated
| process.send({ newEnv }); | ||
| } | ||
| }); | ||
| return resp.status(200).send(); |
There was a problem hiding this comment.
Suggest sending something like { action: "restart", newEnv } just so the nature of the message would be clear in debugging.
This is a new entrypoint, mostly intended for Docker, so we have one simple process controlling the main Grist process. The purpose of this is to be able to make Grist easily restartable with a new environment.
This adds an endpoint for the admin user to be able to signal to a controlling process to restart the server. This is intended for `docker-runner.mjs`.
jordigh
force-pushed
the
jordigh/restart
branch
4 times, most recently
from
7bd806a to
b36bb6c
Compare
jordigh
changed the title
This adds a very simple wrapper process for Docker that will control the main Grist process. Its purpose is to restart the main process upon demand so that the configuration can be changed. This is intended to be used for the UI as a way to give users a way to update their configuration without having to mess with Docker in the backend.