On 09/21/2016 07:29 AM, Archana Singh wrote:



On 09/20/2016 08:33 PM, Aline Manera wrote:


On 09/20/2016 03:41 AM, Archana Singh wrote:



On 09/19/2016 10:19 PM, Aline Manera wrote:


On 09/19/2016 12:33 PM, Harshal Patil wrote:
 
Harshal Patil
 
 
----- Original message -----
From: Aline Manera <alinefm@linux.vnet.ibm.com>
Sent by: kimchi-devel-bounces@ovirt.org
To: harshalp@linux.vnet.ibm.com, Kimchi Devel <kimchi-devel@ovirt.org>
Cc:
Subject: Re: [Kimchi-devel] [PATCH] Issue #999: Attach storage to guest on s390x without libvirt
Date: Sat, Sep 17, 2016 1:03 AM
 

On 09/12/2016 06:55 AM, harshalp@linux.vnet.ibm.com wrote:
> From: Harshal Patil <harshalp@linux.vnet.ibm.com>
>
> This patch enables direct storage attachment to guests running
> on s390x arch without going through libvirt
>
> Signed-off-by: Harshal Patil <harshalp@linux.vnet.ibm.com>
> ---
>   docs/API.md         |  3 +++
>   i18n.py             |  3 +++
>   model/vmstorages.py | 25 +++++++++++++++++++++++--
>   utils.py            |  2 +-
>   4 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/docs/API.md b/docs/API.md
> index 6678ac5..c9d5d2a 100644
> --- a/docs/API.md
> +++ b/docs/API.md
> @@ -239,6 +239,9 @@ Represents a snapshot of the Virtual Machine's primary monitor.
>       * path: Path of cdrom iso.
>       * pool: Storage pool which disk image file locate in.
>       * vol: Storage volume name of disk image.
> +    * dir_path: s390x specific attribute to attach direct storage without libvirt
> +    * format: s390x specific attribute specify the format of direct storage
> +    * size: s390x specific attribute to specify the size of direct storage
>
>   ### Sub-resource: storage
>   **URI:** /plugins/kimchi/vms/*:name*/storages/*:dev*
> diff --git a/i18n.py b/i18n.py
> index fc7dbc7..ceb4286 100644
> --- a/i18n.py
> +++ b/i18n.py
> @@ -332,6 +332,9 @@ messages = {
>       "KCHVMSTOR0016E": _("Volume already in use by other virtual machine."),
>       "KCHVMSTOR0017E": _("Only one of path or pool/volume can be specified to add a new virtual machine disk"),
>       "KCHVMSTOR0018E": _("Volume chosen with format %(format)s does not fit in the storage type %(type)s"),
> +    "KCHVMSTOR0019E": _("On s390x arch one of pool, path of dir_path must be specified"),
> +    "KCHVMSTOR0020E": _("On s390x arch 'format' must be specified while attaching disk to virtual machine"),
> +    "KCHVMSTOR0021E": _("Virtual disk already exists on the system: %(disk_path)s"),
>
>       "KCHSNAP0001E": _("Virtual machine '%(vm)s' must be stopped before creating a snapshot of it."),
>       "KCHSNAP0002E": _("Unable to create snapshot '%(name)s' on virtual machine '%(vm)s'. Details: %(err)s"),
> diff --git a/model/vmstorages.py b/model/vmstorages.py
> index 2e9f783..156a16f 100644
> --- a/model/vmstorages.py
> +++ b/model/vmstorages.py
> @@ -17,6 +17,7 @@
>   # License along with this library; if not, write to the Free Software
>   # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
>
> +import os
>   import string
>   from lxml import etree
>
> @@ -30,6 +31,7 @@ from wok.plugins.kimchi.model.storagevolumes import StorageVolumeModel
>   from wok.plugins.kimchi.model.utils import get_vm_config_flag
>   from wok.plugins.kimchi.model.vms import DOM_STATE_MAP, VMModel
>   from wok.plugins.kimchi.osinfo import lookup
> +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
>
> @@ -82,11 +84,30 @@ class VMStoragesModel(object):
>           # Path will never be blank due to API.json verification.
>           # There is no need to cover this case here.
>           if not ('vol' in params) ^ ('path' in params):
> -            raise InvalidParameter("KCHVMSTOR0017E")
> +            if not is_s390x():
> +                raise InvalidParameter("KCHVMSTOR0017E")
> +

> +            if 'dir_path' not in params:
> +                raise InvalidParameter("KCHVMSTOR0019E")

>The dir_path is exclusively for s390x. You need to add this verification
>too.
Not required. Check the condition again (The line which says 'if not is_s390x()'). Only if the platform is s390x it will enter the code path where 'dir_path' is checked so there is no need to check for s390x arch again. I have tested this flow on x86 as well as on s390x, and it works fine.


Sorry but how the first condition is related to the second one?!

If I am x86 and I have specified a 'vol' or a 'path' and not a 'dir_path' as it is exclusively for s390x, it will raise the dir_path exception (the second condition).

So or you add a is_s390x() condition to the second statement or you place the whole first block in a if conduction:

if not is390x():
     if not (vol in params) ^ (path in params):
        (...)
else:
      if dir_path not in params:
          raise ...

Below is the flow:

1) On x86, if path or volume is not present in params then raise Invalid parameter(KCHVMSTOR0017E). (Same as it is in upstream code)
2) On x86 or s390x, if any one of them is present don't raise invalid parameter(KCHVMSTOR0017E).. (Same as it is in upstream code)
3) On s390x, if path or volume is not present instead of raising invalid parameter, it should go and check for dir_path and if dir_path is also not present then only raise Invalid parameter(KCHVMSTOR0019E).

So on x86, if I have specified a vol or a path then it will not go inside first if itself(not ('vol' in params) ^ ('path' in params) will be false).
and on x86, if I have neither vol nor path then it will go inside first if and also second one and raise invalid parameter.

And on x86, due to first nested 'if' (that is not is_s390x()) it will never go to second nested 'if' ('dir_path' not in params).

In Summary, on x86 if I have neither vol nor path then it should raise exception KCHVMSTOR0017E (same as currently upstream code behaves) and does not proceed further.
and on s390x we should check for vol and path, and if non of it is present then instead of raising exception KCHVMSTOR0017E go ahead and check for addition key dir_path. If dir_path is also not present then only raise exception KCHVMSTOR0019E.

Hope I make sense to you.

Yeap. That makes sense.

But what about on x86 where I have path or vol and nor dir_path? How do you prevent me to go to the second if and raise an exception?

The second if is not nested as you said above.

On x86,  check 'not is_s390x()' will always true and exception will be raised and hence it will never go further to check "'dir_path' not in params'".


That is not true.

The exception on 'not is_s390x' will ONLY be raised if there is NO vol or path parameters. Otherwise, the flow will continue to the 'dir_path' validation.
 


>           dom = VMModel.get_vm(vm_name, self.conn)
>           params['bus'] = _get_device_bus(params['type'], dom)
> -        params['format'] = 'raw'
> +
> +        if is_s390x() and params['type'] == 'disk':
> +            if 'format' not in params:
> +                raise InvalidParameter("KCHVMSTOR0020E")
> +            if 'dir_path' in params:
> +                size = params['size']
> +                name = params['name']
> +                dir_path = params.get('dir_path')
> +                params['path'] = dir_path + "/" + name
> +                if os.path.exists(params['path']):
> +                    raise InvalidParameter("KCHVMSTOR0021E",
> +                                           {'disk_path': params['path']})
> +                create_disk_image(format_type=params['format'],
> +                                  path=params['path'], capacity=size)
> +        else:
> +            params['format'] = 'raw'
>
>           dev_list = [dev for dev, bus in get_vm_disks(dom).iteritems()
>                       if bus == params['bus']]
> diff --git a/utils.py b/utils.py
> index 0fca191..7fdec02 100644
> --- a/utils.py
> +++ b/utils.py
> @@ -305,4 +305,4 @@ def create_disk_image(format_type, path, capacity):
>       if rc != 0:
>           raise OperationFailed("KCHTMPL0035E", {'err': err})
>
> -    return
> \ No newline at end of file
> +    return

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




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