mon: MonmapMonitor: don't expose uncommitted state to client #6854
Conversation
|
I've done some testing on this by running a bash script running anything between 2 and 12 concurrent randomly running 'ceph mon {add,remove}' with a mix of same mon.id/port, different mon.id/same port, same mon.id/different port, different mon.id/different port, and it held. |
| * | ||
| * Once we established the monitor does not exist in the committed state, | ||
| * we can simply go ahead and add the monitor. | ||
| */ | ||
|
|
||
| pending_map.add(name, addr); | ||
| pending_map.last_changed = ceph_clock_now(g_ceph_context); |
There was a problem hiding this comment.
This seems a bit racy. What happens if multiple processes try and use the same name at once?
There was a problem hiding this comment.
Good point, and I need to update the comment just above to reflect any similar doubts that may arise from this.
There's no risk of that actually happening. We don't collate proposals on the MonmapMonitor. Once we return 'true' below, PaxosService::dispatch() will check if we should propose and we will -- because we should always propose in this service (MonmapMonitor::should_propose() returns 0.0 delay). Even if Paxos does not propose right away because reasons, PaxosService::propose_pending() will nonetheless set proposing = true and we will not dispatch any further messages that may write -- including the one that would contain the offending data you mention.
In essence we are serializing write requests on the MonmapMonitor, and that's why doing this will be safe for as long as we maintain this behavior.
|
passed a run through rados suite |
|
is this ready to merge? |
|
as soon as squash and repush the updated comment I promised above. I'll merge it myself as soon as it's done. |
During prepare_command(), we were returning to the user based on pending_map's state. Even though this weren't causing any issues we are aware of, we really shouldn't do that. Signed-off-by: Joao Eduardo Luis <joao@suse.de>
mon: MonmapMonitor: don't expose uncommitted state to client Reviewed-by: Sage Weil <sage@redhat.com> Reviewed-by: Greg Farnum <gfarnum@redhat.com>
During prepare_command(), we were returning to the user based on
pending_map's state. Even though this weren't causing any issues we are
aware of, we really shouldn't do that.
Signed-off-by: Joao Eduardo Luis joao@suse.de