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:

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 >

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: