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

Problem: 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. In all, the original control flow is as follow. GET host/partitions Partitions._get_resources() PartitionsModel.get_list() disks.get_partitions_names() for each partition name: Partition.lookup() PartitionModel.lookup() verify partition name in disks.get_partitions_names() disks.get_partition_details() The solution is to split the block device verification code out of disks.get_partitions_names(), and put it in a new function disks._is_available(). Now get_partitions_names() just returns all the block devices, and disks.get_partition_details() can perform the eligibility check by calling disks._is_available(). The original get_partitions_names() checks eligibility for all the detected device, so calling it for each partition is too slow. The new _is_available() function just checks the given block device. The New control flow is as follow. GET host/partitions Partitions._get_resources() PartitionsModel.get_list() disks.get_partitions_names() for each partition name: Partition.lookup() PartitionModel.lookup() disks.get_partition_details() check if the block device _is_available() v1: Make use of the resource and model arguments to have Partitions collection controller set a flag, so Partition model can inspect the flag and skip the check accordingly. v2: v1 solution is too hack, re-write the patch by refactoring the underlying disks model. Split the batch verification into small peices amortized by each concrete block device. Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; --- src/kimchi/control/host.py | 6 +++++- src/kimchi/disks.py | 37 +++++++++++++++++++++++-------------- src/kimchi/model/host.py | 3 --- 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py index ad34919..ebf1bed 100644 --- a/src/kimchi/control/host.py +++ b/src/kimchi/control/host.py @@ -21,7 +21,7 @@ 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.exception import OperationFailed, NotFoundError from kimchi.template import render @@ -80,6 +80,7 @@ class Partitions(Collection): # sorted by their path def _get_resources(self, flag_filter): res_list = super(Partitions, self)._get_resources(flag_filter) + res_list = filter(lambda x: x.info['available'], res_list) res_list.sort(key=lambda x: x.info['path']) return res_list @@ -90,6 +91,9 @@ class Partition(Resource): @property def data(self): + if not self.info['available']: + raise NotFoundError("KCHPART0001E", {'name': self.info['name']}) + return self.info diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py index 842fd48..a13c753 100644 --- a/src/kimchi/disks.py +++ b/src/kimchi/disks.py @@ -142,7 +142,23 @@ def _get_vgname(devNodePath): return re.findall(r"LVM2_VG_NAME='([^\']*)'", out)[0] -def get_partitions_names(): +def _is_available(name, devtype, fstype, mountpoint, majmin): + devNodePath = _get_dev_node_path(majmin) + # Only list unmounted and unformated and leaf and (partition or disk) + # leaf means a partition, a disk has no partition, or a disk not held + # by any multipath device. Physical volume belongs to no volume group + # is also listed. Extended partitions should not be listed. + if (devtype in ['part', 'disk', 'mpath'] and + fstype in ['', 'LVM2_member'] and + mountpoint == "" and + _get_vgname(devNodePath) == "" and + _is_dev_leaf(devNodePath) and + not _is_dev_extended_partition(devtype, devNodePath)): + return True + return False + + +def get_partitions_names(check=False): names = set() keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT", "MAJ:MIN"] # output is on format key="value", @@ -151,26 +167,17 @@ def get_partitions_names(): # split()[0] to avoid the second part of the name, after the # whiteline name = dev['name'].split()[0] - devNodePath = _get_dev_node_path(dev['maj:min']) - # Only list unmounted and unformated and leaf and (partition or disk) - # leaf means a partition, a disk has no partition, or a disk not held - # by any multipath device. Physical volume belongs to no volume group - # is also listed. Extended partitions should not be listed. - if not (dev['type'] in ['part', 'disk', 'mpath'] and - dev['fstype'] in ['', 'LVM2_member'] and - dev['mountpoint'] == "" and - _get_vgname(devNodePath) == "" and - _is_dev_leaf(devNodePath) and - not _is_dev_extended_partition(dev['type'], devNodePath)): + if check and not _is_available(name, dev['type'], dev['fstype'], + dev['mountpoint'], dev['maj:min']): continue - names.add(name) return list(names) def get_partition_details(name): - dev_path = _get_dev_node_path(_get_dev_major_min(name)) + majmin = _get_dev_major_min(name) + dev_path = _get_dev_node_path(majmin) keys = ["TYPE", "FSTYPE", "SIZE", "MOUNTPOINT"] try: @@ -180,6 +187,8 @@ def get_partition_details(name): "Error getting partition info for %s: %s", name, e) return {} + dev['available'] = _is_available(name, dev['type'], dev['fstype'], + dev['mountpoint'], majmin) if dev['mountpoint']: # Sometimes the mountpoint comes with [SWAP] or other # info which is not an actual mount point. Filtering it diff --git a/src/kimchi/model/host.py b/src/kimchi/model/host.py index 47b0aa1..e9ac487 100644 --- a/src/kimchi/model/host.py +++ b/src/kimchi/model/host.py @@ -245,9 +245,6 @@ class PartitionModel(object): pass def lookup(self, name): - if name not in disks.get_partitions_names(): - raise NotFoundError("KCHPART0001E", {'name': name}) - return disks.get_partition_details(name)