WIP: Massive pacemaker-based refactors #4011
Draft
+4,715
−4,391
Conversation
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
nrwahl2
force-pushed
the
nrwahl2-based
branch
3 times, most recently
from
deaa1a7 to
bc1dd5d
Compare
nrwahl2
force-pushed
the
nrwahl2-based
branch
3 times, most recently
from
852e402 to
5241abc
Compare
nrwahl2
force-pushed
the
nrwahl2-based
branch
8 times, most recently
from
3b1444a to
f96428d
Compare
This shouldn't change behavior but is a good practice. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We only use three possible cred types. We always use certificate if they're configured. Otherwise, some callers default to pre-shared keys, while remote CIB clients and listeners default to anonymous authentication. Also improve the doc comments, since it was not obvious to me how this worked and why. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also add an error message on TLS init failure, don't log "Starting TLS listener..." until we've initialized TLS, and warn that the clear-text listener is not recommended. It seems cleaner to handle this little piece of condtional code in the caller, rather than passing a boolean argument. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
No other code changes, only moving functions around. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
No other code changes, only moving functions around. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
No other code changes, only moving functions around. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also rename the two destroy callbacks so that their purposes are clearer. Typically we wouldn't use the "based_" prefix since these are static. However, there are similarly named callbacks in liblrmd (using the "lrmd_" prefix) and we typically don't like having multiple functions with the same name even if they're static (due to difficulty of grepping through source code). No code changes aside from the renames, moves, and dropping "return" statements at the end of two void functions. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is a GSourceFunc, so use G_SOURCE_REMOVE for clarity. Also add Doxygen. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
I got confused for a little while about remote_auth_timeout_cb(). I thought returning G_SOURCE_REMOVE would cause g_source_remove() to get called on client->remote->source. But that is not the case. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is a more logical place for it. We drop the ipc_internal.h include from remote_internal.h to avoid circular includes. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
An anonymous struct is fine for the typedef. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Previously, we were looking up the user, making sure we found it, looking up the user's primary group, and checking whether it was CRM_DAEMON_GROUP. If not, then we fell back to looking up CRM_DAEMON_GROUP and checking whether user was in its member list. That fallback is all we need. The additional checks were added by commit 14d9ae4. No reasoning was provided. I'm guessing they were added in the name of efficiency in the common case. But they required two additional system calls, so I doubt that was any more efficient than iterating over the list of group members and doing string comparisons. And since this is done only once per remote client, efficiency gains probably don't matter much. Also add Doxygen. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And other minor changes Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
No code changes other than moving function definitions. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
These are mutually exclusive with the other cases that appeared earlier in the if/else chain. * If op is CRM_OP_PING or PCMK__CIB_REQUEST_SHUTDOWN, then it isn't PCMK__CIB_REQUEST_REPLACE, PCMK__CIB_REQUEST_SYNC, or PCMK__CIB_REQUEST_UPGRADE. * If op is PCMK__CIB_REQUEST_SHUTDOWN, then the cib__op_attr_modifies flag isn't set. Also, return true explicitly from the shutdown case, and don't set *process. It's already set to true when this function is called. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This was added by commit 15c4d2a to deal with legacy mode. We haven't supported legacy mode (which was for clusters with pre-1.1.12 systems) since 3.0.0. Currently, the only modifying ops that may set PCMK__XA_CIB_ISREPLYTO are replace (as a sync reply) and upgrade. Those two cases are explicitly addressed here. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is an incremental change; more is coming. Yes, this introduces duplication. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Any PCMK__CIB_REQUEST_UPGRADE with PCMK__XA_CIB_UPGRADE_RC set also has PCMK__XA_CIB_ISREPLYTO set. The cluster message is addressed to the isreplyto host (see based_process_upgrade()), which must be non-NULL because origin is non-NULL. So we cannot receive a cluster message with PCMK__XA_CIB_UPGRADE_RC and a NULL host. If we receive a message that is addressed to some OTHER host, we drop it. cib_cs_dispatch() calls pcmk__cpg_message_data(), which ignores messages that aren't for the local node. Therefore, if we're processing a PCMK__CIB_REQUEST_UPGRADE request with PCMK__XA_CIB_UPGRADE_RC set, then is_reply must be true. Note that the equivalent of PCMK__XA_CIB_UPGRADE_RC was introduced by commit 1f05f5e. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
...in parse_peer_options(). Another incremental change for easy review.
Note that the direct negation of the conditions that came before the
previous "return false" would be
if (((max != NULL) || !based_is_primary)
&& (max == NULL)) {
...
}
It cannot be the case that (max == NULL) and (max != NULL), so we can
replace ((max != NULL) || !based_is_primary) with !based_is_primary.
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
But not the code below it. I've come to appreciate a nice "goto done" or similar, but in this case removing the label seems to make things less convoluted, at least for now. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It doesn't really do anything now. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
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 argument argument. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Save some lines. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
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>
To mirror attrd and the fencer. 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>
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.
No description provided.