mgr,mon: subscribe to config-key changes from mgr; maintain mgr-side ConfigMap by liewegas · Pull Request #39504 · ceph/ceph

liewegas · 2021-02-16T23:45:11Z

convert mon ConfigKeyService to KVMonitor (PaxosService)
allow subscriptions to config-key prefixes
make mgr subscribe to mgr/, device/. This results in cleaner/faster startup and allows mgr to see config-key changes made via the CLI
make mgr subscribe to config/ and maintain a ConfigMap
implement mgr_module get_foreign_ceph_option(entity, name) to get any ceph config option for any daemon (excluding mgr module options)
update cephadm to use get_foreign_ceph_option() instead of 'config get'

For the most part there are no upgrade/downgrade compat issues here, except that a new mgr may be running against a mon cluster that doesn't (all) support kv subscriptions, in which case it still needs to do the old slow thing of fetching all of the config-key values individually.

src/mon/KVMonitor.cc

jdurgin · 2021-02-17T02:22:46Z

src/mon/KVMonitor.cc

+  cmd_getval(cmdmap, "prefix", prefix);
+  string key;
+  cmd_getval(cmdmap, "key", key);
+


are permissions already checked elsewhere? I'd expect those checks here

also in prepare_command()

they're checked in the caller at https://github.com/ceph/ceph/blob/master/src/mon/Monitor.cc#L3422

jdurgin · 2021-02-17T02:47:57Z

src/mgr/Mgr.cc

+  monc->sub_want("kv:config/", 0, 0);
+  monc->sub_want("kv:mgr/", 0, 0);
+  monc->sub_want("kv:device/", 0, 0);
+  monc->sub_want("kv:config/", 0, 0);


duplicate kv:config/

jdurgin · 2021-02-17T03:26:03Z

src/mon/ConfigMonitor.cc

      dout(10) << __func__ << " unrecognized option '" << name << "'" << dendl;
      opt = new Option(name, Option::TYPE_STR, Option::LEVEL_UNKNOWN);
-      // FIXME: this will be leaked!
+      config_map.stray_options.push_back(opt);


it'd be great to get rid of raw pointers here

Convert this into a normal PaxosService. This gives us (1) code uniformity and also (2) a changelog in the form of the paxos commits for recently changed keys that we can use to allow clients to subscribe to changes. For upgrades this is pretty painless: - the actual kv data is in the same place - an old mon will still see updates made by a new monitor - a new mon will still see updates made by an old monitor, but won't get a paxosservice version bump. Signed-off-by: Sage Weil <sage@newdream.net>

Allow subscription to config-key/kv data. Initially we'll send a full dump of the prefix. As changes occur, we'll send incremental diffs, unless the subscriber is too far behind, in which case we'll send a full dump again. There is a new message, MKVData, to support this. No compat issues since old clients won't subscribe to this stream unless they know how to handle it. Signed-off-by: Sage Weil <sage@newdream.net>

Include the config/ prefix (which we weren't loading before). Before we are active, we collect these changes, and then pass them to the ActivePyModules ctor. No change in functionality here, except that when we make a change from a mgr module, we'll (1) put it in our local cache store, (2) send the mon command to set it, and (3) get a notification that updates the same value. Since this whole process is synchronous (see ActivePyModules::set_store()), and the notification will generally arrive *before* the command ack, there is no change in behavior. If the mon cluster is not yet pacific, we still need to load kv values the old way. If it's a mixed-mon cluster (and, e.g., our current mon has the feature but not all of them do), we'll get this data both ways, but no harm is done. Signed-off-by: Sage Weil <sage@newdream.net>

The const Option* needs to remain alive only until the next clear(). Keep the reference in ConfigMap and clean it up then. Signed-off-by: Sage Weil <sage@newdream.net>

Signed-off-by: Sage Weil <sage@newdream.net>

Fetch a config value that applies to an arbitrary daemon in the cluster. Initial implementaiton is not very efficient, but it is better than a round-trip query to the monitor. Note: this currently excludes mgr/ options (which should hopefully not be of interest on other daemons!). Signed-off-by: Sage Weil <sage@newdream.net>

…n command Signed-off-by: Sage Weil <sage@newdream.net>

PyModule::config_prefix is "mgr/", so this has no effect. Signed-off-by: Sage Weil <sage@newdream.net>

This code is working with config option names, not config-key keys. Signed-off-by: Sage Weil <sage@newdream.net>

The prefix for module kv store is "mgr/", but it is not *config* (which lives under /config/mgr/...). Rename the constant. Signed-off-by: Sage Weil <sage@newdream.net>

neha-ojha · 2021-02-17T22:47:18Z

src/pybind/mgr/cephadm/upgrade.py

+            # complete mon upgrade?
+            if daemon_type == 'mon':
+                if not self.mgr.get("have_local_config_map"):
+                    logger.info('Upgrade: Restarting mgr now that mons are runnig pacific')


I seem to recall we had some issues in the past due to which the mgr needed to be upgraded first, wondering if that doesn't hold any longer.

jecluis

It looks good to me.

If we are upgrading to pacific, we need to restart the mgr *after* the mons have a pacific quorum so that they can get the kv subscriptions. Signed-off-by: Sage Weil <sage@newdream.net>

liewegas requested a review from a team as a code owner February 16, 2021 23:45

github-actions bot added build/ops cephadm core mgr mon pybind labels Feb 16, 2021

liewegas requested review from jdurgin, jecluis and neha-ojha February 16, 2021 23:45

jdurgin reviewed Feb 17, 2021

View reviewed changes

src/mon/KVMonitor.cc Outdated Show resolved Hide resolved

jdurgin reviewed Feb 17, 2021

View reviewed changes

liewegas force-pushed the mgr-configmap branch 3 times, most recently from b46bbf5 to be16d18 Compare February 17, 2021 17:13

liewegas added 11 commits February 17, 2021 12:27

mon/ConfigMap: fix stray option leak

9397a46

The const Option* needs to remain alive only until the next clear(). Keep the reference in ConfigMap and clean it up then. Signed-off-by: Sage Weil <sage@newdream.net>

mon/ConfigMap: factor out parse_key helper

ceace01

Signed-off-by: Sage Weil <sage@newdream.net>

mgr: maintain a ConfigMap copy

18a12f8

Signed-off-by: Sage Weil <sage@newdream.net>

mgr/cephadm: use get_foreign_ceph_option() instead of 'config get' mo…

9b5cbf4

…n command Signed-off-by: Sage Weil <sage@newdream.net>

mgr/MgrStandby: simplify weird code

95f80dd

PyModule::config_prefix is "mgr/", so this has no effect. Signed-off-by: Sage Weil <sage@newdream.net>

mgr: fix config option prefix

b0a4bff

This code is working with config option names, not config-key keys. Signed-off-by: Sage Weil <sage@newdream.net>

mgr: rename config_prefix -> mgr_store_prefix

3bafb5e

The prefix for module kv store is "mgr/", but it is not *config* (which lives under /config/mgr/...). Rename the constant. Signed-off-by: Sage Weil <sage@newdream.net>

liewegas force-pushed the mgr-configmap branch from be16d18 to 3bafb5e Compare February 17, 2021 17:28

liewegas added the wip-sage4-testing label Feb 17, 2021

neha-ojha reviewed Feb 17, 2021

View reviewed changes

jecluis approved these changes Feb 19, 2021

View reviewed changes

liewegas added wip-sage-testing and removed wip-sage4-testing wip-sage-testing labels Feb 19, 2021

liewegas force-pushed the mgr-configmap branch from 2b5878f to 8ea3451 Compare February 21, 2021 00:04

mgr/cephadm/upgrade: restart mgr after mons upgrade to pacific

4d7dd00

If we are upgrading to pacific, we need to restart the mgr *after* the mons have a pacific quorum so that they can get the kv subscriptions. Signed-off-by: Sage Weil <sage@newdream.net>

liewegas force-pushed the mgr-configmap branch from 8ea3451 to 4d7dd00 Compare February 21, 2021 00:05

liewegas merged commit 458ea4c into ceph:master Feb 21, 2021

liewegas mentioned this pull request Feb 21, 2021

pacific: subscribe to config-key changes from mgr; maintain mgr-side ConfigMap #39595

Merged

sebastian-philipp mentioned this pull request Mar 3, 2021

pacific: cephadm: Batch backport March (1) #39807

Merged

This was referenced Mar 22, 2021

octopus: mon/ConfigMap: fix stray option leak #40298

Merged

nautilus: mon/ConfigMap: fix stray option leak #40299

Merged

smithfarm mentioned this pull request Apr 10, 2021

octopus: mon/MgrMonitor: populate available_modules from promote_standby() #40757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr,mon: subscribe to config-key changes from mgr; maintain mgr-side ConfigMap#39504

mgr,mon: subscribe to config-key changes from mgr; maintain mgr-side ConfigMap#39504
liewegas merged 12 commits intoceph:masterfrom
liewegas:mgr-configmap

liewegas commented Feb 16, 2021

Uh oh!

Uh oh!

jdurgin Feb 17, 2021

Uh oh!

jdurgin Feb 17, 2021

Uh oh!

liewegas Feb 17, 2021

Uh oh!

jdurgin Feb 17, 2021

Uh oh!

liewegas Feb 17, 2021

Uh oh!

jdurgin Feb 17, 2021

Uh oh!

neha-ojha Feb 17, 2021

Uh oh!

jecluis left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

liewegas commented Feb 16, 2021

Uh oh!

Uh oh!

jdurgin Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

jdurgin Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

liewegas Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

jdurgin Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

liewegas Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

jdurgin Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

neha-ojha Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

jecluis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants