Skip to content

mon/MDSMonitor: send reply to beacons with state=DNE#60327

Merged
batrick merged 1 commit intoceph:mainfrom
MaxKellermann:mdsmonitor_beacon_dne_reply
Nov 13, 2024
Merged

mon/MDSMonitor: send reply to beacons with state=DNE#60327
batrick merged 1 commit intoceph:mainfrom
MaxKellermann:mdsmonitor_beacon_dne_reply

Conversation

@MaxKellermann
Copy link
Member

@MaxKellermann MaxKellermann commented Oct 15, 2024

During shutdown, the MDS sends a MSG_MDS_BEACON with MDSMap::STATE_DNE (in MDSDaemon::suicide()) and then waits for a MSG_MDS_BEACON reply from the MON.

The MON, however, suppresses replies to STATE_DNE; in MDSMonitor::preprocess_beacon(), it returns early on STATE_DNE and MDSMonitor::prepare_beacon() silently evicts the dying MDS without any reply.

This delays the MDS shutdown until the MDS times out.

Since MDSDaemon::suicide() has code to wait for a beacon reply, I figure that the MON reply was suppressed accidently, therefore I suggest adding it.

This used to be part of #60320

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)

@MaxKellermann MaxKellermann requested a review from a team as a code owner October 15, 2024 14:49
@github-actions github-actions bot added cephfs Ceph File System core mon labels Oct 15, 2024
auto beacon = make_message<MMDSBeacon>(mon.monmap->fsid,
m->get_global_id(), m->get_name(), get_fsmap().get_epoch(),
m->get_state(), m->get_seq(), CEPH_FEATURES_SUPPORTED_DEFAULT);
mon.send_reply(op, beacon.detach());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you don't want this backported? I think it makes sense to. (In which case, please make a tracker ticket...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I havn't decided about this - I don't know how Ceph does this, and I can't decide what should be backported. All the PRs I submit here are forward-ported from my Reef branch; we're using Reef (to be updated to Squid), but never main. My optimizations will be pushed to our production clusters soon. Most of my optimizations probably will never be backported anyway, so for my work, it doesn't make a difference; I have to maintain a fork anyway, no matter how much you approve for merging or how much of that you backport afterwards.

@MaxKellermann
Copy link
Member Author

During shutdown, the MDS sends a `MSG_MDS_BEACON` with
`MDSMap::STATE_DNE` (in `MDSDaemon::suicide()`) and then waits for a
`MSG_MDS_BEACON` reply from the MON.

The MON, however, suppresses replies to `STATE_DNE`; in
`MDSMonitor::preprocess_beacon()`, it returns early on `STATE_DNE` and
`MDSMonitor::prepare_beacon()` silently evicts the dying MDS without
any reply.

This delays the MDS shutdown until the MDS times out.

Since `MDSDaemon::suicide()` has code to wait for a beacon reply, I
figure that the MON reply was suppressed accidently, therefore I
suggest adding it.

Fixes: https://tracker.ceph.com/issues/68761
Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann MaxKellermann force-pushed the mdsmonitor_beacon_dne_reply branch from 01c35df to fd9c404 Compare October 29, 2024 20:41
@batrick
Copy link
Member

batrick commented Nov 6, 2024

This PR is under test in https://tracker.ceph.com/issues/68859.

@batrick
Copy link
Member

batrick commented Nov 13, 2024

@batrick batrick merged commit 8ae8988 into ceph:main Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants