Skip to content

based refactors: Use pcmk__request_t in the CIB manager#4154

Draft
nrwahl2 wants to merge 20 commits into
ClusterLabs:mainfrom
nrwahl2:nrwahl2-based_first
Draft

based refactors: Use pcmk__request_t in the CIB manager#4154
nrwahl2 wants to merge 20 commits into
ClusterLabs:mainfrom
nrwahl2:nrwahl2-based_first

Conversation

@nrwahl2

@nrwahl2 nrwahl2 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

This is the next batch from #4011.

@nrwahl2
Refactor: various: Drop cluster/internal.h includes from .c files
8867d48 
Our .c files include crm_internal.h, which includes
crm/cluster/internal.h. The explicit include makes sense for header
files, since we don't include crm_internal.h there, but not for .c
files.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
nrwahl2 requested a review from clumens July 23, 2026 00:41
@nrwahl2 nrwahl2 added the needs attention PRs that someone needs to look at label Jul 23, 2026
nrwahl2 added 8 commits July 22, 2026 18:12
@nrwahl2
Refactor: various: Don't include cluster/election_internal.h directly
6b63bca 
Include cluster/internal.h instead. Note that cluster/internal.h is
included by crm_internal.h.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: various: Drop common/internal.h includes from .c files
a16fb66 
Our .c files include crm_internal.h, which includes
crm/common/internal.h. The explicit include makes sense for header
files, since we don't include crm_internal.h there, but not for .c
files.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Clean up shutdown logic
7d016f5 
And rename based_terminate() to based_quit_main_loop().

The main loop processes events in batches. On each iteration, it checks
to see what sources are ready (that is, pending dispatch), and then it
dispatches them. It iterates over the list of ready sources and runs
each source's callback function, provided the source has not been
removed/destroyed.

When we call g_main_loop_quit(), we call it from within a source's
callback. g_main_loop_quit() ensures that no additional loop iterations
will occur. However, any sources that are already in the pending list
will still be dispatched.

This means that if we quit the main loop, we need to be prepared for any
pending source callbacks. This commit handles that by returning early
from functions that shouldn't do anything during shutdown, if we're
in the process of shutting down (that is, if we've called
g_main_loop_quit()).

This lets us quit based the same way regardless of the exit code and
consolidate cleanup at the end of main().

It also hopefully makes behavior more reliable when we quit the main
loop. Previously, if we were quitting with CRM_EX_OK (due to a SIGTERM),
we would free based_cib, cib_root, and possibly other important data
structures that we generally assume are non-NULL. If some function tried
to dereference those when processing pending sources, we might seg
fault.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: based_cib should always be non-NULL after init now
b0aa2d9 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: various: Unref main loop consistently
b2a68da 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: attrd: Drop mainloop_destroy_signal() from attrd_shutdown()
6dab86f 
mainloop_add_signal() adds mainloop_signal_handler() as a "wrapper"
handler for the given signal. If the signal is received,
mainloop_signal_handler() sets a boolean trigger value to true and
returns. This is a lightweight operation. The "wrapped" signal handler
gets called in a later main loop iteration.

However, this can't happen after we've called attrd_shutdown(). Either
we exit immediately or we quit the main loop. In either case, there will
be no additional main loop iterations.

Note that the controller is a bit different. There, we "drain" the main
loop, which may run more loop iterations. So it makes some sense to
destroy signal handlers before doing so.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: attrd: Assume main loop is running in attrd_shutdown()
dd2805c 
There are two call sites.
* attrd_cpg_destroy: This is called only via the main loop. See
  pcmk_cluster_set_destroy_fn() and pcmk__cpg_connect().

* attrd_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 attrd_shutdown(). That trigger can't do anything
  unless the main loop is running.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Don't init node caches explicitly
d828c19 
And assume the cache is already initialized when we call
based_peer_callback(). pcmk__corosync_connect() initializes the node
caches by calling pcmk__get_node().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
nrwahl2 force-pushed the nrwahl2-based_first branch from 34b8ecb to 4569cc4 Compare July 23, 2026 01:13
@nrwahl2
nrwahl2 marked this pull request as draft July 23, 2026 01:27
@nrwahl2

nrwahl2 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Election storm in cts-lab.

@nrwahl2 nrwahl2 added status: in progress PRs that aren't ready yet waiting for author Review has been provided, and we're waiting for a response from the author of the pull request and removed needs attention PRs that someone needs to look at labels Jul 23, 2026
@nrwahl2
nrwahl2 removed the request for review from clumens July 23, 2026 05:09
nrwahl2 added 11 commits July 22, 2026 23:18
@nrwahl2
Refactor: based: Drop based_peer_callback()
36c5ee7 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
API: libcib: Deprecate cib_query and cib_connected_query
cf8ff0e 
This doesn't meaningfully change behavior except in the following ways.
If you call the signon() method for a cib_native cib_t object, and you
pass cib_query for the type:
* The resulting cib_t:state value on success is cib_connected_command.
  Previously it was cib_connected_query.
* The resulting connection can be used for read/write requests.
  Previously it could only be used for read-only requests.

The cib_query (read-only) vs. cib_command (read/write) API was added
when the CIB API was first created, in commit 58fdec7 (2004). No
rationale was given.

All cib_file and cib_remote clients use cib_command unconditionally.
Their signon() methods ignore the type argument. Requests that the CIB
manager receives via the cluster layer (Corosync) are also given
read/write privileges. Read-only connections have been restricted to
cib_native clients that sign on with type=cib_query.

The asymmetry of cib_native clients vs. cib_remote clients doesn't seem
to make sense, and cib_query doesn't seem especially useful. One can
make an argument that it promotes safety/least-privilege by allowing a
client to ensure that it doesn't modify the CIB. However, this can be
achieved with some basic discipline, and it already wasn't available for
cib_remote connections.

Supporting read-only connections complicates our CIB manager
implementation substantially. We will now treat connections created with
cib_query the same as those created with cib_command, and this will
simplify things by a lot.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: remoted: Drop cib_proxy_accept_ro()
a75fff0 
As of the previous commit, no remote client can connect to the
PCMK__SERVER_BASED_RO channel. (cib_native clients now use
PCMK__SERVER_BASED_RW unconditionally.)

So replace the two separate cib_proxy_accept_*() functions with a single
based_proxy_accept().

Keep the two ipcs objects (based_ipcs_ro and based_ipcs_rw) for now,
since pcmk__serve_based_ipc() needs both. Their callbacks are the same,
however.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Check for old PCMK__SERVER_BASED_RW instance
9fc318d 
If there was a PCMK__SERVER_BASED_RO instance, there also should have
been a PCMK__SERVER_BASED_RW instance. We're moving toward using
PCMK__SERVER_BASED_RW everywhere, so use it here.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Make PCMK__SERVER_BASED_RO behave like _RW
cbf2751 
This affects only proxied connections from older Pacemaker Remote nodes.
IPC clients on cluster nodes and on same-version Pacemaker Remote nodes
use PCMK__SERVER_BASED_RW unconditionally (as of a recent commit that
updated cib_native_signon()).

This shouldn't cause any breakages or noticeable changes for existing
clients. The capabilities of PCMK__SERVER_BASED_RW were already a proper
superset of the capabilities of PCMK__SERVER_BASED_RO. That is, a client
connected to the PCMK__SERVER_BASED_RW channel could do anything that a
client connected to the PCMK__SERVER_BASED_RO channel can do, but not
vice-versa. So nothing that previously worked should break.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Drop privileged argument of based_process_request()
47f1a56 
We always pass true.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: libcib: Drop cib__op_attr_privileged
36095d3 
Nothing uses it anymore.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: liblrmd: Map proxied PCMK__SERVER_BASED_RO clients to _RW
8485ed7 
This happens only on the cluster node that is providing an IPC proxy for
a Pacemaker Remote client.

This way, we'll treat the client as being associated with
PCMK__SERVER_BASED_RW in every way.

We no longer have anything using PCMK__SERVER_BASED_RO except for this
function that maps it to PCMK__SERVER_BASED_RW, and pcmk__parse_server()
(which should no longer be receiving PCMK__SERVER_BASED_RO as an
argument).

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: libcrmcommon: pcmk__serve_based_ipc() starts only one server
a0687f1 
Now that there is no possibility of receiving an IPC client connection
(either local or proxied, whether in a rolling upgrade or not) with
server name set to PCMK__SERVER_BASED_RO, we don't need to serve
anything with that name anymore. _RO already behaved the same as _RW.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Standardize based_cpg_dispatch()
ac60e5c 
Make it look like fenced_cpg_dispatch() and attrd_cpg_dispatch(), and
have it call a based_peer_message() (structured similarly to
fenced_peer_message() and attrd_peer_message()). Not everything is in
accordance with my preferences here, but the goal is to make it look as
similar as possible to the other daemons.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Standardize based_ipc_dispatch()
f195534 
Make it look like fenced_ipc_dispatch() and attrd_ipc_dispatch(). Not
everything is in accordance with my preferences here, but the goal is to
make it look as similar as possible to the other daemons.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
nrwahl2 force-pushed the nrwahl2-based_first branch from 4569cc4 to f195534 Compare July 23, 2026 06:22
nrwahl2
nrwahl2 commented Jul 23, 2026
View reviewed changes
Comment thread daemons/controld/pacemaker-controld.c
@nrwahl2

nrwahl2 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Election storm in cts-lab.

Somehow this is being caused by the "Standardize based_ipc_dispatch()" commit, which is baffling to me at the moment. I guess I'll have to do that commit one line at a time until I find the problematic piece.

nrwahl2
nrwahl2 commented Jul 23, 2026
View reviewed changes
Comment thread daemons/based/based_ipc.c
@nrwahl2

nrwahl2 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Somehow this is being caused by the "Standardize based_ipc_dispatch()" commit, which is baffling to me at the moment. I guess I'll have to do that commit one line at a time until I find the problematic piece.

It's the client->name change:

     if (client->name == NULL) {
         const char *value = pcmk__xe_get(msg, PCMK__XA_CIB_CLIENTNAME);
 
-        if (value == NULL) {
-            client->name = pcmk__itoa(client->pid);
-        } else {
-            client->name = pcmk__str_copy(value);
-        }
+        client->name = pcmk__assert_asprintf("%s.%u", pcmk__s(value, "unknown"),
+                                             client->pid);
     }

This was copied from fenced_ipc.c. I think the assumption was that it was only used for logging. But based_ipc_dispatch() first sets client->name and then (re)sets PCMK__XA_CIB_CLIENTNAME to client->name. The immediate problem with this -- there may be others -- seems to be that attrd and controld pass PCMK__XA_CIB_CLIENTNAME to cib__client_triggers_refresh().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status: in progress PRs that aren't ready yet waiting for author Review has been provided, and we're waiting for a response from the author of the pull request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nrwahl2