[Kimchi-devel] [PATCH] Issue #999: Attach storage to guest on s390x without libvirt
Archana Singh
archus at linux.vnet.ibm.com
Wed Sep 21 10:29:32 UTC 2016
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'".
>
>>
>>>>
>>>>
>>>> > 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/a684ebdc/attachment.html>
More information about the Kimchi-devel
mailing list