mon/MgrMonitor: read cmd descs if empty on update_from_paxos() #17846
Conversation
If the MgrMonitor's `command_descs` is empty, the monitor will not send the mgr commands to clients on `get_descriptions`. This, in turn, has the clients sending the commands to the monitors, which will have no idea how to handle them. Therefore, make sure to read the `command_descs` from disk if the vector is empty. Fixes: http://tracker.ceph.com/issues/21300 Signed-off-by: Joao Eduardo Luis <joao@suse.de>
|
no need for needs-backport tag if there is a tracker ticket with the backport field populated. |
|
very good, hope to catch up with 12.2.1 |
| @@ -84,8 +84,9 @@ void MgrMonitor::update_from_paxos(bool *need_bootstrap) | |||
| check_subs(); | |||
|
|
|||
| if (version == 1 | |||
| || (map.get_available() | |||
| && (!old_available || old_gid != map.get_active_gid()))) { | |||
| || command_descs.empty() | |||
There was a problem hiding this comment.
if there is no active mgr, why shall we load the cmd desc for the mon client and redirect it to a nonexistent mgr? the net result is that it will be waiting for an active mgr in mgrmap. and in theory, the command_desc could be stale once the new mgr is activated, as it will send a beacon to mon and update it with the commands it will be able to serve. and the "commands" here could be different from the ones stored in monstore.
There was a problem hiding this comment.
This patch relies on the assumption that a client will always block if there is no active mgr. That was a question I posed on ceph-devel before working on the patch, and Sage's reply was that the client is supposed to hang.
Given it's the monitor's responsibility to tag (by means of flags) which commands are the mgr's, we need to have a list of mgr commands that can be relayed to the client -- even if that means to relay a stale list, given we know of no active mgr's command list, so that the client can hang waiting for an active mgr.
Now, if it is established that the client is not supposed to hang, instead getting an EINVAL because the monitor does not handle the command, nor does it know of any manager that will, then a different patch needs to be worked on.
There was a problem hiding this comment.
the client is supposed to hang.
ahh, lgtm then.
|
holding back merge a little bit longer to finish some local tests. |
src/mon/MgrMonitor.cc
Outdated
| // command_descs vector. This likely means we came from kraken, where | ||
| // we wouldn't populate the vector, nor would we write it to disk, on | ||
| // create_initial(). | ||
| create_initial(); |
There was a problem hiding this comment.
yes, we should have this in luminous. but i am not sure if we need it in master (i.e. mimic) if our upgrade path is something like: jewel/kraken => luminous (not optional) => mimic.
There was a problem hiding this comment.
@liewegas can you please confirm what kefu mentioned about luminous being a required step in the upgrade path from kraken to mimic? if this is confirmed, I'll be happy to have this patch targeting solely luminous.
|
Yep, confirmed. We've already merged a ton of stuff ot master ripping out pre-luminous support. |
If the MgrMonitor's
command_descsis empty, the monitor will not sendthe mgr commands to clients on
get_descriptions. This, in turn, hasthe clients sending the commands to the monitors, which will have no
idea how to handle them.
Therefore, make sure to read the
command_descsfrom disk if the vectoris empty.
Fixes: http://tracker.ceph.com/issues/21300
Signed-off-by:
Joao Eduardo Luis <joao@suse.de>