based refactors: Simplify IPC channels and cleanup#4144
Open
nrwahl2 wants to merge 22 commits into
Open
Conversation
The responsible commits were written before we replaced all gpointers with void pointers. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There were four allowed values: shared-mem, socket, posix, and sysv. Libqb removed support for posix and sysv (message queues) in 2012 via commit 70a9623a. It simply returns an error if you try to set up those types of IPC. I spoke with the libqb maintainers last night, and they said that socket IPC has never worked correctly and that they are considering removing it. Sockets were mainly intended for systems where shared memory was not available. That is believed to be no longer applicable -- shared memory should be available on all the platforms that Pacemaker and libqb target for support. That leaves only shared memory as a working option. We are dropping this option rather than deprecating it. As discussed above, two of the values (posix and sysv) will fail immediately during IPC setup, and socket is very buggy. So user-facing behavior should not be affected. If the option is configured, it will simply be ignored. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
QB_IPC_NATIVE tells libqb to use shared memory (QB_IPC_SHM) unless it's
shared-memory IPC is specifically disabled at build time because it's
unavailable on the system. In that case, libqb will use sockets
(QB_IPC_SOCKET).
I spoke with the libqb maintainers last night, and they said that socket
IPC has never worked correctly and that they are considering removing
it. They told me never to use libqb socket IPC.
Libqb socket IPC was mainly intended for systems where shared memory was
not available. That is believed to be no longer applicable -- shared
memory should be available on all the platforms that Pacemaker and libqb
target for support.
The maintainers weakly suggested using QB_IPC_NATIVE (rather than
QB_IPC_SHM). I suppose this was for future-proofing ("let libqb decide
what to use"). Nonetheless, since we have a couple of use cases where we
MUST avoid blocking, and QB_IPC_SHM is non-blocking, we'll just use
QB_IPC_SHM everywhere until we have a reason not to. Besides, this makes
things explicit.
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
...argument.
QB_IPC_NATIVE tells libqb to use shared memory (QB_IPC_SHM) unless
shared-memory IPC is specifically disabled at build time because it's
unavailable on the system. In that case, libqb will use sockets
(QB_IPC_SOCKET).
I spoke with the libqb maintainers last night, and they said that socket
IPC has never worked correctly and that they are considering removing
it. They told me never to use libqb socket IPC.
Libqb socket IPC was mainly intended for systems where shared memory was
not available. That is believed to be no longer applicable -- shared
memory should be available on all the platforms that Pacemaker and libqb
target for support.
The maintainers weakly suggested using QB_IPC_NATIVE (rather than
QB_IPC_SHM). I suppose this was for future-proofing ("let libqb decide
what to use"). Nonetheless, since we have a couple of use cases where we
MUST avoid blocking, and QB_IPC_SHM is non-blocking, we'll just use
QB_IPC_SHM everywhere until we have a reason not to.
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It behaves identically to cib_command. This became even more clear with the recent "Use QB_IPC_SHM in pcmk__serve_DAEMON_ipc()" commit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And associated code. It behaves identically to PCMK__SERVER_BASED_RW. A key point to note is that only non-proxied cib_native clients connected to PCMK__SERVER_BASED_SHM (prior to deprecation of cib_command_nonblocking in a previous commit). cib_remote clients and proxied connections used only PCMK__SERVER_BASED_RO and PCMK__SERVER_BASED_RW. So backward compatibility should not be a concern -- this affects only local clients on a cluster node. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It has one caller and saves one line at most. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And make the mainloop variable static. No other code changes. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There is still more work to do to consolidate/reorganize the based exit/shutdown-related code. I'm keeping the pieces small to try to make it easier to reason about. based_terminate() seems too complicated. Note that based_ipc_cleanup() calls pcmk__client_cleanup(), which is why we drop the call from main. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There are two call sites. * based_cpg_destroy: This is called only via the main loop. See pcmk_cluster_set_destroy_fn() and pcmk__cpg_connect(). * based_shutdown: We can reach this only through the main loop. It's set up as a main loop signal handler via mainloop_add_signal(). The true signal handler is mainloop_signal_handler(), which sets a main loop trigger to call based_shutdown(). That trigger can't do anything unless the main loop is running. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Mostly for sanity. Between the g_main_loop_quit() call and the main loop actually stopping, the main loop runs callbacks for any sources that have already been dispatched. In case some callback actually runs, it should be able to call based_shutting_down() and get a true return value. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We were allocating an int to hold the server socket file descriptor, and we were never freeing it. (Its destroy callback, based_remote_listener_destroy(), did not free it.) Don't allocate an int, but instead call GINT_TO_POINTER() and GPOINTER_TO_INT() to cast it to and from a pointer. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We will use this soon. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Previously we were only closing the file descriptors. Instead we destroy the mainloop_io_t objects returned by mainloop_add_fd(). In order to do that, we need to start storing those objects in the first place. Previously we were ignoring the return value of mainloop_add_fd() in init_remote_listener(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Return false early where possible instead. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also take a const argument. This function may kill a process, but it doesn't modify the mainloop_child_t. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We index it by stringified PID. pid_t is specified as a signed integer type, but there's no guarantee that it's a 32-bit integer. So GINT_TO_POINTER() is technically unsafe (although it's likely fine in practice). The semantics of using a hash table seem like a better fit for looking up a child record by PID. Removing an entry is also simpler, and we can set the destroy function when we create the table. Now that I've looked at most of the mainloop child code, I REALLY want to start using GSubprocess if possible. Note: I started working on this so that the CIB manager could free a mainloop_child_t that it created with mainloop_child_add(), without calling mainloop_child_kill() and killing it. I now realize this is not necessary, since mainloop_cleanup() frees any children that we haven't already freed. However, a hash table still seems like a better data structure for storing these children. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This is the next batch from #4011.