Reviewed-by: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; Test-by: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; Great, you can list a pv do not belongs to any VG. jiust a minor comments as follow. On 12/23/2013 10:44 PM, Zhou Zheng Sheng wrote: > Physical volume belongs to no volume group can be considered a free > block device to use. > > Implement a _get_vgname() function to get volume group name of a device > node. If the requested block device is not a physical volume, it returns > empty string, otherwise returns volume group name. Then it inspects the > volume group name for all potential devices and filters out in use > physical volumes. > > Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; > --- > src/kimchi/disks.py | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py > index 34b9e01..ceaa182 100644 > --- a/src/kimchi/disks.py > +++ b/src/kimchi/disks.py > @@ -99,6 +99,20 @@ def _parse_lsblk_output(output, keys): > return r > > > +def _get_vgname(devNodePath): > + """ Return volume group name of a physical volume. If the device > node path > + is not a physical volume, return empty string. """ > + pvs = subprocess.Popen( > + ["pvs", "--unbuffered", "--nameprefixes", "--noheadings", > + "-o", "vg_name", devNodePath], > + stdout=subprocess.PIPE, stderr=subprocess.PIPE) > + out, err = pvs.communicate() > + if pvs.returncode != 0: > + return "" > + > + return re.findall(r"LVM2_VG_NAME='([^\']*)'", out)[0] > + > + > def get_partitions_names(): > names = [] > keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT", "MAJ:MIN"] > @@ -111,10 +125,12 @@ def get_partitions_names(): > 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. > + # by any multipath device. Physical volume belongs to no > volume group > + # is also listed. > if not (dev['type'] in ['part', 'disk'] and > - dev['fstype'] == "" and > + dev['fstype'] in ['', 'LVM2_member'] and I have see a strange fstype on meina's host. so How can a constant at the head of this file.: EXCLUDE_FSTYEP = ['', 'LVM2_member'] we can easily to add more fstype that not need to be listed. and here: + dev['fstype'] in EXCLUDE_FSTYEP and

> dev['mountpoint'] == "" and > + _get_vgname(devNodePath) == "" and > _is_dev_leaf(devNodePath)): > continue >

Physical volume belongs to no volume group can be considered a free block device to use. Implement a _get_vgname() function to get volume group name of a device node. If the requested block device is not a physical volume, it returns empty string, otherwise returns volume group name. Then it inspects the volume group name for all potential devices and filters out in use physical volumes. Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; --- src/kimchi/disks.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py index 34b9e01..ceaa182 100644 --- a/src/kimchi/disks.py +++ b/src/kimchi/disks.py @@ -99,6 +99,20 @@ def _parse_lsblk_output(output, keys): return r +def _get_vgname(devNodePath): + """ Return volume group name of a physical volume. If the device node path + is not a physical volume, return empty string. """ + pvs = subprocess.Popen( + ["pvs", "--unbuffered", "--nameprefixes", "--noheadings", + "-o", "vg_name", devNodePath], + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + out, err = pvs.communicate() + if pvs.returncode != 0: + return "" + + return re.findall(r"LVM2_VG_NAME='([^\']*)'", out)[0] + + def get_partitions_names(): names = [] keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT", "MAJ:MIN"] @@ -111,10 +125,12 @@ def get_partitions_names(): 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. + # by any multipath device. Physical volume belongs to no volume group + # is also listed. if not (dev['type'] in ['part', 'disk'] and - dev['fstype'] == "" and + dev['fstype'] in ['', 'LVM2_member'] and dev['mountpoint'] == "" and + _get_vgname(devNodePath) == "" and _is_dev_leaf(devNodePath)): continue

Reviewed-by: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; Test-by: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt;

On 12/23/2013 10:44 PM, Zhou Zheng Sheng wrote: > Some devices are parent of other devices. We should just list the leaf > devices but not parent devices. For example, if a disk contains > partitions, we should only list the partitions. If a disk is held by > multipath device, we should not list the disk. > > Currently we strip the last charactter from "vda1" to get "vda" and > ignore the parent "vda" device. This may fails on disk contains lots of > logical partitions, for example "vda10"'s parent is not "vda1". Another > problem is this method can not find the parent-child relation ship > between a multipath device and its slaves. > > The most accurate information is on sysfs, and lsblk lists all children > device of the requested device based on sysfs. So when "lsblk -P > devices" prints only one line, it's the device itself, when it prints > more lines, they are the children devices. This patch use this method to > detect if a device a leaf one. > > This patch also avoids start new "cat" process to read uevent file, it > opens the uevent file and read it directly. > > v2: > Assign string literal to constants thus avoid breaking the line. > > v3: > Save N lsblk invocations when list all devices by collecting major:min > and device node path in one run. N is number of block devices. > Use short-circuiting operator "and" instead of all to save extra lsblk > invocation. > > Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; > --- > src/kimchi/disks.py | 83 > ++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 50 insertions(+), 33 deletions(-) > > diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py > index a054961..34b9e01 100644 > --- a/src/kimchi/disks.py > +++ b/src/kimchi/disks.py > @@ -27,27 +27,13 @@ from kimchi.exception import OperationFailed > from kimchi.utils import kimchi_log > > > -def _get_partition_path(name): > - """ Returns device path given a partition name """ > - dev_path = None > - 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 > +def _get_dev_node_path(maj_min): > + """ Returns device node path given the device number 'major:min' """ > + uevent = "/sys/dev/block/%s/uevent" % maj_min > + with open(uevent) as ueventf: > + content = ueventf.read() > > - 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) > - > - data = dict(re.findall(r'(\S+)=(".*?"|\S+)', out.replace("\n", " > "))) > + data = dict(re.findall(r'(\S+)=(".*?"|\S+)', > content.replace("\n", " "))) > > return "/dev/%s" % data["DEVNAME"] > > @@ -63,6 +49,39 @@ def _get_lsblk_devs(keys, devs=[]): > return _parse_lsblk_output(out, keys) > > > +def _get_dev_major_min(name): > + 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 > + else: > + msg = "Failed to find major and minor number for %s" % name > + raise OperationFailed(msg) > + > + return maj_min > + > + > +def _is_dev_leaf(devNodePath): > + try: > + # By default, lsblk prints a device information followed by > children > + # device information > + childrenCount = len( > + _get_lsblk_devs(["NAME"], [devNodePath])) - 1 > + except OperationFailed as e: > + # lsblk is known to fail on multipath devices > + # Assume these devices contain children > + kimchi_log.error( > + "Error getting device info for %s: %s", devNodePath, e) > + return False > + else: > + return childrenCount == 0 > + > + > def _parse_lsblk_output(output, keys): > # output is on format key="value", > # where key can be NAME, TYPE, FSTYPE, SIZE, MOUNTPOINT, etc > @@ -82,32 +101,30 @@ def _parse_lsblk_output(output, keys): > > def get_partitions_names(): > names = [] > - ignore_names = [] > - keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT"] > + keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT", "MAJ:MIN"] > # 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]) > + 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. > + if not (dev['type'] in ['part', 'disk'] and > + dev['fstype'] == "" and > + dev['mountpoint'] == "" and > + _is_dev_leaf(devNodePath)): > continue > > names.append(name) > > - return list(set(names) - set(ignore_names)) > + return names > > > def get_partition_details(name): > - dev_path = _get_partition_path(name) > + dev_path = _get_dev_node_path(_get_dev_major_min(name)) > > keys = ["TYPE", "FSTYPE", "SIZE", "MOUNTPOINT"] > try:

On 12/23/2013 12:44 PM, Zhou Zheng Sheng wrote: > Some devices are parent of other devices. We should just list the leaf > devices but not parent devices. For example, if a disk contains > partitions, we should only list the partitions. If a disk is held by > multipath device, we should not list the disk. > > Currently we strip the last charactter from "vda1" to get "vda" and > ignore the parent "vda" device. This may fails on disk contains lots of > logical partitions, for example "vda10"'s parent is not "vda1". Another > problem is this method can not find the parent-child relation ship > between a multipath device and its slaves. > > The most accurate information is on sysfs, and lsblk lists all children > device of the requested device based on sysfs. So when "lsblk -P > devices" prints only one line, it's the device itself, when it prints > more lines, they are the children devices. This patch use this method to > detect if a device a leaf one. > > This patch also avoids start new "cat" process to read uevent file, it > opens the uevent file and read it directly. > > v2: > Assign string literal to constants thus avoid breaking the line. > > v3: > Save N lsblk invocations when list all devices by collecting major:min > and device node path in one run. N is number of block devices. > Use short-circuiting operator "and" instead of all to save extra lsblk > invocation. > > Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; > --- > src/kimchi/disks.py | 83 > ++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 50 insertions(+), 33 deletions(-) > > diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py > index a054961..34b9e01 100644 > --- a/src/kimchi/disks.py > +++ b/src/kimchi/disks.py > @@ -27,27 +27,13 @@ from kimchi.exception import OperationFailed > from kimchi.utils import kimchi_log > > > -def _get_partition_path(name): > - """ Returns device path given a partition name """ > - dev_path = None > - 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 > +def _get_dev_node_path(maj_min): > + """ Returns device node path given the device number 'major:min' """ > + uevent = "/sys/dev/block/%s/uevent" % maj_min > + with open(uevent) as ueventf: > + content = ueventf.read() > > - 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) > - > - data = dict(re.findall(r'(\S+)=(".*?"|\S+)', out.replace("\n", " > "))) > + data = dict(re.findall(r'(\S+)=(".*?"|\S+)', > content.replace("\n", " "))) > > return "/dev/%s" % data["DEVNAME"] > > @@ -63,6 +49,39 @@ def _get_lsblk_devs(keys, devs=[]): > return _parse_lsblk_output(out, keys) > > > +def _get_dev_major_min(name): > + 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 > + else: > + msg = "Failed to find major and minor number for %s" % name > + raise OperationFailed(msg) > + > + return maj_min > + > + > +def _is_dev_leaf(devNodePath): > + try: > + # By default, lsblk prints a device information followed by > children > + # device information > + childrenCount = len( > + _get_lsblk_devs(["NAME"], [devNodePath])) - 1 > + except OperationFailed as e: > + # lsblk is known to fail on multipath devices > + # Assume these devices contain children > + kimchi_log.error( > + "Error getting device info for %s: %s", devNodePath, e) > + return False > + else: As I commented before the 'else' isn't needed as it is the normal code flow

> + return childrenCount == 0 > + > + > def _parse_lsblk_output(output, keys): > # output is on format key="value", > # where key can be NAME, TYPE, FSTYPE, SIZE, MOUNTPOINT, etc > @@ -82,32 +101,30 @@ def _parse_lsblk_output(output, keys): > > def get_partitions_names(): > names = [] > - ignore_names = [] > - keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT"] > + keys = ["NAME", "TYPE", "FSTYPE", "MOUNTPOINT", "MAJ:MIN"] > # 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]) > + 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. > + if not (dev['type'] in ['part', 'disk'] and > + dev['fstype'] == "" and > + dev['mountpoint'] == "" and > + _is_dev_leaf(devNodePath)): > continue > > names.append(name) > > - return list(set(names) - set(ignore_names)) > + return names > > > def get_partition_details(name): > - dev_path = _get_partition_path(name) > + dev_path = _get_dev_node_path(_get_dev_major_min(name)) > > keys = ["TYPE", "FSTYPE", "SIZE", "MOUNTPOINT"] > try: