mgr: load MgrModule.OPTIONS and use it in upgrade

Modules will now only migrate items from config-key
into config options if they appear in the module's
schema.

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

diff --git a/src/mgr/PyModule.cc b/src/mgr/PyModule.cc
index 89927a72c64b5669dfd11c9fc231f85cc400cfac..65656d0a60a864ced379399089e14d73f622f0c4 100644 (file)
--- a/src/mgr/PyModule.cc
+++ b/src/mgr/PyModule.cc
@@ -363,8 +363,17 @@ int PyModule::load(PyThreadState *pMainThreadState)
 
     r = load_commands();
     if (r != 0) {
-      derr << "Missing COMMAND attr in module '" << module_name << "'" << dendl;
-      error_string = "Missing COMMAND attribute";
+      derr << "Missing or invalid COMMANDS attribute in module '"
+          << module_name << "'" << dendl;
+      error_string = "Missing or invalid COMMANDS attribute";
+      return r;
+    }
+
+    r = load_options();
+    if (r != 0) {
+      derr << "Missing or invalid OPTIONS attribute in module '"
+          << module_name << "'" << dendl;
+      error_string = "Missing or invalid OPTIONS attribute";
       return r;
     }
 
@@ -421,63 +430,111 @@ int PyModule::load(PyThreadState *pMainThreadState)
   return 0;
 }
 
-int PyModule::load_commands()
+int PyModule::walk_dict_list(
+    const std::string &attr_name,
+    std::function<int(PyObject*)> fn)
 {
-  // Don't need a Gil here -- this is called from load(),
-  // which already has one.
-  PyObject *command_list = PyObject_GetAttrString(pClass, "COMMANDS");
+  PyObject *command_list = PyObject_GetAttrString(pClass, attr_name.c_str());
   if (command_list == nullptr) {
-    // Even modules that don't define command should still have the COMMANDS
-    // from the MgrModule definition.  Something is wrong!
-    derr << "Module " << get_name() << " has missing COMMANDS member" << dendl;
+    derr << "Module " << get_name() << " has missing " << attr_name
+         << " member" << dendl;
     return -EINVAL;
   }
   if (!PyObject_TypeCheck(command_list, &PyList_Type)) {
     // Relatively easy mistake for human to make, e.g. defining COMMANDS
     // as a {} instead of a []
-    derr << "Module " << get_name() << " has COMMANDS member of wrong type ("
-            "should be a list)" << dendl;
+    derr << "Module " << get_name() << " has " << attr_name
+         << " member of wrong type (should be a list)" << dendl;
     return -EINVAL;
   }
+
+  // Invoke fn on each item in the list
+  int r = 0;
   const size_t list_size = PyList_Size(command_list);
   for (size_t i = 0; i < list_size; ++i) {
     PyObject *command = PyList_GetItem(command_list, i);
     assert(command != nullptr);
 
-    ModuleCommand item;
+    if (!PyDict_Check(command)) {
+      derr << "Module " << get_name() << " has non-dict entry "
+           << "in " << attr_name << " list" << dendl;
+      return -EINVAL;
+    }
 
-    PyObject *pCmd = PyDict_GetItemString(command, "cmd");
+    r = fn(command);
+    if (r != 0) {
+      break;
+    }
+  }
+  Py_DECREF(command_list);
+
+  return r;
+}
+
+int PyModule::load_commands()
+{
+  int r = walk_dict_list("COMMANDS", [this](PyObject *pCommand) -> int {
+    ModuleCommand command;
+
+    PyObject *pCmd = PyDict_GetItemString(pCommand, "cmd");
     assert(pCmd != nullptr);
-    item.cmdstring = PyString_AsString(pCmd);
+    command.cmdstring = PyString_AsString(pCmd);
 
-    dout(20) << "loaded command " << item.cmdstring << dendl;
+    dout(20) << "loaded command " << command.cmdstring << dendl;
 
-    PyObject *pDesc = PyDict_GetItemString(command, "desc");
+    PyObject *pDesc = PyDict_GetItemString(pCommand, "desc");
     assert(pDesc != nullptr);
-    item.helpstring = PyString_AsString(pDesc);
+    command.helpstring = PyString_AsString(pDesc);
 
-    PyObject *pPerm = PyDict_GetItemString(command, "perm");
+    PyObject *pPerm = PyDict_GetItemString(pCommand, "perm");
     assert(pPerm != nullptr);
-    item.perm = PyString_AsString(pPerm);
+    command.perm = PyString_AsString(pPerm);
 
-    item.polling = false;
-    PyObject *pPoll = PyDict_GetItemString(command, "poll");
+    command.polling = false;
+    PyObject *pPoll = PyDict_GetItemString(pCommand, "poll");
     if (pPoll) {
       std::string polling = PyString_AsString(pPoll);
       if (boost::iequals(polling, "true")) {
-        item.polling = true;
+        command.polling = true;
       }
     }
 
-    item.module_name = module_name;
+    command.module_name = module_name;
 
-    commands.push_back(item);
-  }
-  Py_DECREF(command_list);
+    commands.push_back(std::move(command));
+
+    return 0;
+  });
 
   dout(10) << "loaded " << commands.size() << " commands" << dendl;
 
-  return 0;
+  return r;
+}
+
+int PyModule::load_options()
+{
+  int r = walk_dict_list("OPTIONS", [this](PyObject *pOption) -> int {
+    PyObject *pName = PyDict_GetItemString(pOption, "name");
+    assert(pName != nullptr);
+
+    ModuleOption option;
+    option.name = PyString_AsString(pName);
+    dout(20) << "loaded option " << option.name << dendl;
+
+    options[option.name] = std::move(option);
+
+    return 0;
+  });
+
+  dout(10) << "loaded " << options.size() << " options" << dendl;
+
+  return r;
+}
+
+bool PyModule::is_option(const std::string &option_name)
+{
+  Mutex::Locker l(lock);
+  return options.count(option_name) > 0;
 }
 
 int PyModule::load_subclass_of(const char* base_class, PyObject** py_class)

diff --git a/src/mgr/PyModule.h b/src/mgr/PyModule.h
index 512313d60da0a32f50546bb675da5206f31e2143..d9ce1695609cab73a6a7d2dfcf1eb88f1e2e47d6 100644 (file)
--- a/src/mgr/PyModule.h
+++ b/src/mgr/PyModule.h
@@ -41,6 +41,15 @@ public:
   std::string module_name;
 };
 
+
+/**
+ * An option declared by the python module in its configuration schema
+ */
+class ModuleOption {
+  public:
+  std::string name;
+};
+
 class PyModule
 {
   mutable Mutex lock{"PyModule::lock"};
@@ -67,9 +76,17 @@ private:
   // Populated if loaded, can_run or failed indicates a problem
   std::string error_string;
 
+  // Helper for loading OPTIONS and COMMANDS members
+  int walk_dict_list(
+      const std::string &attr_name,
+      std::function<int(PyObject*)> fn);
+
   int load_commands();
   std::vector<ModuleCommand> commands;
 
+  int load_options();
+  std::map<std::string, ModuleOption> options;
+
 public:
   static std::string config_prefix;
 
@@ -84,6 +101,8 @@ public:
 
   ~PyModule();
 
+  bool is_option(const std::string &option_name);
+
   int load(PyThreadState *pMainThreadState);
 #if PY_MAJOR_VERSION >= 3
   static PyObject* init_ceph_logger();

diff --git a/src/mgr/PyModuleRegistry.cc b/src/mgr/PyModuleRegistry.cc
index 940d65f60541fcbdb6bf4ce3b3a2a8716417d263..2364b1cfb8b79980242ec79809c27d14d1f40b97 100644 (file)
--- a/src/mgr/PyModuleRegistry.cc
+++ b/src/mgr/PyModuleRegistry.cc
@@ -433,9 +433,41 @@ void PyModuleRegistry::upgrade_config(
         continue;
       }
 
-      module_config.set_config(monc, module_name, key, i.second);
-      dout(4) << "Rewrote configuration module:key "
-              << module_name << ":" << key << dendl;
+      // Check that the named module exists
+      auto module_iter = modules.find(module_name);
+      if (module_iter == modules.end()) {
+        dout(1) << "KV store contains data for unknown module '"
+                << module_name << "'" << dendl;
+        continue;
+      }
+      PyModuleRef module = module_iter->second;
+
+      // Parse option name out of key
+      std::string option_name;
+      auto slash_loc = key.find("/");
+      if (slash_loc != std::string::npos) {
+        if (key.size() > slash_loc + 1) {
+          // Localized option
+          option_name = key.substr(slash_loc + 1);
+        } else {
+          // Trailing slash: garbage.
+          derr << "Invalid mgr store key: '" << key << "'" << dendl;
+          continue;
+        }
+      } else {
+        option_name = key;
+      }
+
+      // Consult module schema to see if this is really
+      // a configuration value
+      if (!option_name.empty() && module->is_option(option_name)) {
+        module_config.set_config(monc, module_name, key, i.second);
+        dout(4) << "Rewrote configuration module:key "
+                << module_name << ":" << key << dendl;
+      } else {
+        dout(4) << "Leaving store module:key " << module_name
+                << ":" << key << " in store, not config" << dendl;
+      }
     }
   } else {
     dout(10) << "Module configuration contains "