New subject: [project-kimchi] [PATCH 1/3] PEP 8: cleanup src/kimchi/disks.py

18 Dec 2013

On 12/19/2013 01:22 AM, Aline Manera wrote:
> From: Zhou Zheng Sheng <zhshzhou@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
>
> 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@linux.vnet.ibm.com>
> Signed-off-by: Aline Manera <alinefm@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

-- 
project-kimchi mailing list <project-kimchi@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@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[project-kimchi] [PATCH 0/3 V2] Logical pool fixes

Aline Manera

Aline Manera

Crístian Viana

Aline Manera

Crístian Viana

Sheldon

Zhou Zheng Sheng

Aline Manera

Crístian Viana

Aline Manera

tags

participants (4)