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

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.

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