Introduce a pcmk__daemon_t object and use it for mainloop/shutdown - #4157
Open
clumens wants to merge 62 commits into
Open
Introduce a pcmk__daemon_t object and use it for mainloop/shutdown#4157clumens wants to merge 62 commits into
clumens wants to merge 62 commits into
Conversation
* Protect everything in the block against being given a NULL, which allows getting rid of the initialized variable. * Rearrange things so similar functions are next to each other.
We traditionally haven't used atexit in pacemaker, but I don't think there's a good reason for that and it can definitely be handy. Here, I'm using it to clean up command line processing stuff which I think is a great use and should be duplicated throughout. There's a couple rules I want to set down for its use: * For now, this should only be used for cleaning up command line processing. This should include any variables allocated in the GOptionEntry struct. None of this stuff gets cleaned up otherwise because the glib argument processing calls exit() after printing the help (see T1019). * It's not possible to use formatted output in the atexit handler because that will have been freed already. It's also not possible to do anything with a mainloop, logging, or XML because crm_exit -> pcmk_common_cleanup will have been called. * Because there's no explicit call to the atexit handler (it's registered, and then just happens eventually), we need to keep it simple to prevent confusion. One additional reason I want to do this in the daemons in particular is that it's possible for there to be multiple exit paths in the daemon code (for instance, attrd_cib_destroy_cb -> attrd_shutdown calls crm_exit) and without the atexit handler, this memory will never be freed.
We can't clean anything up until after the main loop has exited, since there might still be pending sources to be dispatched.
For the moment, this is just for organizational purposes. Once I introduce the pcmk__daemon_t object, this may become a function on that object instead. I haven't decided yet.
attrd_quit_main_loop just kills the main loop and sets the exit code. It can then be be called from anywhere that was previously calling attrd_shutdown + crm_exit. In particular, attrd_cib_destroy_cb and attrd_cpg_destroy can use this new function instead. Despite how it may initially seem, these functions are main loop sources, so calling attrd_quit_main_loop in them will cause us to return back to the done label in main() once they finish. attrd_shutdown is essentially just a wrapper at this point to provide a function with the correct type to pass to mainloop_add_signal. This means that - aside from pcmk__serve_attrd_ipc - there should be no other places in attrd that directly call crm_exit. All exit paths flow through terminating the main loop, to the done label in main(), and through the cleanup functions there. There are, of course, plenty of places in the pacemaker libraries that call crm_exit, but that's a much bigger project.
It's not used outside of the main file anymore.
This will eventually become an object that controls all the mainloop, IPC, and CPG aspects of a daemon. For the moment, all it does is handles the mainloop of the attribute daemon and not much of that either. The shutdown side is going to take some more work. Not all daemons are ready to be converted to use this object. At the moment, it's limited to attrd, execd, fenced, pacemakerd, and schedulerd. I think I'm going to take the approach of converting each daemon to use the mainloop support before adding anything else to the object. One thing to note about this is that I'm using pcmk_ipc_server as the identifier for the server type. We don't have any other server type enum that I can find, and all servers connect to IPC so this is probably good enough. This is all internal API, so it's easy to change this type later.
clumens
force-pushed
the
daemon-object
branch
2 times, most recently
from
July 28, 2026 21:01
d3a0b11 to
fd94b13
Compare
This will eventually become an object that controls all the mainloop, IPC, and CPG aspects of a daemon. For the moment, all it does is handles the mainloop of the attribute daemon and not much of that either. The shutdown side is going to take some more work. Not all daemons are ready to be converted to use this object. At the moment, it's limited to attrd, execd, fenced, pacemakerd, and schedulerd. I think I'm going to take the approach of converting each daemon to use the mainloop support before adding anything else to the object. One thing to note about this is that I'm using pcmk_ipc_server as the identifier for the server type. We don't have any other server type enum that I can find, and all servers connect to IPC so this is probably good enough. This is all internal API, so it's easy to change this type later.
This is useful for starting up a daemon where it doesn't connect to the CIB or any other daemons, which is handy for running unit tests (for instance, cts-attrd). Not all daemons support this, but several do and it's arguable that all should eventually.
At the moment this isn't used anywhere outside of the daemon itself, but that will change.
Basically, move everything that's in attrd_quit_main_loop into pcmk__daemon_quit and get rid of the former function. This makes it more useful in other daemons in the future. I wanted to move the mainloop_add_signal(SIGTERM, ...) function into the more generic daemon code as well, but that's not really possible. In order to do that, we need a function with the right type to pass as the callback. At the moment, attrd_shutdown fills that role. Anything I can think of to do just involves rewriting that same function somewhere else. I might as well leave it alone.
This is intended to be a function that all daemons can use to determine if a previous instance is running before continuing to start up. At the moment, however, options to use it are fairly limited: * based, controld, execd, and fenced have not been converted to use the pcmk_ipc_api_t client interface that this function requires. For that matter, I don't even see where execd checks for a previous instance using the old interface either. * pacemakerd does some very different and daemon-specific stuff if it finds a previous instance, so it's not really suitable for conversion. * schedulerd doesn't check for a previous instance, which seems like an oversight to me. However, maybe there's a good reason for it?
This is all part of the plan to reduce the number of places crm_exit is called in the daemons so all shutdown goes through the same place. It turns out there's an awful lot of places a daemon can shut down. Here's one more. Instead, attrd_ipc_init -> pcmk__serve_attrd_ipc should both return an error code on failure and then the caller can decide what to do. I've left the error printing where it is so it doesn't have to be duplicated. And while I'm nearby, don't call crm_exit from ipc_proxy_init. Setting the exit code in ipc_proxy_init seems a little pointless at the moment, but it will be useful much later on.
See a previous commit to attrd for an explanation of this commit. We're just doing the same thing in execd that is now being done in attrd.
The real point of this commit is to make sure that lrmd_drain_alerts can be called even if mloop is NULL but while I'm here, I might as well fully unindent both of these functions.
This is just to make the shutdown paths a little bit easier to follow so it's easier to simplify them later.
For the moment, it does still exit as well. However, a future commit will change it to be more like attrd (where we have a cleanup function and a shutdown function), and renaming it in a separate step makes things a little clearer later.
Shutting down in execd is complicated by the fact that it's really two programs in one - pacemaker-execd, and pacemaker_remoted. Thus, it seems easiest to switch the shutdown process over to being focused on quitting the main loop one piece at a time, instead of in a single patch like we could with attrd. This patch handles the pacemaker-execd case, switching it to using lrmd_shutdown to catch the SIGTERM signal, which calls execd_quit_main_loop to kill the main loop, which then falls through to execd_cleanup to free stuff and then continue through the rest of the done label.
The only case not covered here is handle_shutdown_nack, which has a complicated enough explanation that I'm going to put it into its own patch. To keep things working, for the moment, I'm just calling crm_exit there. For the rest of the cases, no longer call crm_exit at the end of execd_cleanup and instead call execd_quit_main_loop which ensures we take the same paths as in the non-remote case. lrmd_client_destroy is slightly more complicated, but it's called from functions that are mainloop sources so it should be fine.
The idea here is that all shutdown paths proceed back to the done label in main() so we can do the same cleanups in every case. This is complicated in handle_shutdown_nack since we're so far down in the call stack and there's a bunch of callbacks involved. Here's exactly what's happening: * handle_shutdown_nack returns ESHUTDOWN to its caller... * ipc_proxy_forward_client. All we need to do here is pass the return code on up the call stack to... * handle_ipc_fwd_request. We see the special ESHUTDOWN return code and return a NULL reply. handle_ipc_fwd_request is a part of the execd_handlers struct and is called as a function pointer so next... * We return to pcmk__process_request and then to execd_handle_request. We don't do anything here because we have a NULL reply. Next we return to... * We return to either lrmd_remote_client_msg or execd_ipc_dispatch. Both of these functions end up being called from the main loop (the latter is much harder to follow because it passes through libqb, but the key in understanding this is knowing that we add GIO sources to the main loop for the libqb callbacks). Thus, once we finally return from these functions, we'll be back in the main loop. That will lead us back to after g_main_loop_run is called, which puts us at the done label main(), where we call execd_cleanup just like everywhere else.
And then add execd_shutdown as the signal handler, paralleling what's going on in attrd.
At the moment, we're not even calling pcmk__daemon_init on it, which should be fine because we're not using it for anything else. This is just to make it easier to follow the patches that convert execd.
At the moment this is entirely optional - a daemon can choose to implement it, or not. If implemented, the idea is that it can perform certain shutdown tasks (nothing that cleans up, though) and decides whether or not to continue with shutdown. This is needed for execd.
Because shutting down execd is a little more complicated due to it being two programs in one, we need to use the new quit() function on a pcmk__daemon_t object to help decide whether or not to continue with shutdown.
…akerd. See a previous commit to attrd for an explanation of this commit. We're just doing the same thing in fenced that is now being done in attrd.
Instead, return a value and use that to decide what to do. In general, we want to limit the number of places crm_exit is called from in the daemons.
* Rename to pcmkd_read_config, which is more consistent with other functions internal to this daemon. * Return bool instead of gboolean. * Unindent the do-while loop at the top of the function to make it easier to follow. * Make the call to crm_ipc_is_authentic_process easier to follow. * Minor cleanups at the call sites.
This daemon doesn't do a lot of cleanup, but what it does should go into its own function just like all the other daemons.
This is just so that as part of my ongoing war on crm_exit in daemons, I remember that it's okay to call it in this spot. We've fork()ed a child process, and so if starting execlp() fails, we want to exit instead of letting the child return and run whatever would happen next.
The only interesting thing happening here is the end of pcmk_shutdown_worker. It's okay to change the return value here because there's no way to reach the end of the function without killing the main loop.
At the moment, we're not even calling pcmk__daemon_init on it, which should be fine because we're not using it for anything else. This is just to make it easier to follow the patches that convert pacemakerd.
…ulerd. See a previous commit to attrd for an explanation of this commit. We're just doing the same thing in fenced that is now being done in attrd.
This is more in line with what's happening in other daemons. Surprisingly, there's very little to clean up in this one.
* Move everything out of pengine_shutdown and into the bottom of the main function, just like in other daemons. * Rename pengine_shutdown to schedulerd_shutdown and just have it kill the main loop.
At the moment, we're not even calling pcmk__daemon_init on it, which should be fine because we're not using it for anything else. This is just to make it easier to follow the patches that convert schedulerd.
I'm not working on converting based over to using pcmk__daemon_t at the moment, but this can be called from execd which means there's still a path for execd to call crm_exit. So just do the bare minimum to ensure that doesn't happen without touching too much in based.
For some reason, there was no check for a previous instance of schedulerd. This is easy to add since it already supports pcmk_ipc_api_t, so I might as well. It brings the daemons further in line with each other too.
Contributor
Author
|
Okay, that should be enough pushes for now. The coverity thing is fixed up, as are some (sigh) claude review findings I ran just to see what would happen. |
nrwahl2
requested changes
Jul 30, 2026
nrwahl2
left a comment
nrwahl2
left a comment
Contributor
There was a problem hiding this comment.
Good work. I've only made it through the "Introduce the pcmk__daemon_t object" commit, though I have peeked at the final product to try to make sure my comments remain accurate.
| static gboolean stand_alone = false; | ||
| gchar **log_files = NULL; | ||
|
|
||
| static gchar **log_files = NULL; |
Contributor
There was a problem hiding this comment.
I'm not opposed to atexit. But I also kind of like the idea of everything returning to main(). And it looks like commits after this one are making strides in that direction.
It could be worth adding your rules to Pacemaker Development.
This should include any variables allocated in the GOptionEntry struct.
I know what you mean, but we don't allocate any variables in the GOptionEntry struct.
We may want to add something like "only one atexit handler." But the rule of "only use it for command line processing cleanup for now" makes that redundant. And there may be some use for other, per-file atexit handlers in the future (like for cleaning up library state for libcrmcommon or even replacing crm_exit()).
I would've sworn there was some type of command line argument that resulted in us fast-exiting without freeing anything and was going to be annoying to rework so that things got freed. I thought it was --version. But it looks like out->version() doesn't exit, and then we goto done. I was going to say, whatever that was, we could mention it in the commit message as a good reason for atexit. (Edit: Oh... the commit message talks about GLib exiting after printing help. That's probably what I was thinking of. But I would rephrase "None of this stuff gets cleaned up otherwise because..." -- this applies only for <command> --help I think.)
I could see double-frees becoming a point of confusion, especially if we start using atexit for more things -- freeing something explicitly before exit, when it will also be freed by the atexit handler.
I dislike having to add more global variables (even file-scope) for this, but that's common for callbacks and such. It's unfortunate that C doesn't allow functions to create and return new functions -- nice as a way to insert data into functions for use in future calls.
| return); | ||
|
|
||
| g_main_loop_quit(d->mainloop); | ||
| g_main_loop_unref(d->mainloop); |
Contributor
There was a problem hiding this comment.
Again I think this unref should either go in pcmk__daemon_run() or its callers. Maybe a pcmk__daemon_free() or pcmk__daemon_cleanup() wrapper if we want something symmetrical with pcmk__daemon_init().
I feel like quit() is the counterpart of run(), while free() or cleanup() would be the counterpart of init().
Contributor
Author
There was a problem hiding this comment.
Again I think this unref should either go in
pcmk__daemon_run()or its callers.
This is on my list for post-#4154 as well.
Maybe a
pcmk__daemon_free()orpcmk__daemon_cleanup()wrapper if we want something symmetrical withpcmk__daemon_init().I feel like
quit()is the counterpart ofrun(), whilefree()orcleanup()would be the counterpart ofinit().
pcmk__daemon_free doesn't make a lot of sense to me since we don't allocate them (which, we could switch to that but I kind of like just declaring the thing outright and then always having it available). pcmk__daemon_cleanup sounds better. There's just... a lot of cleanup functions involved here. I kind of dread having to make more.
Though, every daemon does have a daemon_cleanup() function it calls at the done label. That could make sense as a new daemon-specific function pointer.
| goto done; | ||
| } | ||
|
|
||
| rc = pcmk__daemon_init(&attrd); |
Contributor
There was a problem hiding this comment.
I feel like -- especially since returning a failure code is impossible -- we should move the pcmk__daemon_init() call and the mainloop_add_signal() call to right before the pcmk__daemon_run() call.
nrwahl2
reviewed
Jul 30, 2026
nrwahl2
reviewed
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the beginnings of condensing all the IPC/CPG code across daemons. I haven't touched based (given #4011) or controld (given we haven't started on that), but all other daemons are addressed. This will need some modification in light of #4154, but that doesn't prevent the rest from being reviewed.