[project-kimchi] [PATCH v1 1/3] PEP 8: cleanup src/kimchi/disks.py
Make this file PEP 8 clean Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- Makefile.am | 1 + src/kimchi/disks.py | 37 ++++++++++++++++++++----------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/Makefile.am b/Makefile.am index b79919c..e57d3b6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -40,6 +40,7 @@ EXTRA_DIST = \ PEP8_WHITELIST = \ src/kimchi/asynctask.py \ src/kimchi/config.py.in \ + src/kimchi/disks.py \ src/kimchi/server.py \ plugins/__init__.py \ plugins/sample/__init__.py \ diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py index 93d5dbc..874f669 100644 --- a/src/kimchi/disks.py +++ b/src/kimchi/disks.py @@ -18,26 +18,26 @@ # # You should have received a copy of the GNU Lesser General Public # License along with this library; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA -import collections -import os import re import subprocess -import sys 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) + 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): """ Returns device path given a partition name """ dev_path = None @@ -49,11 +49,13 @@ def get_partition_path(name, dev_paths): if dev_path: return dev_path + 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) + 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 @@ -67,11 +69,12 @@ def get_partitions_names(): expression = r"%s=\".*?\"" % key match = re.search(expression, line) field = match.group() - k, v = field.split('=',1) + k, v = field.split('=', 1) d[k.lower()] = v[1:-1] - if d['type'] not in ['part','lvm']: + if d['type'] not in ['part', 'lvm']: continue - # split()[0] to avoid the second part of the name, after the whiteline + # 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 @@ -84,18 +87,18 @@ def get_partitions_names(): except: raise OperationFailed("Unable to retrieve partitions' full path.") + def get_partition_details(name): try: # Find device path - dev_path = get_partition_path(name, - get_partitions_paths()) + 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) + 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, @@ -106,7 +109,7 @@ def get_partition_details(name): expression = r"%s=\".*?\"" % key match = re.search(expression, output) field = match.group() - k, v = field.split('=',1) + k, v = field.split('=', 1) d[k.lower()] = v[1:-1] if d['mountpoint']: # Sometimes the mountpoint comes with [SWAP] or other -- 1.7.11.7 -- 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.
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. 2. Devices not listed by "blkid -o device" Some devices are not listed by "blkid -o device", but actually they can be used. For example, a disk without patition table. Disk with partitions are not listed by blkid but the partitions are listed, this is OK. However, a disk with partition table but with no patitions is not listed in "blkid -o device", while it can be used actually. 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> --- src/kimchi/disks.py | 168 ++++++++++++++++++++++++++++------------------------ 1 file changed, 92 insertions(+), 76 deletions(-) diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py index 874f669..7c7798a 100644 --- a/src/kimchi/disks.py +++ b/src/kimchi/disks.py @@ -23,22 +23,25 @@ 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.") +def _get_partitions_paths(): + """ Returns all available partitions path of the host """ + blkid = subprocess.Popen( + ["blkid", "-o", "device"], + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + out, err = blkid.communicate() + if blkid.returncode != 0: + raise OperationFailed('Error executing blkid: %s' % err) + + dev_paths = out.rstrip("\n").split("\n") + return dev_paths -def get_partition_path(name, dev_paths): +def _get_partition_path(name, dev_paths): """ Returns device path given a partition name """ dev_path = None regexp = re.compile(r"^.*"+name+"$") @@ -46,79 +49,92 @@ def get_partition_path(name, dev_paths): if regexp.search(path) is not None: dev_path = path break - if dev_path: - return dev_path + return dev_path -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.") +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 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 _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(): + # Will be used later to double check the partition + dev_paths = _get_partitions_paths() + 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] + # 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. + path = _get_partition_path(name, dev_paths) + + # Only list unmounted and unformated partition or disk. + if not all([dev['type'] in ['part', 'disk'], + dev['fstype'] == "", + dev['mountpoint'] == "", + path is not None]): + continue + + names.append(name) + return names + + +def get_partition_details(name): + # Find device path + try: + dev_paths = _get_partitions_paths() + except OperationFailed as e: + kimchi_log.error( + "Error getting partition path for %s: %s", (name, e)) + return {} + + dev_path = _get_partition_path(name, dev_paths) + if dev_path is None: + return {} + + 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)) + 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 -- 1.7.11.7 -- 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.
The current front-end implementation filter out disks from the free block device listings. This is not needed after the previous back-end pat correctly listing free disks. Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- ui/js/src/kimchi.storagepool_add_main.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/js/src/kimchi.storagepool_add_main.js b/ui/js/src/kimchi.storagepool_add_main.js index 7efbfa0..02323f5 100644 --- a/ui/js/src/kimchi.storagepool_add_main.js +++ b/ui/js/src/kimchi.storagepool_add_main.js @@ -45,7 +45,7 @@ kimchi.initStorageAddPage = function() { var deviceHtml = $('#partitionTmpl').html(); var listHtml = ''; $.each(data, function(index, value) { - if (value.type === 'part') { + if (value.type === 'part' || value.type === 'disk') { listHtml += kimchi.template(deviceHtml, value); } }); -- 1.7.11.7 -- 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.
Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 12/18/2013 06:20 PM, Zhou Zheng Sheng wrote:
The current front-end implementation filter out disks from the free block device listings. This is not needed after the previous back-end pat correctly listing free disks.
Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- ui/js/src/kimchi.storagepool_add_main.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ui/js/src/kimchi.storagepool_add_main.js b/ui/js/src/kimchi.storagepool_add_main.js index 7efbfa0..02323f5 100644 --- a/ui/js/src/kimchi.storagepool_add_main.js +++ b/ui/js/src/kimchi.storagepool_add_main.js @@ -45,7 +45,7 @@ kimchi.initStorageAddPage = function() { var deviceHtml = $('#partitionTmpl').html(); var listHtml = ''; $.each(data, function(index, value) { - if (value.type === 'part') { + if (value.type === 'part' || value.type === 'disk') { listHtml += kimchi.template(deviceHtml, value); } });
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.
Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 12/18/2013 06:20 PM, Zhou Zheng Sheng wrote:
Make this file PEP 8 clean
Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- Makefile.am | 1 + src/kimchi/disks.py | 37 ++++++++++++++++++++----------------- 2 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/Makefile.am b/Makefile.am index b79919c..e57d3b6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -40,6 +40,7 @@ EXTRA_DIST = \ PEP8_WHITELIST = \ src/kimchi/asynctask.py \ src/kimchi/config.py.in \ + src/kimchi/disks.py \ src/kimchi/server.py \ plugins/__init__.py \ plugins/sample/__init__.py \ diff --git a/src/kimchi/disks.py b/src/kimchi/disks.py index 93d5dbc..874f669 100644 --- a/src/kimchi/disks.py +++ b/src/kimchi/disks.py @@ -18,26 +18,26 @@ # # You should have received a copy of the GNU Lesser General Public # License along with this library; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-import collections -import os import re import subprocess -import sys
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) + 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): """ Returns device path given a partition name """ dev_path = None @@ -49,11 +49,13 @@ def get_partition_path(name, dev_paths): if dev_path: return dev_path
+ 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) + 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 @@ -67,11 +69,12 @@ def get_partitions_names(): expression = r"%s=\".*?\"" % key match = re.search(expression, line) field = match.group() - k, v = field.split('=',1) + k, v = field.split('=', 1) d[k.lower()] = v[1:-1] - if d['type'] not in ['part','lvm']: + if d['type'] not in ['part', 'lvm']: continue - # split()[0] to avoid the second part of the name, after the whiteline + # 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 @@ -84,18 +87,18 @@ def get_partitions_names(): except: raise OperationFailed("Unable to retrieve partitions' full path.")
+ def get_partition_details(name): try: # Find device path - dev_path = get_partition_path(name, - get_partitions_paths()) + 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) + 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, @@ -106,7 +109,7 @@ def get_partition_details(name): expression = r"%s=\".*?\"" % key match = re.search(expression, output) field = match.group() - k, v = field.split('=',1) + k, v = field.split('=', 1) d[k.lower()] = v[1:-1] if d['mountpoint']: # Sometimes the mountpoint comes with [SWAP] or other
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.
participants (2)
-
Sheldon -
Zhou Zheng Sheng