[kimchi-devel][PATCH 0/6] Pool on existent VG
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Royce Lv (6): Storagepool on existent VG: Update document Add helper function to retrieve vg information Update controller Update model for existent VG Update mockmodel tests Fix fh leak in run_command subprocess docs/API.md | 20 ++++++++++++++++ src/kimchi/control/host.py | 6 +++++ src/kimchi/i18n.py | 2 ++ src/kimchi/mockmodel.py | 9 ++++++++ src/kimchi/model/libvirtstoragepool.py | 11 +++++---- src/kimchi/model/storagepools.py | 28 +++++++++++++---------- src/kimchi/model/vgs.py | 42 ++++++++++++++++++++++++++++++++++ src/kimchi/utils.py | 22 ++++++++++++++++-- tests/test_mock_storagepool.py | 9 +++++++- 9 files changed, 130 insertions(+), 19 deletions(-) create mode 100644 src/kimchi/model/vgs.py -- 2.1.0
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Add interfaces to report existent volume group on host. the returning detail will include what pool this volume group belongs to. If this VG is not used by any storage pool, its storage_pool will be empty string. So the process of creating pool on existent VG will be: 1. List VG not occupied by storage pool: GET /host/vgs?pool='' This will retrieve volume group path and user can choose from one. 2. Create storage pool from chosen VG: POST /storagepools/ {'path':volume_group_path, 'name': storagepool_name, 'type': 'logical'} Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/docs/API.md b/docs/API.md index 3f7925f..4aa708f 100644 --- a/docs/API.md +++ b/docs/API.md @@ -986,6 +986,26 @@ stats history * mountpoint: If the partition is mounted, represents the mountpoint. Otherwise blank. +### Collection: Volume groups + +**URI:** /host/vgs + +**Methods:** + +* **GET**: Retrieves list of volume groups. + +### Resource: Volume group + +**URI:** /host/vgs/*:name* + +**Methods:** + +* **GET**: Retrieve information of a single volume group. + * pool: logical storage pool name which based on this volume group. + * path: path of this storage volume group. + * size: Total size of volume group. + * name: Name of the volume group. + ### Collection: Devices **URI:** /host/devices -- 2.1.0
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 2 ++ src/kimchi/utils.py | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index e4e1a89..63f57d4 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -49,6 +49,8 @@ messages = { "KCHDISKS0001E": _("Error while getting block devices. Details: %(err)s"), "KCHDISKS0002E": _("Error while getting block device information for %(device)s."), + "KCHLVM0001E": _("Error while getting lvm information %(err)s."), + "KCHDL0001E": _("Unable to find distro file: %(filename)s"), "KCHDL0002E": _("Unable to parse distro file: %(filename)s. Make sure, it is a JSON file."), diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index d71338a..0b11fcb 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -245,6 +245,24 @@ def parse_cmd_output(output, output_items): return res +def find_vgs(): + cmd = ["vgs", "--noheadings", "-o", "vg_name, vg_size"] + try: + out, error, ret = run_command(cmd, 5) + except TimeoutExpired: + kimchi_log.warning("volume group query timeout") + return list() + + if error: + raise OperationFailed("KCHLVM0001E", {'err': error}) + + vgs = parse_cmd_output(out, ['name', 'size']) + for vg in vgs: + vg['path'] = os.path.join('/dev/', vg['name']) + vg['pool'] = '' + return vgs + + def patch_find_nfs_target(nfs_server): cmd = ["showmount", "--no-headers", "--exports", nfs_server] try: -- 2.1.0
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/control/host.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py index 5e736db..4746c9b 100644 --- a/src/kimchi/control/host.py +++ b/src/kimchi/control/host.py @@ -39,6 +39,7 @@ class Host(Resource): self.repositories = Repositories(self.model) self.swupdate = self.generate_action_handler_task('swupdate') self.cpuinfo = CPUInfo(self.model) + self.vgs = VGs(self.model) @property def data(self): @@ -99,6 +100,11 @@ class Devices(Collection): self.resource = Device +class VGs(SimpleCollection): + def __init__(self, model): + super(VGs, self).__init__(model) + + class VMHolders(SimpleCollection): def __init__(self, model, device_id): super(VMHolders, self).__init__(model) -- 2.1.0
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Avoid scan and build VG when using an existent VG to consturct a logical storage pool. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/libvirtstoragepool.py | 11 +++++---- src/kimchi/model/storagepools.py | 28 +++++++++++++---------- src/kimchi/model/vgs.py | 42 ++++++++++++++++++++++++++++++++++ src/kimchi/utils.py | 4 ++-- 4 files changed, 67 insertions(+), 18 deletions(-) create mode 100644 src/kimchi/model/vgs.py diff --git a/src/kimchi/model/libvirtstoragepool.py b/src/kimchi/model/libvirtstoragepool.py index c6deafc..b3349c4 100644 --- a/src/kimchi/model/libvirtstoragepool.py +++ b/src/kimchi/model/libvirtstoragepool.py @@ -140,15 +140,18 @@ class LogicalPoolDef(StoragePoolDef): # Required parameters # name: # type: + # Optional parameters # source[devices]: pool = E.pool(type='logical') pool.append(E.name(self.poolArgs['name'])) - source = E.source() - for device_path in self.poolArgs['source']['devices']: - source.append(E.device(path=device_path)) + if 'source' in self.poolArgs: + source = E.source() + for device_path in self.poolArgs['source']['devices']: + source.append(E.device(path=device_path)) + + pool.append(source) - pool.append(source) pool.append(E.target(E.path(self.path))) return ET.tostring(pool, encoding='unicode', pretty_print=True) diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index b85f3b4..642496a 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -68,6 +68,7 @@ class StoragePoolsModel(object): {'err': e.get_error_message()}) def create(self, params): + build = True task_id = None conn = self.conn.get() try: @@ -79,17 +80,20 @@ class StoragePoolsModel(object): # used before but a volume group will already exist with this name # So check the volume group does not exist to create the pool if params['type'] == 'logical': - vgdisplay_cmd = ['vgdisplay', name.encode('utf-8')] - output, error, returncode = run_command(vgdisplay_cmd) - # From vgdisplay error codes: - # 1 error reading VGDA - # 2 volume group doesn't exist - # 3 not all physical volumes of volume group online - # 4 volume group not found - # 5 no volume groups found at all - # 6 error reading VGDA from lvmtab - if returncode not in [2, 4, 5]: - raise InvalidOperation("KCHPOOL0036E", {'name': name}) + if'source' in params: + vgdisplay_cmd = ['vgdisplay', name.encode('utf-8')] + output, error, returncode = run_command(vgdisplay_cmd) + # From vgdisplay error codes: + # 1 error reading VGDA + # 2 volume group doesn't exist + # 3 not all physical volumes of volume group online + # 4 volume group not found + # 5 no volume groups found at all + # 6 error reading VGDA from lvmtab + if returncode not in [2, 4, 5]: + raise InvalidOperation("KCHPOOL0036E", {'name': name}) + else: + build = False if params['type'] == 'kimchi-iso': task_id = self._do_deep_scan(params) @@ -118,7 +122,7 @@ class StoragePoolsModel(object): return name pool = conn.storagePoolDefineXML(xml, 0) - if params['type'] in ['logical', 'dir', 'netfs', 'scsi']: + if build and params['type'] in ['logical', 'dir', 'netfs', 'scsi']: pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW) # autostart dir, logical, netfs and scsi storage pools created # from kimchi diff --git a/src/kimchi/model/vgs.py b/src/kimchi/model/vgs.py new file mode 100644 index 0000000..2a3c260 --- /dev/null +++ b/src/kimchi/model/vgs.py @@ -0,0 +1,42 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014-2015 +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# 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 + +from kimchi.utils import find_vgs +from kimchi.model.storagepools import StoragePoolsModel, StoragePoolModel + + +class VGsModel(object): + def __init__(self, **kargs): + self.conn = kargs['conn'] + self.objstore = kargs['objstore'] + + def get_list(self): + def _get_path(name): + return StoragePoolModel( + conn=self.conn, objstore=self.objstore).lookup(name)['path'] + vgs = find_vgs() + pool_name = StoragePoolsModel( + conn=self.conn, objstore=self.objstore).get_list() + pool_info = zip(pool_name, map(_get_path, pool_name)) + for info in pool_info: + for vg in vgs: + if info[1] == vg['path']: + vg['pool'] = info[0] + + return vgs diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index 0b11fcb..4b08112 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -37,7 +37,7 @@ from cherrypy.lib.reprconf import Parser from kimchi.asynctask import AsyncTask from kimchi.config import paths, PluginPaths -from kimchi.exception import InvalidParameter, TimeoutExpired +from kimchi.exception import InvalidParameter, TimeoutExpired, OperationFailed kimchi_log = cherrypy.log.error_log @@ -246,7 +246,7 @@ def parse_cmd_output(output, output_items): def find_vgs(): - cmd = ["vgs", "--noheadings", "-o", "vg_name, vg_size"] + cmd = ["vgs", "--noheadings", "-o", "vg_name,vg_size"] try: out, error, ret = run_command(cmd, 5) except TimeoutExpired: -- 2.1.0
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 9 +++++++++ tests/test_mock_storagepool.py | 9 ++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 413ac5d..faa517e 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -306,6 +306,15 @@ class MockModel(Model): return self._model_storagevolume_lookup(pool, vol) + def _mock_vgs_get_list(self): + vgs = list() + + for i in range(3): + vg = dict(path='/dev/vg%s' % i, name='vg%s' % i, size='10G') + vgs.append(vg) + + return vgs + def _mock_partitions_get_list(self): return self._mock_partitions.partitions.keys() diff --git a/tests/test_mock_storagepool.py b/tests/test_mock_storagepool.py index 1dc9277..7b70d3e 100644 --- a/tests/test_mock_storagepool.py +++ b/tests/test_mock_storagepool.py @@ -71,6 +71,10 @@ class MockStoragepoolTests(unittest.TestCase): fc_devs = json.loads(self.request('/host/devices?_cap=fc_host').read()) fc_devs = [dev['name'] for dev in fc_devs] + # MockModel always returns 3 vgs + vgs = json.loads(self.request('/host/vgs').read()) + self.assertEquals(3, len(vgs)) + poolDefs = [ {'type': 'dir', 'name': u'kīмсhīUnitTestDirPool', 'path': '/tmp/kimchi-images'}, @@ -83,7 +87,10 @@ class MockStoragepoolTests(unittest.TestCase): 'source': {'host': '127.0.0.1', 'target': 'iqn.2015-01.localhost.kimchiUnitTest'}}, {'type': 'logical', 'name': u'kīмсhīUnitTestLogicalPool', - 'source': {'devices': [devs[0]]}}] + 'source': {'devices': [devs[0]]}}, + {'type': 'logical', 'name': vgs[0]['name'], + 'path': vgs[0]['path']} + ] def _do_test(params): name = params['name'] -- 2.1.0
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Because command popen created without close parent fds, it may cause various bugs in execution. when running testcases test_mock_storagepool, because fds are still open in child process, following error will print out when command execution: rc: 5 error: File descriptor 4 (socket:[776684]) leaked on vgdisplay invocation. Parent PID 13122: python File descriptor 5 (socket:[777857]) leaked on vgdisplay invocation. Parent PID 13122: python File descriptor 6 (/dev/null) leaked on vgdisplay invocation. Parent PID 13122: python File descriptor 8 (/dev/null) leaked on vgdisplay invocation. Parent PID 13122: python File descriptor 10 (/dev/null) leaked on vgdisplay invocation. Parent PID 13122: python File descriptor 11 (/dev/null) leaked on vgdisplay invocation. Parent PID 13122: python Volume group name "kīмсhīUnitTestLogicalPool" has invalid characters. Cannot process volume group kīмсhīUnitTestLogicalPool returned from cmd: vgdisplay kīмсhīUnitTestLogicalPool Fix this bug by forcing close fds when popen execution. REF: http://bugs.python.org/issue7213 Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index 4b08112..95babe0 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -193,7 +193,7 @@ def run_command(cmd, timeout=None): try: proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + stderr=subprocess.PIPE, close_fds=True) if timeout is not None: timer = Timer(timeout, kill_proc, [proc, timeout_flag]) timer.setDaemon(True) -- 2.1.0
I had a few problems when testing this patchset. Below is the sequence of commands I tried, their outputs and my opinions. I'm running Fedora 21. First, I tried to query the current VGs on my system: GET /host/vgs [ { "path":"/dev/fedora", "name":"fedora", "pool":"", "size":"19,51g" } ] That is the default VG created by the Fedora installer. The field "size" contains "19,51g" and I think that's not a desirable output due to these two reasons: 1) The value contains a unit. No other API on Kimchi returns a size value with a unit, the return unit is always documented. For example, in docs/API.md there is the following description for /storagepools/<pool>: "capacity: The total space which can be used to store volumes. *The unit is Bytes*". Notice how the unit is specified on the documentation. This makes it easy for the user to parse the output value because they always know which unit the value will be in. On Kimchi, we have the function "kimchi.utils.convert_data_size" which converts values from any unit to any other unit. I suggest we use bytes as the VG size unit as it is also the unit used elsewhere. 2) The value contains a comma instead of a dot. Probably that's because my system is using a Brazilian Portuguese locale (for context: in Brazilian Portuguese we use commas instead of dots to split decimal values, e.g. "1,5" = one and a half, unlike in English, where they use "1.5"). We shouldn't output different values based on the system's locale, otherwise, the user will never know how to parse the server's output. If we use bytes as the unit as suggested above, we wouldn't even need to use a decimal separator here, because we don't deal with values less than one byte anyway. Both of the problems above are probably related to the output of "vgs". We should normalize that output so the user always know what to expect. Another problem I'm having here is that I can't filter the VGs based on the field 'pool': GET /host/vgs?pool='x' [ { "path":"/dev/fedora", "name":"fedora", "pool":"", "size":"19,51g" } ] If I understand correctly, I should get an empty list here because I filtered for "pool=x", and the result doesn't have "pool=x". Then I created a new VG using "vgcreate" and I queried Kimchi again: GET /host/vgs [ { "path":"/dev/fedora", "name":"fedora", "pool":"", "size":"19,51g" }, { "path":"/dev/kimchivg", "name":"kimchivg", "pool":"", "size":"5,00g" } ] And then I created a new storage pool with that VG: POST /storagepools/ '{"path": "kimchivg", "name": "kimchi-logical" "type": "logical"}' { "available":0, "nr_volumes":0, "path":"/dev/kimchi-logical", "allocated":0, "capacity":0, "name":"kimchi-logical", "persistent":true, "source":{}, "state":"inactive", "autostart":false, "type":"logical" } However, if I query the VGs again, "kimchivg" still doesn't report that it now belongs to "kimchi-logical": GET /host/vgs?name='kimchivg' [ { "path":"/dev/fedora", "name":"fedora", "pool":"", "size":"19,51g" }, { "path":"/dev/kimchivg", "name":"kimchivg", "pool":"", "size":"5,00g" } ] Notice how I couldn't filter by the VG name as well. And "pool" in "kimchivg" is empty. As far as I understand, it should contain "kimchi-logical" now. And lastly, when I tried to activate that new storage pool, I got an error: POST /storagepools/kimchi-logical/activate { "reason":"KCHPOOL0009E: Unable to activate storage pool kimchi-logical. Details: internal error: Child process (/usr/sbin/vgchange -aly kimchi-logical) unexpected exit status 5: Volume group \"kimchi-logical\" not found\n Cannot process volume group kimchi-logical\n", "code":"500 Internal Server Error", "call_stack":"Traceback (most recent call last):\n File \"/usr/lib/python2.7/site-packages/cherrypy/_cprequest.py\", line 656, in respond\n response.body = self.handler()\n File \"/usr/lib/python2.7/site-packages/cherrypy/lib/encoding.py\", line 188, in __call__\n self.body = self.oldhandler(*args, **kwargs)\n File \"/usr/lib/python2.7/site-packages/cherrypy/_cpdispatch.py\", line 34, in __call__\n return self.callable(*self.args, **self.kwargs)\n File \"/home/kimchi/kimchi/src/kimchi/control/base.py\", line 129, in wrapper\n raise cherrypy.HTTPError(500, e.message)\nHTTPError: (500, u'KCHPOOL0009E: Unable to activate storage pool kimchi-logical. Details: internal error: Child process (/usr/sbin/vgchange -aly kimchi-logical) unexpected exit status 5: Volume group \"kimchi-logical\" not found\\n Cannot process volume group kimchi-logical\\n')\n" By looking at that error message, I guess Kimchi is assuming that "kimchi-logical" is the VG name. Currently, when Kimchi creates the VGs before creating the logical storage pools, we use the VG name the same as the pool name. With this patchset, that may not be the case anymore; I have a VG named "kimchivg" and a storage pool named "kimchi-logical", and I believe that's the root of this error here. Please correct me if I misunderstood something about this new patchset. As this is still a new feature, I may have assumed something wrong. On 12-04-2015 11:13, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Royce Lv (6): Storagepool on existent VG: Update document Add helper function to retrieve vg information Update controller Update model for existent VG Update mockmodel tests Fix fh leak in run_command subprocess
docs/API.md | 20 ++++++++++++++++ src/kimchi/control/host.py | 6 +++++ src/kimchi/i18n.py | 2 ++ src/kimchi/mockmodel.py | 9 ++++++++ src/kimchi/model/libvirtstoragepool.py | 11 +++++---- src/kimchi/model/storagepools.py | 28 +++++++++++++---------- src/kimchi/model/vgs.py | 42 ++++++++++++++++++++++++++++++++++ src/kimchi/utils.py | 22 ++++++++++++++++-- tests/test_mock_storagepool.py | 9 +++++++- 9 files changed, 130 insertions(+), 19 deletions(-) create mode 100644 src/kimchi/model/vgs.py
participants (2)
-
Crístian Deives -
lvroyce＠linux.vnet.ibm.com