common: modify md_config_obs_impl API by ronen-fr · Pull Request #61394 · ceph/ceph

ronen-fr · 2025-01-15T12:07:52Z

Modify the md_config_cacher_t to implement the new get_tracked_conf_key() API.

Apart from resulting in a clearer code, this also solves the potential for a dangling
pointer issue that was present in the existing code.

This is a continuation of #61289. That PR was
enough to solve existing config-cacher bugs, and was created with backporting
in mind - but it was just one step on the road of improving the cacher. The current
PR is the next step.

rzarzynski · 2025-01-16T16:45:43Z

src/common/config_cacher.h

 class md_config_cacher_t : public md_config_obs_t {
  ConfigProxy& conf;
-  const char* keys[2];
+  const std::string option_name;


Yeah, this is so cold part of the code that employing std::string seems purely beneficial.

rzarzynski · 2025-01-16T16:46:59Z

src/common/config_obs.h

+   * a leaner version of get_tracked_conf_keys() that returns a single key.
+   * If returns a valid string - get_tracked_conf_keys() will not be called.
+   */
+  virtual std::optional<std::string> get_tracked_conf_key() const noexcept {


Is there a plan to deprecate / drop the old get_tracked_conf_keys()?
How much of work this would need?

Not really, as most of the users register multiple keys.
I do want to change the signatures to 'const char * const *' - to better handle const strings as arguments.
But it might be a good idea to modify the multi-key interface altogether - to use strings and be copied directly into the strings of the config manager.

But that's for the next PR...

I'm not really seeing a big benefit for this PR while it complicates the interface. It's not like we have a ton of config obs which only care about one option name?

I do agree something like std::array<std::string> const& would be preferable as a return type.

@batrick

we have a ton of config obs which only care about one option name?

Yes - we will have (in the OSD), as we plan to move more 'change aware' configs to be wrapped in the single-key md_config_cacher_t (currently we have 11 of those, but that's just the beginning). The reason for this direction is that we've learned that the get<> interface for the configs is very slow.

std::arraystd::string const&

(I do not think we can use std::array, but std::vector is a valid choice).
I did not want to have to change all clients of the existing interface in this PR, and preferred to make that the next step. Not sure about the 'const&', though. I will have to play with that. If possible, I'd wish the actual texts to be moved-to during registration.

I have modified the PR, discarding the one-key API. See comment below.

ronen-fr · 2025-01-23T16:52:42Z

@batrick - can you take a look? appreciate your input. Thanks.

batrick · 2025-01-24T21:51:44Z

src/common/config_obs.h

+   * a leaner version of get_tracked_conf_keys() that returns a single key.
+   * If returns a valid string - get_tracked_conf_keys() will not be called.
+   */
+  virtual std::optional<std::string> get_tracked_conf_key() const noexcept {


I'm not really seeing a big benefit for this PR while it complicates the interface. It's not like we have a ton of config obs which only care about one option name?

I do agree something like std::array<std::string> const& would be preferable as a return type.

Adding a new version of the API used by the configuration manager to find out what keys are handled by the specific observer (and thus should be registered to be updated on changes). The new interface (get_tracked_keys()) returns a vector of moveable strings. Modify the ObserverMgr to use this new interface when avaiable. The existing 'char**' based interface is maintained for now, until all its clients are updated. Signed-off-by: Ronen Friedman <[email protected]>

Modify the md_config_cacher_t to implement the new get_tracked_keys() callback, instead of the deprecated char* based get_tracked_conf_keys(). Signed-off-by: Ronen Friedman <[email protected]>

ronen-fr · 2025-01-25T15:52:10Z

@batrick , @rzarzynski

Following the review comments, I have modified the PR as follows:

The Observer now has one new registration function:

  virtual std::vector<std::string> get_tracked_keys() const noexcept {
    ...
  }

this new one would be used for all registrations - including 'single key' ones.

The old const char** get_tracked_conf_keys() is maintained in
this PR as a deprecated legacy interface. I will create separate PRs
to change the dozens of clients that use that one, then remove it.

Note: the new interface returns a vector of strings that are moved
into the manager's main map.

rzarzynski · 2025-01-29T22:31:21Z

src/common/ceph_context.cc

-    static const char *KEYS[] = {"lockdep", NULL};
-    return KEYS;
+  std::vector<std::string> get_tracked_keys() const noexcept override {
+    return std::vector<std::string>{"lockdep"s};


nit: not sure we really need to repeat the type; perhaps return {"lockdep"s}; would be enough.

You are right. Fixed.

modify some configuration object registrations in common/ceph_context to use the updated md_config_obs_t::get_tracked_keys() API Signed-off-by: Ronen Friedman <[email protected]>

Dismissing, as code changed per request, and re-reviewed by Radek

ronen-fr · 2025-01-30T10:43:15Z

jenkins test make check

ronen-fr · 2025-02-02T13:51:25Z

Merging based on my Teuthology tests, both as part of wip-rf-combined-30j, and as a separate branch.

.. with get_tracked_keys(). This PR is but one of a set. Following ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

.. with get_tracked_keys(). This PR is but one of a set. Following ceph/ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]> (cherry picked from commit f9e30c8)

.. with get_tracked_keys(). Following ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

.. with get_tracked_keys(). As per the original code in this specific case: the vector of keys is expected to be sorted. Following ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

.. with get_tracked_keys(). Following ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

.. with get_tracked_keys(). As per the original code in this specific case: the vector of keys is expected to be sorted. Following ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

.. with get_tracked_keys(). Following ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

.. with get_tracked_keys(). As per the original code in this specific case: the vector of keys is expected to be sorted. Following ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

.. with get_tracked_keys(). Following ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

.. with get_tracked_keys(). This PR is but one of a set. Following ceph/ceph#61394, all uses of the deprecated interface will be updated, and that old interface will be removed. Signed-off-by: Ronen Friedman <[email protected]>

github-actions bot added the common label Jan 15, 2025

ronen-fr force-pushed the wip-rf-cacher-v2 branch from 10ee3d4 to bf27274 Compare January 15, 2025 12:27

ronen-fr requested review from NitzanMordhai and aclamk January 15, 2025 14:12

ronen-fr force-pushed the wip-rf-cacher-v2 branch from bf27274 to e2ea8ad Compare January 16, 2025 07:40

ronen-fr marked this pull request as ready for review January 16, 2025 12:39

ronen-fr requested review from neha-ojha and rzarzynski January 16, 2025 12:39

rzarzynski reviewed Jan 16, 2025

View reviewed changes

rzarzynski requested a review from batrick January 16, 2025 16:47

ronen-fr requested a review from rzarzynski January 23, 2025 09:17

batrick previously requested changes Jan 24, 2025

View reviewed changes

ronen-fr requested a review from batrick January 25, 2025 06:44

ronen-fr added 2 commits January 25, 2025 09:41

common: modify md_config_cacher_t to implement get_tracked_keys()

9d224ab

Modify the md_config_cacher_t to implement the new get_tracked_keys() callback, instead of the deprecated char* based get_tracked_conf_keys(). Signed-off-by: Ronen Friedman <[email protected]>

ronen-fr force-pushed the wip-rf-cacher-v2 branch from e2ea8ad to daa164c Compare January 25, 2025 15:42

rzarzynski approved these changes Jan 29, 2025

View reviewed changes

common: ceph_context: make use of get_tracked_keys()

7c8f081

modify some configuration object registrations in common/ceph_context to use the updated md_config_obs_t::get_tracked_keys() API Signed-off-by: Ronen Friedman <[email protected]>

ronen-fr force-pushed the wip-rf-cacher-v2 branch from daa164c to 7c8f081 Compare January 30, 2025 08:03

ronen-fr merged commit 519844b into ceph:main Feb 2, 2025
12 checks passed

This was referenced Feb 2, 2025

rgw: replace obsolete get_tracked_conf_keys() #61619

Merged

common,osd: replace obsolete get_tracked_conf_keys() #61623

Merged

ronen-fr mentioned this pull request Feb 19, 2025

crimson/osd: replace obsolete get_tracked_conf_keys() #61903

Merged

ronen-fr mentioned this pull request Mar 2, 2025

bluestore, librados, msg: replace obsolete get_tracked_conf_keys() #62075

Merged

ronen-fr mentioned this pull request Mar 5, 2025

tests: replace obsolete get_tracked_conf_keys() #62133

Merged

ronen-fr mentioned this pull request Mar 9, 2025

test/crimson/test_config: replace obsolete get_tracked_conf_keys() #62182

Merged

ronen-fr mentioned this pull request Mar 9, 2025

kernel/KernelDevice: replace obsolete get_tracked_conf_keys() #62185

Merged

ronen-fr mentioned this pull request Mar 9, 2025

mds,nvmeof,rbd: replace obsolete get_tracked_conf_keys() #62188

Merged

Conversation

ronen-fr commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronen-fr Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronen-fr commented Jan 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronen-fr commented Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronen-fr commented Jan 30, 2025

Uh oh!

ronen-fr commented Feb 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ronen-fr commented Jan 15, 2025 •

edited

Loading

ronen-fr Jan 25, 2025 •

edited

Loading

ronen-fr commented Jan 25, 2025 •

edited

Loading