[Kimchi-devel] [PATCH] host/partitions: avoid calling disks.get_partitions_names() for each partition

[Zhou Zheng Sheng]

on 2014/05/06 23:11, Sheldon wrote:
> On 05/06/2014 05:04 PM, Zhou Zheng Sheng wrote:
>> In PartitionModel.lookup(), it calls disks.get_partitions_names() to
>> verify the given partition is eligible to be used in a new logical
>> storage pool as follow.
>>    if name not in disks.get_partitions_names():
>>        raise NotFoundError(...)
>>
>> When the front-end issues GET request to host/partitions, the back-end
>> would call get_partitions_names() for each partition. If there are many
>> disks and partitions in the host, get_partitions_names() takes a long
>> time to finish, and calling get_partitions_names() for each partition
>> would make the GET request timeout.
>>
>> This patch is to do the following.
>>    When the partition is listed from the URI host/partitions, we should
>> avoid calling get_partitions_names() for each of them.
>>    When the partition is listed from the URI host/partitions/sdaX, we
>> should verify sdaX is among the eligible partitions.
>>
>> The solution is that we make use of the resource_args in Partitions
>> collection class and override the _get_resources() method. In the
>> _get_resources() method, it sets resource_args to "[True]", indicating
>> that the listed partitions are already verified and eligible, so
>> PartitionModel.lookup() would skip the check. When we are outside of
>> _get_resources() method, resource_args is set to "[False]", and
>> PartitionModel.lookup() enables the check.
>>
>> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
>> ---
>>   src/kimchi/control/host.py | 26 +++++++++++++++++++++-----
>>   src/kimchi/model/host.py   |  4 ++--
>>   2 files changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py
>> index ad34919..11673d5 100644
>> --- a/src/kimchi/control/host.py
>> +++ b/src/kimchi/control/host.py
>> @@ -17,11 +17,14 @@
>>   # 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 threading
>> +
>>   import cherrypy
>>
>>   from kimchi.control.base import Collection, Resource, SimpleCollection
>>   from kimchi.control.utils import UrlSubNode, validate_method
>>   from kimchi.exception import OperationFailed
>> +from kimchi.rollbackcontext import RollbackContext
>>   from kimchi.template import render
>>
>>
>> @@ -75,18 +78,31 @@ class Partitions(Collection):
>>       def __init__(self, model):
>>           super(Partitions, self).__init__(model)
>>           self.resource = Partition
>> +        self._reset_resource_args()
>> +        self._resource_args_lock = threading.Lock()
>> +
>> +    def _reset_resource_args(self):
>> +        self.resource_args = [False]
>>
>> -    # Defining get_resources in order to return list of partitions in UI
>> -    # sorted by their path
>>       def _get_resources(self, flag_filter):
>> -        res_list = super(Partitions, self)._get_resources(flag_filter)
>> -        res_list.sort(key=lambda x: x.info['path'])
>> +        with RollbackContext() as rollback:
>> +            self._resource_args_lock.acquire()
>> +            rollback.prependDefer(self._resource_args_lock.release)
>> +
>> +            self.resource_args = [True]
>> +            rollback.prependDefer(self._reset_resource_args)
>> +
>> +            # Defining get_resources in order to return list of
>> partitions in
>> +            # UI sorted by their path
>> +            res_list = super(Partitions,
>> self)._get_resources(flag_filter)
> yes, verified flag for resource can avoid call get_partitions_names twice.
> Now Partition resource is a special case, it's __init__ accept extra
> argument "verified".
> Maybe there will be more special cases.
> 
> Not sure we can find a better way to define the Partition as follow:
> class Partition(Resource):
> def __init__(self, model, id, verified=False):
> ....
> def lookup(self, name, verified=False):
> ...
> 
> from the super _get_resources, it pass the follow args to it's resource.
> args = self.resource_args + [ident]
> 
> Not sure, Is it a good idea for extra self.resource_kargs, and
> self.model_kargs?
> 

I gave up refactoring the controller framework. I sent v2 patch to
refactor the underlying disks module, and split the eligibility checking
code out.

>> +            res_list.sort(key=lambda x: x.info['path'])
>>           return res_list
>>
>>
>>   class Partition(Resource):
>> -    def __init__(self, model, id):
>> +    def __init__(self, model, verified, id):
>>           super(Partition, self).__init__(model, id)
>> +        self.model_args = [verified] + list(self.model_args)
>>
>>       @property
>>       def data(self):
>> diff --git a/src/kimchi/model/host.py b/src/kimchi/model/host.py
>> index 47b0aa1..ebd6f79 100644
>> --- a/src/kimchi/model/host.py
>> +++ b/src/kimchi/model/host.py
>> @@ -244,8 +244,8 @@ class PartitionModel(object):
>>       def __init__(self, **kargs):
>>           pass
>>
>> -    def lookup(self, name):
>> -        if name not in disks.get_partitions_names():
>> +    def lookup(self, verified, name):
>> +        if not verified and name not in disks.get_partitions_names():
>>               raise NotFoundError("KCHPART0001E", {'name': name})
>>
>>           return disks.get_partition_details(name)
> 
> 


-- 
Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397