Skip to content

Refactor: based, libcib: More refactors around based_process_request() and helpers#4122

Merged
clumens merged 28 commits into
ClusterLabs:mainfrom
nrwahl2:nrwahl2-based_first
Jun 24, 2026
Merged

Refactor: based, libcib: More refactors around based_process_request() and helpers#4122
clumens merged 28 commits into
ClusterLabs:mainfrom
nrwahl2:nrwahl2-based_first

Conversation

@nrwahl2

@nrwahl2 nrwahl2 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Next batch from #4011

@nrwahl2 nrwahl2 requested a review from clumens June 1, 2026 22:17
nrwahl2 added 16 commits June 13, 2026 13:22
@nrwahl2
Refactor: libcib: Avoid unnecessary copy in process_request()
5508f23 
If file_perform_op_delegate()'s output_data argument is NULL, then we
would just free the copy (output) as soon as we return from
process_request().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: libcib: Don't copy output that's part of the current CIB's doc
0dde2d4 
cib_file.c was already checking for this and would not free output if it
was part of the same doc as the current CIB. The CIB manager was
preserving output only if it was equal to the current CIB. So we just
have to update the CIB manager to compare the docs, and update
cib__perform_op_ro() to skip the copy if the docs are the same.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: libcib: Minor cib__perform_op_ro() cleanup
8741415 
And drop an unhelpful trace message in cib__perform_op_rw().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: based_process_request gets last create_cib_reply check
60ee563 
The other two were already in based_process_request().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Pull read-only ops out of cib_process_command()
325a54e 
And rename to based_perform_op_rw().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Low: based: Free digest_timer on exit
bf32c20 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Move notify to done section in based_perform_op_rw()
a9b8261 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Unindent successful common case in based_perform_op_rw
8f9ae34 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Drop dead output log msg for pcmk_rc_schema_validation
9182ac2 
"Create" is the only read-write operation that may set *output. There is
no path by which a create operation can return
pcmk_rc_schema_validation.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Fix: based: Fix handling of intermediate transaction results
dee9558 
If a request within a transaction succeeds, we don't want to discard the
result CIB. We need it for the next request in the transaction, or for
the end result if this is the last request to process. So we want to
activate it.

However, we do NOT want to write it to disk. If the system or CIB
manager crashes while committing a transaction, and we restore an
intermediate result, the transaction was not atomic.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Log: based: Drop redundant log in based_process_request()
82bda7e 
We get the same information from log_op_result() and do_local_notify().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Unindent "if (process)" block of based_process_request
bf82d34 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Don't call option parser when cib_transaction is set
3635963 
We make this change despite the previous comment saying we wanted log
messages and the target check. Some of the messages are misleading or
confusing in the context of a within-transaction request, and skipping
that call will facilitate some other changes.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Drop needs_forward variable
69484db 
Pull the check inline. If it's true, then we can call forward_request()
and return immediately.

We can also drop the trace message, because we have one in
forward_request().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Log: based: Drop log message from forward_request()
e66d10b 
It doesn't seem very helpful. We know that if we receive a request from
a local client and it's either a modifying request or addressed to
another node (or to all nodes), we'll forward it.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Doc: based: Drop comment in parse_peer_options()
77579ee 
I really don't think it's relevant now, though I could be wrong.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2 nrwahl2 force-pushed the nrwahl2-based_first branch from 40551d8 to 62ea742 Compare June 13, 2026 20:27
@nrwahl2

nrwahl2 commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on main to resolve conflict.

clumens
clumens reviewed Jun 16, 2026
View reviewed changes
Comment thread daemons/based/based_callbacks.c

} else if (!parse_peer_options(operation, request, &local_notify,
&needs_reply, &process)) {
return pcmk_rc_ok;

@clumens clumens Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch makes me a little bit nervous, though without really digging into the based code I don't have any specific reason for it. The call to process_ping_reply hiding in parse_peer_options certainly doesn't help matters.

@nrwahl2 nrwahl2 Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch makes me a little bit nervous, though without really digging into the based code I don't have any specific reason for it.

All we really need to worry about is checking what the values of these booleans are before calling parse_peer_options(), and making sure this preserves behavior. They all have default values of either true or false. There aren't that many changes to look at. I'll break it up to try to make it clearer though.

@nrwahl2 nrwahl2 Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to process_ping_reply hiding in parse_peer_actions certainly doesn't help matters.

Agreed. This will go away in 3fa5f4e.

Comment thread lib/cib/cib_client.c Outdated
Comment thread daemons/based/based_callbacks.c

if (pcmk__str_eq(host, OUR_NODENAME, pcmk__str_casei)) {
pcmk__trace("Processing %s request sent to us from %s", op, originator);
*needs_reply = true;

@clumens clumens Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like one of these two needs_reply lines still needs to be here.

@nrwahl2 nrwahl2 Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. I think I address that a few lines later with the early return true followed by *needs_reply = false. I'm not sure why I have return true instead of logging the trace message though.

The upcoming push has each of the three variables in its own commit. So hopefully that will make these changes easier to review.

@clumens clumens added the review: in progress PRs that are currently being reviewed label Jun 16, 2026
nrwahl2 added 9 commits June 16, 2026 14:37
@nrwahl2
API: libcib: Deprecate cib_api_operations_t:sync
5ddda63 
Nothing uses it internally. sync_from is still available.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Minor easy changes to parse_peer_options()
c4bb777 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Move two parse_peer_options() early returns upward
a5dbc1b 
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>
@nrwahl2
Refactor: based: Drop legacy code from parse_peer_options()
c7e9b5d 
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>
@nrwahl2
Refactor: based: Move is_reply block to where it's used
7e62c9c 
This is an incremental change; more is coming. Yes, this introduces
duplication.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Drop redundant check for upgrade RC
2bb56da 
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>
@nrwahl2
Refactor: based: Move early return to beginning of upgrade block
f6b0367 
...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>
@nrwahl2
Refactor: based: Drop skip_is_reply label
8c70c31 
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>
@nrwahl2
Refactor: based: parse_local_options() -> log_local_options()
2484d2e 
It doesn't really do anything now.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2 added 3 commits June 17, 2026 00:10
@nrwahl2
Refactor: based: Set process variable only if it overrides default
6d161f2 
The process variable is set to true as a default value in
based_process_request(). In a couple of places, we explicitly set it to
true again. This was likely to clarify the end state, since the parsing
is rather convoluted. However, removing these redundant sets is an
incremental step toward simplifying the option parsing.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Set local_notify variable only if it overrides default
b70ef68 
The local_notify variable is set to false as a default value in
based_process_request(). In a couple of places, we explicitly set it to
false again. This was likely to clarify the end state, since the parsing
is rather convoluted. However, removing these redundant sets is an
incremental step toward simplifying the option parsing.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: based: Set needs_reply variable only if it overrides default
11565df 
The needs_reply variable is set to true as a default value in
based_process_request(). In a couple of places, we explicitly set it to
true again. This was likely to clarify the end state, since the parsing
is rather convoluted. However, removing these redundant sets is an
incremental step toward simplifying the option parsing.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2 nrwahl2 force-pushed the nrwahl2-based_first branch from 62ea742 to 11565df Compare June 17, 2026 07:29
@clumens clumens merged commit 6e29c64 into ClusterLabs:main Jun 24, 2026
1 check passed
@nrwahl2 nrwahl2 deleted the nrwahl2-based_first branch June 24, 2026 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clumens clumens clumens approved these changes

Assignees

No one assigned

Labels

review: in progress PRs that are currently being reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nrwahl2 @clumens