WIP: Massive pacemaker-based refactors#4011
Draft
nrwahl2 wants to merge 292 commits intoClusterLabs:mainfrom
Draft
WIP: Massive pacemaker-based refactors#4011nrwahl2 wants to merge 292 commits intoClusterLabs:mainfrom
nrwahl2 wants to merge 292 commits intoClusterLabs:mainfrom
Conversation
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
Merged
nrwahl2
force-pushed
the
nrwahl2-based
branch
10 times, most recently
from
3416148 to
2dd98b1
Compare
nrwahl2
commented
Jan 8, 2026
daemons/based/based_remote.c
Outdated
| * reference to the client's \c GSource has been removed. There is likely | ||
| * only one reference when we call this function, and thus the client is | ||
| * likely to be freed before we return. The current GLib code looks as if | ||
| * this is always the case. However, that is a GLib implementation detail. |
Contributor
Author
There was a problem hiding this comment.
g_main_context_dispatch() dispatches all pending sources in a batch before moving to the next main loop iteration. If we're running this function, then we're running it as a result of a particular source being dispatched. So I think there may be another reference to the source we're removing if it's also being dispatched as part of this batch.
So I'll weaken this assumption. But I still don't think we need to wait for remote clients to be destroyed by the callback before dropping clients. (See a couple of commits ahead of here.)
nrwahl2
force-pushed
the
nrwahl2-based
branch
10 times, most recently
from
ffad0bf to
d6b2ff4
Compare
Have it create a pcmk__request_t and look more like lrmd_remote_client_msg() and based_peer_callback(). Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
And rename it to based_handle_request(), to align with other daemons. This commit aims to make basically the minimal changes necessary for this function to take a pcmk__request_t. There are more improvements to be made. Signed-off-by: Reid Wahl <[email protected]>
If needs_reply is true, then client == NULL. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
This TODO has been present since commit 83919a1 (2004). So the code is probably (though not necesssarily) okay as-is. Signed-off-by: Reid Wahl <[email protected]>
This makes no difference, just consolidates the freeing. Signed-off-by: Reid Wahl <[email protected]>
I'm not sure whether or how this ever worked. Signed-off-by: Reid Wahl <[email protected]>
We dropped support for legacy mode in 3.0.0. Signed-off-by: Reid Wahl <[email protected]>
Prior to Pacemaker 3.0.0, it was possible to send these requests via the cib_api_operations_t:is_primary() method. However, as far as I can tell, nothing internal ever used that method or sent a cib_ismaster request directly. I also didn't find any instance of sbd or dlm using it. Since the is_primary() method was public API, technically this breaks compatibility if some external program is calling it on Pacemaker Remote nodes running versions older than 3.0.0. That's why I'm adding this commit to the change log. However, this seems very unlikely to affect anyone in practice. Signed-off-by: Reid Wahl <[email protected]>
Three CIB operations had the cib__op_attr_local flag. All of them are supported only by the CIB manager (cib_native and cib_remote); they're unsupported for cib_file clients. There's no straightforward way to misuse these. The API methods (fetch_schemas(), set_primary(), set_secondary()) don't take a host argument. They all three call cib__internal_op() with a NULL host argument, so that PCMK__XA_CIB_HOST is unset in the resulting request. As far as I can tell, sending one of these requests with a PCMK__XA_CIB_HOST attribute (whether set to the local node name or to something else) is impossible without some pretty devious and irresponsible hacking, like sending an XML payload directly without using the libcib functions to do so. This commit adds a CRM_CHECK() call to each processor function, just in case this ever happens due to malice or to future programmer error. I don't see any reason to wait for a compatibility break for this change. Signed-off-by: Reid Wahl <[email protected]>
The behavior change here is pretty minor. See commit 3f65618 -- this request type apparently never worked since it was added in 2008. It always returned EINVAL. Now, the CIB manager will still return EINVAL for this request, as that will be the return value from cib__get_operation(). The only difference I see is that we'll no longer send the local client a notification. So I see no reason to wait for a formal backward compatibility break to make this change. Signed-off-by: Reid Wahl <[email protected]>
Nothing has used it since c13a9d9. Signed-off-by: Reid Wahl <[email protected]>
Write to disk if the configuration section changed. Otherwise, there seems to be no reason to treat the replace and commit-transaction operations specially. If the PCMK_XE_CONFIGURATION element or any of its children was created, modified, moved, or deleted, then it will have the pcmk__xf_dirty flag set and/or be in the doc's deleted objects list. See is_config_change() in patchset.c. The "always write to disk for replace ops" logic began with commit 8d83bd9 (2010). We were firmly in the "legacy mode" era (pre-1.1.12) at that time. The change detection logic appeared to be quite different. Now we examine the v2 patchset to see if the config changed. That should find the ordering changes that we were concerned about. The writes_through flag was set for the commit-transaction operation when that operation was added (commit c252b7b). It seems that my reasoning at the time was "a commit-transaction operation is quite similar to a replace operation, since we replace the pre-transaction CIB with the post-transaction CIB on success. So we should set this flag for it too." Signed-off-by: Reid Wahl <[email protected]>
It's always true since 2c4737b. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
PCMK__CIB_REQUEST_COMMIT_TRANSACT/cib__op_commit_transact is now the only request type that has cib__op_attr_modifies but does not have cib__op_attr_transaction. There are no plans to change that, so we can drop cib__op_attr_transaction and check for that operation type directly. Signed-off-by: Reid Wahl <[email protected]>
Replace it with a boolean "modifies_cib". Signed-off-by: Reid Wahl <[email protected]>
Using include-what-you-use. Signed-off-by: Reid Wahl <[email protected]>
We return early if cib_discard_reply is set. Signed-off-by: Reid Wahl <[email protected]>
Use cib_api_operations_t:query() instead. A CIB query should always be processed locally. Except for brief intervals when changes are being processed, the CIB contents should be the same on all nodes within a cluster partition. As of this commit, a query is in fact always processed locally. The host argument of query_from is ignored. Signed-off-by: Reid Wahl <[email protected]>
parse_peer_options() is the only place where needs_reply can get set to true. If we're in stand-alone mode, we can't receive a message from a cluster peer. Signed-off-by: Reid Wahl <[email protected]>
This is intended to be equivalent to the existing code. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Send a reply even if the cib_discard_reply flag is set. The client will still discard the reply. (TODO: The client discards the reply for sync requests but still needs to discard it for async.) Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-based
branch
2 times, most recently
from
8fb0089 to
6659392
Compare
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.
No description provided.