[PATCH] [Kimchi 0/2] Allow disks to update cache and io flags
Ramon Medeiros (2): Allow disks to update cache and io flags Add tests to verify if cache and io of a disk can be changed API.json | 11 ++++++++++- docs/API.md | 3 +++ i18n.py | 1 - model/vmstorages.py | 57 +++++++++++++++++++++++++++++++++-------------------- tests/test_model.py | 7 +++++++ xmlutils/disk.py | 12 ++++++++++- 6 files changed, 67 insertions(+), 24 deletions(-) -- 2.9.3
This is a part of Bug fix #1092 solution Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- API.json | 11 ++++++++++- docs/API.md | 3 +++ i18n.py | 1 - model/vmstorages.py | 57 +++++++++++++++++++++++++++++++++-------------------- xmlutils/disk.py | 12 ++++++++++- 5 files changed, 60 insertions(+), 24 deletions(-) diff --git a/API.json b/API.json index 5729e5d..abe980c 100644 --- a/API.json +++ b/API.json @@ -761,8 +761,17 @@ "description": "Path of iso image file or disk mount point", "type": "string", "pattern": "^(|(/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", - "required": true, "error": "KCHVMSTOR0003E" + }, + "cache": { + "description": "Cache options", + "type": "string", + "pattern": "^(none|writethrough|writeback|directsync|unsafe|default)$" + }, + "io": { + "description": "I/O options", + "type": "string", + "pattern": "^(native|threads|default)$" } }, "additionalProperties": false diff --git a/docs/API.md b/docs/API.md index 3ecc7a0..0414aed 100644 --- a/docs/API.md +++ b/docs/API.md @@ -253,6 +253,9 @@ Represents a snapshot of the Virtual Machine's primary monitor. * bus: Bus type of disk attached. * **PUT**: Update storage information * path: Path of cdrom iso. Can not be blank. Now just support cdrom type. + * io: IO settings for disks. Values accepted: native, threads and default. + * threads: Threads settings for disks. Values accepted: none, writethrough, + writeback, directsync, unsafe and default. * **DELETE**: Remove the storage. **Actions (POST):** diff --git a/i18n.py b/i18n.py index 4460ce5..4bb3a4f 100644 --- a/i18n.py +++ b/i18n.py @@ -327,7 +327,6 @@ messages = { "KCHVMSTOR0002E": _("Invalid storage type. Types supported: 'cdrom', 'disk'"), "KCHVMSTOR0003E": _("The path '%(value)s' is not a valid local/remote path for the device"), - "KCHVMSTOR0006E": _("Only CDROM path can be update."), "KCHVMSTOR0007E": _("The storage device %(dev_name)s does not exist in the virtual machine %(vm_name)s"), "KCHVMSTOR0008E": _("Error while creating new storage device: %(error)s"), "KCHVMSTOR0009E": _("Error while updating storage device: %(error)s"), diff --git a/model/vmstorages.py b/model/vmstorages.py index db68121..143225c 100644 --- a/model/vmstorages.py +++ b/model/vmstorages.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM Corp, 2015-2016 +# Copyright IBM Corp, 2015-2017 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -35,7 +35,6 @@ from wok.plugins.kimchi.utils import create_disk_image, is_s390x from wok.plugins.kimchi.xmlutils.disk import get_device_node, get_disk_xml from wok.plugins.kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks - HOTPLUG_TYPE = ['scsi', 'virtio'] @@ -232,26 +231,42 @@ class VMStorageModel(object): dom = VMModel.get_vm(vm_name, self.conn) dev_info = self.lookup(vm_name, dev_name) - if dev_info['type'] != 'cdrom': - raise InvalidOperation("KCHVMSTOR0006E") - - params['path'] = params.get('path', '') - old_disk_path = dev_info['path'] - new_disk_path = params['path'] - if new_disk_path != old_disk_path: - # An empty path means a CD-ROM was empty or ejected: - if old_disk_path is not '': - old_disk_used_by = get_disk_used_by(self.conn, old_disk_path) - if new_disk_path is not '': - new_disk_used_by = get_disk_used_by(self.conn, new_disk_path) - - dev_info.update(params) - dev, xml = get_disk_xml(dev_info) - try: - dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) - except Exception as e: - raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) + # only change path to cdrom devices + if dev_info['type'] == 'cdrom': + + dev_info.update(params) + dev, xml = get_disk_xml(dev_info) + + params['path'] = params.get('path', '') + old_disk_path = dev_info['path'] + new_disk_path = params['path'] + if new_disk_path != old_disk_path: + # An empty path means a CD-ROM was empty or ejected: + if old_disk_path is not '': + old_disk_used_by = get_disk_used_by(self.conn, + old_disk_path) + if new_disk_path is not '': + new_disk_used_by = get_disk_used_by(self.conn, + new_disk_path) + + try: + dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) + except Exception as e: + raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) + else: + old_dev, old_xml = get_disk_xml(dev_info) + dev_info.update(params) + dev, xml = get_disk_xml(dev_info) + + # remove + self.delete(vm_name, dev_name) + + try: + dom.attachDeviceFlags(xml) + except Exception as e: + dom.attachDeviceFlags(old_xml) + raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) try: if old_disk_used_by is not None and \ diff --git a/xmlutils/disk.py b/xmlutils/disk.py index 8edb991..9a9987a 100644 --- a/xmlutils/disk.py +++ b/xmlutils/disk.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM Corp, 2015-2016 +# Copyright IBM Corp, 2015-2017 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -50,6 +50,8 @@ def get_disk_xml(params): if disk_type is None: disk_type = _get_disk_type(path) if len(path) > 0 else 'file' disk = E.disk(type=disk_type, device=params['type']) + + # <driver /> driver = E.driver(name='qemu', type=params['format']) if params['type'] != 'cdrom': driver.set('cache', 'none') @@ -57,6 +59,14 @@ def get_disk_xml(params): if params.get('pool_type') == "netfs": driver.set("io", "native") + # set io + if params.get("io") is not None: + driver.set("io", params.get("io")) + + # set cache + if params.get("cache") is not None: + driver.set("cache", params.get("cache")) + disk.append(driver) # Get device name according to bus and index values -- 2.9.3
On 02/28/2017 04:06 PM, Ramon Medeiros wrote:
This is a part of Bug fix #1092 solution
Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- API.json | 11 ++++++++++- docs/API.md | 3 +++ i18n.py | 1 - model/vmstorages.py | 57 +++++++++++++++++++++++++++++++++-------------------- xmlutils/disk.py | 12 ++++++++++- 5 files changed, 60 insertions(+), 24 deletions(-)
diff --git a/API.json b/API.json index 5729e5d..abe980c 100644 --- a/API.json +++ b/API.json @@ -761,8 +761,17 @@ "description": "Path of iso image file or disk mount point", "type": "string", "pattern": "^(|(/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", - "required": true, "error": "KCHVMSTOR0003E" + }, + "cache": { + "description": "Cache options", + "type": "string", + "pattern": "^(none|writethrough|writeback|directsync|unsafe|default)$" + }, + "io": { + "description": "I/O options", + "type": "string", + "pattern": "^(native|threads|default)$" } }, "additionalProperties": false diff --git a/docs/API.md b/docs/API.md index 3ecc7a0..0414aed 100644 --- a/docs/API.md +++ b/docs/API.md @@ -253,6 +253,9 @@ Represents a snapshot of the Virtual Machine's primary monitor. * bus: Bus type of disk attached. * **PUT**: Update storage information * path: Path of cdrom iso. Can not be blank. Now just support cdrom type.
+ * io: IO settings for disks. Values accepted: native, threads and default. + * threads: Threads settings for disks. Values accepted: none, writethrough, + writeback, directsync, unsafe and default.
Shouldn't it be 'cache' instead of 'threads'? At least, the API.md should reflect the settings on API.json file.
* **DELETE**: Remove the storage.
**Actions (POST):** diff --git a/i18n.py b/i18n.py index 4460ce5..4bb3a4f 100644 --- a/i18n.py +++ b/i18n.py @@ -327,7 +327,6 @@ messages = {
"KCHVMSTOR0002E": _("Invalid storage type. Types supported: 'cdrom', 'disk'"), "KCHVMSTOR0003E": _("The path '%(value)s' is not a valid local/remote path for the device"), - "KCHVMSTOR0006E": _("Only CDROM path can be update."), "KCHVMSTOR0007E": _("The storage device %(dev_name)s does not exist in the virtual machine %(vm_name)s"), "KCHVMSTOR0008E": _("Error while creating new storage device: %(error)s"), "KCHVMSTOR0009E": _("Error while updating storage device: %(error)s"), diff --git a/model/vmstorages.py b/model/vmstorages.py index db68121..143225c 100644 --- a/model/vmstorages.py +++ b/model/vmstorages.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM Corp, 2015-2016 +# Copyright IBM Corp, 2015-2017 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -35,7 +35,6 @@ from wok.plugins.kimchi.utils import create_disk_image, is_s390x from wok.plugins.kimchi.xmlutils.disk import get_device_node, get_disk_xml from wok.plugins.kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks
- HOTPLUG_TYPE = ['scsi', 'virtio']
@@ -232,26 +231,42 @@ class VMStorageModel(object): dom = VMModel.get_vm(vm_name, self.conn)
dev_info = self.lookup(vm_name, dev_name) - if dev_info['type'] != 'cdrom': - raise InvalidOperation("KCHVMSTOR0006E") - - params['path'] = params.get('path', '') - old_disk_path = dev_info['path'] - new_disk_path = params['path'] - if new_disk_path != old_disk_path: - # An empty path means a CD-ROM was empty or ejected: - if old_disk_path is not '': - old_disk_used_by = get_disk_used_by(self.conn, old_disk_path) - if new_disk_path is not '': - new_disk_used_by = get_disk_used_by(self.conn, new_disk_path) - - dev_info.update(params) - dev, xml = get_disk_xml(dev_info)
- try: - dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) - except Exception as e: - raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) + # only change path to cdrom devices + if dev_info['type'] == 'cdrom': +
+ dev_info.update(params) + dev, xml = get_disk_xml(dev_info) + + params['path'] = params.get('path', '') + old_disk_path = dev_info['path'] + new_disk_path = params['path'] + if new_disk_path != old_disk_path: + # An empty path means a CD-ROM was empty or ejected: + if old_disk_path is not '': + old_disk_used_by = get_disk_used_by(self.conn, + old_disk_path) + if new_disk_path is not '': + new_disk_used_by = get_disk_used_by(self.conn, + new_disk_path) + + try: + dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) + except Exception as e: + raise OperationFailed("KCHVMSTOR0009E", {'error': e.message})
The above block is exact the same code of the 'else' statement we have today in the code. So I'd say to keep the logic as it is today (if type != cdrom) and just add an 'else' to the code above. It will simplify the changes and make easier to see the changes added by this patch.
+ else: + old_dev, old_xml = get_disk_xml(dev_info) + dev_info.update(params) + dev, xml = get_disk_xml(dev_info) + + # remove + self.delete(vm_name, dev_name) + + try: + dom.attachDeviceFlags(xml) + except Exception as e: + dom.attachDeviceFlags(old_xml) + raise OperationFailed("KCHVMSTOR0009E", {'error': e.message})
try: if old_disk_used_by is not None and \ diff --git a/xmlutils/disk.py b/xmlutils/disk.py index 8edb991..9a9987a 100644 --- a/xmlutils/disk.py +++ b/xmlutils/disk.py @@ -1,7 +1,7 @@ # # Project Kimchi # -# Copyright IBM Corp, 2015-2016 +# Copyright IBM Corp, 2015-2017 # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -50,6 +50,8 @@ def get_disk_xml(params): if disk_type is None: disk_type = _get_disk_type(path) if len(path) > 0 else 'file' disk = E.disk(type=disk_type, device=params['type']) + + # <driver /> driver = E.driver(name='qemu', type=params['format']) if params['type'] != 'cdrom': driver.set('cache', 'none') @@ -57,6 +59,14 @@ def get_disk_xml(params): if params.get('pool_type') == "netfs": driver.set("io", "native")
+ # set io + if params.get("io") is not None: + driver.set("io", params.get("io")) + + # set cache + if params.get("cache") is not None: + driver.set("cache", params.get("cache")) + disk.append(driver)
# Get device name according to bus and index values
Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- tests/test_model.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test_model.py b/tests/test_model.py index 95c9e08..e49e556 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -198,6 +198,13 @@ class ModelTests(unittest.TestCase): task = inst.task_lookup(task['id']) self.assertEquals('finished', task['status']) + # add io and cache to disk + inst.vmstorage_update("kimchi-vm", "vda", {"io": "threads"}) + inst.vmstorage_update("kimchi-vm", "vda", {"cache": "unsafe"}) + vda = inst.vmstorage_lookup("kimchi-vm", "vda") + self.assertEquals("unsafe", vda["cache"]) + self.assertEquals("threads", vda["io"]) + vms = inst.vms_get_list() self.assertTrue('kimchi-vm' in vms) -- 2.9.3
Hi Ramon, Please, add tests using the REST API as well. Just to confirm it is working as expected On 02/28/2017 04:06 PM, Ramon Medeiros wrote:
Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- tests/test_model.py | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/tests/test_model.py b/tests/test_model.py index 95c9e08..e49e556 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -198,6 +198,13 @@ class ModelTests(unittest.TestCase): task = inst.task_lookup(task['id']) self.assertEquals('finished', task['status'])
+ # add io and cache to disk + inst.vmstorage_update("kimchi-vm", "vda", {"io": "threads"}) + inst.vmstorage_update("kimchi-vm", "vda", {"cache": "unsafe"}) + vda = inst.vmstorage_lookup("kimchi-vm", "vda") + self.assertEquals("unsafe", vda["cache"]) + self.assertEquals("threads", vda["io"]) + vms = inst.vms_get_list() self.assertTrue('kimchi-vm' in vms)
participants (2)
-
Aline Manera -
Ramon Medeiros