On 21/07/16 10:12, Jan Cholasta wrote:
Hi,

On 20.7.2016 14:32, David Kupka wrote:
On 15/07/16 12:53, David Kupka wrote:
Hello!

After Honza introduced thin client that builds plugins and commands
dynamically from schema client became much slower. This is only logical,
instead of importing a module client now must fetch the schema from
server, parse it and instantiate the commands using the data.

First step to speed it up was addition of schema cache to client. That
removed the RTT and download time of fetching schema every time.

Now the most time consuming task became displaying help for lists of
topics and command and displaying individual topics. This is simply
because of the need to instantiate all the commands to find the
relations between topics and commands.

All the necessary bits for server commands and topics are already in the
schema cache so we can skip this part and generate help from it, right?
Not so fast!

There are client plugins with commands and topics. So we can generate
basic bits (list of all topics, list of all commands, list of commands
for each topic) from schema and store it in cache. Then we need to go
through all client plugins and get similar bits for client plugins. Then
we can merge and print.

Still the client response is not as fast as before and I this it even
can't be. Also first time you display particular topic or list takes
longer because it must be freshly generated and stored in cache for next
use. And this is what the attached patches do.

https://fedorahosted.org/freeipa/ticket/6048

Reimplemented so there is no need to distinguish client plugins and
remote plugins.
The main idea of this approach is to avoid creating instances of the
commands just to get the information about topic, name and summary
needed for displaying help. Instead class properties are used to access
the information directly in schema.

Patch 0112:

I think this would better be done in Schema.read_namespace_member,
because Schema is where all the state is.

(BTW does _SchemaNameSpace.__getitem__ raise KeyError for non-existent
keys? It looks like it doesn't.)


Patch 0113:

How about setting _schema_cached to False in Schema.__init__() rather
that getattr()-ing it in _ensure_cached()?


Patch 0116:

ClientCommand.doc should be a class property as well, otherwise .summary
won't work on it correctly.

_SchemaCommand.doc should not be a property, as it's not needed for
.summary to work on it correctly.


Otherwise works fine for me.

Honza


Updated patches attached.

--
David Kupka
From bae93c2a57b51c8b5c11d481f9df2a62f330fb81 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 27 Jul 2016 10:46:40 +0200
Subject: [PATCH 1/6] schema: Speed up schema cache

Check presence of schema in cache (and download it if necessary) on
__init__ instead of with each __getitem__ call. Prefill internal
dictionary with empty record for each command to be able to quickly
determine if requested command exist in schema or not. Rest of schema
data are read from cache on first attempt to retrive them.

https://fedorahosted.org/freeipa/ticket/6048
https://fedorahosted.org/freeipa/ticket/6069
---
 ipaclient/remote_plugins/schema.py | 297 ++++++++++++++++++++++---------------
 1 file changed, 175 insertions(+), 122 deletions(-)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index cd1d5d607978899254325f634ccec91d2c92f59b..70cd7536d836ec113236bfdae8bbabb2d843725d 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -5,10 +5,8 @@
 import collections
 import errno
 import fcntl
-import glob
 import json
 import os
-import re
 import sys
 import time
 import types
@@ -65,8 +63,6 @@ USER_CACHE_PATH = (
         '.cache'
     )
 )
-SCHEMA_DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'schema')
-SERVERS_DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'servers')
 
 logger = log_mgr.get_logger(__name__)
 
@@ -274,15 +270,6 @@ class _SchemaObjectPlugin(_SchemaPlugin):
     schema_key = 'classes'
 
 
-def _ensure_dir_created(d):
-    try:
-        os.makedirs(d)
-    except OSError as e:
-        if e.errno != errno.EEXIST:
-            raise RuntimeError("Unable to create cache directory: {}"
-                               "".format(e))
-
-
 class _LockedZipFile(zipfile.ZipFile):
     """ Add locking to zipfile.ZipFile
     Shared lock is used with read mode, exclusive with write mode.
@@ -308,7 +295,10 @@ class _SchemaNameSpace(collections.Mapping):
         self._schema = schema
 
     def __getitem__(self, key):
-        return self._schema.read_namespace_member(self.name, key)
+        try:
+            return self._schema.read_namespace_member(self.name, key)
+        except KeyError:
+            raise KeyError(key)
 
     def __iter__(self):
         for key in self._schema.iter_namespace(self.name):
@@ -322,6 +312,62 @@ class NotAvailable(Exception):
     pass
 
 
+class ServerInfo(collections.MutableMapping):
+    _DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'servers')
+
+    def __init__(self, api):
+        hostname = DNSName(api.env.server).ToASCII()
+        self._path = os.path.join(self._DIR, hostname)
+        self._dict = {}
+        self._dirty = False
+
+        self._read()
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, *_exc_info):
+        if self._dirty:
+            self._write()
+
+    def _read(self):
+        try:
+            with open(self._path, 'r') as sc:
+                self._dict = json.load(sc)
+        except EnvironmentError as e:
+            if e.errno != errno.ENOENT:
+                logger.warning('Failed to read server info: {}'.format(e))
+
+    def _write(self):
+        try:
+            try:
+                os.makedirs(self._DIR)
+            except EnvironmentError as e:
+                if e.errno != errno.EEXIST:
+                    raise
+            with open(self._path, 'w') as sc:
+                json.dump(self._dict, sc)
+        except EnvironmentError as e:
+            logger.warning('Failed to write server info: {}'.format(e))
+
+    def __getitem__(self, key):
+        return self._dict[key]
+
+    def __setitem__(self, key, value):
+        self._dirty = key not in self._dict or self._dict[key] != value
+        self._dict[key] = value
+
+    def __delitem__(self, key):
+        del self._dict[key]
+        self._dirty = True
+
+    def __iter__(self):
+        return iter(self._dict)
+
+    def __len__(self):
+        return len(self._dict)
+
+
 class Schema(object):
     """
     Store and provide schema for commands and topics
@@ -342,38 +388,76 @@ class Schema(object):
     u'Ping the remote IPA server to ...'
 
     """
-    schema_path_template = os.path.join(SCHEMA_DIR, '{}')
-    servers_path_template = os.path.join(SERVERS_DIR, '{}')
-    ns_member_pattern_template = '^{}/(?P<name>.+)$'
-    ns_member_path_template = '{}/{}'
     namespaces = {'classes', 'commands', 'topics'}
     schema_info_path = 'schema'
+    _DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'schema')
 
-    @classmethod
-    def _list(cls):
-        for f in glob.glob(cls.schema_path_template.format('*')):
-            yield os.path.splitext(os.path.basename(f))[0]
-
-    @classmethod
-    def _in_cache(cls, fingeprint):
-        return os.path.exists(cls.schema_path_template.format(fingeprint))
-
-    def __init__(self, api, client):
-        self._api = api
-        self._client = client
+    def __init__(self, api, server_info, client):
         self._dict = {}
+        self._namespaces = {}
+        self._help = None
 
-    def _open_server_info(self, hostname, mode):
-        encoded_hostname = DNSName(hostname).ToASCII()
-        path = self.servers_path_template.format(encoded_hostname)
-        return open(path, mode)
+        for ns in self.namespaces:
+            self._dict[ns] = {}
+            self._namespaces[ns] = _SchemaNameSpace(self, ns)
 
-    def _get_schema(self):
-        client = self._client
+        is_known = False
+        if not api.env.force_schema_check:
+            try:
+                self._fingerprint = server_info['fingerprint']
+                self._expiration = server_info['expiration']
+            except KeyError:
+                pass
+            else:
+                is_known = True
+
+        if is_known:
+            try:
+                self._read_schema()
+            except Exception:
+                pass
+            else:
+                return
+
+        try:
+            self._fetch(client)
+        except NotAvailable:
+            raise
+        else:
+            self._write_schema()
+        finally:
+            try:
+                server_info['fingerprint'] = self._fingerprint
+                server_info['expiration'] = self._expiration
+            except AttributeError:
+                pass
+
+    def _open_schema(self, filename, mode):
+        path = os.path.join(self._DIR, filename)
+        return _LockedZipFile(path, mode)
+
+    def _get_schema_fingerprint(self, schema):
+        schema_info = json.loads(schema.read(self.schema_info_path))
+        return schema_info['fingerprint']
+
+    def _fetch(self, client):
         if not client.isconnected():
             client.connect(verbose=False)
 
-        fps = [unicode(f) for f in Schema._list()]
+        fps = []
+        try:
+            files = os.listdir(self._DIR)
+        except EnvironmentError:
+            pass
+        else:
+            for filename in files:
+                try:
+                    with self._open_schema(filename, 'r') as schema:
+                        fps.append(
+                            unicode(self._get_schema_fingerprint(schema)))
+                except Exception:
+                    continue
+
         kwargs = {u'version': u'2.170'}
         if fps:
             kwargs[u'known_fingerprints'] = fps
@@ -386,110 +470,80 @@ class Schema(object):
             ttl = e.ttl
         else:
             fp = schema['fingerprint']
-            ttl = schema['ttl']
-            self._store(fp, schema)
-        finally:
-            client.disconnect()
+            ttl = schema.pop('ttl', 0)
 
-        exp = ttl + time.time()
-        return (fp, exp)
+            for key, value in schema.items():
+                if key in self.namespaces:
+                    value = {m['full_name']: m for m in value}
+                self._dict[key] = value
 
-    def _ensure_cached(self):
-        no_info = False
-        try:
-            # pylint: disable=access-member-before-definition
-            fp = self._server_schema_fingerprint
-            exp = self._server_schema_expiration
-        except AttributeError:
-            try:
-                with self._open_server_info(self._api.env.server, 'r') as sc:
-                    si = json.load(sc)
+        self._fingerprint = fp
+        self._expiration = ttl + time.time()
 
-                fp = si['fingerprint']
-                exp = si['expiration']
-            except Exception as e:
-                no_info = True
-                if not (isinstance(e, EnvironmentError) and
-                        e.errno == errno.ENOENT):  # pylint: disable=no-member
-                    logger.warning('Failed to load server properties: {}'
-                                   ''.format(e))
-
-        force_check = ((not getattr(self, '_schema_checked', False)) and
-                       self._api.env.force_schema_check)
-
-        if (force_check or
-                no_info or exp < time.time() or not Schema._in_cache(fp)):
-            (fp, exp) = self._get_schema()
-            self._schema_checked = True
-            _ensure_dir_created(SERVERS_DIR)
-            try:
-                with self._open_server_info(self._api.env.server, 'w') as sc:
-                    json.dump(dict(fingerprint=fp, expiration=exp), sc)
-            except Exception as e:
-                logger.warning('Failed to store server properties: {}'
-                               ''.format(e))
-
-        if not self._dict:
-            self._dict['fingerprint'] = fp
-            schema_info = self._read(self.schema_info_path)
+    def _read_schema(self):
+        with self._open_schema(self._fingerprint, 'r') as schema:
+            self._dict['fingerprint'] = self._get_schema_fingerprint(schema)
+            schema_info = json.loads(schema.read(self.schema_info_path))
             self._dict['version'] = schema_info['version']
-            for ns in self.namespaces:
-                self._dict[ns] = _SchemaNameSpace(self, ns)
 
-        self._server_schema_fingerprintr = fp
-        self._server_schema_expiration = exp
+            for name in schema.namelist():
+                ns, _slash, key = name.partition('/')
+                if ns in self.namespaces:
+                    self._dict[ns][key] = {}
 
     def __getitem__(self, key):
-        self._ensure_cached()
-        return self._dict[key]
+        try:
+            return self._namespaces[key]
+        except KeyError:
+            return self._dict[key]
 
-    def _open_archive(self, mode, fp=None):
-        if not fp:
-            fp = self['fingerprint']
-        arch_path = self.schema_path_template.format(fp)
-        return _LockedZipFile(arch_path, mode)
+    def _write_schema(self):
+        try:
+            os.makedirs(self._DIR)
+        except EnvironmentError as e:
+            if e.errno != errno.EEXIST:
+                logger.warning("Failed ti write schema: {}".format(e))
+                return
 
-    def _store(self, fingerprint, schema={}):
-        _ensure_dir_created(SCHEMA_DIR)
+        with self._open_schema(self._fingerprint, 'w') as schema:
+            schema_info = {}
+            for key, value in self._dict.items():
+                if key in self.namespaces:
+                    ns = value
+                    for member in ns:
+                        path = '{}/{}'.format(key, member)
+                        schema.writestr(path, json.dumps(ns[member]))
+                else:
+                    schema_info[key] = value
 
-        schema_info = dict(version=schema['version'],
-                           fingerprint=schema['fingerprint'])
-
-        with self._open_archive('w', fingerprint) as zf:
-            # store schema information
-            zf.writestr(self.schema_info_path, json.dumps(schema_info))
-            # store namespaces
-            for namespace in self.namespaces:
-                for member in schema[namespace]:
-                    path = self.ns_member_path_template.format(
-                        namespace,
-                        member['full_name']
-                    )
-                    zf.writestr(path, json.dumps(member))
+            schema.writestr(self.schema_info_path, json.dumps(schema_info))
 
     def _read(self, path):
-        with self._open_archive('r') as zf:
+        with self._open_schema(self._fingerprint, 'r') as zf:
             return json.loads(zf.read(path))
 
     def read_namespace_member(self, namespace, member):
-        path = self.ns_member_path_template.format(namespace, member)
-        return self._read(path)
+        value = self._dict[namespace][member]
+
+        if (not value) or ('full_name' not in value):
+            path = '{}/{}'.format(namespace, member)
+            value = self._dict[namespace].setdefault(
+                member, {}
+            ).update(self._read(path))
+
+        return value
 
     def iter_namespace(self, namespace):
-        pattern = self.ns_member_pattern_template.format(namespace)
-        with self._open_archive('r') as zf:
-            for name in zf.namelist():
-                r = re.match(pattern, name)
-                if r:
-                    yield r.groups('name')[0]
+        return iter(self._dict[namespace])
 
 
 def get_package(api, client):
     try:
         schema = api._schema
     except AttributeError:
-        schema = Schema(api, client)
-        object.__setattr__(api, '_schema', schema)
+        with ServerInfo(api.env.hostname) as server_info:
+            schema = Schema(api, server_info, client)
+            object.__setattr__(api, '_schema', schema)
 
     fingerprint = str(schema['fingerprint'])
     package_name = '{}${}'.format(__name__, fingerprint)
@@ -509,10 +563,9 @@ def get_package(api, client):
     module = types.ModuleType(module_name)
     module.__file__ = os.path.join(package_dir, 'plugins.py')
     module.register = plugable.Registry()
-    for key, plugin_cls in (('commands', _SchemaCommandPlugin),
-                            ('classes', _SchemaObjectPlugin)):
-        for full_name in schema[key]:
-            plugin = plugin_cls(full_name)
+    for plugin_cls in (_SchemaCommandPlugin, _SchemaObjectPlugin):
+        for full_name in schema[plugin_cls.schema_key]:
+            plugin = plugin_cls(str(full_name))
             plugin = module.register()(plugin)
     sys.modules[module_name] = module
 
-- 
2.7.4

From f172c67a9e72ef74a4efc437963e48c28d6323e7 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 20 Jul 2016 13:23:33 +0200
Subject: [PATCH 2/6] frontend: Change doc, summary, topic and NO_CLI to class
 properties

Avoid need to instantiate all commands just to get information for
displaying help.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/frontend.py                 | 32 ++++++++++++++-------
 ipaclient/plugins/automount.py        |  9 ++++--
 ipaclient/plugins/otptoken_yubikey.py | 11 +++++---
 ipaclient/plugins/vault.py            | 35 ++++++++++++++---------
 ipaclient/remote_plugins/schema.py    | 53 +++++++++++++++++++++++++++++++----
 ipalib/frontend.py                    | 10 ++++---
 ipalib/plugable.py                    | 17 ++++++-----
 7 files changed, 120 insertions(+), 47 deletions(-)

diff --git a/ipaclient/frontend.py b/ipaclient/frontend.py
index 1525c88b3dfeadccd8115cb4b6ba149caef22103..aeaed550771d3c6af04a9b34fcae414faacb47d7 100644
--- a/ipaclient/frontend.py
+++ b/ipaclient/frontend.py
@@ -2,9 +2,11 @@
 # Copyright (C) 2016  FreeIPA Contributors see COPYING for license
 #
 
+from ipalib import api
 from ipalib.frontend import Command, Method
 from ipalib.parameters import Str
 from ipalib.text import _
+from ipalib.util import classproperty
 
 
 class ClientCommand(Command):
@@ -111,20 +113,30 @@ class CommandOverride(Command):
     def __init__(self, api):
         super(CommandOverride, self).__init__(api)
 
-        next_class = api.get_plugin_next(type(self))
+        next_class = self.__get_next()
         self.next = next_class(api)
 
-    @property
-    def doc(self):
-        return self.next.doc
+    @classmethod
+    def __get_next(cls):
+        return api.get_plugin_next(cls)
 
-    @property
-    def NO_CLI(self):
-        return self.next.NO_CLI
+    @classmethod
+    def __doc_getter(cls):
+        return cls.__get_next().doc
 
-    @property
-    def topic(self):
-        return self.next.topic
+    doc = classproperty(__doc_getter)
+
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return cls.__get_next().NO_CLI
+
+    NO_CLI = classproperty(__NO_CLI_getter)
+
+    @classmethod
+    def __topic_getter(cls):
+        return cls.__get_next().topic
+
+    topic = classproperty(__topic_getter)
 
     @property
     def forwarded_name(self):
diff --git a/ipaclient/plugins/automount.py b/ipaclient/plugins/automount.py
index c6537bc6c24b905a8e1f7fb6a7e2c931b95374c7..925b635ff27411fc7e2f8c3dae17c747216d7fb6 100644
--- a/ipaclient/plugins/automount.py
+++ b/ipaclient/plugins/automount.py
@@ -27,6 +27,7 @@ from ipalib import api, errors
 from ipalib import Flag, Str
 from ipalib.frontend import Command, Method, Object
 from ipalib.plugable import Registry
+from ipalib.util import classproperty
 from ipalib import _
 from ipapython.dn import DN
 
@@ -52,11 +53,13 @@ class _fake_automountlocation_show(Method):
 
 @register(override=True, no_fail=True)
 class automountlocation_tofiles(MethodOverride):
-    @property
-    def NO_CLI(self):
-        return isinstance(self.api.Command.automountlocation_show,
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return isinstance(api.Command.automountlocation_show,
                           _fake_automountlocation_show)
 
+    NO_CLI = classproperty(__NO_CLI_getter)
+
     def output_for_cli(self, textui, result, *keys, **options):
         maps = result['result']['maps']
         keys = result['result']['keys']
diff --git a/ipaclient/plugins/otptoken_yubikey.py b/ipaclient/plugins/otptoken_yubikey.py
index 423b670de15dd7f803db1dcbb759bd0254827072..bfa244c88b7827a31e155b1192f272e311d96bba 100644
--- a/ipaclient/plugins/otptoken_yubikey.py
+++ b/ipaclient/plugins/otptoken_yubikey.py
@@ -23,10 +23,11 @@ import six
 import usb.core
 import yubico
 
-from ipalib import _, IntEnum
+from ipalib import _, api, IntEnum
 from ipalib.errors import NotFound
 from ipalib.frontend import Command, Method, Object
 from ipalib.plugable import Registry
+from ipalib.util import classproperty
 
 if six.PY3:
     unicode = str
@@ -74,11 +75,13 @@ class otptoken_add_yubikey(Command):
     )
     has_output_params = takes_options
 
-    @property
-    def NO_CLI(self):
-        return isinstance(self.api.Command.otptoken_add,
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return isinstance(api.Command.otptoken_add,
                           _fake_otptoken_add)
 
+    NO_CLI = classproperty(__NO_CLI_getter)
+
     def get_args(self):
         for arg in self.api.Command.otptoken_add.args():
             yield arg
diff --git a/ipaclient/plugins/vault.py b/ipaclient/plugins/vault.py
index 73ad09b38316d55b466b7973dbeffefc1b7bb528..ed75a0e22c7079d78753bd5c38385aa76ea9b597 100644
--- a/ipaclient/plugins/vault.py
+++ b/ipaclient/plugins/vault.py
@@ -38,7 +38,8 @@ import nss.nss as nss
 
 from ipaclient.frontend import MethodOverride
 from ipalib.frontend import Local, Method, Object
-from ipalib import errors
+from ipalib.util import classproperty
+from ipalib import api, errors
 from ipalib import Bytes, Flag, Str
 from ipalib.plugable import Registry
 from ipalib import _
@@ -202,11 +203,13 @@ class vault_add(Local):
         ),
     )
 
-    @property
-    def NO_CLI(self):
-        return isinstance(self.api.Command.vault_add_internal,
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return isinstance(api.Command.vault_add_internal,
                           _fake_vault_add_internal)
 
+    NO_CLI = classproperty(__NO_CLI_getter)
+
     def get_args(self):
         for arg in self.api.Command.vault_add_internal.args():
             yield arg
@@ -400,11 +403,13 @@ class vault_mod(Local):
         ),
     )
 
-    @property
-    def NO_CLI(self):
-        return isinstance(self.api.Command.vault_mod_internal,
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return isinstance(api.Command.vault_mod_internal,
                           _fake_vault_mod_internal)
 
+    NO_CLI = classproperty(__NO_CLI_getter)
+
     def get_args(self):
         for arg in self.api.Command.vault_mod_internal.args():
             yield arg
@@ -579,11 +584,13 @@ class vault_archive(Local):
         ),
     )
 
-    @property
-    def NO_CLI(self):
-        return isinstance(self.api.Command.vault_archive_internal,
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return isinstance(api.Command.vault_archive_internal,
                           _fake_vault_archive_internal)
 
+    NO_CLI = classproperty(__NO_CLI_getter)
+
     def get_args(self):
         for arg in self.api.Command.vault_archive_internal.args():
             yield arg
@@ -828,11 +835,13 @@ class vault_retrieve(Local):
         ),
     )
 
-    @property
-    def NO_CLI(self):
-        return isinstance(self.api.Command.vault_retrieve_internal,
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return isinstance(api.Command.vault_retrieve_internal,
                           _fake_vault_retrieve_internal)
 
+    NO_CLI = classproperty(__NO_CLI_getter)
+
     def get_args(self):
         for arg in self.api.Command.vault_retrieve_internal.args():
             yield arg
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 70cd7536d836ec113236bfdae8bbabb2d843725d..b183fa1374b8a518bec0b278017c369d23606906 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -89,10 +89,32 @@ class _SchemaPlugin(object):
     bases = None
     schema_key = None
 
-    def __init__(self, full_name):
+    def __init__(self, schema, full_name):
         self.name, _slash, self.version = full_name.partition('/')
         self.full_name = full_name
-        self.__class = None
+        self._schema = schema
+        self._class = None
+
+    @property
+    def doc(self):
+        if self._class is not None:
+            return self._class.doc
+        else:
+            schema = self._schema[self.schema_key][self.full_name]
+            try:
+                return schema['doc']
+            except KeyError:
+                return None
+
+    @property
+    def summary(self):
+        if self._class is not None:
+            return self._class.summary
+        else:
+            if self.doc is not None:
+                return self.doc.split('\n\n', 1)[0].strip()
+            else:
+                return u'<%s>' % self.full_name
 
     def _create_default_from(self, api, name, keys):
         cmd_name = self.full_name
@@ -200,18 +222,37 @@ class _SchemaPlugin(object):
         return self.name, self.bases, class_dict
 
     def __call__(self, api):
-        if self.__class is None:
+        if self._class is None:
             schema = api._schema[self.schema_key][self.full_name]
             name, bases, class_dict = self._create_class(api, schema)
-            self.__class = type(name, bases, class_dict)
+            self._class = type(name, bases, class_dict)
 
-        return self.__class(api)
+        return self._class(api)
 
 
 class _SchemaCommandPlugin(_SchemaPlugin):
     bases = (_SchemaCommand,)
     schema_key = 'commands'
 
+    @property
+    def topic(self):
+        if self._class is not None:
+            return self._class.topic
+        else:
+            schema = self._schema[self.schema_key][self.full_name]
+            try:
+                return str(schema['topic_topic']).partition('/')[0]
+            except KeyError:
+                return None
+
+    @property
+    def NO_CLI(self):
+        if self._class is not None:
+            return self._class.NO_CLI
+        else:
+            schema = self._schema[self.schema_key][self.full_name]
+            return 'cli' in schema.get('exclude', [])
+
     def _create_output(self, api, schema):
         if schema.get('multivalue', False):
             type_type = (tuple, list)
@@ -565,7 +606,7 @@ def get_package(api, client):
     module.register = plugable.Registry()
     for plugin_cls in (_SchemaCommandPlugin, _SchemaObjectPlugin):
         for full_name in schema[plugin_cls.schema_key]:
-            plugin = plugin_cls(str(full_name))
+            plugin = plugin_cls(schema, str(full_name))
             plugin = module.register()(plugin)
     sys.modules[module_name] = module
 
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index cb00841f21bd5a3e3b4095dcd17a8816ca24400f..455b222d4d7fcbb65b43c4d8e1ffbbaf3e131d22 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -38,7 +38,7 @@ from ipalib.errors import (ZeroArgumentError, MaxArgumentError, OverlapError,
     ValidationError, ConversionError)
 from ipalib import errors, messages
 from ipalib.request import context, context_frame
-from ipalib.util import json_serialize
+from ipalib.util import classproperty, json_serialize
 
 if six.PY3:
     unicode = str
@@ -426,9 +426,11 @@ class Command(HasParam):
 
     api_version = API_VERSION
 
-    @property
-    def topic(self):
-        return type(self).__module__.rpartition('.')[2]
+    @classmethod
+    def __topic_getter(cls):
+        return cls.__module__.rpartition('.')[2]
+
+    topic = classproperty(__topic_getter)
 
     @property
     def forwarded_name(self):
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index 26fbeaa26d7986f2e184f0974ef396bd323d6bf5..073ad05d7aee5e83cae5c6e20bac8f9439505198 100644
--- a/ipalib/plugable.py
+++ b/ipalib/plugable.py
@@ -155,19 +155,22 @@ class Plugin(ReadOnly):
 
     bases = classproperty(__bases_getter)
 
-    @property
-    def doc(self):
-        return type(self).__doc__
+    @classmethod
+    def __doc_getter(cls):
+        return cls.__doc__
 
-    @property
-    def summary(self):
-        doc = self.doc
+    doc = classproperty(__doc_getter)
+
+    @classmethod
+    def __summary_getter(cls):
+        doc = cls.doc
         if not _(doc).msg:
-            cls = type(self)
             return u'<%s.%s>' % (cls.__module__, cls.__name__)
         else:
             return unicode(doc).split('\n\n', 1)[0].strip()
 
+    summary = classproperty(__summary_getter)
+
     @property
     def api(self):
         """
-- 
2.7.4

From 6a49949906892ba512e6b745b03b5d0f32464de5 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 27 Jul 2016 10:54:16 +0200
Subject: [PATCH 3/6] schema: Introduce schema cache format

Information about schema cache format is stored in every cache item.
When schema cache format changes in incompatible way format will be
increased. When format stored in cache doesn't match currently used
format the entry in cache is ignored.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/schema.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index b183fa1374b8a518bec0b278017c369d23606906..fcff8bf019c2300546e18f1fd30e82f48a7c5484 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -23,6 +23,8 @@ from ipapython.dn import DN
 from ipapython.dnsutil import DNSName
 from ipapython.ipa_log_manager import log_mgr
 
+FORMAT = '0'
+
 if six.PY3:
     unicode = str
 
@@ -478,6 +480,14 @@ class Schema(object):
         return _LockedZipFile(path, mode)
 
     def _get_schema_fingerprint(self, schema):
+        try:
+            fmt = json.loads(schema.read('format'))
+        except KeyError:
+            fmt = '0'
+
+        if fmt != FORMAT:
+            raise RuntimeError('invalid format')
+
         schema_info = json.loads(schema.read(self.schema_info_path))
         return schema_info['fingerprint']
 
-- 
2.7.4

From aed6363f290e75cba48351e502f923f4ebe48831 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Tue, 2 Aug 2016 08:16:30 +0200
Subject: [PATCH 4/6] schema: Generate bits for help load them on request

Store name, summary, topic_topic and exclude in single entry in cache
for all commands. These data are needed for help and storing and
loading them together allows fast help response.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/schema.py | 54 +++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index fcff8bf019c2300546e18f1fd30e82f48a7c5484..9d5c08b85c13ebbed29ac15cdd9ad513ecb4639e 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -23,7 +23,7 @@ from ipapython.dn import DN
 from ipapython.dnsutil import DNSName
 from ipapython.ipa_log_manager import log_mgr
 
-FORMAT = '0'
+FORMAT = '1'
 
 if six.PY3:
     unicode = str
@@ -113,9 +113,11 @@ class _SchemaPlugin(object):
         if self._class is not None:
             return self._class.summary
         else:
-            if self.doc is not None:
-                return self.doc.split('\n\n', 1)[0].strip()
-            else:
+            self._schema.load_help()
+            schema = self._schema[self.schema_key][self.full_name]
+            try:
+                return schema['summary']
+            except KeyError:
                 return u'<%s>' % self.full_name
 
     def _create_default_from(self, api, name, keys):
@@ -241,6 +243,7 @@ class _SchemaCommandPlugin(_SchemaPlugin):
         if self._class is not None:
             return self._class.topic
         else:
+            self._schema.load_help()
             schema = self._schema[self.schema_key][self.full_name]
             try:
                 return str(schema['topic_topic']).partition('/')[0]
@@ -252,6 +255,7 @@ class _SchemaCommandPlugin(_SchemaPlugin):
         if self._class is not None:
             return self._class.NO_CLI
         else:
+            self._schema.load_help()
             schema = self._schema[self.schema_key][self.full_name]
             return 'cli' in schema.get('exclude', [])
 
@@ -432,7 +436,6 @@ class Schema(object):
 
     """
     namespaces = {'classes', 'commands', 'topics'}
-    schema_info_path = 'schema'
     _DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'schema')
 
     def __init__(self, api, server_info, client):
@@ -488,8 +491,7 @@ class Schema(object):
         if fmt != FORMAT:
             raise RuntimeError('invalid format')
 
-        schema_info = json.loads(schema.read(self.schema_info_path))
-        return schema_info['fingerprint']
+        return json.loads(schema.read('fingerprint'))
 
     def _fetch(self, client):
         if not client.isconnected():
@@ -522,6 +524,7 @@ class Schema(object):
         else:
             fp = schema['fingerprint']
             ttl = schema.pop('ttl', 0)
+            schema.pop('version', None)
 
             for key, value in schema.items():
                 if key in self.namespaces:
@@ -534,8 +537,6 @@ class Schema(object):
     def _read_schema(self):
         with self._open_schema(self._fingerprint, 'r') as schema:
             self._dict['fingerprint'] = self._get_schema_fingerprint(schema)
-            schema_info = json.loads(schema.read(self.schema_info_path))
-            self._dict['version'] = schema_info['version']
 
             for name in schema.namelist():
                 ns, _slash, key = name.partition('/')
@@ -548,6 +549,27 @@ class Schema(object):
         except KeyError:
             return self._dict[key]
 
+    def _generate_help(self, schema):
+        halp = {}
+
+        for namespace in ('commands', 'topics'):
+            halp[namespace] = {}
+
+            for member_schema in schema[namespace].values():
+                member_full_name = member_schema['full_name']
+
+                topic = halp[namespace].setdefault(member_full_name, {})
+                topic['name'] = member_schema['name']
+                if 'doc' in member_schema:
+                    topic['summary'] = (
+                        member_schema['doc'].split('\n\n', 1)[0].strip())
+                if 'topic_topic' in member_schema:
+                    topic['topic_topic'] = member_schema['topic_topic']
+                if 'exclude' in member_schema:
+                    topic['exclude'] = member_schema['exclude']
+
+        return halp
+
     def _write_schema(self):
         try:
             os.makedirs(self._DIR)
@@ -557,7 +579,6 @@ class Schema(object):
                 return
 
         with self._open_schema(self._fingerprint, 'w') as schema:
-            schema_info = {}
             for key, value in self._dict.items():
                 if key in self.namespaces:
                     ns = value
@@ -565,9 +586,10 @@ class Schema(object):
                         path = '{}/{}'.format(key, member)
                         schema.writestr(path, json.dumps(ns[member]))
                 else:
-                    schema_info[key] = value
+                    schema.writestr(key, json.dumps(value))
 
-            schema.writestr(self.schema_info_path, json.dumps(schema_info))
+            schema.writestr('_help',
+                            json.dumps(self._generate_help(self._dict)))
 
     def _read(self, path):
         with self._open_schema(self._fingerprint, 'r') as zf:
@@ -587,6 +609,14 @@ class Schema(object):
     def iter_namespace(self, namespace):
         return iter(self._dict[namespace])
 
+    def load_help(self):
+        if not self._help:
+            self._help = self._read('_help')
+
+            for ns in self._help:
+                for member in self._help[ns]:
+                    self._dict[ns][member].update(self._help[ns][member])
+
 
 def get_package(api, client):
     try:
-- 
2.7.4

From 3189614330ade677b32b0bb8a75bf929265e8cbc Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 20 Jul 2016 13:24:03 +0200
Subject: [PATCH 5/6] help: Do not create instances to get information about
 commands and topics

Creating instance requires that complete schema for the command is
read from schema cache and passed to constructor. This operation takes
a lot of time. Utilizing class properties and pregenerated help bits
allows to get the necessary information directly from classes reducing
time it takes significantly.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipalib/cli.py      | 15 ++++++++-------
 ipalib/plugable.py |  7 +++++--
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/ipalib/cli.py b/ipalib/cli.py
index 1faf8285c47d950d5fd311154b6936c8371d746c..d89a5320853ecf575c7ba710b2db2e62e1003141 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -727,8 +727,8 @@ class help(frontend.Local):
         self._builtins = []
 
         # build help topics
-        for c in self.api.Command():
-            if c is not self.api.Command[c.name]:
+        for c in self.api.Command:
+            if c is not self.api.Command.get_plugin(c.name):
                 continue
             if c.NO_CLI:
                 continue
@@ -793,13 +793,14 @@ class help(frontend.Local):
             self.print_commands(name, outfile)
         elif name == "commands":
             mcl = 0
-            for cmd in self.Command():
-                if cmd is not self.Command[cmd.name]:
+            for cmd_plugin in self.Command:
+                if cmd_plugin is not self.Command.get_plugin(cmd_plugin.name):
                     continue
-                if cmd.NO_CLI:
+                if cmd_plugin.NO_CLI:
                     continue
-                mcl = max(mcl, len(cmd.name))
-                writer('%s  %s' % (to_cli(cmd.name).ljust(mcl), cmd.summary))
+                mcl = max(mcl, len(cmd_plugin.name))
+                writer('%s  %s' % (to_cli(cmd_plugin.name).ljust(mcl),
+                                   cmd_plugin.summary))
         else:
             raise HelpError(topic=name)
 
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index 073ad05d7aee5e83cae5c6e20bac8f9439505198..af35f5bae27a17230267d5b10b5fdc4f784a4760 100644
--- a/ipalib/plugable.py
+++ b/ipalib/plugable.py
@@ -318,9 +318,12 @@ class APINameSpace(collections.Mapping):
         self.__enumerate()
         return iter(self.__plugins)
 
+    def get_plugin(self, key):
+        self.__enumerate()
+        return self.__plugins_by_key[key]
+
     def __getitem__(self, key):
-        self.__enumerate()
-        plugin = self.__plugins_by_key[key]
+        plugin = self.get_plugin(key)
         return self.__api._get(plugin)
 
     def __call__(self):
-- 
2.7.4

From 83d170febad3c9f25b9827900b2df3cec7ce98d2 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Tue, 26 Jul 2016 13:35:22 +0200
Subject: [PATCH 6/6] compat: Save server's API version in for pre-schema
 servers

When client comunicates with server that doesn't support 'schema'
command it needs to determine its api version to be able to use the
right compat code. Storing information about server version reduces the
need to call 'env' or 'ping' command only to first time the server is
contacted.

https://fedorahosted.org/freeipa/ticket/6069
---
 ipaclient/remote_plugins/__init__.py | 85 ++++++++++++++++++++++++++++++++----
 ipaclient/remote_plugins/compat.py   | 29 ++++++------
 ipaclient/remote_plugins/schema.py   | 77 +++-----------------------------
 ipaplatform/base/paths.py            | 15 +++++++
 4 files changed, 112 insertions(+), 94 deletions(-)

diff --git a/ipaclient/remote_plugins/__init__.py b/ipaclient/remote_plugins/__init__.py
index 6454a4f4ef956a1ef545b82a649ebf26ef6edd7b..444651d30fd0cd96299fecb7ee7b5e4532b0abd4 100644
--- a/ipaclient/remote_plugins/__init__.py
+++ b/ipaclient/remote_plugins/__init__.py
@@ -2,23 +2,90 @@
 # Copyright (C) 2016  FreeIPA Contributors see COPYING for license
 #
 
+import collections
+import errno
+import json
+import os
+
 from . import compat
 from . import schema
 from ipaclient.plugins.rpcclient import rpcclient
+from ipaplatform.paths import paths
+from ipapython.dnsutil import DNSName
+from ipapython.ipa_log_manager import log_mgr
+
+logger = log_mgr.get_logger(__name__)
+
+
+class ServerInfo(collections.MutableMapping):
+    _DIR = os.path.join(paths.USER_CACHE_PATH, 'ipa', 'servers')
+
+    def __init__(self, api):
+        hostname = DNSName(api.env.server).ToASCII()
+        self._path = os.path.join(self._DIR, hostname)
+        self._dict = {}
+        self._dirty = False
+
+        self._read()
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, *_exc_info):
+        if self._dirty:
+            self._write()
+
+    def _read(self):
+        try:
+            with open(self._path, 'r') as sc:
+                self._dict = json.load(sc)
+        except EnvironmentError as e:
+            if e.errno != errno.ENOENT:
+                logger.warning('Failed to read server info: {}'.format(e))
+
+    def _write(self):
+        try:
+            try:
+                os.makedirs(self._DIR)
+            except EnvironmentError as e:
+                if e.errno != errno.EEXIST:
+                    raise
+            with open(self._path, 'w') as sc:
+                json.dump(self._dict, sc)
+        except EnvironmentError as e:
+            logger.warning('Failed to write server info: {}'.format(e))
+
+    def __getitem__(self, key):
+        return self._dict[key]
+
+    def __setitem__(self, key, value):
+        self._dirty = key not in self._dict or self._dict[key] != value
+        self._dict[key] = value
+
+    def __delitem__(self, key):
+        del self._dict[key]
+        self._dirty = True
+
+    def __iter__(self):
+        return iter(self._dict)
+
+    def __len__(self):
+        return len(self._dict)
 
 
 def get_package(api):
     if api.env.in_tree:
         from ipaserver import plugins
     else:
-        client = rpcclient(api)
-        client.finalize()
-        try:
-            plugins = schema.get_package(api, client)
-        except schema.NotAvailable:
-            plugins = compat.get_package(api, client)
-        finally:
-            if client.isconnected():
-                client.disconnect()
+        with ServerInfo(api) as server_info:
+            client = rpcclient(api)
+            client.finalize()
+            try:
+                plugins = schema.get_package(api, server_info, client)
+            except schema.NotAvailable:
+                plugins = compat.get_package(api, server_info, client)
+            finally:
+                if client.isconnected():
+                    client.disconnect()
 
     return plugins
diff --git a/ipaclient/remote_plugins/compat.py b/ipaclient/remote_plugins/compat.py
index aef5718fcaade157487c0e65562c3bc8a11ad7de..b6d099a075deaaa17143f8ddddfb11d97b75f0ed 100644
--- a/ipaclient/remote_plugins/compat.py
+++ b/ipaclient/remote_plugins/compat.py
@@ -31,23 +31,26 @@ class CompatObject(Object):
     pass
 
 
-def get_package(api, client):
-    if not client.isconnected():
-        client.connect(verbose=False)
-
-    env = client.forward(u'env', u'api_version', version=u'2.0')
+def get_package(api, server_info, client):
     try:
-        server_version = env['result']['api_version']
+        server_version = server_info['version']
     except KeyError:
-        ping = client.forward(u'ping', version=u'2.0')
+        if not client.isconnected():
+            client.connect(verbose=False)
+        env = client.forward(u'env', u'api_version', version=u'2.0')
         try:
-            match = re.search(u'API version (2\.[0-9]+)', ping['summary'])
+            server_version = env['result']['api_version']
         except KeyError:
-            match = None
-        if match is not None:
-            server_version = match.group(1)
-        else:
-            server_version = u'2.0'
+            ping = client.forward(u'ping', u'api_version', version=u'2.0')
+            try:
+                match = re.search(u'API version (2\.[0-9]+)', ping['summary'])
+            except KeyError:
+                match = None
+            if match is not None:
+                server_version = match.group(1)
+            else:
+                server_version = u'2.0'
+        server_info['version'] = server_version
     server_version = LooseVersion(server_version)
 
     package_names = {}
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 9d5c08b85c13ebbed29ac15cdd9ad513ecb4639e..a215452ea0d2c1278a6121b3806b0daee02abd6e 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -19,6 +19,7 @@ from ipalib import errors, parameters, plugable
 from ipalib.frontend import Object
 from ipalib.output import Output
 from ipalib.parameters import DefaultFrom, Flag, Password, Str
+from ipaplatform.paths import paths
 from ipapython.dn import DN
 from ipapython.dnsutil import DNSName
 from ipapython.ipa_log_manager import log_mgr
@@ -55,17 +56,6 @@ _PARAMS = {
     'str': parameters.Str,
 }
 
-USER_CACHE_PATH = (
-    os.environ.get('XDG_CACHE_HOME') or
-    os.path.join(
-        os.environ.get(
-            'HOME',
-            os.path.expanduser('~')
-        ),
-        '.cache'
-    )
-)
-
 logger = log_mgr.get_logger(__name__)
 
 
@@ -359,62 +349,6 @@ class NotAvailable(Exception):
     pass
 
 
-class ServerInfo(collections.MutableMapping):
-    _DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'servers')
-
-    def __init__(self, api):
-        hostname = DNSName(api.env.server).ToASCII()
-        self._path = os.path.join(self._DIR, hostname)
-        self._dict = {}
-        self._dirty = False
-
-        self._read()
-
-    def __enter__(self):
-        return self
-
-    def __exit__(self, *_exc_info):
-        if self._dirty:
-            self._write()
-
-    def _read(self):
-        try:
-            with open(self._path, 'r') as sc:
-                self._dict = json.load(sc)
-        except EnvironmentError as e:
-            if e.errno != errno.ENOENT:
-                logger.warning('Failed to read server info: {}'.format(e))
-
-    def _write(self):
-        try:
-            try:
-                os.makedirs(self._DIR)
-            except EnvironmentError as e:
-                if e.errno != errno.EEXIST:
-                    raise
-            with open(self._path, 'w') as sc:
-                json.dump(self._dict, sc)
-        except EnvironmentError as e:
-            logger.warning('Failed to write server info: {}'.format(e))
-
-    def __getitem__(self, key):
-        return self._dict[key]
-
-    def __setitem__(self, key, value):
-        self._dirty = key not in self._dict or self._dict[key] != value
-        self._dict[key] = value
-
-    def __delitem__(self, key):
-        del self._dict[key]
-        self._dirty = True
-
-    def __iter__(self):
-        return iter(self._dict)
-
-    def __len__(self):
-        return len(self._dict)
-
-
 class Schema(object):
     """
     Store and provide schema for commands and topics
@@ -436,7 +370,7 @@ class Schema(object):
 
     """
     namespaces = {'classes', 'commands', 'topics'}
-    _DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'schema')
+    _DIR = os.path.join(paths.USER_CACHE_PATH, 'ipa', 'schema')
 
     def __init__(self, api, server_info, client):
         self._dict = {}
@@ -618,13 +552,12 @@ class Schema(object):
                     self._dict[ns][member].update(self._help[ns][member])
 
 
-def get_package(api, client):
+def get_package(api, server_info, client):
     try:
         schema = api._schema
     except AttributeError:
-        with ServerInfo(api.env.hostname) as server_info:
-            schema = Schema(api, server_info, client)
-            object.__setattr__(api, '_schema', schema)
+        schema = Schema(api, server_info, client)
+        object.__setattr__(api, '_schema', schema)
 
     fingerprint = str(schema['fingerprint'])
     package_name = '{}${}'.format(__name__, fingerprint)
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index bb0071a8e1a951e8df661d65058584c595c7a23a..b1fedf5d7b2abfd59291be4ba95b0d9c3dd0c9e9 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -21,6 +21,8 @@
 This base platform module exports default filesystem paths.
 '''
 
+import os
+
 
 class BasePathNamespace(object):
     BASH = "/bin/bash"
@@ -353,4 +355,17 @@ class BasePathNamespace(object):
     IPA_CUSTODIA_AUDIT_LOG = '/var/log/ipa-custodia.audit.log'
     IPA_GETKEYTAB = '/usr/sbin/ipa-getkeytab'
 
+    @property
+    def USER_CACHE_PATH(self):
+        return (
+            os.environ.get('XDG_CACHE_HOME') or
+            os.path.join(
+                os.environ.get(
+                    'HOME',
+                    os.path.expanduser('~')
+                ),
+                '.cache'
+            )
+        )
+
 path_namespace = BasePathNamespace
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to