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

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.

>> >> >> > 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