mgr: python interface rework + enable modules to run in standby mode #16651

jcsp · 2017-07-28T11:33:57Z

Update documentation + release notes for "mgr services" command.
Add mechanism for python modules to serve in standby mode

wido · 2017-10-05T08:09:50Z

Do we already want to include the "can_run()" method here? Like we discussed on the Ceph day. Meaning that we check if a module is able to run before we attempt it.

jcsp · 2017-10-05T09:06:34Z

@wido definitely want it, but this PR is already huge so let's do it separately afterwards

ceph-jenkins · 2017-10-06T17:05:25Z

OK - docs built

wido · 2017-10-09T08:12:46Z

@jcsp: Understood! Was scanning through the code and didn't see it. Just wanted to double-check I didn't miss anything.

ceph-jenkins · 2017-10-24T10:02:53Z

make check succeeded

tchaikov · 2017-10-26T05:58:35Z

src/mgr/PyState.cc



+typedef struct {


nit, no need to typedef a struct in C++.

tchaikov · 2017-10-26T06:46:47Z

src/mgr/PyModuleRegistry.cc

+  // Load python code
+  for (const auto& module_name : mgr_map.modules) {
+    dout(1) << "Loading python module '" << module_name << "'" << dendl;
+    auto mod = std::unique_ptr<PyModule>(new PyModule(module_name));


could use make_unique()

tchaikov · 2017-10-26T07:09:50Z

src/pybind/mgr/dashboard/module.py

@@ -184,6 +184,14 @@ def notify(self, notify_type, notify_val):
                    self.log_buffer.appendleft(notify_val)
        elif notify_type == "pg_summary":
            self.update_pool_stats()
+
+            #pg_summary = self.get("pg_summary")


this change is not related to the noop stub of os._exit(). can we drop it?

oops! done.

tchaikov · 2017-10-26T07:14:41Z

src/mgr/PyModuleRunner.h

+
+  PyModuleRunnerThread thread;
+
+  std::string const &get_name() { return module_name; }


nit, make this method const.

tchaikov · 2017-10-26T07:27:51Z

src/mgr/ActivePyModules.cc

-    serve_threads[i.first].reset(thread);
-  }
+  int r = modules[module_name]->load(this);
+  if (r != 0) {


shall we remove the module failed to load from modules?

For the moment I suggest we keep them there -- later we might want to add a mechanism for reporting to the MgrMonitor about the list of modules that we tried and failed to load, so they need remembering somewhere.

tchaikov · 2017-10-26T07:37:49Z

src/mgr/ActivePyModule.cc

@@ -134,7 +134,19 @@ int ActivePyModule::load_commands()
  // Don't need a Gil here -- this is called from ActivePyModule::load(),
  // which already has one.
  PyObject *command_list = PyObject_GetAttrString(pClassInstance, "COMMANDS");
-  assert(command_list != nullptr);
+  if (command_list == nullptr) {


http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es87-dont-add-redundant--or--to-conditions

Hmm. I kind of disagree with the guidelines on this one, I like being explicit that I'm checking a pointer rather than evaluating a class instance for truthiness. I don't remember if the google C++ guide takes a position on this.

tchaikov · 2017-10-26T07:40:45Z

src/mgr/PyModuleRegistry.cc

-    Py_INCREF(&BaseMgrModuleType);
-    PyModule_AddObject(ceph_module, "BaseMgrModule",
-                       (PyObject *)&BaseMgrModuleType);
+    auto load_class = [ceph_module](std::string name, PyTypeObject *type)


nit, could just pass const char* instead of a std::string

tchaikov · 2017-10-26T07:42:20Z

src/mgr/PyModuleRegistry.cc

+    Py_InitModule("ceph_crushmap", CRUSHMapMethods);
+    */
+
+    auto load_class = [ceph_module](std::string name, PyTypeObject *type)


nit, pass const char* instead.

tchaikov · 2017-10-26T07:44:38Z

@jcsp, could we split the mds related changes into separated PR(s)?

liewegas · 2017-10-30T20:43:39Z

http://pulpito.ceph.com/sage-2017-10-30_14:46:06-rados-wip-sage-testing-2017-10-30-0722-distro-basic-smithi/

Separate out the *loading* of modules from the *running* of modules. This is a precursor to enabling modules to run in standby mode. Signed-off-by: John Spray <john.spray@redhat.com>

Signed-off-by: John Spray <john.spray@redhat.com>

Modules can implement a second, separate class that has access to very little state about the system and can't implement commands. They have just enough information to redirect or forward incoming requests/traffic to the active instance of the module on the active mgr. This enables module authors to create modules that end users can access via any (running) mgr node at any time, rather than having to first work out which mgr node is active. Signed-off-by: John Spray <john.spray@redhat.com>

...they still don't have access to any config though. Signed-off-by: John Spray <john.spray@redhat.com>

Signed-off-by: John Spray <john.spray@redhat.com>

These would throw an exception when passed a status code. Signed-off-by: John Spray <john.spray@redhat.com>

...by using PyModuleRunner class from ActivePyModule too. Signed-off-by: John Spray <john.spray@redhat.com>

Was passing a reference to a local stringstream into Thread::create, not realising that it was taking a char* reference instead of a copy. Result was garbage (or usually, all threads having the name of the last one created) Signed-off-by: John Spray <john.spray@redhat.com>

Was calling way too early, which did a Py_Finalize before the modules had been joined. Signed-off-by: John Spray <john.spray@redhat.com>

Signed-off-by: Sage Weil <sage@redhat.com>

Implement real python classes from the C side, rather than exposing only module methods. Signed-off-by: John Spray <john.spray@redhat.com>

A bunch of the previous commits were done before this class existed, so updating in one go instead of trying to edit history in fine detail. Signed-off-by: John Spray <john.spray@redhat.com>

This was doing a Py_DECREF outside of the Gil. Fixes: http://tracker.ceph.com/issues/21593 Signed-off-by: John Spray <john.spray@redhat.com>

These didn't need to keep the GIL to go and do their pure C++ parts, and by keeping it they could deadlock while trying to take ActiveMgrModules::lock. Signed-off-by: John Spray <john.spray@redhat.com>

Signed-off-by: John Spray <john.spray@redhat.com>

Some extra coverage of the dashboard, including its standby redirect mode and the publishing of URIs. Also invoking the command_spam mode of the selftest module. Signed-off-by: John Spray <john.spray@redhat.com>

This was still using Kraken era settings Signed-off-by: John Spray <john.spray@redhat.com>

Signed-off-by: John Spray <john.spray@redhat.com>

...including the new "mgr services" command. Signed-off-by: John Spray <john.spray@redhat.com>

Otherwise, when someone wants to see what's possible to do with `mgr module enable` they have to trawl through the whole mgr map dump. Signed-off-by: John Spray <john.spray@redhat.com>

ceph-jenkins · 2017-11-01T12:24:15Z

all commits in this PR are signed

ceph-jenkins · 2017-11-01T12:25:23Z

submodules for project are unmodified

ceph-jenkins · 2017-11-01T12:30:17Z

OK - docs built

ceph-jenkins · 2017-11-01T13:01:05Z

make check succeeded

ceph-jenkins · 2017-11-01T13:09:06Z

make check succeeded

jcsp added the mgr label Jul 28, 2017

jcsp force-pushed the wip-mgr-module-interface branch from fe2ebda to dd6a676 Compare August 3, 2017 22:46

jcsp force-pushed the wip-mgr-module-interface branch from dd6a676 to c987acb Compare August 15, 2017 10:57

jcsp force-pushed the wip-mgr-module-interface branch from 3022d7f to 41fc275 Compare August 25, 2017 11:38

jcsp force-pushed the wip-mgr-module-interface branch 2 times, most recently from 5644228 to 927920b Compare October 4, 2017 13:52

jcsp changed the title ~~[DNM] mgr module interface improvements~~ mgr: python interface rework + enable modules to run in standby mode Oct 4, 2017

ceph deleted a comment from amitkumar50 Oct 4, 2017

jcsp force-pushed the wip-mgr-module-interface branch from 927920b to 15f4add Compare October 6, 2017 17:00

tchaikov self-requested a review October 9, 2017 08:16

jcsp force-pushed the wip-mgr-module-interface branch 5 times, most recently from 6e5dd29 to 86f0766 Compare October 20, 2017 12:08

jcsp force-pushed the wip-mgr-module-interface branch from c8f0882 to 0baa5b6 Compare October 25, 2017 13:55

ceph deleted a comment from amitkumar50 Oct 26, 2017

tchaikov reviewed Oct 26, 2017

View reviewed changes

tchaikov added the feature label Oct 26, 2017

liewegas added the wip-sage-testing label Oct 30, 2017

John Spray and others added 22 commits November 1, 2017 08:20

mgr: refactor python module management

9718896

Separate out the *loading* of modules from the *running* of modules. This is a precursor to enabling modules to run in standby mode. Signed-off-by: John Spray <john.spray@redhat.com>

mgr: clean up python source file naming

70d45a6

Signed-off-by: John Spray <john.spray@redhat.com>

mgr: standby modules come up and run now

c1471c7

...they still don't have access to any config though. Signed-off-by: John Spray <john.spray@redhat.com>

pybind/mgr: add MgrStandbyModule

3048e85

Signed-off-by: John Spray <john.spray@redhat.com>

mgr/dashboard: implement standby mode

4f7007d

Signed-off-by: John Spray <john.spray@redhat.com>

mon/MgrMonitor: reset services map on drop_active

7b629ae

Signed-off-by: John Spray <john.spray@redhat.com>

mgr: fix os._exit overrides

e2442c1

These would throw an exception when passed a status code. Signed-off-by: John Spray <john.spray@redhat.com>

mgr: cut down duplication between active+standby

df87973

...by using PyModuleRunner class from ActivePyModule too. Signed-off-by: John Spray <john.spray@redhat.com>

mgr: fix py_module_registry shutdown

0d5b1d7

Was calling way too early, which did a Py_Finalize before the modules had been joined. Signed-off-by: John Spray <john.spray@redhat.com>

mgr/PyOSDMap: add CRUSH get_item_weight

eacc902

Signed-off-by: Sage Weil <sage@redhat.com>

mgr: refactor PyOSDMap etc implementation

7e61f79

Implement real python classes from the C side, rather than exposing only module methods. Signed-off-by: John Spray <john.spray@redhat.com>

mgr: update for SafeThreadState

29193a4

A bunch of the previous commits were done before this class existed, so updating in one go instead of trying to edit history in fine detail. Signed-off-by: John Spray <john.spray@redhat.com>

mgr: fix ~MonCommandCompletion

58dfa97

This was doing a Py_DECREF outside of the Gil. Fixes: http://tracker.ceph.com/issues/21593 Signed-off-by: John Spray <john.spray@redhat.com>

mgr: drop GIL around set_uri, set_health_checks

27ee148

These didn't need to keep the GIL to go and do their pure C++ parts, and by keeping it they could deadlock while trying to take ActiveMgrModules::lock. Signed-off-by: John Spray <john.spray@redhat.com>

mgr/selftest: extend test and add background spam mode

a382c3f

Signed-off-by: John Spray <john.spray@redhat.com>

qa: expand mgr testing

05e648b

Some extra coverage of the dashboard, including its standby redirect mode and the publishing of URIs. Also invoking the command_spam mode of the selftest module. Signed-off-by: John Spray <john.spray@redhat.com>

qa: fix mgr caps

9988ebe

This was still using Kraken era settings Signed-off-by: John Spray <john.spray@redhat.com>

doc: describe how to implement standby modules

ceb2a91

Signed-off-by: John Spray <john.spray@redhat.com>

doc: describe using mgr module ... commands

d220e1a

...including the new "mgr services" command. Signed-off-by: John Spray <john.spray@redhat.com>

mon: include disabled modules in mgr module ls

5861c4c

Otherwise, when someone wants to see what's possible to do with `mgr module enable` they have to trawl through the whole mgr map dump. Signed-off-by: John Spray <john.spray@redhat.com>

jcsp force-pushed the wip-mgr-module-interface branch from aff38be to 5861c4c Compare November 1, 2017 12:23

liewegas merged commit 2912364 into ceph:master Nov 1, 2017

jcsp deleted the wip-mgr-module-interface branch November 1, 2017 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr: python interface rework + enable modules to run in standby mode #16651

mgr: python interface rework + enable modules to run in standby mode #16651

jcsp commented Jul 28, 2017 •

edited

wido commented Oct 5, 2017

jcsp commented Oct 5, 2017

ceph-jenkins commented Oct 6, 2017

wido commented Oct 9, 2017

ceph-jenkins commented Oct 24, 2017

tchaikov Oct 26, 2017

tchaikov Oct 26, 2017

jcsp Nov 1, 2017

tchaikov Oct 26, 2017

jcsp Nov 1, 2017

tchaikov Oct 26, 2017

jcsp Nov 1, 2017

tchaikov Oct 26, 2017

jcsp Nov 1, 2017

tchaikov Oct 26, 2017

jcsp Nov 1, 2017

tchaikov Oct 26, 2017

tchaikov Oct 26, 2017

jcsp Nov 1, 2017

tchaikov commented Oct 26, 2017

liewegas commented Oct 30, 2017

ceph-jenkins commented Nov 1, 2017

ceph-jenkins commented Nov 1, 2017

ceph-jenkins commented Nov 1, 2017

ceph-jenkins commented Nov 1, 2017

ceph-jenkins commented Nov 1, 2017


		PyModuleRunnerThread thread;

		std::string const &get_name() { return module_name; }

mgr: python interface rework + enable modules to run in standby mode #16651

mgr: python interface rework + enable modules to run in standby mode #16651

Conversation

jcsp commented Jul 28, 2017 • edited

wido commented Oct 5, 2017

jcsp commented Oct 5, 2017

ceph-jenkins commented Oct 6, 2017

wido commented Oct 9, 2017

ceph-jenkins commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 26, 2017

liewegas commented Oct 30, 2017

ceph-jenkins commented Nov 1, 2017

ceph-jenkins commented Nov 1, 2017

ceph-jenkins commented Nov 1, 2017

ceph-jenkins commented Nov 1, 2017

ceph-jenkins commented Nov 1, 2017

jcsp commented Jul 28, 2017 •

edited