quincy: MgrMonitor: batch commit OSDMap and MgrMap mutations #50979
Conversation
This file contains 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
|
jenkins test make check |
ajarr
approved these changes
Apr 11, 2023
idryomov
reviewed
Apr 14, 2023
There was a problem hiding this comment.
The three commits from #50404 are missing "cherry picked from" annotations.
Also, if not using ceph-backport.sh script for creating backports, please consider replicating what it does as far as the description of the PR goes manually. In particular, providing a link to the main PR(s) is very useful because then the backport PR gets linked from there automatically as well.
There is a race condition between the `mgr fail` handling and `mgrbeacon`.
```diff
diff --git a/src/mon/MgrMonitor.cc b/src/mon/MgrMonitor.cc
index 8ada44e2628..9000b2e0687 100644
--- a/src/mon/MgrMonitor.cc
+++ b/src/mon/MgrMonitor.cc
@@ -1203,7 +1203,9 @@ bool MgrMonitor::prepare_command(MonOpRequestRef op)
}
if (changed && pending_map.active_gid == 0) {
+ dout(5) << "========== changed and active_state == 0" << dendl;
promote_standby();
+ dout(5) << "========== after promote_standby: " << pending_map.active_gid << dendl;
}
} else if (prefix == "mgr module enable") {
string module;
```
```
2022-05-17T00:11:19.602+0200 7f6bd5769700 0 mon.a@0(leader) e1 handle_command mon_command({"prefix": "mgr fail", "who": "x"} v 0) v1
...
2022-05-17T00:11:19.614+0200 7f6bd5769700 5 mon.a@0(leader).mgr e25 ========== changed and active_state == 0
2022-05-17T00:11:19.614+0200 7f6bd5769700 5 mon.a@0(leader).mgr e25 ========== after promote_standby: 0
2022-05-17T00:11:19.614+0200 7f6bd5769700 4 mon.a@0(leader).mgr e25 prepare_command done, r=0
...
2022-05-17T00:11:19.630+0200 7f6bd5769700 4 mon.a@0(leader).mgr e25 selecting new active 4210 x (was 0 )
```
```cpp
bool MgrMonitor::prepare_beacon(MonOpRequestRef op)
if (pending_map.active_gid == m->get_gid()) {
// ...
} else if (pending_map.active_gid == 0) {
// There is no currently active daemon, select this one.
if (pending_map.standbys.count(m->get_gid())) {
drop_standby(m->get_gid(), false);
}
dout(4) << "selecting new active " << m->get_gid()
<< " " << m->get_name()
<< " (was " << pending_map.active_gid << " "
<< pending_map.active_name << ")" << dendl;
pending_map.active_gid = m->get_gid();
pending_map.active_name = m->get_name();
pending_map.active_change = ceph_clock_now()
```
The `25` version of `MgrMap`, when handled at `mgr.x`, doesn't trigger the `respawn()` path:
```
2022-05-17T00:10:11.197+0200 7fa3d1e0a700 10 mgr ms_dispatch2 active mgrmap(e 25) v1
2022-05-17T00:10:11.197+0200 7fa3d1e0a700 4 mgr handle_mgr_map received map epoch 25
2022-05-17T00:10:11.197+0200 7fa3d1e0a700 4 mgr handle_mgr_map active in map: 1 active is 4210
2022-05-17T00:10:11.197+0200 7fa3d6613700 10 --2- 127.0.0.1:0/743576734 >> [v2:127.0.0.1:40929/0,v1:127.0.0.1:40930/0] conn(0x5592635ef400 0x5592635f6580 secure :-1 s=THROTTLE_DONE pgs=130 cs=0 l=1 rev1=1 crypto rx=0x55926362e810 tx=0x559263563b60 comp rx=0 tx=0).handle_read_frame_dispatch tag=17
2022-05-17T00:10:11.197+0200 7fa3d6613700 5 --2- 127.0.0.1:0/743576734 >> [v2:127.0.0.1:40929/0,v1:127.0.0.1:40930/0] conn(0x5592635ef400 0x5592635f6580 secure :-1 s=THROTTLE_DONE pgs=130 cs=0 l=1 rev1=1 crypto rx=0x55926362e810 tx=0x559263563b60 comp rx=0 tx=0).handle_message got 43089 + 0 + 0 byte message. envelope type=1796 src mon.0 off 0
2022-05-17T00:10:11.197+0200 7fa3d1e0a700 10 mgr handle_mgr_map I was already active
```
Fixes: https://tracker.ceph.com/issues/55711
Signed-off-by: Radosław Zarzyński <rzarzyns@redhat.com>
(cherry picked from commit 23c3f76)
in 23c3f76, the change to fail the mgr is proposed immediately. but `MgrMonitor::prepare_command()` method still returns `true` in this case. its indirect caller of `PaxosService::dispatch()` considers this as a sign that it needs to propose the change with `propose_pending()`. but the pending change has already been proposed by `MgrMonitor::prepare_command()`, and `have_pending` is also cleared by this call. as we don't allow consecutive paxos proposals, the second `propose_pending()` call is delayed with a configured latency. but when the timer is fired, this poseponed call would find itself trying to propose nothing. the change to fail the mgr has been proposed. that's why we have `ceph_assert(have_pending)` assertion failures. in this change, the second proposal is not proposed anymore if the proposal is proposed immediately. this should avoid the assertion failure. this change should address the regression introduced by 23c3f76. Fixes: https://tracker.ceph.com/issues/56850 Signed-off-by: Kefu Chai <tchaikov@gmail.com> (cherry picked from commit 5b1c6ad)
The return value is used to indicate whether the pending state should be committed. There is no concept of "handled message" here (unlike preprocess_query). Signed-off-by: Patrick Donnelly <pdonnell@redhat.com> (cherry picked from commit 8caa1fd)
Plugging PAXOS has the effect of batching map updates into a single PAXOS transaction. Since we're updating the OSDMap several times and the MgrMap, plug PAXOS for efficiency. This also has the nice effect of reducing any delay between the active mgr getting dropped and the blocklisting of its clients. This doesn't resolve any race condition as the two maps are never processed in one unit. So the former active manager may process the OSDMap blocklists before learning it is dropped from the MgrMap. Fixes: https://tracker.ceph.com/issues/58923 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com> (cherry picked from commit 2e057bb)
Thanks for the nudge @idryomov . I need to revisit that script as the last time I tried it there were dependency issues. |
idryomov
approved these changes
May 8, 2023
|
jenkins test api |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
backport tracker: https://tracker.ceph.com/issues/59294
backport of #46318, #47834 and #50404
parent tracker: https://tracker.ceph.com/issues/58923