[Freeipa-devel] [PATCH 0112-7] Speeding up cli help

Wed Aug 17 11:21:31 UTC 2016

On 08/08/16 13:26, Jan Cholasta wrote:
> On 4.8.2016 16:32, David Kupka wrote:
>> On 03/08/16 16:33, Jan Cholasta wrote:
>>> On 3.8.2016 16:23, David Kupka wrote:
>>>> 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.
>>>
>>> Thanks, ACK.
>>>
>>> Pushed to master: 229e2a1ed9ea9877cb5e879fadd99f9040f77c96
>>>
>>
>> I've made and reviewer overlooked some errors. Attached patches fix them.
>
> Shame on me.
>
> Anyway,
>
> 1) When schema() raises SchemaUpToDate, the current code explodes:
>
> ipa: ERROR: KeyError: 'fingerprint'
> Traceback (most recent call last):
>   File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1348, in run
>     api.finalize()
>   File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 707,
> in finalize
>     self.__do_if_not_done('load_plugins')
>   File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 422,
> in __do_if_not_done
>     getattr(self, name)()
>   File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 585,
> in load_plugins
>     for package in self.packages:
>   File "/usr/lib/python2.7/site-packages/ipalib/__init__.py", line 919,
> in packages
>     ipaclient.remote_plugins.get_package(self),
>   File
> "/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/__init__.py",
> line 92, in get_package
>     plugins = schema.get_package(api, server_info, client)
>   File
> "/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py",
> line 558, in get_package
>     fingerprint = str(schema['fingerprint'])
>   File
> "/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py",
> line 483, in __getitem__
>     return self._dict[key]
> KeyError: 'fingerprint'
>
>
> 2) We read server info every time get_package() is called. It should be
> cache similarly to how the schema is cached in the api object.
>
>
> 3) Some of the commands are still fully initialized during "ipa help
> commands".
>
>
> Honza
>
Updated patches attached.

-- 
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0119.1-schema-cache-Do-not-reset-ServerInfo-dirty-flag.patch
Type: text/x-patch
Size: 1133 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0120.1-schema-cache-Do-not-read-fingerprint-and-format-from.patch
Type: text/x-patch
Size: 3405 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0121.1-Access-data-for-help-separately.patch
Type: text/x-patch
Size: 4475 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0122.1-frontent-Add-summary-class-property-to-CommandOverri.patch
Type: text/x-patch
Size: 958 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0123.1-schema-cache-Read-server-info-only-once.patch
Type: text/x-patch
Size: 2102 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0124.1-schema-cache-Store-API-schema-cache-in-memory.patch
Type: text/x-patch
Size: 4167 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0125.1-client-Do-not-create-instance-just-to-check-isinstan.patch
Type: text/x-patch
Size: 3626 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0126.1-schema-cache-Read-schema-instead-of-rewriting-it-whe.patch
Type: text/x-patch
Size: 3604 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0127.1-schema-check-Check-current-client-language-against-c.patch
Type: text/x-patch
Size: 1834 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0128.1-compat-Fix-ping-command-call.patch
Type: text/x-patch
Size: 1088 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/0b27cb80/attachment-0009.bin>