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

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(a)linux.vnet.ibm.com&gt; > --- > 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?

> + 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)