[Kimchi-devel] [PATCH] Issue #999: Attach storage to guest on s390x without libvirt
Archana Singh
archus at linux.vnet.ibm.com
Thu Sep 22 11:26:26 UTC 2016
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:-)
>>>
>>>>
>>>>>>
>>>>>>
>>>>>> > 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/6881b087/attachment.html>
More information about the Kimchi-devel
mailing list