On 09/19/2016 12:33 PM, Harshal Patil wrote:
Harshal Patil E-mail: harshal.patil@in.ibm.com <mailto:harshal.patil@in.ibm.com>
----- Original message ----- From: Aline Manera <alinefm@linux.vnet.ibm.com> Sent by: kimchi-devel-bounces@ovirt.org To: harshalp@linux.vnet.ibm.com, Kimchi Devel <kimchi-devel@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@linux.vnet.ibm.com wrote: > From: Harshal Patil <harshalp@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@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 ...
> 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@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel