based refactors: Improve init/cleanup and split Corosync-related functions into new file#4135
Merged
Merged
Conversation
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The NULL checks will get de-duplicated in an upcoming commit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also move the ipcs_{ro,rw,shm} extern declarations from
based_callbacks.h to based_ipc.h, now that there is a more appropriate
home for them. And make ipc_{ro,rw}_callbacks static to based_ipc.c,
since we no longer use them in pacemaker-based.c.
The goal is to look more like attrd, the fencer, and the scheduler.
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The goal is to look more like attrd, the fencer, and the scheduler. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This seems like a more correct location for it. based_terminate() is where we free data structures. This also makes the calls to pcmk__stop_based_ipc() redundant, and allows making the qb_ipcs_service_t declarations static. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also make -1 the "uninitialized" value. 0 is a valid file descriptor, even though it's much more likely to be used for stdin. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To look more like attrd and fenced. More change are coming. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It doesn't make sense to have this handful of unrelated initializations in a separate function, when most of main() is for initializing things. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Place the call right before the first thing that needs it (based_activate_cib()). The rest of the initialization helpers are called in this region, so let's collect them. We can't make them all contiguous though -- based_remote_init() requires that the CIB be read and activated first. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To mirror attrd and fenced. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
pcmk__corosync_connect() doesn't use the caches directly. pcmk__get_node() initializes the caches when it gets called. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Using include-what-you-use. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It shouldn't matter whether this is a Corosync cluster or not. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To mirror attrd and fenced. Notes: * Previously, we weren't freeing the cluster object on exit in based_terminate(). Now we are. * pcmk_cluster_free() calls pcmk__cluster_destroy_node_caches(), which is why we drop the call to that function. * I'm fairly certain that the reason the pcmk_cluster_disconnect() call previously occurred before the done section, is that prior to a recent commit, we weren't NULL-checking the cluster argument before disconnect. We should be able to call based_cluster_disconnect() regardless of how based_terminate() wherever we want to free the cluster object. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And NULL-check the attrd_cluster variable. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2
force-pushed
the
nrwahl2-based_next
branch
from
add6809 to
93dc367
Compare
Contributor
Author
|
I haven't run cts-lab on this in a while (assuming I ever did so), so that should be done before merging. |
Contributor
Author
Running now. 12 tests successful so far. |
Contributor
Author
Did a 35-test run, all good. |
clumens
reviewed
Jul 1, 2026
| if (cluster == NULL) { | ||
| return EINVAL; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
It probably doesn't truly matter since when pcmk_cluster_disconnect is called, we're shutting down. But, I wonder if we should perform this check before destroying the caches.
Contributor
Author
There was a problem hiding this comment.
Maybe I was thinking about the fact that destroying the caches doesn't depend on non-NULL cluster, and so thinking it should come before the NULL check. The idea being that something might be calling this function for the side effect of destroying caches. But if cluster is NULL, we're likely to have other problems due to the dereference in pcmk__cpg_disconnect().
Updating this commit to move the NULL check upward.
| based_cluster_node_name(void) | ||
| { | ||
| return (based_cluster != NULL)? based_cluster->priv->node_name : NULL; | ||
| } |
Contributor
There was a problem hiding this comment.
If you want to go even farther and move the rest of OUR_NODENAME into this function, I'd be fine with that.
Contributor
Author
There was a problem hiding this comment.
As an aside, I don't see where anything uses the stand-alone mode (-s or --stand-alone). Do you? The only thing I found is https://projects.clusterlabs.org/T227, which suggests using it for a proposed regression test.
Contributor
There was a problem hiding this comment.
No, I don't think anything uses it at the moment. It's been around for a long, long time (9be1f96 at least). I'd leave it, since adding a regression test would be a great idea. I did this for attrd a while back.
Contributor
Author
There was a problem hiding this comment.
Yeah I planned to keep it for that reason, even though it means cluttering the code and having a number of untested code paths and scenarios.
Contributor
Author
There was a problem hiding this comment.
Adding a series of commits to remove OUR_NODENAME completely. In most cases we can use based_cluster_node_name(). Using "localhost" appears to be useful only for logging. So I've kept "localhost" in based_process_request() for setting PCMK__XA_SRC, so that other functions that fetch that attribute can log a non-NULL value.
pcmk_cluster_disconnect() now returns EINVAL if passed a NULL argument. Previously, if given a NULL argument in a Corosync cluster, pcmk_cluster_disconnect() would call down to pcmk__cpg_disconnect(), which would dereference the NULL pointer. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To mirror attrd and the fencer. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The CIB manager started sending a shutdown request with commit 1f61358 (2006). There is not much detail about the issue this aimed to fix. However, the commit message says the following: """ Fix for a timing issue where a remote CCM registered the DC exited before the last CIB update(s) from the DC were applied. ... As long as one node has the update, the join process will make sure it ends up on the new DC. If for some reason *none* of our peers reply, we still exit in at most N seconds and we're no worse off than we were before. """ It very much sounds like the problem was tied to legacy mode and however we handled CIB updates back then. As of Pacemaker 1.1.12, all modifying operations get forwarded and processed on all nodes. Commit 15c4d2a: """ Feature: cib: Send all r/w operations via the cluster connection and have all nodes process them The idea of a master cib instance goes away, and all instances apply all raw (and now ordered thanks to corosync) updates (not diffs) This ensures updates are never lost, even if there is no DC """ That was the replacement for legacy mode behavior. Given that modifying ops get forwarded to all nodes, I don't see how "...a remote CCM registered the DC exited before the last CIB update(s) from the DC were applied" would present a problem. If the DC's CIB manager receives a modifying request, it will forward it via the cluster layer to all nodes for processing. A request could always be dropped if a node abruptly leaves the cluster before it's able to forward a request that a client sent, but there's nothing we can do about that. At least the CIB would remain in a consistent state. Note that this also makes the peer status callback pointless. So we drop it, and we also drop the associated "-1" logic from based_terminate(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And make it static. It seems logical to have it in the same file as the main() function. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This gets rid of the last use of based_cluster outside of based_corosync.c. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And rename to cluster. We don't typically care much about making variable names distinct, although we try to make function names distinct. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace cib_shutdown_flag. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace global stand_alone variable. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To drop a global. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The "leave SIGCHLD engaged" logic was introduced by commit 062d038. Apparently the purpose was to wait until after calling crmd_drain_alerts() before destroying the SIGCHLD handler. However, crmd_drain_alerts() was dropped by commit 9a2fd24 (Pacemaker 1.1.18). Currently, we destroy the SIGCHLD handler before we do anything else in both the if block and the else block. So we can just destroy it when we destroy the other handlers. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
These comments are made redundant either by other comments or by self-documenting code. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
These might be useful for debugging issues at shutdown, but we can add ad hoc tracing if necessary. I suppose the one exception would be if a user reports an issue that we can't reproduce. But these messages don't add much beyond making it easier to sequence events that are occurring. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
* Use bool instead of gboolean. * Drop an else. * Compare to NULL explicitly. * Simplify the loop that drains the main loop. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Forwarding a request won't work anyway if we're in stand-alone mode, so we can just call based_cluster_node_name(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
If we call this function, then we're not in stand-alone mode, so we can call based_cluster_node_name() directly. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We shouldn't call sync_our_cib() if we're in stand-alone mode. It will end up being a no-op when we call pcmk__cluster_send_message(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We should never try to forward a request via the cluster when we're in stand-alone mode. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This entails a very minor behavioral change. If the destination host (PCMK__XA_CIB_HOST) is "localhost" and we're in stand-alone mode, we will now log a warning. (We expect the destination host to be NULL.) Other than the warning, behavior should be unchanged. Even the warning won't happen in practice, because nothing appears to use pacemaker-based's stand-alone mode currently. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is the last remaining use of OUR_NODENAME, so we drop it. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2
force-pushed
the
nrwahl2-based_next
branch
from
93dc367 to
3cb83a6
Compare
clumens
approved these changes
Jul 6, 2026
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
2 participants
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.
Next batch from #4011