Re: [Kimchi-devel] [PATCH] Issue #999: Attach storage to guest on s390x without libvirt

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(a)in.ibm.com <mailto:harshal.patil@in.ibm.com> >>>> >>>> ----- Original message ----- >>>> From: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; >>>> Sent by: kimchi-devel-bounces(a)ovirt.org >>>> To: harshalp(a)linux.vnet.ibm.com, Kimchi Devel >>>> <kimchi-devel(a)ovirt.org&gt; >>>> 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(a)linux.vnet.ibm.com wrote: >>>> > From: Harshal Patil <harshalp(a)linux.vnet.ibm.com&gt; >>>> > >>>> > This patch enables direct storage attachment to guests running >>>> > on s390x arch without going through libvirt >>>> > >>>> > Signed-off-by: Harshal Patil <harshalp(a)linux.vnet.ibm.com&gt; >>>> > --- >>>> > 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(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>>> >>>> >>> >>> >>> >>> _______________________________________________ >>> Kimchi-devel mailing list >>> Kimchi-devel(a)ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >> >