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

[Sheldon]

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?

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


-- 
Thanks and best regards!

Sheldon Feng(冯少合)<shaohef at linux.vnet.ibm.com>
IBM Linux Technology Center