Skip to content

Controller refactors: Clean up controld_execd_state.c and add Doxygen - #4159

Open
nrwahl2 wants to merge 54 commits into
ClusterLabs:mainfrom
nrwahl2:nrwahl2-controller_first
Open

Controller refactors: Clean up controld_execd_state.c and add Doxygen#4159
nrwahl2 wants to merge 54 commits into
ClusterLabs:mainfrom
nrwahl2:nrwahl2-controller_first

Conversation

@nrwahl2

@nrwahl2 nrwahl2 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

I've tried to avoid making big substantive changes here. This PR is about adding Doxygen, following best practices, and renaming functions to follow conventions (including moving away from "lrm" and toward "execd"). The idea is to make the logic easier to understand at a glance.

More to come. This covers all of controld_execd_state.c.

Incidentally, I understand why we wanted to break up controld_execd.c since it was (and is) so large -- but which functions are placed in controld_execd_state.c often seems pretty arbitrary. Maybe we can find a better way to organize all this code after we understand it better and untangle it somewhat.

nrwahl2 added 5 commits August 5, 2026 12:12
@nrwahl2
Doc: sysconfig: Drop G_SLICE from /etc/sysconfig/pacemaker boilerplate
583b044 
It's been ignored since GLib 2.76. We only require GLib >= 2.42
currently, but this option doesn't seem very useful anyway.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Document some GDestroyNotify funcs for execd
61d8065 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Doc: controller: Document lrm_state_t:active_ops
a29d03d 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Clean up fail_pending_op()
675d860 
* Add Doxygen.
* Add call_key variable.
* Drop hyphen from "Pre-emptively."
* Set fields in the order in which they're defined for
  lrmd_event_data_t.
* Use true instead of TRUE.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop lrm_state_is_local()
8d4bd38 
It doesn't seem to simplify things much.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
nrwahl2 requested a review from clumens August 5, 2026 19:18
@nrwahl2
nrwahl2 force-pushed the nrwahl2-controller_first branch from f64c53d to 82e7290 Compare August 5, 2026 19:32
nrwahl2
nrwahl2 commented Aug 5, 2026
View reviewed changes
Comment thread daemons/controld/controld_execd_state.c
} else {
/* Ignored for remote connections.
*
* @TODO Do we even need to set this in this function?

@nrwahl2 nrwahl2 Aug 5, 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.

I address this in a later commit (not in this PR). Short answer: no.

@nrwahl2

nrwahl2 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

RemoteBasic and RemoteMigrate have seg faulted for some reason. There are a few more tests still waiting to run.

@nrwahl2
nrwahl2 marked this pull request as draft August 5, 2026 20:56
@nrwahl2
nrwahl2 marked this pull request as ready for review August 5, 2026 21:03
@nrwahl2
nrwahl2 force-pushed the nrwahl2-controller_first branch from 82e7290 to 31e360e Compare August 5, 2026 21:03
nrwahl2 added 18 commits August 5, 2026 14:35
@nrwahl2
Refactor: controller: Drop lrm_state_is_connected()
6c68695 
It doesn't seem to simplify things much.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop lrm_state_poke_connection()
65739c1 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop lrm_state_disconnect()
d96c2eb 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Rename lrm_state_disconnect_only()
15b3016 
...to controld_execd_state_disconnect().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop lrm_state_reset_tables reset_metadata arg
c65d16a 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Rename active_op_t:user_data to transition_key
f5013bc 
This is the only thing it ever holds, so rename it to clarify intent. We
can't rename lrmd_event_data_t:user_data in a straightforward way,
because it's public API.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Return void from verify_stopped()
5f23bd5 
This should have been done in e79c5b3. Nothing checks the return value.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Expose cancel_op() within daemon
7e81d50 
Also rename it to controld_execd_cancel_op() and return bool instead of
gboolean. An upcoming commit will call it within controld_execd_state.c.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Move lrm_state_verify_stopped()
894bc13 
...and its helpers to controld_execd_state.c. This seems like the more
appropriate place.

For ease of review, no code changes in this commit; it only moves.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Best practices in stop_recurring_actions()
fc07458 
* Add Doxygen.
* Rename to cancel_recurring_op.
* Drop "remove" variable.
* Add a "call_key" variable.
* Use const where possible.
* Unindent recurring case.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Some best practices in is_rsc_active()
0972f27 
* Add Doxygen.
* Use bool instead of gboolean.
* Improve whitespace.
* Drop unnecessary elses.
* Compare op_type case-sensitively.
* Use a new "last" variable for readability.
* Use const where appropriate. (lrm_state->resource_history is passed as
  non-const to g_hash_table_lookup(), but it shouldn't get modified.
  Unfortunate GLib function signature.)

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Reorganize and document cases of is_rsc_active()
bc2aab3 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: lrm_state_verify_stopped() returns bool
ae84a30 
...instead of gboolean.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop rc variable from lrm_state_verify_stopped()
664fbc5 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Some lrm_state_verify_stopped() best practices
bff57bc 
* Use const where appropriate.
* Make counter an unsigned int and rename to count.
* Rename gIter and hIter to iter and iter2, respectively.
* Rename key to call_key for consistency with some other functions.
* Improve whitespace.
* Compare explicitly against 0.
* Use pcmk__plural_alt().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Merge two blocks of lrm_state_verify_stopped()
92a237d 
Both blocks require lrm_state->active_ops != NULL, and both of them do
nothing if the size of lrm_state->active_ops is zero.

We drop the nremaining variable because it's the original size minus the
number of items removed. Also, the condition
"(removed > 0) || (nremaining > 0)" is equivalent to the condition that
the original size is nonzero. So we drop that check when we add the
nonzero size check at the beginning of the block.

Clarify the notice message.

Use g_hash_table_foreach() with a new function count_non_recurring_op()
to count the non-recurring operations. For me, this is more readable.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: New log_pending_op()
c306dd4 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Don't NULL-check lrm_state_t hash tables
d31a9bd 
It shouldn't be necessary to assert non-NULL on these either. GLib does
its own equivalents of pcmk__assert() and CRM_CHECK().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2 added 27 commits August 5, 2026 14:35
@nrwahl2
Refactor: controller: Make lrm_state_t:node_name non-const
2d125d8 
We allocate and free it, so it should not be const.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of lrm_state_create()
b309252 
The sole caller always passes non-NULL for node_name.

Insert the newly allocated state object into the table in the caller, as
this seems clearer to me.

The reorderings are just to match the order in the struct definition.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Assert before deref in controld_execd_state.c
a6aefb3 
Do this for GDestroyNotifys, GHFuncs, and GHRFuncs as well. Usually we
don't NULL-check arguments of a static function with only one caller.
However, for these, the arguments are the items in a GHashTable. We
haven't NULL-checked them since they were inserted, so these assertions
act as sanity checks of the tables' contents.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of internal_lrm_state_destroy()
c7dde27 
The deleted comment is redundant due to a similar comment in
controld_remote_proxy_disconnect_node().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of lrm_state_reset_tables()
3385565 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Doc: controller: Doxygen for controld_execd_state_table_{init,free}()
11302ec 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of controld_get_executor_state()
83d8f83 
And rename it to controld_execd_state_get().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop NULL checks for mainloop_del_ipc_client()
94f54c2 
It NULL-checks the argument itself.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of controld_lrm_state_disconnect()
6b166a9 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of controld_connect_local_executor()
e1ffe70 
...and controld_connect_remote_executor().

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop lrm_state_get_metadata() options argument
8c6c1dd 
The sole caller passes 0.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Clean up lrm_state_get_metadata()
548e8e3 
Regarding the lrm_state argument being const: its conn field is passed
as non-const to the get_metadata_params() method. However, that method
completely ignores that argument.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop remote_ra_cancel() lrm_state argument
44087a3 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop two is_remote_lrmd_ra() arguments
ee36eaf 
They're always NULL except for one caller.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of lrm_state_cancel().
c7144b1 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop lrm_state_get_rsc_info() options argument
f7d3698 
All callers pass 0.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop remote_ra_get_rsc_info() lrm_state argument
cb1913b 
It's ignored.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of lrm_state_get_rsc_info()
164e097 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: liblrmd: New lrmd__key_value_add_from_hash()
771cc37 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of controld_execute_resource_agent()
1a7ab03 
The action vs. operation terminology in the updated Doxygen takes cues
from T781.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop lrm_state_register_rsc() options argument
35c6cee 
The sole caller passed lrmd_opt_drop_recurring.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: get_lrm_resource() returns standard code
aaff528 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of lrm_state_register_rsc()
d34cb70 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Drop lrm_state_unregister_rsc() options argument
20db97f 
The sole caller passed 0.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: notify_deleted() takes standard code for rc arg
540be18 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: delete_rsc_entry() takes standard code for rc arg
83cc0f7 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
Refactor: controller: Light cleanup of lrm_state_unregister_rsc()
4f2e241 
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
@nrwahl2
nrwahl2 force-pushed the nrwahl2-controller_first branch from 31e360e to 4f2e241 Compare August 5, 2026 21:35
@nrwahl2

nrwahl2 commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

At this point I'm getting mixed results on cts-lab. The four remote tests aren't failing, but sometimes one of them will hang, in an apparently random fashion. Not always, and not always the same one when it happens. I guess I'll have to test it a few times with main and see if I notice it happen there too. I encourage you to run cts-lab as well if you get around to it.

This is meant to be non-invasive, so I wonder if it's something pre-existing, or a problem with cts-lab, or a problem with my test bed.

I ran two batches of all four tests using this PR, and all ran fine. On the third batch, RemoteMigrate hung. I have seen RemoteStonithd hang sometimes in the past with this PR. When this happens, I don't see any obvious issues in the logs, nor any clear indication of what cts-lab is waiting for.

I reproduced it on main too. So I'm not particularly worried about this PR. I'll build Pacemaker-3.0.1 though, and see if I can reproduce it there.

@nrwahl2

nrwahl2 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

I reproduced it on main too. So I'm not particularly worried about this PR. I'll build Pacemaker-3.0.1 though, and see if I can reproduce it there.

I've done probably 30-40 individual test iterations on 3.0.1 and I haven't seen the issue. So this might be something recently introduced. And since it's pretty hard to reproduce, that displeases me greatly.

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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nrwahl2