orchestrator-process: store PID metadata in temp directory #15810
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guswynn
approved these changes
Oct 31, 2022
benesch
force-pushed
the
pid-metadata-tmp
branch
3 times, most recently
from
203ae33
to
89de11d
Compare
|
Yep, this part was easy. Sadly the rest of #15725 is hard! |
philip-stoev
reviewed
Nov 1, 2022
| #[clap( | ||
| long, | ||
| env = "ORCHESTRATOR_PROCESSDATA_DIRECTORY", | ||
| env = "ORCHESTRATOR_PROCESS_DATA_DIRECTORY", |
There was a problem hiding this comment.
This variable was used with its old name in mzcompose/services.py . I guess, to be safe, services.py needs to set the variable with both its old and new spelling.
|
I pushed #15800 as a temporary work-around until this is sorted out. |
benesch
force-pushed
the
pid-metadata-tmp
branch
from
89de11d
to
8b365e0
Compare
benesch
force-pushed
the
pid-metadata-tmp
branch
from
8b365e0
to
f4377ac
Compare
benesch
force-pushed
the
pid-metadata-tmp
branch
2 times, most recently
from
db3ace4
to
1439b5f
Compare
benesch
force-pushed
the
pid-metadata-tmp
branch
5 times, most recently
from
1357799
to
4870fe1
Compare
Add `SocketAddr`, `Listener`, and `Stream` types to the `mz_ore::netio` module, which abstract over TCP sockets and Unix domain sockets. Then teach storaged and computed to accept their listen addresses using the new `SocketAddr` types, which allows them to bind to either TCP or Unix domain sockets, as desired. This is a key part step towards fixing the process orchestrator (MaterializeInc#15725), as it will allow multiple copies of Materialize to be run concurrently without competing for access to the same ports.
benesch
force-pushed
the
pid-metadata-tmp
branch
from
4870fe1
to
dbfd8a1
Compare
PID files are not valid after a reboot of a machine. In the best case, the referenced PIDs do not exist, and the process orchestrator correctly recreates the services; in the worst case, the PIDs have been reused by different processes entirely, and the process orchestrator incorrectly thinks the services are already running. The worst case scenario is almost a guarantee with containers, where there are only a few processes using the low-numbered containers. This commit fixes the problem by moving the PID metadata files into $TMPDIR/environment-$ID. $TMPDIR is cleared on restart, so the stale PID files will correctly vanish after a restart. Naming the directory after the environment ID ensures that environmentd can find its metadata after a process restart without a machine restart, but allows multiple `environmentd` processes to co-exist, as long as they use different environment IDs. Things work correctly with the `--reset` option to bin/environment, too, as this option generates a new environmentd ID. Touches MaterializeInc#15725. Would close MaterializeInc#15800.
Rewrite the process orchestrator to be more reliable:
* Use Unix domain sockets rather than TCP, to avoid competing with
other `environmentd` processes (e.g., in tests) for a very limited
range of TCP ports.
* Restructure process supervision so that existing processes that are
adopted at startup are monitored for failures and restarted if
necessary.
* Enable process status updates.
* Enable process metrics collection.
Fix MaterializeInc#15725.
Fix MaterializeInc#15336.
benesch
force-pushed
the
pid-metadata-tmp
branch
from
dbfd8a1
to
a233f31
Compare
|
@jkosh44 just a heads up that the process orchestrator looks very different as a result of this PR. So that you're not surprised when you next look at it. |
26 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PID files are not valid after a reboot of a machine. In the best case, the referenced PIDs do not exist, and the process orchestrator correctly recreates the services; in the worst case, the PIDs have been reused by different processes entirely, and the process orchestrator incorrectly thinks the services are already running. The worst case scenario is almost a guarantee with containers, where there are only a few processes using the low-numbered containers.
This commit fixes the problem by moving the PID metadata files into $TMPDIR/environment-$ID. $TMPDIR is cleared on restart, so the stale PID files will correctly vanish after a restart. Naming the directory after the environment ID ensures that environmentd can find its metadata after a process restart without a machine restart, but allows multiple
environmentdprocesses to co-exist, as long as they use different environment IDs. Things work correctly with the--resetoption to bin/environment, too, as this option generates a new environmentd ID.Touches #15725.
Would close #15800.
Motivation
Checklist
This PR has adequate test coverage / QA involvement has been duly considered.
This PR evolves an existing
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.This PR includes the following user-facing behavior changes: