Skip to content

based refactors: Use pcmk__request_t in the CIB manager - #4154

Open
nrwahl2 wants to merge 37 commits into
ClusterLabs:mainfrom
nrwahl2:nrwahl2-based_first
Open

based refactors: Use pcmk__request_t in the CIB manager#4154
nrwahl2 wants to merge 37 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 4 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
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
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 Outdated
@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().

nrwahl2 added 11 commits July 23, 2026 11:07
@nrwahl2
Refactor: controller: Don't set mainloop to NULL in crmd_exit()
56f7ded 
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>
@nrwahl2
Refactor: various: Unref main loop consistently
5a22496 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: attrd: Drop mainloop_destroy_signal() from attrd_shutdown()
05166f4 
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()
536e4b9 
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
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>
@nrwahl2
Refactor: based: Drop based_peer_callback()
19a6288 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
API: libcib: Deprecate cib_query and cib_connected_query
6d0e08c 
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()
dc7089b 
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
7be7e4f 
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
c17d49f 
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()
60391d9 
We always pass true.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2 added 12 commits July 23, 2026 15:08
@nrwahl2
Refactor: libcib: Drop unnecessary rc check in cib_native_signon()
2652d38 
It's initialized to pcmk_ok, and nothing can change it before we reach
this point. (That was not the case when this line was added.)

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: libcib: Unindent some of cib_native_signon()
95611f7 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: libcib: Unindent more of cib_native_signon()
6279154 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: libcib: Unindent one more block of cib_native_signon()
1b431b7 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: libcib: Compare CRM_OP_REGISTER case-sensitively
88c713b 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Feature: libcib: Ignore name argument of cib_api_operations_t:signon()
e8ceff8 
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>
@nrwahl2
Feature: libcib: Don't set cib_t:type
b63210d 
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>
@nrwahl2
API: libcib: Deprecate cib_no_connection
cd3ed6e 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Feature: libcib: Ignore type argument of signon method for native client
f4c78cc 
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>
@nrwahl2
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>
@nrwahl2
Refactor: libcib: Clean up the end of cib_native_signon()
97fb8b3 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
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
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
@nrwahl2
Refactor: various: Use pcmk__is_set() and friends in more places
2147d3f 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
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>
@nrwahl2
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
Refactor: based: Standardize based_ipc_dispatch()
fc7061d 
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
nrwahl2 force-pushed the nrwahl2-based_first branch from 3d74074 to fc7061d Compare July 24, 2026 09:25
@nrwahl2
nrwahl2 requested a review from clumens July 24, 2026 09:34
@nrwahl2
nrwahl2 marked this pull request as ready for review July 24, 2026 09:34
@nrwahl2

nrwahl2 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

retest this please

@nrwahl2

nrwahl2 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

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

@nrwahl2

nrwahl2 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

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

@nrwahl2
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 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
@nrwahl2

nrwahl2 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

retest this please

@clumens clumens mentioned this pull request Jul 28, 2026
Open
@nrwahl2

nrwahl2 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

retest this please

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

Reviewers

@clumens clumens Awaiting requested review from clumens

Assignees

No one assigned

Labels

needs attention PRs that someone needs to look at

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nrwahl2