[Kimchi-devel] [project-kimchi] [PATCH 2/3] Issue #276: logical pool: a quick fix for the device listing rules, back-end
Zhou Zheng Sheng
zhshzhou at linux.vnet.ibm.com
Thu Dec 19 10:50:35 UTC 2013
on 2013/12/19 14:41, Sheldon wrote:
>
> On 12/19/2013 01:22 AM, Aline Manera wrote:
>> From: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
> zhengsheng, it can not work well for mpath.
> I have comment on issue 276
> https://github.com/kimchi-project/kimchi/issues/276
Sheldon, I sent a fix. See email titled 'logical pool fixes: only list
leaf devices, and read file instead of run "cat"'
Thanks for report this bug!
>>
>> The current logical pool implementation has some problems. It lists ext4
>> patitions but in fact partitions with FS should not be listed, because
>> a dual boot system setup can make use of those unmounted partitions. It
>> also fails to run lsblk for some devices.
>>
>> According to the discussion on github issue page[1]. We decide to list
>> only unused partitions and disks, which means, PV and LV will not be
>> listed, mounted partitions and partition with FS will not be listed.
>>
>> When lsblk fails to run on a purticular device, it swallows the error
>> and return an empty dict.
>>
>> Some kinds of free block device still can not be listed by the current
>> quick fix, for this is only a *quick* fix. The following cases are not
>> covered, they should be improved in future patches.
>> 1. PV that doesn't belong to any VG.
>> blkid and lsblk do not provide enough information to detect if a PV
>> belongs to an VG.
>>
>> To get the device path, it uses sysfs uevent file to get the device
>> name and
>> then assume the device path is /dev/<devname>
>>
>> This patch also drops the defensive coding style of the current
>> implementation. Defensive coding is not Pythonic. It makes the code
>> harder to debug, delay the discovery of code error and makes the main
>> logic unclear. This patch also extract a function to run and parse
>> lsblk to avoid repeating similar code.
>>
>> For a long term solution, backend should just list all block devices,
>> along with information describing if they are in use. The front end
>> provides a checkbox "filter in use devices" and let users decide what
>> they want.
>>
>> [1] https://github.com/kimchi-project/kimchi/issues/276
>>
>> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
>> Signed-off-by: Aline Manera <alinefm at br.ibm.com>
>>
>> fix
>> ---
>> src/kimchi/disks.py | 170
>> ++++++++++++++++++++++++++-------------------------
>> 1 file changed, 87 insertions(+), 83 deletions(-)
>>
>> diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py
>> index 874f669..0ae715c 100644
>> --- a/src/kimchi/disks.py
>> +++ b/src/kimchi/disks.py
>> @@ -23,102 +23,106 @@
>> import re
>> import subprocess
>>
>> +from kimchi.utils import kimchi_log
>> from kimchi.exception import OperationFailed
>>
>>
>> -def get_partitions_paths():
>> - try:
>> - """ Returns all available partitions path of the host """
>> - blkid = subprocess.Popen(
>> - ["blkid", "-o", "device"],
>> - stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>> - dev_paths = blkid.communicate()[0].rstrip("\n").split("\n")
>> - except:
>> - raise OperationFailed("Unable to retrieve partitions' full
>> path.")
>> - return dev_paths
>> -
>> -
>> -def get_partition_path(name, dev_paths):
>> +def _get_partition_path(name):
>> """ Returns device path given a partition name """
>> dev_path = None
>> - regexp = re.compile(r"^.*"+name+"$")
>> - for path in dev_paths:
>> - if regexp.search(path) is not None:
>> - dev_path = path
>> + maj_min = None
>> +
>> + keys = ["NAME", "MAJ:MIN"]
>> + dev_list = _get_lsblk_devs(keys)
>> +
>> + for dev in dev_list:
>> + if dev['name'] == name:
>> + maj_min = dev['maj:min']
>> break
>> - if dev_path:
>> - return dev_path
>>
>> + uevent_cmd = "cat /sys/dev/block/%s/uevent" % maj_min
>> + uevent = subprocess.Popen(uevent_cmd, stdout=subprocess.PIPE,
>> + stderr=subprocess.PIPE, shell=True)
>> + out, err = uevent.communicate()
>> + if uevent.returncode != 0:
>> + raise OperationFailed("Error getting partition path for %s",
>> name)
> +with open("/sys/dev/block/%s/uevent" % maj_min) as f:
> + out = f.read()
>>
>> -def get_partitions_names():
>> - try:
>> - """ Returns all the names of available partitions """
>> - lsblk = subprocess.Popen(
>> - ["lsblk", "-Pbo", "NAME,TYPE"],
>> - stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>> - output = lsblk.communicate()[0]
>> - lines_output = output.rstrip("\n").split("\n")
>> - # Will be used later to double check the partition
>> - dev_paths = get_partitions_paths()
>> - names = []
>> - keys = ["NAME", "TYPE"]
>> - # output is on format key="value", where key = NAME, TYPE
>> - for line in lines_output:
>> - d = {}
>> - for key in keys:
>> - expression = r"%s=\".*?\"" % key
>> - match = re.search(expression, line)
>> - field = match.group()
>> - k, v = field.split('=', 1)
>> - d[k.lower()] = v[1:-1]
>> - if d['type'] not in ['part', 'lvm']:
>> - continue
>> - # split()[0] to avoid the second part of the name,
>> - # after the whiteline
>> - name = d['name'].split()[0]
>> - # There are special cases where lsblk reports
>> - # a partition that does not exist in blkid and fdisk
>> (Extended
>> - # partitions), which can't be used for pool creation. We
>> - # need to filter these cases as well.
>> - if not get_partition_path(name, dev_paths):
>> - continue
>> - names.append(name)
>> - return names
>> - except:
>> - raise OperationFailed("Unable to retrieve partitions' full
>> path.")
>> + data = dict(re.findall(r'(\S+)=(".*?"|\S+)', out.replace("\n", "
>> ")))
>>
>> + return "/dev/%s" % data["DEVNAME"]
>>
>> -def get_partition_details(name):
>> - try:
>> - # Find device path
>> - dev_path = get_partition_path(name, get_partitions_paths())
>> - # Couldn't find dev_path.
>> - if not dev_path:
>> - return
>> - # Executing lsblk to get partition/disk info
>> - lsblk = subprocess.Popen(
>> - ["lsblk", "-Pbo", "TYPE,FSTYPE,SIZE,MOUNTPOINT", dev_path],
>> - stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>> - # single line output
>> - output = lsblk.communicate()[0].rstrip("\n")
>> - # output is on format key="value", where key = NAME, TYPE,
>> FSTYPE,
>> - # SIZE, MOUNTPOINT
>> - keys = ["TYPE", "FSTYPE", "SIZE", "MOUNTPOINT"]
>> +
>> +def _get_lsblk_devs(keys, devs=[]):
>> + lsblk = subprocess.Popen(
>> + ["lsblk", "-Pbo"] + [','.join(keys)] + devs,
>> + stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>> + out, err = lsblk.communicate()
>> + if lsblk.returncode != 0:
>> + raise OperationFailed('Error executing lsblk: %s' % err)
>> +
>> + return _parse_lsblk_output(out, keys)
>> +
>> +
>> +def _parse_lsblk_output(output, keys):
>> + # output is on format key="value",
>> + # where key can be NAME, TYPE, FSTYPE, SIZE, MOUNTPOINT, etc
>> + lines = output.rstrip("\n").split("\n")
>> + r = []
>> + for line in lines:
>> d = {}
>> for key in keys:
>> expression = r"%s=\".*?\"" % key
>> - match = re.search(expression, output)
>> + match = re.search(expression, line)
>> field = match.group()
>> k, v = field.split('=', 1)
>> d[k.lower()] = v[1:-1]
>> - if d['mountpoint']:
>> - # Sometimes the mountpoint comes with [SWAP] or other
>> - # info which is not an actual mount point. Filtering it
>> - regexp = re.compile(r"\[.*\]")
>> - if regexp.search(d['mountpoint']) is not None:
>> - d['mountpoint'] = ''
>> - d['path'] = dev_path
>> - d['name'] = name
>> - return d
>> - except:
>> - raise OperationFailed("Unable to retrieve partitions' data.")
>> + r.append(d)
>> + return r
>> +
>> +
>> +def get_partitions_names():
>> + names = []
>> + ignore_names = []
>> + keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT"]
>> + # output is on format key="value",
>> + # where key can be NAME, TYPE, FSTYPE, MOUNTPOINT
>> + for dev in _get_lsblk_devs(keys):
>> + # split()[0] to avoid the second part of the name, after the
>> + # whiteline
>> + name = dev['name'].split()[0]
>> + # Only list unmounted and unformated partition or disk.
>> + if not all([dev['type'] in ['part', 'disk'],
>> + dev['fstype'] == "",
>> + dev['mountpoint'] == ""]):
>> +
>> + # the whole disk must be ignored in it has at least one
>> + # mounted/formatted partition
>> + if dev['type'] == 'part':
>> + ignore_names.append(name[:-1])
> it is OK, if partitions less 10
> but what about: sda10?
> "sda10"[:-1].
>> + continue
>> +
>> + names.append(name)
>> +
>> + return list(set(names) - set(ignore_names))
>> +
>> +
>> +def get_partition_details(name):
>> + dev_path = _get_partition_path(name)
>> +
>> + keys = ["TYPE", "FSTYPE", "SIZE", "MOUNTPOINT"]
>> + try:
>> + dev = _get_lsblk_devs(keys, [dev_path])[0]
>> + except OperationFailed as e:
>> + kimchi_log.error(
>> + "Error getting partition info for %s: %s", (name, e))
>
> should be:
> + kimchi_log.error(
> + "Error getting partition info for %s: %s", name, e)
>
>> + return {}
>> +
>> + if dev['mountpoint']:
>> + # Sometimes the mountpoint comes with [SWAP] or other
>> + # info which is not an actual mount point. Filtering it
>> + regexp = re.compile(r"\[.*\]")
>> + if regexp.search(dev['mountpoint']) is not None:
>> + dev['mountpoint'] = ''
>> + dev['path'] = dev_path
>> + dev['name'] = name
>> + return dev
>
> Sheldon Feng(???)
> IBM Linux Technology Center
>
--
Thanks and best regards!
Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397
--
project-kimchi mailing list <project-kimchi at googlegroups.com>
https://groups.google.com/forum/#!forum/project-kimchi
---
You received this message because you are subscribed to the Google Groups "project-kimchi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to project-kimchi+unsubscribe at googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
More information about the Kimchi-devel
mailing list