Re: [Kimchi-devel] [PATCH v2][Wok 2/8] Changes server behavior to use pluginsmanager

On 06/08/2016 07:18 PM, Rodrigo Trujillo wrote: > > > On 06/08/2016 06:18 PM, Aline Manera wrote: >> >> >> On 06/08/2016 06:06 PM, Rodrigo Trujillo wrote: >>> >>> >>> On 06/08/2016 03:56 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/auth.py | 15 ++++++++------- >>>>> src/wok/root.py | 8 ++++---- >>>>> src/wok/server.py | 57 >>>>> ++++++------------------------------------------------- >>>>> src/wok/utils.py | 52 >>>>> -------------------------------------------------- >>>>> 4 files changed, 18 insertions(+), 114 deletions(-) >>>>> >>>>> diff --git a/src/wok/auth.py b/src/wok/auth.py >>>>> index 0355e86..f8be4b5 100644 >>>>> --- a/src/wok/auth.py >>>>> +++ b/src/wok/auth.py >>>>> @@ -35,15 +35,15 @@ import urllib2 >>>>> from wok import template >>>>> from wok.config import config >>>>> from wok.exception import InvalidOperation, OperationFailed >>>>> -from wok.utils import get_all_tabs, run_command >>>>> +from wok.pluginsmanager import get_all_tabs >>>>> +from wok.utils import run_command >>>>> + >>>>> >>>>> USER_NAME = 'username' >>>>> USER_GROUPS = 'groups' >>>>> USER_ROLES = 'roles' >>>>> REFRESH = 'robot-refresh' >>>> >>>> >>>>> -tabs = get_all_tabs() >>>>> - >>>> >>>> Keep it there and update to point to the new function to avoid >>>> multiple file processing for each server request. >>> >>> This change was necessary because of cyclic imports in the code. >>> Backing to previous implementation will require to change other Wok >>> classes initialization. >>> >> >> I'd say to go for it (change other Wok classes initialization) >> We should not let a code issue impact the server performance on that >> way. > > You are not understanding the problem. "auth" is imported by > Server.py BEFORE server class be initialized > then 'tabs' have to be set, which means that PluginsManager will be > instantiated and initialized before the > server, what is wrong. Server should initiate the Plugins Manager. > I agree that probably I am not understanding all that cyclic imports. My point is to do not call a function to read a configuration file on every request.

> >> >>>> >>>>> def redirect_login(): >>>>> url = "/login.html" >>>>> @@ -85,7 +85,7 @@ class PAMUser(User): >>>>> # after adding support to change user roles that info >>>>> should be read >>>>> # from a specific objstore and fallback to default only >>>>> if any entry is >>>>> # found >>>> >>>>> - self.user[USER_ROLES] = dict.fromkeys(tabs, 'user') >>>>> + self.user[USER_ROLES] = dict.fromkeys(get_all_tabs(), >>>>> 'user') >>>>> >>>>> def get_groups(self): >>>>> out, err, rc = run_command(['id', '-Gn', >>>>> self.user[USER_NAME]]) >>>>> @@ -99,7 +99,7 @@ class PAMUser(User): >>>>> # after adding support to change user roles that >>>>> info should be >>>>> # read from a specific objstore and fallback to >>>>> default only if >>>>> # any entry is found >>>>> - self.user[USER_ROLES] = dict.fromkeys(tabs, 'admin') >>>>> + self.user[USER_ROLES] = >>>>> dict.fromkeys(get_all_tabs(), 'admin') >>>>> >>>>> return self.user[USER_ROLES] >>>>> >>>>> @@ -195,7 +195,7 @@ class LDAPUser(User): >>>>> self.user[USER_GROUPS] = list() >>>>> # FIXME: user roles will be changed according roles >>>>> assignment after >>>>> # objstore is integrated >>>>> - self.user[USER_ROLES] = dict.fromkeys(tabs, 'user') >>>>> + self.user[USER_ROLES] = dict.fromkeys(get_all_tabs(), >>>>> 'user') >>>>> >>>>> @staticmethod >>>>> def authenticate(username, password): >>>>> @@ -236,7 +236,8 @@ class LDAPUser(User): >>>>> "authentication", >>>>> "ldap_admin_id").strip('"').split(',') >>>>> for admin_id in admin_ids: >>>>> if self.user[USER_NAME] == admin_id.strip(): >>>>> - self.user[USER_ROLES] = dict.fromkeys(tabs, >>>>> 'admin') >>>>> + self.user[USER_ROLES] = >>>>> dict.fromkeys(get_all_tabs(), >>>>> + >>>> >>>> That way, on each server request the tab file will be parsed. Move >>>> it a global variable as it was before. >>> >>> Just made a test adding a print in the function and I do not see >>> multiples requests to it. I only see during logging to application. >>> >> >> Which means multiple calls to it. =) >> >> Before this patch, independent on how much users were logging into >> application only one file parsing was needed. Why change that? >> > > I understood that, just did not see the multiple calls problem > before. I changed to fix bad architectural imports made! ... by adding a different issue. Maybe we need to think in a better solution. > >>> >>>> >>>>> 'admin') >>>>> return self.user[USER_ROLES] >>>>> >>>>> def get_user(self): >>>>> diff --git a/src/wok/root.py b/src/wok/root.py >>>>> index 29ea657..cb8b376 100644 >>>>> --- a/src/wok/root.py >>>>> +++ b/src/wok/root.py >>>>> @@ -123,10 +123,10 @@ class Root(Resource): >>>>> >>>>> data['scripts'] = [] >>>>> for plugin, app in cherrypy.tree.apps.iteritems(): >>>>> - if app.root.extends is not None: >>>>> - scripts = app.root.extends.get(script_name, {}) >>>>> - if page in scripts.keys(): >>>>> - data['scripts'].append(scripts[page]) >>>>> + if app.root.extends is not None: >>>>> + scripts = app.root.extends.get(script_name, {}) >>>>> + if page in scripts.keys(): >>>>> + data['scripts'].append(scripts[page]) >>>>> >>>>> if page.endswith('.html'): >>>>> context = template.render('/tabs/' + page, data) >>>>> diff --git a/src/wok/server.py b/src/wok/server.py >>>>> index 902d4bf..3cb257f 100644 >>>>> --- a/src/wok/server.py >>>>> +++ b/src/wok/server.py >>>>> @@ -30,14 +30,14 @@ from string import Template >>>>> from wok import auth >>>>> from wok import config >>>>> from wok.config import config as configParser >>>>> -from wok.config import paths, PluginConfig, WokConfig >>>>> +from wok.config import paths, WokConfig >>>>> from wok.control import sub_nodes >>>>> from wok.model import model >>>>> +from wok.pluginsmanager import Plugins >>>>> from wok.proxy import start_proxy, terminate_proxy >>>>> from wok.reqlogger import RequestLogger >>>>> from wok.root import WokRoot >>>>> from wok.safewatchedfilehandler import SafeWatchedFileHandler >>>>> -from wok.utils import get_enabled_plugins, import_class >>>>> >>>>> >>>>> LOGGING_LEVEL = {"debug": logging.DEBUG, >>>>> @@ -171,61 +171,16 @@ class Server(object): >>>>> >>>>> self.app = cherrypy.tree.mount(WokRoot(model_instance, >>>>> dev_env), >>>>> config=self.configObj) >>>>> - self._load_plugins(options) >>>>> + >>>>> + # Initiate and handles plugins >>>>> + plugins = Plugins(options) >>>>> + plugins.load_plugins() >>>>> >>>>> # Terminate proxy when cherrypy server is terminated >>>>> cherrypy.engine.subscribe('exit', terminate_proxy) >>>>> >>>>> cherrypy.lib.sessions.init() >>>>> >>>>> - def _load_plugins(self, options): >>>>> - for plugin_name, plugin_config in get_enabled_plugins(): >>>>> - try: >>>>> - plugin_class = ('plugins.%s.%s' % >>>>> - (plugin_name, >>>>> - plugin_name[0].upper() + >>>>> plugin_name[1:])) >>>>> - script_name = plugin_config['wok']['uri'] >>>>> - del plugin_config['wok'] >>>>> - >>>>> - plugin_config.update(PluginConfig(plugin_name)) >>>>> - except KeyError: >>>>> - continue >>>>> - >>>>> - try: >>>>> - plugin_app = import_class(plugin_class)(options) >>>>> - except ImportError, e: >>>>> - cherrypy.log.error_log.error( >>>>> - "Failed to import plugin %s, " >>>>> - "error: %s" % (plugin_class, e.message) >>>>> - ) >>>>> - continue >>>>> - >>>>> - # 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: >>>>> - plugin_config.update(get_custom_conf()) >>>>> - >>>>> - # dynamically add tools.wokauth.on = True to extra >>>>> plugin APIs >>>>> - try: >>>>> - sub_nodes = >>>>> import_class('plugins.%s.control.sub_nodes' % >>>>> - plugin_name) >>>>> - >>>>> - urlSubNodes = {} >>>>> - for ident, node in sub_nodes.items(): >>>>> - if node.url_auth: >>>>> - ident = "/%s" % ident >>>>> - urlSubNodes[ident] = >>>>> {'tools.wokauth.on': True} >>>>> - >>>>> - plugin_config.update(urlSubNodes) >>>>> - >>>>> - except ImportError, e: >>>>> - cherrypy.log.error_log.error( >>>>> - "Failed to import subnodes for plugin %s, " >>>>> - "error: %s" % (plugin_class, e.message) >>>>> - ) >>>>> - >>>>> - cherrypy.tree.mount(plugin_app, script_name, >>>>> plugin_config) >>>>> - >>>>> def start(self): >>>>> # Subscribe to SignalHandler plugin >>>>> if hasattr(cherrypy.engine, 'signal_handler'): >>>>> diff --git a/src/wok/utils.py b/src/wok/utils.py >>>>> index 4b3b0ac..10f2850 100644 >>>>> --- a/src/wok/utils.py >>>>> +++ b/src/wok/utils.py >>>>> @@ -32,16 +32,13 @@ import sqlite3 >>>>> import subprocess >>>>> import sys >>>>> import traceback >>>>> -import xml.etree.ElementTree as ET >>>>> import locale >>>>> >>>>> -from cherrypy.lib.reprconf import Parser >>>>> from datetime import datetime, timedelta >>>>> from multiprocessing import Process, Queue >>>>> from threading import Timer >>>>> >>>>> from wok.asynctask import AsyncTask >>>>> -from wok.config import paths, PluginPaths >>>>> from wok.exception import InvalidParameter, TimeoutExpired >>>>> >>>>> >>>>> @@ -76,55 +73,6 @@ def is_digit(value): >>>>> return False >>>>> >>>>> >>>>> -def _load_plugin_conf(name): >>>>> - plugin_conf = PluginPaths(name).conf_file >>>>> - if not os.path.exists(plugin_conf): >>>>> - cherrypy.log.error_log.error("Plugin configuration file %s" >>>>> - " doesn't exist." % >>>>> plugin_conf) >>>>> - return >>>>> - try: >>>>> - return Parser().dict_from_file(plugin_conf) >>>>> - except ValueError as e: >>>>> - cherrypy.log.error_log.error("Failed to load plugin " >>>>> - "conf from %s: %s" % >>>>> - (plugin_conf, e.message)) >>>>> - >>>>> - >>>>> -def get_enabled_plugins(): >>>>> - plugin_dir = paths.plugins_dir >>>>> - try: >>>>> - dir_contents = os.listdir(plugin_dir) >>>>> - except OSError: >>>>> - return >>>>> - for name in dir_contents: >>>>> - if os.path.isdir(os.path.join(plugin_dir, name)): >>>>> - plugin_config = _load_plugin_conf(name) >>>>> - try: >>>>> - if plugin_config['wok']['enable']: >>>>> - yield (name, plugin_config) >>>>> - except (TypeError, KeyError): >>>>> - continue >>>>> - >>>>> - >>>>> -def get_all_tabs(): >>>>> - files = [] >>>>> - >>>>> - for plugin, _ in 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 >>>>> - >>>>> - >>>>> def get_plugin_from_request(): >>>>> """ >>>>> Returns name of plugin being requested. If no plugin, >>>>> returns 'wok'. >>>> >>>> _______________________________________________ >>>> 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