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)

On 05/07/2014 03:17 AM, Zhou Zheng Sheng wrote: > 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']}) > + This logic should be in model

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