[Kimchi-devel] [PATCH] Issue #999: Attach storage to guest on s390x without libvirt
Aline Manera
alinefm at linux.vnet.ibm.com
Thu Sep 22 18:02:52 UTC 2016
On 09/22/2016 08:26 AM, Archana Singh wrote:
>
>
>
> On 09/21/2016 06:46 PM, Aline Manera wrote:
>>
>>
>> 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.
> Aline, on x86, ppc case it will raise exception ONLY when there is NO
> vol or path parameters.
> Now think about if one of them either path or vol exists, on x86, ppc
> or even on s390x case, second if (if 'dir_path' not in params:) will
> not be reached since first if ( if not ('vol' in params) ^ ('path' in
> params):) will become *false *
> Please look into the tab indentation of the second if and think about
> the logic:-)
\o/ I got it now. Thanks
>
>>>>
>>>>>
>>>>>>>
>>>>>>>
>>>>>>> > 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/20160922/36b0f4df/attachment.html>
More information about the Kimchi-devel
mailing list