mds: drop client metrics during recovery

[batrick]

Fixes: https://tracker.ceph.com/issues/65660

Checklist

• Tracker (select at least one)
  • References tracker ticket
• Component impact
  • No impact that needs to be tracked
• Documentation (select at least one)
  • No doc update is appropriate
• Tests (select at least one)
  • No tests

Show available Jenkins commands

• jenkins retest this please
• jenkins test classic perf
• jenkins test crimson perf
• jenkins test signed
• jenkins test make check
• jenkins test make check arm64
• jenkins test submodules
• jenkins test dashboard
• jenkins test dashboard cephadm
• jenkins test api
• jenkins test docs
• jenkins render docs
• jenkins test ceph-volume all
• jenkins test ceph-volume tox
• jenkins test windows
• jenkins test rook e2e

[batrick]

jenkins test make check

[leonid-s-usov]

I don't see where you are resetting it. I'd suggest that you put the code at the end of handle_mds_map:

    m_is_active = is_active();

[batrick]

resetting?

[lxbsz]

IMO no need to reset it.

@batrick BTW, why not just check the MDS' state from the mdsmap instead of adding a new m_is_active ? For lockless case ?

[batrick]

You need the mds_lock to look at the MDSMap. We don't want the metrics aggregator to be acquiring that lock generally.

[lxbsz]

Makes sense.

[leonid-s-usov]

I appreciate that as of today an instance of MDSRank may never experience a transition from active -> inactive in a way that would affect this code. However, relying on that fact here creates an implicit dependency on how MDSRank instances should be managed that isn't unit-tested (as of today). We should try to minimize such implicit dependencies, IMO

[batrick]

I appreciate that as of today an instance of MDSRank may never experience a transition from active -> inactive in a way that would affect this code. However, relying on that fact here creates an implicit dependency on how MDSRank instances should be managed that isn't unit-tested (as of today). We should try to minimize such implicit dependencies, IMO

up:active has always been treated as a "terminal" state for a rank and is reflected in the code everywhere. I don't see a benefit for making this somehow resilient to that changing.

[lxbsz]

@batrick I think this change will fix the case that when connecting to the old clients, which haven't included my previous fixes.

[rishabh-d-dave]

@batrick Picking this PR for QA

[rishabh-d-dave]

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

[leonid-s-usov]

I understand that MDS can't deactivate except for shutting down, hence I'll approve as-is.

To reduce ambiguity for future readers, who, like me, could be confused by the naming and why this state never gets reset, I'd suggest the name: m_did_activate. IMO this better encodes the one-way nature of the flag.

[rishabh-d-dave]

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