[Kimchi-devel] [PATCH] Issue #999: Attach storage to guest on s390x without libvirt
Aline Manera
alinefm at linux.vnet.ibm.com
Wed Sep 21 13:16:43 UTC 2016
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
>>>>> E-mail: harshal.patil at in.ibm.com <mailto:harshal.patil at in.ibm.com>
>>>>>
>>>>> ----- Original message -----
>>>>> From: Aline Manera <alinefm at linux.vnet.ibm.com>
>>>>> Sent by: kimchi-devel-bounces at ovirt.org
>>>>> To: harshalp at linux.vnet.ibm.com, Kimchi Devel
>>>>> <kimchi-devel at 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 at linux.vnet.ibm.com wrote:
>>>>> > From: Harshal Patil <harshalp at 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 at 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 at ovirt.org
>>>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Kimchi-devel mailing list
>>>> Kimchi-devel at ovirt.org
>>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/kimchi-devel/attachments/20160921/8083e58d/attachment.html>
More information about the Kimchi-devel
mailing list