Convert execd to use pcmk__request_t #3915
Conversation
clumens
force-pushed
the
execd-request_t
branch
4 times, most recently
from
1b3051b to
6b0ff95
Compare
daemons/execd/execd_messages.c
Outdated
|
|
||
| if (!c->name) { | ||
| const char *value = crm_element_value(msg, | ||
| PCMK__XA_LRMD_CLIENTNAME); |
There was a problem hiding this comment.
I don't see why the attribute would ever be missing, so I
think it's safe to get rid of the lengthier approach that grabs the
client PID as a last option.
I don't think c->name gets set anywhere outside of this function and lrmd_remote_client_msg().
PCMK__XA_LRMD_CLIENTNAME gets set when an API client connects. (And it strangely gets re-set to the value of c->name here and in lrmd_remote_client_msg(), which seems redundant.)
lrmd_t:connect()
-> lrmd_handshake()
-> lrmd_handshake_hello_msg()
But the lrmd API is public, and the name argument to lrmd_t:connect() can be NULL.
This is the beginnings of using the pcmk__request_t interface in the server side of pacemaker-execd. For now, there's no substantial code changes - just moving the code into a new file.
This brings them more in line with usage in pe_ipc_dispatch (I'm hoping to condense all this code one day), and frees up the name "request" to be used by pcmk__request_t in the future.
At the moment, this function is fairly unfortunately named (there's already a process_lrmd_message). However, the new function is gradually going to be taking code from the old function until the old one is completely gone. So it's just a temporary confusion we'll need to live with for a bit. This new function holds the framework for handling commands with pcmk__request_t instead of the old system. It's a little tricky to convert these commands over one at a time and still keep everything working, which is why there's the big TODO comment.
Both places that call execd_process_message first add several additional attributes to the request XML, so it makes sense to move that code into execd_process_message instead. Note that there are two different approaches to setting client->name if it's missing. I don't see why the attribute would ever be missing, so I think it's safe to get rid of the lengthier approach that grabs the client PID as a last option.
| { | ||
| int call_id = 0; | ||
| int rc = pcmk_rc_ok; | ||
| bool allowed = pcmk_is_set(request->ipc_client->flags, |
There was a problem hiding this comment.
It might eventually be nice to do something like we have with cib__op_attr_privileged, to reduce some duplication in the handlers. Maybe also a flag for needing call ID, then based on that, getting it and CRM_CHECK-ing that we succeeded; or something. Not worth the trouble right now.
See also enum crm_rsc_flags.
| handle_unknown_request(pcmk__request_t *request) | ||
| { | ||
| pcmk__ipc_send_ack(request->ipc_client, request->ipc_id, request->ipc_flags, | ||
| PCMK__XE_ACK, NULL, CRM_EX_INVALID_PARAM); |
There was a problem hiding this comment.
Should this be a NACK? Like I said in another comment, I don't at all understand how we use ACK/NACK in these.
Also should it be CRM_EX_INVALID_PARAM or CRM_EX_PROTOCOL as we use immediately below?
There was a problem hiding this comment.
The other daemons that send an ACK on unknown request are schedulerd and pacemakerd which do the exact same thing as I'm doing here. On the other hand, I wrote both of those too so I could be wrong everywhere. On the other other hand, Ken approved those.
There was a problem hiding this comment.
Yeah I really don't know. My question wasn't rhetorical or leading. We're figuring stuff out as we go, and things we did in the past that we use as boilerplate may or may not be the best ideas. (Ken has missed outright breaking changes in my patches occasionally.)
Some of this stuff doesn't make much sense to me. I know you've had more immersion in it and might have a better idea of usage and what depends on what.
This commit is typical of how the commit for each command is going to work: * Expose a function in execd_commands.c to the rest of the exec daemon and give it a name that is consistent with "public" functions. * Have it return a standard pacemaker return code. * Remove the old handler block from process_lrmd_message (this is the older function, not the newer confusingly named function). * Add a new handler function and add that function to the handler table.
...and rename them to make them consistent. These are needed to handle processing certain message types.
Handling this message type means we also need to send out a generic notify message after sending the reply. For the moment, the rc value being passed to execd_send_generic_notify doesn't matter because execd_process_rsc_register doesn't return anything (previously, it only ever returned pcmk_ok).
Additionally, execd_process_rsc_exec should return a standard pacemaker return code. It doesn't do anything useful with the call_id value it extracts and returns, and the caller already knows the call_id (and doesn't do anything with the return value from execd_process_rsc_exec aside from check if it's an error or not).
This additionally changes a couple functions to return a standard Pacemaker return code.
This requires a little extra work, in addition to the steps of exposing a function and changing its return type to be a standard Pacemaker return code. Sending a generic notify for unregistering a resource also requires inspecting the return value, which we luckily have in the reply XML so we can just grab it from there as needed.
Now that all messages are supported by the new code, we can add the unknown message handler to catch anything else. And that means everything is being handled by execd_process_message now, so process_lrmd_message can be removed. Fixes T126
and rename the existing function to be execd_create_reply_as. This gets rid of the need to pass __func__ as the first argument every time.
|
I've addressed most review comments, but there's still the ack/nack and response return code stuff to straighten out. |
We have a long-standing project to convert all daemons to use the
pcmk__request_ttype. Once this is done, we can start reducing code duplication across the daemons and start doing a bunch more cleanups (see projects related to T126). This PR addresses execd.I've done this a little bit weird in order to make it easier to develop. Since there's a bunch of different IPC messages in execd and it's easier to develop and debug if you only convert them one at a time, but we also have a default handler, I've added a commit titled "XXX DON'T PUSH THIS" and then a later commit that reverts it. These two get rid of the default handler, allowing the old code to still be used as a fallback for messages I haven't converted yet.
Obviously, I would get rid of these before pushing this PR for real. But, doing that will also make it difficult to bisect through this PR should we ever need to. So, I'm open to suggestions.
I haven't given this the thorough testing it requires yet due to the problem I mentioned in slack where sometimes resources don't start, but in less comprehensive testing, this has been working fine.