based refactors: Use pcmk__request_t in the CIB manager by nrwahl2 · Pull Request #4154 · ClusterLabs/pacemaker

nrwahl2 · 2026-07-23T00:41:03Z

This is the next batch from #4011.

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>

Include cluster/internal.h instead. Note that cluster/internal.h is included by crm_internal.h. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

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>

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>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 · 2026-07-23T01:27:16Z

Election storm in cts-lab.

nrwahl2 · 2026-07-23T12:19:38Z

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 · 2026-07-23T12:57:04Z

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().

The comment above the assignment says "Don't re-enter this block." However, that's already impossible thanks to the in_progress static variable. If it's true, we exit at the beginning of the function. If it's false, we set it to true after checking it. That in_progress logic was not present as of commit 0f9b4c1, when the "Don't re-enter" comment was added. Interestingly though, the in_progress logic was added one hour after the "Don't re-enter" and NULL assignment, but the latter were never removed. See commit 716d9d7. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

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>

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>

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>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

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>

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>

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>

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>

We always pass true. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Instead, always use crm_system_name if set, or fall back to "client" otherwise. All internal callers pass crm_system_name. Further, crm_system_name should always be non-NULL if crm_log_preinit() has been called -- which is always true for internal callers. Up to now, callers have been allowed to pass an arbitrary client name. However, this does not seem to be useful for anything except logging, and certain names may cause Pacemaker daemons to treat requests from the client specially. It seems safer to use crm_system_name unconditionally (falling back to "client" if NULL). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Nothing checks it internally, and there's no apparent use case for anything external to check it. Further, cib_native clients already don't set it at all during signon. cib_file and cib_remote clients set it during signon; all clients set it back to cib_no_connection at signoff. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

cib_file and cib_remote clients already ignored the type argument. cib_native clients simply returned an ENOTCONN error if the type was invalid; otherwise, they ignored it. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

cib_command is the only type we ever use, so this enum is now meaningless. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Just for future-proofing and consistency. This doesn't fix a bug. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

We don't use these attrs when processing CRM_OP_REGISTER and PCMK__VALUE_CIB_NOTIFY. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

And set client->name only when dispatching a register request. In practice, this means that a client must call the signon() method before any other methods. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Make it look more 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 · 2026-07-24T09:34:58Z

retest this please

nrwahl2 · 2026-07-24T09:35:25Z

Running cts-lab again now. No election storm. We'll see what happens...

nrwahl2 · 2026-07-24T16:48:41Z

@clumens A cts-lab run of 50 iterations passed with no BadNews. The CI failures are unrelated -- 404 errors for repos.

Should have been done as part of a6bf06d. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 · 2026-07-24T23:10:53Z

retest this please

nrwahl2 · 2026-07-30T00:08:46Z

retest this please

clumens · 2026-08-07T13:27:36Z

        goto done;
    }

-    pcmk__cluster_init_node_caches();


If I'm following this correctly, node caches are created by based_cluster_connect -> pcmk_cluster_connect -> pcmk__corosync_connect -> pcmk__get_node. This chain won't happen in standalone mode. Does standalone mode require node caches for any reason?

clumens · 2026-08-07T13:55:13Z

-            // @COMPAT cib_command_nonblocking is deprecated since 3.0.2
+        case cib_query:
+            /* @COMPAT cib_command_nonblocking and cib_query are deprecated
+             * since 3.0.2


cib_query will actually be deprecated as of whatever release comes after 3.0.3.

clumens · 2026-08-07T14:15:16Z

            r"getinfo response error: 1$",
            r"sbd.* error: inquisitor_child: DEBUG MODE IS ACTIVE",
-            r"sbd.* pcmk:\s*error:.*Connection to cib_ro.* (failed|closed)",
+            r"sbd.* pcmk:\s*error:.*Connection to cib_rw.* (failed|closed)",


I think this commit pairs pretty well with https://projects.clusterlabs.org/T339. We can achieve the same thing, more comprehensively, by applying access controls at a different level. We'd just have to find time for it.

clumens · 2026-08-07T14:36:31Z

    }

+    /* @COMPAT Proxied clients from Pacemaker Remote nodes older than version
+     * 3.0.2 can connect using PCMK__SERVER_BASED_RO. Since we use


This should be whatever version comes after 3.0.3.

clumens · 2026-08-07T15:08:12Z

+     * \brief Sign on a client to the CIB API
+     *
+     * \param[in,out] cib   CIB connection (client)
+     * \param[in]     name  Ignored


I haven't traced it out, but will this patch have any consequences for 40cae9e ?

clumens · 2026-08-07T15:34:55Z

    }
-    if ((target_stat->st_mode & (S_IRUSR | S_IWUSR)) == 0) {
+
+    if (!pcmk__any_flags_set(target_stat->st_mode, S_IRUSR|S_IWUSR)) {


This should be pcmk__all_flags_set, shouldn't it?

I don't see why it should. The old code says "if none of the flags in S_IRUSR | S_IWUSR is set in target_stat->st_mode, then ...". That's the same as "if it's false that any of the flags in S_IRUSR|S_IWUSR is set in target_stat->st_mode, then ...".

Am I overlooking or misreading anything?

clumens · 2026-08-07T15:35:02Z

    }

-    if ((target_stat->st_mode & (S_IRGRP | S_IWGRP)) == 0) {
+    if (!pcmk__any_flags_set(target_stat->st_mode, S_IRGRP|S_IWGRP)) {


clumens · 2026-08-07T15:40:22Z

-    CRM_LOG_ASSERT(client->user != NULL);
-    pcmk__update_acl_user(msg, PCMK__XA_CIB_USER, client->user);
-
    pcmk__log_xml_trace(msg, "ipc-request");


Move this log line along with the rest of the code so it'll reflect the other attributes we set.

nrwahl2 requested a review from clumens July 23, 2026 00:41

nrwahl2 added the needs attention PRs that someone needs to look at label Jul 23, 2026

nrwahl2 added 4 commits July 22, 2026 18:12

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>

Refactor: based: based_cib should always be non-NULL after init now

b0aa2d9

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 force-pushed the nrwahl2-based_first branch from 34b8ecb to 4569cc4 Compare July 23, 2026 01:13

nrwahl2 marked this pull request as draft July 23, 2026 01:27

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 removed the request for review from clumens July 23, 2026 05:09

nrwahl2 force-pushed the nrwahl2-based_first branch from 4569cc4 to f195534 Compare July 23, 2026 06:22

nrwahl2 commented Jul 23, 2026

View reviewed changes

Comment thread daemons/controld/pacemaker-controld.c

nrwahl2 commented Jul 23, 2026

View reviewed changes

Comment thread daemons/based/based_ipc.c Outdated

nrwahl2 added 11 commits July 23, 2026 11:07

Refactor: various: Unref main loop consistently

5a22496

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: based: Don't init node caches explicitly

cc1ab8a

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>

Refactor: based: Drop based_peer_callback()

19a6288

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: based: Drop privileged argument of based_process_request()

60391d9

We always pass true. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 added 9 commits July 23, 2026 16:07

Refactor: libcib: Unindent one more block of cib_native_signon()

1b431b7

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcib: Compare CRM_OP_REGISTER case-sensitively

88c713b

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

API: libcib: Deprecate cib_no_connection

cd3ed6e

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

API: libcib: Deprecate enum cib_conn_type

b391967

cib_command is the only type we ever use, so this enum is now meaningless. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcib: Clean up the end of cib_native_signon()

97fb8b3

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: based: goto cleanup consistently in based_ipc_dispatch()

adbf9fd

Just for future-proofing and consistency. This doesn't fix a bug. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 force-pushed the nrwahl2-based_first branch from f195534 to 3d74074 Compare July 24, 2026 09:07

nrwahl2 added 4 commits July 24, 2026 02:25

Refactor: various: Use pcmk__is_set() and friends in more places

2147d3f

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: based: Add extra XML attrs only before based_process_request()

e8c609b

We don't use these attrs when processing CRM_OP_REGISTER and PCMK__VALUE_CIB_NOTIFY. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Low: based: Ignore requests from unregistered clients

5a4a4ef

And set client->name only when dispatching a register request. In practice, this means that a client must call the signon() method before any other methods. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 force-pushed the nrwahl2-based_first branch from 3d74074 to fc7061d Compare July 24, 2026 09:25

nrwahl2 requested a review from clumens July 24, 2026 09:34

nrwahl2 marked this pull request as ready for review July 24, 2026 09:34

Refactor: libcrmcommon: Assert non-NULL args in pcmk__serve_execd_ipc()

e985f33

Should have been done as part of a6bf06d. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 added needs attention PRs that someone needs to look at and removed 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 labels Jul 24, 2026

clumens mentioned this pull request Jul 28, 2026

Introduce a pcmk__daemon_t object and use it for mainloop/shutdown #4157

Open

clumens added review: in progress PRs that are currently being reviewed and removed needs attention PRs that someone needs to look at labels Aug 6, 2026

clumens reviewed Aug 7, 2026

View reviewed changes

Uh oh!

Conversation

nrwahl2 commented Jul 23, 2026

Uh oh!

nrwahl2 commented Jul 23, 2026

Uh oh!

Uh oh!

nrwahl2 commented Jul 23, 2026

Uh oh!

Uh oh!

nrwahl2 commented Jul 23, 2026

Uh oh!

nrwahl2 commented Jul 24, 2026

Uh oh!

nrwahl2 commented Jul 24, 2026

Uh oh!

nrwahl2 commented Jul 24, 2026

Uh oh!

nrwahl2 commented Jul 24, 2026

Uh oh!

nrwahl2 commented Jul 30, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants