Re: [Kimchi-devel] [PATCH v2][Wok 1/8] Creates pluginmanager.py module

On 06/08/2016 06:15 PM, Aline Manera wrote: > > > On 06/08/2016 05:59 PM, Rodrigo Trujillo wrote: >> >> >> On 06/08/2016 05:28 PM, Aline Manera wrote: >>> >>> >>> On 06/08/2016 05:16 PM, Rodrigo Trujillo wrote: >>>> >>>> >>>> On 06/08/2016 03:52 PM, Aline Manera wrote: >>>>> >>>>> >>>>> On 06/06/2016 04:13 PM, Rodrigo Trujillo wrote: >>>>>> Signed-off-by: Rodrigo Trujillo >>>>>> <rodrigo.trujillo(a)linux.vnet.ibm.com&gt; >>>>>> --- >>>>>> src/wok/pluginsmanager.py | 238 >>>>>> ++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 238 insertions(+) >>>>>> create mode 100644 src/wok/pluginsmanager.py >>>>>> >>>>>> diff --git a/src/wok/pluginsmanager.py b/src/wok/pluginsmanager.py >>>>>> new file mode 100644 >>>>>> index 0000000..d799f11 >>>>>> --- /dev/null >>>>>> +++ b/src/wok/pluginsmanager.py >>>>>> @@ -0,0 +1,238 @@ >>>>>> +# >>>>>> +# Project Wok >>>>>> +# >>>>>> +# Copyright IBM Corp, 2016 >>>>>> +# >>>>>> +# This library is free software; you can redistribute it and/or >>>>>> +# modify it under the terms of the GNU Lesser General Public >>>>>> +# License as published by the Free Software Foundation; either >>>>>> +# version 2.1 of the License, or (at your option) any later >>>>>> version. >>>>>> +# >>>>>> +# This library is distributed in the hope that it will be useful, >>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>>>>> +# Lesser General Public License for more details. >>>>>> +# >>>>>> +# You should have received a copy of the GNU Lesser General Public >>>>>> +# License along with this library; if not, write to the Free >>>>>> Software >>>>>> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, >>>>>> MA 02110-1301 USA >>>>>> +# >>>>>> + >>>>>> + >>>>>> +import cherrypy >>>>>> +import os >>>>>> +import xml.etree.ElementTree as ET >>>>>> +from cherrypy.lib.reprconf import Parser >>>>>> +from configobj import ConfigObj >>>>>> + >>>>>> + >>>>>> +from basemodel import Singleton >>>>>> +from wok.config import paths, PluginConfig, PluginPaths >>>>>> +from wok.exception import OperationFailed >>>>>> +from wok.utils import import_class, wok_log >>>>>> + >>>>>> + >>>>>> +class Plugins(): >>>>>> + __metaclass__ = Singleton >>>>>> + >>>>>> + def __init__(self, options=None): >>>>>> + # { '<PLUGIN_NAME>': { >>>>>> + # config: <PLUGIN_CHERRYPY_CONFIG>, >>>>>> + # enabled: <TRUE/FALSE>, >>>>>> + # uri: <PLUGIN_URI_FROM_FILE_CONFIG>, >>>>>> + # app: <PLUGIN_MAIN_MODULE> >>>>>> + # conf_file: <PLUGIN_CONFIGURATION_FILE> >>>>>> + self._plugins_dict = {} >>>>>> + self.options = options >>>>>> + self._init_all_plugins() >>>>>> + >>>>>> + def _load_plugin_app(self, name): >>>>>> + """ >>>>>> + Loads the plugin main module >>>>>> + """ >>>>>> + plugin_class = ('plugins.%s.%s' % (name, >>>>>> name[0].upper() + name[1:])) >>>>>> + try: >>>>>> + plugin_app = import_class(plugin_class)(self.options) >>>>>> + except ImportError, e: >>>>>> + wok_log.error("Failed to import plugin %s, error: >>>>>> %s" % >>>>>> + (plugin_class, e.message)) >>>>>> + self._plugins_dict[name]['enabled'] = False >>>>>> + self._plugins_dict[name]['app'] = None >>>>>> + return >>>>>> + self._plugins_dict[name]['app'] = plugin_app >>>>>> + >>>>>> + def _load_plugin_app_config(self, name): >>>>>> + """ >>>>>> + Sets the plugin's cherrypy configuration. That is a >>>>>> merge between Wok >>>>>> + standard configuration plus plugin's self configuration >>>>>> + """ >>>>>> + app_conf = {} >>>>>> + app_conf.update(PluginConfig(name)) >>>>>> + plugin_app = self._plugins_dict[name]['app'] >>>>>> + if plugin_app is None: >>>>>> + return >>>>>> + >>>>>> + # dynamically extend plugin config with custom data, if >>>>>> provided >>>>>> + get_custom_conf = getattr(plugin_app, >>>>>> "get_custom_conf", None) >>>>>> + if get_custom_conf is not None: >>>>>> + app_conf.update(get_custom_conf()) >>>>>> + >>>>>> + # dynamically add tools.wokauth.on = True to extra >>>>>> plugin APIs >>>>>> + try: >>>>>> + sub_nodes = >>>>>> import_class('plugins.%s.control.sub_nodes' % >>>>>> + name) >>>>>> + urlSubNodes = {} >>>>>> + for ident, node in sub_nodes.items(): >>>>>> + if node.url_auth: >>>>>> + ident = "/%s" % ident >>>>>> + urlSubNodes[ident] = {'tools.wokauth.on': >>>>>> True} >>>>>> + app_conf.update(urlSubNodes) >>>>>> + except ImportError, e: >>>>>> + wok_log.error("Failed to import subnodes for plugin >>>>>> %s, error: %s" >>>>>> + % (name, e.message)) >>>>>> + self._plugins_dict[name]['config'] = app_conf >>>>>> + >>>>>> + def _load_plugin_config_file(self, name): >>>>>> + """ >>>>>> + Loads the information from Wok section in <plugin>.conf >>>>>> file. Currently >>>>>> + two tags/options are required to Wok and section looks >>>>>> like: >>>>>> + [wok] >>>>>> + enable = True >>>>>> + uri = '/plugins/mypluginpath' >>>>>> + """ >>>>>> + plugin_conf_file = PluginPaths(name).conf_file >>>>>> + config = {} >>>> >>>> I have set "config" here, to avoid the problems you reported below >>>> >>> >>> Oh. ok! I haven't noticed that before. >>> >>>>>> + if not os.path.exists(plugin_conf_file): >>>>>> + wok_log.error("Plugin configuration file %s doesn't >>>>>> exist." % >>>>>> + plugin_conf_file) >>>>>> + else: >>>>>> + try: >>>>>> + config = Parser().dict_from_file(plugin_conf_file) >>>>>> + except ValueError as e: >>>>>> + msg = "Failed to load plugin conf from %s: %s" >>>>>> + wok_log.error(msg % (plugin_conf_file, e.message)) >>>>>> + >>>>> >>>>>> + plugin = self._plugins_dict[name] >>>>>> + plugin['conf_file'] = plugin_conf_file >>>>>> + plugin['enabled'] = config.get('wok', {}).get('enable', >>>>>> False) >>>>>> + plugin['uri'] = config.get('wok', {}).get('uri', 'Unknow') >>>>>> + >>>>> >>>>> This block should be on 'else' statement (as part of the 'try' >>>>> block') or add a 'return' in the 'if' statement as the config >>>>> file was not found. >>>>> Otherwise, it will raise an exception as 'config' may be not >>>>> defined if it does not entered on 'else' block. The same will >>>>> happen if it goes to the 'except' block. So move it to 'try' block. >>>>> >>>>> Tip: if you add 'return' in the 'if' statement you don't need the >>>>> 'else' anymore and eliminate one indentation level. >>>>> >>>> See my comment above. I want to load the "possible" plugin as >>>> disabled, even if the config file is missing >>>> >>> >>> What about the uri? Setting it to 'unknown' is a good IMO. >>> We should log that the plugin was not loaded and ignore it from a >>> cherrypy perspective. >> >> That is exactly what I do in "_set_cherrypy_app". If the plugin is >> disabled it will not load into cherrypy > > I am not talking about disable plugins. I am talking about plugins > which does not have a configuration file and there is any problem > there prevent Wok to read its configuration. > In that case, we should not load the plugin as 'unknown" we should > log an error and that is all. I am not following you... I am not loading and I do log an error! I think you did not understood the workflow. Plugins Manager is not loading the plugin unless it is enable and someone request. The first thing that Plugins Manager do is search for directories and conf file in /plugin, and instantiate a dictionary for each. PM must know every possible plugin state and problems.

+ if not os.path.exists(plugin_conf_file): + wok_log.error("Plugin configuration file %s doesn't exist." % + plugin_conf_file) + else: + try: + config = Parser().dict_from_file(plugin_conf_file) + except ValueError as e: + msg = "Failed to load plugin conf from %s: %s" + wok_log.error(msg % (plugin_conf_file, e.message)) +

+ plugin = self._plugins_dict[name] + plugin['conf_file'] = plugin_conf_file + plugin['enabled'] = config.get('wok', {}).get('enable', False) + plugin['uri'] = config.get('wok', {}).get('uri', 'Unknow') +

> >> >>> >>>>>> + def _init_all_plugins(self): >>>>>> + """ >>>>>> + Initializes internal plugin dictionary, searching all >>>>>> directories in >>>>>> + <install_path>/wok/plugins, each directory should store >>>>>> a plugin >>>>>> + content. Then loads its configuration file in order to >>>>>> set as enabled >>>>>> + or disabled. >>>>>> + """ >>>>>> + plugin_dir = paths.plugins_dir >>>>>> + try: >>>>>> + dir_contents = os.listdir(plugin_dir) >>>>>> + except OSError as e: >>>>>> + wok_log.error("Failed to fetch plugins from '%s': >>>>>> %s" % >>>>>> + (plugin_dir, e.message)) >>>>>> + return >>>>>> + for name in dir_contents: >>>>>> + if os.path.isdir(os.path.join(plugin_dir, name)): >>>>>> + # TODO: >>>>>> + # Add command line option to disable plugin by >>>>>> its name >>>>>> + # >>>>>> + self._plugins_dict[name] = {} >>>>>> + self._plugins_dict[name]['config'] = {} >>>>>> + self._plugins_dict[name]['app'] = None >>>>>> + self._load_plugin_config_file(name) >>>>>> + >>>>> >>>>>> + # Disable all plugins but 'sample' in test mode >>>>>> + if (self.options is not None) and >>>>>> self.options.test: >>>>>> + self._plugins_dict[name]['enabled'] = (name == 'sample') >>>>>> + >>>>> >>>>> Why? I could be able to see all plugins running on test mode. >>>>> For example, Kimchi provides an MockModel to allow user tests >>>>> Kimchi without making any change in the host system. >>>> >>>> My idea here was only considering wok running its own tests. >>>> I my opinion, when wok run its tests, there is no reason to load >>>> any extra plugin but sample for testing. >>>> >>>> Thing is that I forgot to test if this behavior could impact other >>>> plugins running their own tests, and indeed >>>> (just tested) Kimchi is impacted. >>>> >>>> So, I would like to propose a differentiation I Wok behavior >>>> between running its own tests and running plugins test. >>>> For instance we can continue with 'test' option and create >>>> 'plugintest' option which should be set by plugins when >>>> they are in test mode. >>>> Or (2), create a new option "woktest" that will be passed by wok >>>> tests only, in order to disable all plugins and enable Sample. >>>> >>> >>> No No! The test mode is not only for running tests. It is for the >>> whole application to run on a mock environment. >>> >>> So a user running wok on test mode, will see exactly the same >>> he/she would see when running on non-test mode. The difference is >>> how the backend is implemented. >>> In the first case (test mode) it will not do persistent changes on >>> system and the last case, it will do. >>> >>> If you want to enable/disable the sample test for unit test >>> proposals, you should do that during the test case. Before starting >>> the test case, enable the plugin by editing its configuration file, >>> do the tests, disable it again. >>> >>> There is also a --enable-sample to autogen command to enable the >>> Sample plugin. >>> >> >> Today Wok does not know what options.test is (grep the code). So >> options.test is only used to be passed to plugins "expecting" that >> they run some mock model. >> Also: "In the first case (test mode) it will not do persistent >> changes on system" -- this is false for Wok, for instance, it will >> save logs in the system (maybe a new feature here). >> > > Wok does not use it because it does not have any API (at least before > this patch set) when user can do something. > >> My real problem is disable all plugins before Wok run its tests. And >> I do not want to touch their configuration files (too much intrusive). >> Just thought a better solution that I can implement quickly to fix >> this: add a new option "disableplugins" (I already had thoughts >> about plugins command line options). >> Then I can use it in the tests and does not touch "options.test" >> > > How useful is that for the final user? You are trying to fix a unit > test problem by changing how the server behaves. >

No I am trying to expand and improve Wok option. Have even left a comment about in the code: + # TODO: + # Add command line option to disable plugin by its name + # My idea is do something similar to YUM , add options: "enableallplugins", "disableallplugins", "disableplugin=X", "enableplugin=X"

> Why do you need to get all other plugins disabled to do the tests? > How are they impacting the test cases? >

Give you some reasons: - Wok tests don't care about load plugins and its configurations;

- Today, the impact is in the time, specially due to kimchi/ginger feature tests;

- There is a waste of memory;

- More important, think about scalability: * I cannot guaranty that a future plugin is going to be loaded quickly;

* tests are for dev environment, I can have a upstream plugins code that does not load and break the tests;

* what if I need more future tests that instantiate the server 2, 3, 4 times, each plugin will be loaded that much;

I give you the questions back: Why have plugins enabled ? What are the gains ?

>>>>> >>>>>> + if not self._plugins_dict[name]['enabled']: >>>>>> + wok_log.info("Initializing plugin: %s >>>>>> [DISABLED]" % name) >>>>>> + else: >>>>>> + wok_log.info("Initializing plugin: %s >>>>>> [ENABLED]" % name) >>>>>> + >>>>>> + def _set_cherrypy_app(self, name): >>>>>> + """ >>>>>> + Load plugin module and cherrypy configuration, then set >>>>>> it as cherrypy >>>>>> + app. >>>>>> + """ >>>>>> + self._load_plugin_app(name) >>>>>> + self._load_plugin_app_config(name) >>>>>> + if self._plugins_dict[name]['enabled']: >>>>>> + cherrypy.tree.mount( >>>>>> + self._plugins_dict[name]['app'], >>>>>> + self._plugins_dict[name]['uri'], >>>>>> + self._plugins_dict[name]['config']) >>>>>> + wok_log.info("Plugin '%s' loaded" % >>>>>> + self._plugins_dict[name]['app']) >>>>>> + >>>>>> + def load_plugins(self): >>>>>> + """ >>>>>> + Set enabled plugins into Cherrypy >>>>>> + """ >>>>>> + for plugin in self.get_enabled_plugins(): >>>>>> + self._set_cherrypy_app(plugin) >>>>>> + >>>>>> + def get_all_plugins_info(self): >>>>>> + return self._plugins_dict >>>>>> + >>>>>> + def get_plugin_info(self, name): >>>>>> + return self._plugins_dict.get(name, {}) >>>>>> + >>>>>> + def get_all_plugins_names(self): >>>>>> + ret = self._plugins_dict.keys() >>>>>> + ret.sort() >>>>>> + return ret >>>>>> + >>>>>> + def get_enabled_plugins(self): >>>>>> + ret = [plugin for plugin in self._plugins_dict.keys() if >>>>>> + self._plugins_dict[plugin]['enabled']] >>>>>> + ret.sort() >>>>>> + return ret >>>>>> + >>>>>> + def _enable_plugin_conf_file(self, f_conf, enable=True): >>>>>> + try: >>>>>> + # 'unrepr' makes ConfigObj read and write >>>>>> characters like ("'?) >>>>>> + conf = ConfigObj(infile=f_conf, unrepr=True) >>>>>> + conf['wok']['enable'] = enable >>>>>> + with open(f_conf, 'wb') as f: >>>>>> + conf.write(f) >>>>>> + except Exception as e: >>>>>> + wok_log.error('Error updating plugin conf file. ' + >>>>>> e.message) >>>>>> + raise >>>>>> + >>>>>> + def _change_plugin_state(self, name, enable=True): >>>>>> + plugin = self._plugins_dict[name] >>>>>> + if plugin['enabled'] == enable: >>>>>> + return >>>>>> + try: >>>>>> + self._enable_plugin_conf_file(plugin['conf_file'], enable) >>>>>> + except: >>>>>> + raise OperationFailed('WOKPLUG0002E', {'plugin': >>>>>> name}) >>>>>> + plugin['enabled'] = enable >>>>>> + >>>>>> + def enable_plugin(self, plugin): >>>>>> + wok_log.info("PluginsManager: Enabling plugin '%s'" % >>>>>> plugin) >>>>>> + self._change_plugin_state(plugin, True) >>>>>> + >>>>>> + def disable_plugin(self, plugin): >>>>>> + wok_log.info("PluginsManager: Disabling plugin '%s'" % >>>>>> plugin) >>>>>> + self._change_plugin_state(plugin, False) >>>>>> + >>>>>> + >>>>>> +def get_all_tabs(): >>>>>> + files = [] >>>>>> + >>>>>> + for plugin in Plugins().get_enabled_plugins(): >>>>>> + files.append(os.path.join(PluginPaths(plugin).ui_dir, >>>>>> + 'config/tab-ext.xml')) >>>>>> + >>>>>> + tabs = [] >>>>>> + for f in files: >>>>>> + try: >>>>>> + root = ET.parse(f) >>>>>> + except (IOError): >>>>>> + wok_log.debug("Unable to load %s", f) >>>>>> + continue >>>>>> + tabs.extend([t.text.lower() for t in >>>>>> root.getiterator('title')]) >>>>>> + >>>>>> + return tabs >>>>> >>>>> _______________________________________________ >>>>> Kimchi-devel mailing list >>>>> Kimchi-devel(a)ovirt.org >>>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>>>> >>>> >>> >>> _______________________________________________ >>> Kimchi-devel mailing list >>> Kimchi-devel(a)ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>> >> >> _______________________________________________ >> Kimchi-devel mailing list >> Kimchi-devel(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >> > > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel