add: restart library to run restartable applications - #5139
Conversation
|
@mkleczek Please don't forget to add the |
305eb75 to
fe305de
Compare
Done |
wolfgangwalther
left a comment
There was a problem hiding this comment.
This is a first review. I'm certain that I will have more comments in the next iteration, when I understand more of the code.
There was a problem hiding this comment.
We should not require all *.nix files to be in nix/. This is a test, it just happens to be written in .nix. It should be in test/restart.
The nix/ folder is essentially just short for "nix tooling".
There was a problem hiding this comment.
We should not require all
*.nixfiles to be innix/. This is a test, it just happens to be written in.nix. It should be intest/restart.The
nix/folder is essentially just short for "nix tooling".
@steve-chavez - are you OK with moving this out from nix/?
| "PORT=${toString port} SERVICE=${service} pytest -q /tmp/process-restart-pytest", | ||
| timeout=30, | ||
| ) | ||
| ''; |
There was a problem hiding this comment.
So essentially, we just want a simple test that sends SIGHUP and then checks whether the PID of the process supervised by systemd has changed, right?
I think we should keep the test really simple and use the NixOS test framework: Instead of copying a pytest file into the machine, just write a simple script in here.
I don't think we need to have any http involved either: The test binary could just print its mode on stdout instead. This should be easily inspected with the NixOS test framework.
One thing that I would like to look at later, but doesn't need to be part of this initial PR: I would like to replace the binary with a slightly different version (for example printing "v1" vs "v2" or so) via specialization and then hook up the NixOS service the correct way to handle this via SIGHUP - i.e. a full E2E test of integrating this replacement-restart into NixOS.
There was a problem hiding this comment.
So essentially, we just want a simple test that sends SIGHUP and then checks whether the PID of the process supervised by systemd has changed, right?
Not really, that would be too simple.
There are two main things we want to test:
- That restart process parent-child interaction works in such a way that parent is alive until after child reports it is ready (and systemd is notified about the new
MAINPID, if executing under systemd). The goal is to make the restart zero-downtime, so we really validate zero-downtime restarts. - That systemd interaction works correctly, meaning: after first
systemctl reloadwe want to know that a new process is running and that subsequentsystemctl reloadtargets that new process - this is the ultimate test of proper systemd interaction.
I think we should keep the test really simple and use the NixOS test framework: Instead of copying a pytest file into the machine, just write a simple script in here.
I've done it like that first, but the need for Nix rebuild every time test suite changes is unacceptably annoying. Having it this way decouples test scenarios from Nix builds.
I don't think we need to have any http involved either: The test binary could just print its mode on stdout instead. This should be easily inspected with the NixOS test framework.
Using http for this is really just a byproduct. The main reason to use a simple http server as a test application is to make sure the restarts are zero-downtime.
One thing that I would like to look at later, but doesn't need to be part of this initial PR: I would like to replace the binary with a slightly different version (for example printing "v1" vs "v2" or so) via specialization and then hook up the NixOS service the correct way to handle this via SIGHUP - i.e. a full E2E test of integrating this replacement-restart into NixOS.
I am not Nix expert - happy to see an example of what you are talking about.
There was a problem hiding this comment.
- That restart process parent-child interaction works in such a way that parent is alive until after child reports it is ready (and systemd is notified about the new
MAINPID, if executing under systemd). The goal is to make the restart zero-downtime, so we really validate zero-downtime restarts.
Right. Can we test that by logging the relevant events, i.e. ready/stop etc. in the executable? We should then find a sequence of first logging "replacement ready" and then "original stop", right?
This would also implicitly test something that I would find very relevant, but have no idea whether it currently happens: Are stdout/stderr of both processes logged to the journal in this setup? This seems to be quite important to me.
(side note: we might want to somehow identify log lines, for example via PID, if that's not built-in somehow already - otherwise it would be really confusing?)
2. That systemd interaction works correctly, meaning: after first
systemctl reloadwe want to know that a new process is running and that subsequentsystemctl reloadtargets that new process - this is the ultimate test of proper systemd interaction.
I see, thanks. That can be done with the non-pytest test, too, though.
I've done it like that first, but the need for Nix rebuild every time test suite changes is unacceptably annoying. Having it this way decouples test scenarios from Nix builds.
I don't think there is any difference between the two regarding rebuilds. You still need to rebuild the NixOS test when the test script changes - the pytest test script is part of the test derivation, you can't avoid that. It's copied to the nix store at build time and then copied from the host nix store into the machine at runtime. But the test derivation always changes.
Or did I misunderstand the nature of rebuilds you were talking about?
There was a problem hiding this comment.
I don't think there is any difference between the two regarding rebuilds. You still need to rebuild the NixOS test when the test script changes - the pytest test script is part of the test derivation, you can't avoid that.
No, it is not. Nix test script copies the pytest files to the VM at runtime using machine.copy_from_host and then executes pytest inside it.
There was a problem hiding this comment.
Nix test script copies the pytest files to the VM at runtime using
machine.copy_from_hostand then executespytestinside it.
I understand that, but the line is:
machine.copy_from_host("${./process-restart-pytest}", "/tmp/process-restart-pytest")
The ${./process-restart-pytest} syntax is a Nix path, which means the whole folder will be copied to a Nix store path at build time. The test derivation then has a path to the nix store in this place, so the command that is actually run will be something like machine.copy_from_host("/nix/store/....", ...).
This store path changes whenever you change the test script, which means that the nix derivation for the test also changes - the NixOS test is still rebuild every time.
There was a problem hiding this comment.
machine.copy_from_host("${./process-restart-pytest}", "/tmp/process-restart-pytest")The
${./process-restart-pytest}syntax is a Nix path, which means the whole folder will be copied to a Nix store
Ahh... good catch - that was not intended and I have run a newer version of the script locally.
| replacementCfg <- Restart.currentReplacementConfig | ||
| Restart.runRestartableWithSIGHUP replacementCfg $ \mode ready -> do |
There was a problem hiding this comment.
Do we absolutely need to get the replacement config from the consumer side or can the relevant runRestartable variant do that for us?
Do we even need a ReplacementConfig at all? Couldn't startReplacement just read this stuff directly?
There was a problem hiding this comment.
Do we absolutely need to get the replacement config from the consumer side or can the relevant
runRestartablevariant do that for us?Do we even need a
ReplacementConfigat all? Couldn'tstartReplacementjust read this stuff directly?
Really the idea is to have a generic primitive that can be wrapped in higher level utility functions. Hence existence of ReplacementConfig (agree the name could be better).
There was a problem hiding this comment.
Hm, that argument doesn't convince me, yet. It creates indirection and complexity. If the replacement config is not used anywhere else, we should create it as local to its usage as possible.
| , HandoverError | ||
| ) where | ||
|
|
||
| import System.Process.Restart.Impl (runRestartable, runRestartableWithSIGHUP) |
There was a problem hiding this comment.
(rant: a bit annoying to have to look up a second PR to see how these things are used....)
(rant2: I ended up adding that comment in the wrong PR first... multiple PRs for one atomic feature are just stupid!)
So it seems to me that:
runRestartableis later used by the postgrest executable,runRestartableWithSIGHUPis used by the NixOS test executable
This kind of defeats the point of the test a bit, if the code tested is not the same as executed later, doesn't it?
Why do we need two different functions here? Can we not make postgrest use the runRestartableWithSIGHUP variant?
There was a problem hiding this comment.
runRestartableis later used by the postgrest executable,runRestartableWithSIGHUPis used by the NixOS test executableThis kind of defeats the point of the test a bit, if the code tested is not the same as executed later, doesn't it?
runRestartable is a generic primitive used by runRestartableWithSIGHUP - so both are tested.
Remember - this is (as requested) a generic library that provides API not targeted to PostgREST only.
Why do we need two different functions here? Can we not make
postgrestuse therunRestartableWithSIGHUPvariant?
We could but since PostgREST already installs lifecycle managing signal handlers (ie. SIGTERM) I decided to leave it this way. Not really attached to this decision too much.
There was a problem hiding this comment.
runRestartableis a generic primitive used byrunRestartableWithSIGHUP- so both are tested.
Right, when writing that comment I had not looked at the implementation of that, yet.
Remember - this is (as requested) a generic library that provides API not targeted to PostgREST only.
True, but we need to find the right balance between "too specific" and "too generic". While it should be generic (as in entirely independent of the PostgREST code), it doesn't need to be usable in different scenarios than our own. We're not publishing it this way.
If we decide to keep signal handling out of the generic part, then I'd say we should do the same in the test executable...
We could but since PostgREST already installs lifecycle managing signal handlers (ie.
SIGTERM) I decided to leave it this way. Not really attached to this decision too much.
... however, I think it would make more sense (to me!) if installing signal handlers was split instead. PostgREST installs its own handlers and the restart library does, too. This encapsulates the logic much better, which is important when trying to understand this from the perspective of reading the PostgREST code. Otherwise you'll always need to know "I see runRestartable here and this only works because I install a specific signal handler elsewhere..." - which is hard in a bigger code base.
|
|
||
| type AppRun res = HandoverMode -> Ready -> IO res | ||
|
|
||
| type Ready = forall a b. IO a -> (IO a -> IO b) -> IO (Maybe b) |
There was a problem hiding this comment.
This should really have some explanation of what arguments we're looking at here.
There was a problem hiding this comment.
This should really have some explanation of what arguments we're looking at here.
Agreed.
| terminationGracePeriodMicroseconds = | ||
| 3 * 1000 * 1000 |
There was a problem hiding this comment.
We're using 3_000_000 style elsewhere, right?
There was a problem hiding this comment.
We're using
3_000_000style elsewhere, right?
Indeed, thanks for catching.
| -- | Process mode detected by the handover runner. | ||
| newtype HandoverMode = HandoverMode { | ||
| isReplacement :: Bool | ||
| } | ||
| deriving (Eq, Show) |
There was a problem hiding this comment.
Why do we need a new type here instead of just passing a Boolean around?
There was a problem hiding this comment.
Why do we need a new type here instead of just passing a Boolean around?
To encapsulate raw Bool to future-proof the API. I find passing raw primitive types not wrapped in newtype in public API a bad practice.
There was a problem hiding this comment.
To encapsulate raw
Boolto future-proof the API. I find passing raw primitive types not wrapped innewtypein public API a bad practice.
Right, but that's the great thing about "private generic library". We don't need to do this, because we have every consumer in our control and can just refactor when the time comes. We don't intend to publish this independently.
So I'd really prefer if we could make this as complex as required and as simple as possible otherwise, because that helps readability / maintainability a lot.
There was a problem hiding this comment.
To encapsulate raw
Boolto future-proof the API. I find passing raw primitive types not wrapped innewtypein public API a bad practice.Right, but that's the great thing about "private generic library". We don't need to do this, because we have every consumer in our control and can just refactor when the time comes. We don't intend to publish this independently.
So I'd really prefer if we could make this as complex as required and as simple as possible otherwise, because that helps readability / maintainability a lot.
Having a dedicated type representing "startup mode" facilitates understanding of the API and makes usage less error prone (granted - maybe StartupMode is a better name, WDYT?).
We can discuss if it is better to have a newtype wrapping a Bool value or it should be an ADT like data HandoverMode = Standalone | Replacement. Nevertheless - it should not be a raw Bool.
We should follow good API design practices regardless of whether it is internal or external library. Why would we decide to have subpar API exposed by a library just because it is internal???
There was a problem hiding this comment.
We can discuss if it is better to have a
newtypewrapping aBoolvalue or it should be an ADT likedata HandoverMode = Standalone | Replacement. Nevertheless - it should not be a rawBool.
Right. I'm not so much concerned about whether we have a newtype here - I can agree with that. I didn't word it correctly, but what I find odd is the single field record type. So both these alternatives you proposed sound much better to me.
| -- | Opaque replacement process configuration. | ||
| data ReplacementConfig = ReplacementConfig | ||
| { replacementExecutable :: FilePath | ||
| , replacementArguments :: [String] | ||
| , replacementEnv :: [(String, String)] | ||
| } deriving (Eq, Show) |
There was a problem hiding this comment.
I think a better name would be something like ProcessConfig? It actually never describes the environment of the replacement, because the handover FDs are added just before executing the replacement.
(although I do question whether we need this at all, see other comment)
There was a problem hiding this comment.
I think a better name would be something like
ProcessConfig?
Agreed.
It actually never describes the environment of the replacement, because the handover FDs are added just before executing the replacement.
Not sure what you mean, fds are parent-child protocol private, other than this the environment is passed as requested to the replacement process.
There was a problem hiding this comment.
Not sure what you mean,
fds are parent-child protocol private, other than this the environment is passed as requested to the replacement process.
I just meant to say that:
- The environment stored in
ReplacementConfigis without FDs. - The replacement runs with an environment with FDs.
- Which means that
ReplacementConfignever describes 100% what the replacement is run with, but what the original ran with.
(doesn't really matter if we rename or remove it entirely anyway)
|
|
||
| # NixOS VM tests | ||
| nixpkgs-nixos-test = runTest postgrestStatic (pkgs.path + "/nixos/tests/postgrest.nix"); | ||
| process-restart-systemd-test = runTest (lib.dontCheck postgrest) ./nix/tests/process-restart-systemd.nix; |
There was a problem hiding this comment.
We probably don't want to add another full build of postgrest. So at the minimum we should re-use something existing, such as postgrestPackage.
However, it would be great if we could find a way to make this a separate build that only builds the test executable and nothing else. We'd ideally move the test executable into a separate .cabal file in test/restart as well - there is really no point in exposing it to consumers.
With the current setup, it would be built for the regular Nixpkgs package, for example. It probably appears in the docker image we create as well.
There was a problem hiding this comment.
However, it would be great if we could find a way to make this a separate build that only builds the test executable and nothing else.
Agreed, will try to come up with something.
| if flag(dev) | ||
| ghc-options: -O0 -fwrite-ide-info | ||
| if flag(hpc) | ||
| ghc-options: -fhpc -hpcdir .hpc | ||
| else | ||
| ghc-options: -O2 | ||
| if impl(ghc >= 9.12) | ||
| -- Makes GHC consider cross-module specialization for polymorphic functions | ||
| -- without explicitly needing to add INLINE, INLINABLE or SPECIALIZE pragmas. | ||
| -- Slightly increases the binary size but improves performance considerably. | ||
| ghc-options: -fexpose-overloaded-unfoldings -fspecialise-aggressively |
There was a problem hiding this comment.
We'll probably want to move this kind of stuff (and other things, too) into a reusable piece in postgrest.cabal before duplicating it many times.
Thanks for this one! |
| ## Source Layout | ||
|
|
||
| - `common/System/Process/Restart.hs` is the public module. It re-exports the | ||
| platform-independent API and the selected platform implementation. | ||
| - `common/System/Process/Restart/Shared.hs` defines public types shared by all | ||
| platforms, including `ProcessConfig`, `StartupMode`, `AppRun`, `Ready`, | ||
| and `HandoverError`. | ||
| - `posix/System/Process/Restart/Impl.hs` implements real process handover for | ||
| POSIX platforms. It starts replacement processes, coordinates the private | ||
| READY/COMMIT protocol over pipes, installs the optional SIGHUP restart | ||
| handler, and integrates with systemd notifications when `NOTIFY_SOCKET` is | ||
| available. | ||
| - `windows/System/Process/Restart/Impl.hs` preserves the same public API on | ||
| Windows, but runs applications without replacement handover because the POSIX | ||
| protocol is not available there. | ||
|
|
||
| The Cabal stanza always includes `src/restart/common` and adds either | ||
| `src/restart/posix` or `src/restart/windows` through platform-specific | ||
| `hs-source-dirs`. |
There was a problem hiding this comment.
@mkleczek Many thanks for this doc, it helps a lot in understanding the big picture.
I suggest to remove some internal details that are prone to change and are not necessary for the big picture:
| ## Source Layout | |
| - `common/System/Process/Restart.hs` is the public module. It re-exports the | |
| platform-independent API and the selected platform implementation. | |
| - `common/System/Process/Restart/Shared.hs` defines public types shared by all | |
| platforms, including `ProcessConfig`, `StartupMode`, `AppRun`, `Ready`, | |
| and `HandoverError`. | |
| - `posix/System/Process/Restart/Impl.hs` implements real process handover for | |
| POSIX platforms. It starts replacement processes, coordinates the private | |
| READY/COMMIT protocol over pipes, installs the optional SIGHUP restart | |
| handler, and integrates with systemd notifications when `NOTIFY_SOCKET` is | |
| available. | |
| - `windows/System/Process/Restart/Impl.hs` preserves the same public API on | |
| Windows, but runs applications without replacement handover because the POSIX | |
| protocol is not available there. | |
| The Cabal stanza always includes `src/restart/common` and adds either | |
| `src/restart/posix` or `src/restart/windows` through platform-specific | |
| `hs-source-dirs`. | |
| ## Code Map | |
| - `common/System/Process/Restart.hs` is the public module. It re-exports the | |
| platform-independent API and the selected platform implementation. | |
| - `common/System/Process/Restart/Shared.hs` defines public types shared by all | |
| platforms. | |
| - `posix/System/Process/Restart/Impl.hs` implements real process handover for | |
| POSIX platforms. It starts replacement processes, coordinates the private | |
| READY/COMMIT protocol over pipes, installs the optional SIGHUP restart | |
| handler, and integrates with systemd notifications when `NOTIFY_SOCKET` is | |
| available. | |
| - `windows/System/Process/Restart/Impl.hs` preserves the same public API on | |
| Windows, but runs applications without replacement handover because the POSIX | |
| protocol is not available there. |
Renaming as Code Map to follow the same pattern as https://docs.postgrest.org/en/v14/explanations/architecture.html#code-map
|
@mkleczek At a high level, could you explain what are the differences compared to Nginx' Upgrading Executable on the Fly? (at the bottom of the page) I'm still not understanding why we didn't chose to have a "master process" to have zero-downtime upgrades, given that would also unlock other enhancements like #2429 |
Nginx signal handling is quite complex and forces the user to understand its master/worker architecture. I don't see any advantages of such an architecture for PostgREST TBH.
I don't see what we would gain by complicating the runtime process architecture and how it would address #2429 - can you elaborate what advantabes master/worker process architecture has? |
fe7dbf0 to
6f51acd
Compare
6f51acd to
8804c23
Compare
There was a problem hiding this comment.
Hackage does not support multiple .cabal files in a single project. That means we need to either:
- give up on publishing to hackage or
- publish the
restartlibrary separately
I disagree with both options.
I think we should revert to what you had previously: The source code in top-level src/, the library restart stanza in postgrest.cabal.
Now - you created a separate .cabal file for the test app - and that's fine. This will still solve the problem of not distributing that test executable with the regular executable, e.g. via Nix.
Arguably, we should move most of our tests to maybe a separate test/postgrest-tests.cabal or something, because it makes no sense to ship them via hackage - they can't run without our internal Nix tooling anyway. I think doctests are the lone exception.
There was a problem hiding this comment.
Hackage does not support multiple
.cabalfiles in a single project.
How come https://github.com/yesodweb/wai successfully publishes multiple libraries from their monorepo in Hackage???
There was a problem hiding this comment.
That's precisely what I am saying: They publish multiple libraries to hackage. This was the second option in my list of two. I'd like to keep this library internal, though.
There was a problem hiding this comment.
That's precisely what I am saying: They publish multiple libraries to hackage. This was the second option in my list of two. I'd like to keep this library internal, though.
I don't understand: is it all or nothing?
There was a problem hiding this comment.
I don't understand your question.
When we publish the current state of this PR to hackage, then only the stuff in postgrest.cabal will be published (because multiple .cabal files in a single hackage project are not supported!). That file refers to restart as a library. When, for example, I will build PostgREST from hackage in Nixpkgs, the code will need to resolve that reference to restart. This will only work if restart is published as another hackage library.
I don't want to publish restart to hackage.
Did that answer your question?
There was a problem hiding this comment.
That's precisely what I am saying: They publish multiple libraries to hackage. This was the second option in my list of two. I'd like to keep this library internal, though.
I don't understand: is it all or nothing?
@wolfgangwalther nevermind - got it. We have to publish dependencies of PostgREST.
Will move it back to postgrest.cabal then. But I would leave the directory structure though.
There was a problem hiding this comment.
But I would leave the directory structure though.
This would not be in spirit of aa7d442, which aimed to use the src/ and test/ top-level folders for all libraries, too.
However, I'm open to discuss that - it would just mean we should probably rethink that commit as well. When I last thought this through, I ended up liking the top-level src/ folder for all source code better, but I guess I could be convinced otherwise, too...
No description provided.