mgr: In plugins 'module' classes need not to be called "Module" anymore #18171

bhavishyagopesh · 2017-10-09T05:42:46Z

Signed-off-by: bhavishyagopesh bhavishyagopesh@gmail.com

tchaikov · 2017-10-09T08:06:53Z

src/mgr/MgrPyModule.cc

@@ -172,31 +172,43 @@ int MgrPyModule::load()

  // Find the class
  // TODO: let them call it what they want instead of just 'Module'
-  auto pClass = PyObject_GetAttrString(pModule, (const char*)"Module");
+  auto tempClass = PyObject_GetAttrString(pModule, (const char*)"MgrModule");


yes, checking the tp_subclasses might work. but because tp_subclasses is an internal member variable and only for CPython's internal usage, this is fragile. we should get using PyModule_GetDict() and check them using PyObject_IsSubclass(), the pseudo code looks like

auto locals = PyModule_GetDict(pModule); PyObject *key, *value; Py_ssize_t pos = 0; while (PyDict_Next(locals, &pos, &key, &value)) { if (!PyType_Check(value)) { continue; } if (PyObject_IsSubclass(value, mgr_module_type)) { // ... } }

where mgr_module_type is thePyObject of MgrModule .

bhavishyagopesh · 2017-10-09T20:13:20Z

@tchaikov @jcsp I have made the appropriate changes, thanks.

tchaikov · 2017-10-10T03:14:45Z

src/mgr/MgrPyModule.cc

+
+  if (!PyType_Check(value)) {
+    continue;
+    }


wrong indent

tchaikov · 2017-10-10T03:15:19Z

src/mgr/MgrPyModule.cc

+    continue;
+    }
+
+  if (PyObject_IsSubclass(value, mgr_module_type)) {


if (!PyObject_IsSubclass(value, mgr_module_type)) { continue; }

to reduce the indent level.

@bhavishyagopesh could you reconsider this suggestion?

tchaikov · 2017-10-10T03:16:01Z

src/mgr/MgrPyModule.cc

+  PyObject *key, *value;
+  Py_ssize_t pos = 0;
+
+  if (mgr_module_type == nullptr) {


move this check to where mgr_module_type is initialized.

tchaikov · 2017-10-10T03:18:40Z

src/mgr/MgrPyModule.cc

+      Py_DECREF(pyHandle);
+      Py_DECREF(pArgs);
+      if (pClassInstance == nullptr) {
+        derr << "Failed to construct class in '" << module_name << "'" << dendl;


it would be helpful if we can print out the name (key) of the class.

tchaikov · 2017-10-10T03:19:10Z

src/mgr/MgrPyModule.cc

+        derr << handle_pyerror() << dendl;
+        return -EINVAL;
+      } else {
+        dout(1) << "Constructed class from module: " << module_name << dendl;


tchaikov · 2017-10-10T03:19:20Z

src/mgr/MgrPyModule.cc

+        dout(1) << "Constructed class from module: " << module_name << dendl;
+      }
+    }
+


remove this empty line, please.

bhavishyagopesh · 2017-10-10T07:53:10Z

@tchaikov Any other suggestions?

tchaikov · 2017-10-10T07:58:15Z

src/mgr/MgrPyModule.cc

-  Py_DECREF(pModule);
-  if (pClass == nullptr) {
+  auto  mgr_module_type = PyObject_GetAttrString(pModule, (const char*)"MgrModule");
+


remove this empty line.

tchaikov · 2017-10-10T07:58:47Z

src/mgr/MgrPyModule.cc

-    return -EINVAL;
-  } else {
-    dout(1) << "Constructed class from module: " << module_name << dendl;
+  Py_DECREF(pModule);


we cannot deref pModule yet. we are actually referencing it right after this line.

tchaikov · 2017-10-10T07:59:04Z

src/mgr/MgrPyModule.cc

+  Py_ssize_t pos = 0;
+
+  while (PyDict_Next(locals, &pos, &key, &value)) {
+


remove this empty line.

tchaikov · 2017-10-10T07:59:08Z

src/mgr/MgrPyModule.cc

+    if (!PyType_Check(value)) {
+      continue;
+    }
+


tchaikov · 2017-10-10T07:59:16Z

src/mgr/MgrPyModule.cc

+    }
+
+    if (PyObject_IsSubclass(value, mgr_module_type)) {
+


tchaikov · 2017-10-10T08:02:50Z

src/mgr/MgrPyModule.cc

+        Py_DECREF(pyHandle);
+        Py_DECREF(pArgs);
+        if (pClassInstance == nullptr) {
+          derr << "Failed to construct class '" << PyObject_Str(pClass) << "'" << " in " << "'" << module_name << "'" << dendl;


why not use key? also PyObject_Str() returns a new reference, we should deref after using it. and what's the point of printing a PyObjecter pointer?

fwiw, use PyString_AsString(key) to retrieve the c string from it.

tchaikov · 2017-10-10T08:03:31Z

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes

tchaikov · 2017-10-10T08:07:43Z

src/mgr/MgrPyModule.cc

-  if (pClass == nullptr) {
+  auto  mgr_module_type = PyObject_GetAttrString(pModule, (const char*)"MgrModule");
+
+  if (mgr_module_type == nullptr) {


i think the plugin is not required to expose MgrModule. can we load this class from mgr_module instead?

bhavishyagopesh · 2017-10-10T19:35:25Z

@tchaikov Any more improvements?

ceph-jenkins · 2017-10-11T02:11:51Z

OK - docs built

tchaikov · 2017-10-11T02:45:24Z

src/mgr/MgrPyModule.cc

+        } else {
+          dout(1) << "Constructed class: " << PyString_AsString(key) << " from module: " << module_name << dendl;
+        }
+        Py_DECREF(pClass);


nit, could move this line next to Py_DECREF(pyHandle);.

tchaikov · 2017-10-11T02:46:40Z

src/mgr/MgrPyModule.cc

-  auto pClass = PyObject_GetAttrString(pModule, (const char*)"Module");
-  Py_DECREF(pModule);
-  if (pClass == nullptr) {
+  auto  mgr_module_type =  PyImport_ImportModule("mgr_module.MgrModule");


remove the extra space after auto.

please Py_DECREF(mgr_module_type) after done with it.

tchaikov · 2017-10-11T02:50:32Z

src/mgr/MgrPyModule.cc

-  Py_DECREF(pModule);
-  if (pClass == nullptr) {
+  auto  mgr_module_type =  PyImport_ImportModule("mgr_module.MgrModule");
+  if (mgr_module_type == nullptr) {
    derr << "Class not found in module '" << module_name << "'" << dendl;


change the error message to something like

Unable to import mgr_module from MgrModule

as we are not importing the Module from from $module_name here.

tchaikov · 2017-10-11T02:53:03Z

src/mgr/MgrPyModule.cc

+  auto locals = PyModule_GetDict(pModule);
+  PyObject *key, *value;
+  Py_ssize_t pos = 0;
+  Py_DECREF(pModule);


nit, move this line right after PyModule_GetDict(pModule).

bhavishyagopesh · 2017-10-11T19:30:48Z

@tchaikov any more suggestions?

tchaikov · 2017-10-12T05:25:49Z

src/mgr/MgrPyModule.cc

+    pClassInstance = PyObject_CallObject(pClass, pArgs);
+    Py_DECREF(pyHandle);
+    Py_DECREF(pArgs);
+    Py_DECREF(pClass);


please don't deref pClass here, as we are not holding a reference to it.

tchaikov · 2017-10-12T05:28:06Z

src/mgr/MgrPyModule.cc

+    Py_DECREF(pArgs);
+    Py_DECREF(pClass);
+    if (pClassInstance == nullptr) {
+      derr << "Failed to construct class '" << PyString_AsString(key) << "'" << " in " << "'" << module_name << "'" << dendl;


might want to have a variable holding the retval of PyString_AsString(key), like

auto class_name = PyString_AsString(key);

as you are referencing the class using pClass, while referencing its name using PyString_AsString(key).

bhavishyagopesh · 2017-10-13T08:22:01Z

@tchaikov Probably due to some ci/infra issues build is not being reported...but if you still find any improvement you could mention it. Thanks.

tchaikov · 2017-10-13T08:59:57Z

@bhavishyagopesh IIRC, this PR is addressing a ticket a tracker, could you reference it in the commit message? like

Fixes: http://tracker.ceph.com/issues/123456
Signed-off-by: bhavishyagopesh <bhavishyagopesh@gmail.com>

please replace "123456" with the actual ticket id.

bhavishyagopesh · 2017-10-16T14:10:39Z

@tchaikov is it okay now?

jcsp · 2017-10-16T14:18:12Z

@bhavishyagopesh please could you test this yourself and let us know if it is working

bhavishyagopesh · 2017-10-17T02:43:52Z

@jcsp Are there any available Dockerfiles that I can run on local system...only for testing purpose.

bhavishyagopesh · 2017-10-17T20:59:52Z

@tchaikov @jcsp Is there some way of running the qa suite without openstack/bare-metal... I lack the hardware to run test-suite.

tchaikov · 2017-10-18T02:05:38Z

please run the minimal test, vstart would suffice. see http://docs.ceph.com/docs/master/dev/quick_guide/

bhavishyagopesh · 2017-10-18T09:14:20Z

@tchaikov two tests fail but they do not look related to plugins.

     The following tests FAILED:
	  7 - run-tox-ceph-disk (Failed)
	  8 - run-tox-ceph-detect-init (Failed)

tchaikov · 2017-10-18T10:11:55Z

@bhavishyagopesh how did you test this change?

bhavishyagopesh · 2017-10-19T03:58:36Z

@tchaikov I ran the run-make-check.sh script and vstart script inside an ubuntu docker(as the scripts do not work for my base installation)...but i guess for testing this specific plugin I should do ceph mgr module enable $plugin but due to above errors there is no ceph binary

tchaikov · 2017-10-19T04:15:16Z

restful, status, dashboard and balancer are enabled by default in vstart.sh. all you need to do is to make sure they are detected and loaded by ceph-mgr.

ceph cli is not a binary, it's a python script. and should be built by default.

bhavishyagopesh · 2017-10-19T08:47:42Z

@tchaikov it does detect restful, status, dashboard and balancer...though restful should this error MGR Restful is not working, perhaps the package is not installed?
`

tchaikov · 2017-10-19T08:51:20Z

it does detect restful, status, dashboard and balancer

again, how do you test this?

bhavishyagopesh · 2017-10-19T14:49:33Z

@tchaikov I now ran the prometheus plugin on localhostport-forwarded from docker) and also I was able to access dashboard on an internal url ...so is that test sufficient..

bhavishyagopesh · 2017-10-23T07:48:12Z

@jcsp @tchaikov is the above test insufficient to ensure that plugins are loaded properly("this change is okay")?

tchaikov · 2017-10-23T08:12:02Z

src/mgr/MgrPyModule.cc

-  Py_DECREF(pModule);
-  if (pClass == nullptr) {
-    derr << "Class not found in module '" << module_name << "'" << dendl;
+  PyObject *mgr_module_name = PyString_FromString("mgr_module");


@bhavishyagopesh why don't you just use PyImport_ImportModule() ?

see. #18171 (comment)

@tchaikov I messed that up, I have changed it now.

Fixes: http://tracker.ceph.com/issues/17454 Signed-off-by: bhavishyagopesh <bhavishyagopesh@gmail.com>

tchaikov · 2017-10-23T11:46:58Z

@bhavishyagopesh the test above would do.

tchaikov

i am posting a fixed version at #18526

tchaikov · 2017-10-25T05:07:49Z

src/mgr/MgrPyModule.cc

+    }
+    // Just using the module name as the handle, replace with a
+    // uuidish thing if needed
+    auto pClass = value;


there is chance the last loaded class is MgrModule , which overwrites the one we want to load. so pClassInstance will be a MgrModule instance.

tchaikov · 2017-10-25T05:12:27Z

closing in favor of #18526

tchaikov · 2017-10-25T05:35:17Z

http://pulpito.ceph.com/kchai-2017-10-24_16:44:55-rados-wip-kefu-testing-2017-10-24-2303-distro-basic-mira/

failures are due to localpool and influx plugins' failure to load.

jcsp · 2017-10-31T16:34:12Z

Seems like the failures are not exactly from failures to load but more from the way the tests are a bit racy (doesn't guarantee command descriptions have loaded before running).

The tests get more robust in #16651

tchaikov reviewed Oct 9, 2017

View reviewed changes

tchaikov requested a review from jcsp October 9, 2017 08:07

tchaikov added the mgr label Oct 9, 2017

bhavishyagopesh force-pushed the feat/17454 branch from 88f532c to d9a6378 Compare October 9, 2017 19:28

tchaikov reviewed Oct 10, 2017

View reviewed changes

bhavishyagopesh force-pushed the feat/17454 branch 2 times, most recently from 4dab29a to f0a6b56 Compare October 10, 2017 06:19

tchaikov requested changes Oct 10, 2017

View reviewed changes

tchaikov reviewed Oct 10, 2017

View reviewed changes

bhavishyagopesh force-pushed the feat/17454 branch from f0a6b56 to 57f9fa2 Compare October 10, 2017 09:34

jcsp added the feature label Oct 10, 2017

bhavishyagopesh force-pushed the feat/17454 branch from 57f9fa2 to 89f1c41 Compare October 11, 2017 02:07

tchaikov reviewed Oct 11, 2017

View reviewed changes

tchaikov changed the title ~~Module classes are not forced to be called "Module". fixes #17454~~ mgr: In plugins 'module' classes need not to be called "Module" anymore Oct 11, 2017

tchaikov reviewed Oct 11, 2017

View reviewed changes

bhavishyagopesh force-pushed the feat/17454 branch from 89f1c41 to 5ec26ad Compare October 11, 2017 08:26

tchaikov requested changes Oct 12, 2017

View reviewed changes

bhavishyagopesh force-pushed the feat/17454 branch from 5ec26ad to 72b6c5d Compare October 12, 2017 10:36

tchaikov approved these changes Oct 13, 2017

View reviewed changes

tchaikov added the needs-qa label Oct 13, 2017

bhavishyagopesh force-pushed the feat/17454 branch from 1fe58d2 to e4a851d Compare October 16, 2017 08:13

tchaikov reviewed Oct 23, 2017

View reviewed changes

bhavishyagopesh force-pushed the feat/17454 branch from e4a851d to 4a95b4f Compare October 23, 2017 09:30

mgr: In plugins 'module' classes need not to be called "Module" anymore.

4a95b4f

Fixes: http://tracker.ceph.com/issues/17454 Signed-off-by: bhavishyagopesh <bhavishyagopesh@gmail.com>

tchaikov added the needs-qa label Oct 23, 2017

tchaikov approved these changes Oct 23, 2017

View reviewed changes

tchaikov added the wip-kefu-testing label Oct 24, 2017

tchaikov requested changes Oct 25, 2017

View reviewed changes

tchaikov closed this Oct 25, 2017

		Py_ssize_t pos = 0;

		while (PyDict_Next(locals, &pos, &key, &value)) {

mgr: In plugins 'module' classes need not to be called "Module" anymore #18171

mgr: In plugins 'module' classes need not to be called "Module" anymore #18171

Conversation

bhavishyagopesh commented Oct 9, 2017

tchaikov Oct 9, 2017 • edited

Choose a reason for hiding this comment

bhavishyagopesh commented Oct 9, 2017

Choose a reason for hiding this comment

tchaikov Oct 10, 2017 • edited

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

bhavishyagopesh commented Oct 10, 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

tchaikov commented Oct 10, 2017

Choose a reason for hiding this comment

bhavishyagopesh commented Oct 10, 2017

ceph-jenkins commented Oct 11, 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

bhavishyagopesh commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhavishyagopesh commented Oct 13, 2017

tchaikov commented Oct 13, 2017

bhavishyagopesh commented Oct 16, 2017 • edited

jcsp commented Oct 16, 2017

bhavishyagopesh commented Oct 17, 2017

bhavishyagopesh commented Oct 17, 2017

tchaikov commented Oct 18, 2017 • edited

bhavishyagopesh commented Oct 18, 2017 • edited

tchaikov commented Oct 18, 2017

bhavishyagopesh commented Oct 19, 2017

tchaikov commented Oct 19, 2017 • edited

bhavishyagopesh commented Oct 19, 2017

tchaikov commented Oct 19, 2017

bhavishyagopesh commented Oct 19, 2017

bhavishyagopesh commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 23, 2017

tchaikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 25, 2017

tchaikov commented Oct 25, 2017

jcsp commented Oct 31, 2017

tchaikov Oct 9, 2017 •

edited

tchaikov Oct 10, 2017 •

edited

bhavishyagopesh commented Oct 16, 2017 •

edited

tchaikov commented Oct 18, 2017 •

edited

bhavishyagopesh commented Oct 18, 2017 •

edited

tchaikov commented Oct 19, 2017 •

edited