[Kimchi-devel] [PATCH] [Kimchi 1/2] Allow disks to update cache and io flags

Ramon Medeiros posted 2 patches 7 years, 2 months ago
[Kimchi-devel] [PATCH] [Kimchi 1/2] Allow disks to update cache and io flags
Posted by Ramon Medeiros 7 years, 2 months ago
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

_______________________________________________
Kimchi-devel mailing list
Kimchi-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/kimchi-devel
Re: [Kimchi-devel] [PATCH] [Kimchi 1/2] Allow disks to update cache and io flags
Posted by Aline Manera 7 years, 2 months ago

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

_______________________________________________
Kimchi-devel mailing list
Kimchi-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/kimchi-devel