On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

This patch creates basic functions that allow kimchi users to create an libvirt SCSI storagepool using the rest API.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- docs/API.md | 6 ++++++ src/kimchi/API.json | 19 +++++++++++++++++- src/kimchi/model.py | 57 ++++++++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/docs/API.md b/docs/API.md index a86b884..d00cdd5 100644 --- a/docs/API.md +++ b/docs/API.md @@ -242,6 +242,12 @@ Represents a snapshot of the Virtual Machine's primary monitor. Pool types: 'iscsi'. * username: Login username of the iSCSI target. * password: Login password of the iSCSI target.

+ * adapter_type: Host adapter type: 'scsi_host' or 'fc_host'. + * adapter_name: Scsi host name. + Required if 'scsi_host' type is selected. + * wwnn: Word Wide Node Name. + * wwpn: Word Wide Port Name. +

From my view, only adapter_name is required to create a scsi pool. So all others should be marked as optional The user just need to provide the scsi host and kimchi will get all those info based on that

### Resource: Storage Pool

diff --git a/src/kimchi/API.json b/src/kimchi/API.json index e942824..b7eb27b 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -37,7 +37,8 @@ "type": { "description": "The type of the defined Storage Pool", "type": "string",

- "pattern": "^dir|netfs|logical|kimchi-iso$", + "pattern": "^dir|netfs|logical|kimchi-iso|scsi$", + "pattern": "^dir|netfs|logical|scsi$",

Why are there 2 pattern entries there?

"required": true }, "path": { @@ -76,6 +77,22 @@ "minimum": 1, "maximum": 65535 }, + "adapter_type": { + "description": "Host adapter type: 'scsi_host' or 'fc_host'", + "type": "string" + },

If there are only 2 possible values you should use 'pattern' to only allow them (instead of string)

+ "adapter_name": { + "description": "SCSI host name", + "type": "string" + }, + "wwnn": { + "description": "Fibre channel Word Wide Node Name", + "type": "string" + }, + "wwpn": { + "description": "Fibre channel Word Wide Port Name", + "type": "string" + }, "auth": { "description": "Storage back-end authentication information", "type": "object", diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 51a4427..0eb8cc4 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -75,8 +75,9 @@ from kimchi.objectstore import ObjectStore from kimchi.rollbackcontext import RollbackContext from kimchi.scan import Scanner from kimchi.screenshot import VMScreenshot -from kimchi.utils import get_enabled_plugins, is_digit, kimchi_log -from kimchi.utils import run_command, parse_cmd_output, patch_find_nfs_target +from kimchi.utils import get_enabled_plugins, is_digit +from kimchi.utils import is_libvirt_version_lesser, kimchi_log, parse_cmd_output +from kimchi.utils import patch_find_nfs_target, run_command from kimchi.vmtemplate import VMTemplate

@@ -86,7 +87,11 @@ HOST_STATS_INTERVAL = 1 VM_STATIC_UPDATE_PARAMS = {'name': './name'} VM_LIVE_UPDATE_PARAMS = {} STORAGE_SOURCES = {'netfs': {'addr': '/pool/source/host/@name', - 'path': '/pool/source/dir/@path'}} + 'path': '/pool/source/dir/@path'}, + 'scsi': {'adapter_type': '/pool/source/adapter/@type', + 'adapter_name': '/pool/source/adapter/@name', + 'wwnn': '/pool/source/adapter/@wwnn', + 'wwpn': '/pool/source/adapter/@wwpn'}}

def _uri_to_name(collection, uri): @@ -1095,9 +1100,10 @@ class Model(object): return name

pool = conn.storagePoolDefineXML(xml, 0) - if params['type'] in ['logical', 'dir', 'netfs']: + if params['type'] in ['logical', 'dir', 'netfs', 'scsi']: pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW) - # autostart dir and logical storage pool created from kimchi + # autostart dir, logical, netfs and scsi storage pools created + # from kimchi pool.setAutostart(1) else: # disable autostart for others @@ -1735,6 +1741,47 @@ class LogicalPoolDef(StoragePoolDef): return xml

+class ScsiPoolDef(StoragePoolDef): + poolType = 'scsi' + + def prepare(self, conn=None):

+ # Adapters type are only available in libvirt >= 1.0.5 + if is_libvirt_version_lesser('1.0.5'):

As I commented in previous patch we should avoid comparing versions to decide between implementations Please, add a feature test for that.

+ self.poolArgs['source']['adapter_type'] = 'scsi_host'

You are always overriding the 'adapter_type'. You should set values only if user doesn't pass it

+ tmp_name = self.poolArgs['source']['adapter_name'] + self.poolArgs['source']['adapter_name'] = tmp_name.replace('scsi_','') + msg = "Libvirt version <= 1.0.5. Setting SCSI host name as '%s'; "\ + "setting SCSI adapter type as 'scsi_host'; "\ + "ignoring wwnn and wwpn." %tmp_name + kimchi_log.info(msg) +

And what about the code for the 'else' statement?

+ @property + def xml(self): + # Required parameters + # name: + # source[adapter_type]: + # source[adapter_name]: + # source[wwnn]: + # source[wwpn]: + # path: +

+ xml = """ + <pool type='scsi'> + <name>{name}</name> + <source> + <adapter type='{source[adapter_type]}'\ + name='{source[adapter_name]}'\ + wwnn='{source[wwnn]}'\ + wwpn='{source[wwpn]}'/> + </source> + <target> + <path>{path}</path> + </target> + </pool> + """.format(**self.poolArgs)

Does this xml work for all libvirt versions in supported distros? I thought the xml will be different according to libvirt support.

+ return xml + + class IscsiPoolDef(StoragePoolDef): poolType = 'iscsi'

On 01/25/2014 08:27 PM, Aline Manera wrote:

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

...

As Kimchi supports a large range of Linux distributions and libvirt versions may differ. Libvirt functionalities may not be available, what requires different implementations of kimchi code. This patch implements a function to help identify libvirt version.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/utils.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index c7ececf..0475eda 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -28,11 +28,12 @@ import urllib2

from cherrypy.lib.reprconf import Parser -from kimchi.exception import TimeoutExpired +from libvirt import getVersion +from threading import Timer

from kimchi import config -from threading import Timer +from kimchi.exception import TimeoutExpired

kimchi_log = cherrypy.log.error_log @@ -178,3 +179,16 @@ def patch_find_nfs_target(nfs_server): target['type'] = 'nfs' target['host_name'] = nfs_server return targets + +

...

+def is_libvirt_version_lesser(version): + """ + Receives an string as version (ex: '0.7.1' or '1.1.2') and compares with + system libvirt version. + Returns booleanr: True if libvirt version lesser than given version + False if libvirt version is greater or equal + """ + # Versions numbers are integers: 1000000*major + 1000*minor + release + ver = version.split('.') + test_version = 1000000*int(ver[0]) + 1000*int(ver[1]) + int(ver[2]) + return (cmp(getVersion(), test_version) < 0)

Please, don't compare versions Create a feature test to check libvirt has or not support for what you want.

From libvirt documentation, I got the following xml: <pool type='scsi'> <name>poolvhba0</name> <source> <adapter type='fc_host' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> </source> <target> <path>/dev/disk/by-path</path> </target> </pool> Use is in your feature test. If you can create a pool using this xml, you know libvirt has support for 'fc_host' adapter Otherwise, you need to use the older libvirt xml to create the pool From my tests: # on libvirt 0.9.13 $ sudo virsh pool-define ../scsi-pool.xml error: Failed to define pool from ../scsi-pool.xml error: XML error: missing storage pool source adapter name # on libvirt 1.1.2 virsh # pool-undefine poolvhba0 Pool poolvhba0 has been undefined

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

On 01/25/2014 08:27 PM, Aline Manera wrote:

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

...

As Kimchi supports a large range of Linux distributions and libvirt versions may differ. Libvirt functionalities may not be available, what requires different implementations of kimchi code. This patch implements a function to help identify libvirt version.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/utils.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index c7ececf..0475eda 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -28,11 +28,12 @@ import urllib2

from cherrypy.lib.reprconf import Parser -from kimchi.exception import TimeoutExpired +from libvirt import getVersion +from threading import Timer

from kimchi import config -from threading import Timer +from kimchi.exception import TimeoutExpired

kimchi_log = cherrypy.log.error_log @@ -178,3 +179,16 @@ def patch_find_nfs_target(nfs_server): target['type'] = 'nfs' target['host_name'] = nfs_server return targets + +

...

+def is_libvirt_version_lesser(version): + """ + Receives an string as version (ex: '0.7.1' or '1.1.2') and compares with + system libvirt version. + Returns booleanr: True if libvirt version lesser than given version + False if libvirt version is greater or equal + """ + # Versions numbers are integers: 1000000*major + 1000*minor + release + ver = version.split('.') + test_version = 1000000*int(ver[0]) + 1000*int(ver[1]) + int(ver[2]) + return (cmp(getVersion(), test_version) < 0)

Please, don't compare versions Create a feature test to check libvirt has or not support for what you want.

I don't agree, but its ok. ACK

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

This patch implements basic routines to add a disk (scsi) to a new vm template. At this moment, the LUN that will be assigned to guest template will be the first and with more space found.

Which LUN used to assigned to a VM should be provided by user. As you suggested in RFC patches, we will show the user the LUNs and he will select which one he want to assign to vm

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/model.py | 24 ++++++++++++++++++++++++ src/kimchi/vmtemplate.py | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 0eb8cc4..552f69d 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -1534,9 +1534,33 @@ class LibvirtVMTemplate(VMTemplate): xml = pool.XMLDesc(0) return xmlutils.xpath_get_text(xml, "/pool/target/path")[0]

+ def _get_storage_type(self): + pool = self._storage_validate() + xml = pool.XMLDesc(0) + return xmlutils.xpath_get_text(xml, "/pool/@type")[0] +

+ def _get_available_volumes(self): + pool = self._storage_validate() + vol_names = pool.listVolumes() + res = [] + for vol in vol_names: + # Considering a volume as available if there is not any format type + xml = pool.storageVolLookupByName(vol).XMLDesc(0) + if (xmlutils.xpath_get_text(xml, "/volume/target/format/@type")[0] + == 'unknown'): + res.append(( + xmlutils.xpath_get_text(xml, "/volume/target/path")[0], + int (xmlutils.xpath_get_text(xml, + "/volume/capacity")[0]) / 1024**3 ) + ) + res.sort(key=lambda x: x[1]) + return res +

Not sure we need this. The user may want to assign an existing volume to a VM. We should show all of them and he choice what he want to use.

def fork_vm_storage(self, vm_uuid): # Provision storage: # TODO: Rebase on the storage API once upstream

+ if self._get_storage_type() == 'scsi': + return []

You should not call fork_vm_storage() in that case. Move this condition to before it.

pool = self._storage_validate() vol_list = self.to_volume_list(vm_uuid) for v in vol_list: diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 58147e3..d6f807a 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -31,6 +31,7 @@ from kimchi import isoinfo from kimchi import osinfo from kimchi.exception import InvalidParameter, IsoFormatError from kimchi.isoinfo import IsoImage +from kimchi.utils import is_libvirt_version_lesser

QEMU_NAMESPACE = "xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'" @@ -180,6 +181,32 @@ class VMTemplate(object): graphics_xml = graphics_xml + spicevmc_xml return graphics_xml

+ def _get_scsi_disks_xml(self): + ret = "" + # Passthrough configuration + disk_xml = """ + <disk type='volume' device='lun'> + <driver name='qemu' type='raw'/> + <source dev='%(src)s'/> + <target dev='%(dev)s' bus='scsi'/> + </disk>""" + if is_libvirt_version_lesser('1.0.5'): + disk_xml = disk_xml.replace('volume','block') +

+ luns = self._get_available_volumes() + for i, disk in enumerate(self.info['disks']): + index = disk.get('index', i) + try: + # Get luns with more space firstly + src = luns.pop()[0] + except IndexError: + # Skip if there is not available luns + continue

That info is provided by user. It should be: def _get_scsi_vol_xml(self, vol)

+ dev = "sd%s" % string.lowercase[index] + params = {'src': src, 'dev': dev} + ret = ret + disk_xml % params + return ret + def to_volume_list(self, vm_uuid): storage_path = self._get_storage_path() ret = [] @@ -225,7 +252,6 @@ class VMTemplate(object): params = dict(self.info) params['name'] = vm_name params['uuid'] = vm_uuid - params['disks'] = self._get_disks_xml(vm_uuid) params['networks'] = self._get_networks_xml() params['qemu-namespace'] = '' params['cdroms'] = '' @@ -233,6 +259,12 @@ class VMTemplate(object): graphics = kwargs.get('graphics') params['graphics'] = self._get_graphics_xml(graphics)

+ # SCSI FC disk are different + if self._get_storage_type() == 'scsi': + params['disks'] = self._get_scsi_disks_xml() + else: + params['disks'] = self._get_disks_xml(vm_uuid) +

Then my vm only can have scsi volumes or file ones? never both? You can add a 'type' key to 'disks' So we can have: disks = [{index: 0, size: 10, volume: "/home/alinefm/my-disk.img", type: file}, {index:1, size: 0, volume: 'unit-0-0-1', type: lun}] Then _get_disks_xml() get the correct xml according to file

qemu_stream_dns = kwargs.get('qemu_stream_dns', False) libvirt_stream = kwargs.get('libvirt_stream', False) cdrom_xml = self._get_cdrom_xml(libvirt_stream, qemu_stream_dns) @@ -292,3 +324,9 @@ class VMTemplate(object):

def _get_storage_path(self): return '' + + def _get_storage_type(self): + return '' + + def _get_available_volumes(self): + return []

On 01/28/2014 07:19 PM, Rodrigo Trujillo wrote:

...

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

...

This patch implements basic routines to add a disk (scsi) to a new vm template. At this moment, the LUN that will be assigned to guest template will be the first and with more space found.

Which LUN used to assigned to a VM should be provided by user. As you suggested in RFC patches, we will show the user the LUNs and he will select which one he want to assign to vm The LUN can and cannot be passed through the JSON API when creating

On 01/25/2014 09:08 PM, Aline Manera wrote: the VM, if not passed, a LUN will be selected automatically

Nope! If user want to create a vm using a scsi pool he *need* to pass the LUN Otherwise, we are assuming much risk in selecting one for him

...

...

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/model.py | 24 ++++++++++++++++++++++++ src/kimchi/vmtemplate.py | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 0eb8cc4..552f69d 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -1534,9 +1534,33 @@ class LibvirtVMTemplate(VMTemplate): xml = pool.XMLDesc(0) return xmlutils.xpath_get_text(xml, "/pool/target/path")[0]

+ def _get_storage_type(self): + pool = self._storage_validate() + xml = pool.XMLDesc(0) + return xmlutils.xpath_get_text(xml, "/pool/@type")[0] +

...

+ def _get_available_volumes(self): + pool = self._storage_validate() + vol_names = pool.listVolumes() + res = [] + for vol in vol_names: + # Considering a volume as available if there is not any format type + xml = pool.storageVolLookupByName(vol).XMLDesc(0) + if (xmlutils.xpath_get_text(xml, "/volume/target/format/@type")[0] + == 'unknown'): + res.append(( + xmlutils.xpath_get_text(xml, "/volume/target/path")[0], + int (xmlutils.xpath_get_text(xml, + "/volume/capacity")[0]) / 1024**3 ) + ) + res.sort(key=lambda x: x[1]) + return res +

Not sure we need this. The user may want to assign an existing volume to a VM. We should show all of them and he choice what he want to use.

This only happens when user has UI. I left the option to not pass any lun, when consuming the API strictly, so higher LUN will be selected automatically

Nope! As I said above

...

...

def fork_vm_storage(self, vm_uuid): # Provision storage: # TODO: Rebase on the storage API once upstream

...

+ if self._get_storage_type() == 'scsi': + return []

You should not call fork_vm_storage() in that case. Move this condition to before it.

ACK

...

...

pool = self._storage_validate() vol_list = self.to_volume_list(vm_uuid) for v in vol_list: diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 58147e3..d6f807a 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -31,6 +31,7 @@ from kimchi import isoinfo from kimchi import osinfo from kimchi.exception import InvalidParameter, IsoFormatError from kimchi.isoinfo import IsoImage +from kimchi.utils import is_libvirt_version_lesser

QEMU_NAMESPACE = "xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'" @@ -180,6 +181,32 @@ class VMTemplate(object): graphics_xml = graphics_xml + spicevmc_xml return graphics_xml

+ def _get_scsi_disks_xml(self): + ret = "" + # Passthrough configuration + disk_xml = """ + <disk type='volume' device='lun'> + <driver name='qemu' type='raw'/> + <source dev='%(src)s'/> + <target dev='%(dev)s' bus='scsi'/> + </disk>""" + if is_libvirt_version_lesser('1.0.5'): + disk_xml = disk_xml.replace('volume','block') +

...

+ luns = self._get_available_volumes() + for i, disk in enumerate(self.info['disks']): + index = disk.get('index', i) + try: + # Get luns with more space firstly + src = luns.pop()[0] + except IndexError: + # Skip if there is not available luns + continue

That info is provided by user.

It should be:

def _get_scsi_vol_xml(self, vol)

The volume information provided by user is being modified in patch [6/7] The xml provides a DISK, just followed previous functions pattern.

...

...

+ dev = "sd%s" % string.lowercase[index] + params = {'src': src, 'dev': dev} + ret = ret + disk_xml % params + return ret + def to_volume_list(self, vm_uuid): storage_path = self._get_storage_path() ret = [] @@ -225,7 +252,6 @@ class VMTemplate(object): params = dict(self.info) params['name'] = vm_name params['uuid'] = vm_uuid - params['disks'] = self._get_disks_xml(vm_uuid) params['networks'] = self._get_networks_xml() params['qemu-namespace'] = '' params['cdroms'] = '' @@ -233,6 +259,12 @@ class VMTemplate(object): graphics = kwargs.get('graphics') params['graphics'] = self._get_graphics_xml(graphics)

...

+ # SCSI FC disk are different + if self._get_storage_type() == 'scsi': + params['disks'] = self._get_scsi_disks_xml() + else: + params['disks'] = self._get_disks_xml(vm_uuid) +

Then my vm only can have scsi volumes or file ones? never both?

At this moment, yes, as Kimchi only supports 1 disk.

But we need to change it to avoid rework in future

...

You can add a 'type' key to 'disks'

So we can have:

disks = [{index: 0, size: 10, volume: "/home/alinefm/my-disk.img", type: file}, {index:1, size: 0, volume: 'unit-0-0-1', type: lun}]

Then _get_disks_xml() get the correct xml according to file

I think this is another scope and another feature: "Support multiple disks". I would love code this, but not in the scope of this patch

Nope. Adjust it. As you can see the current code allow multiple disk from REST API. It is only blocked though UI.

...

...

qemu_stream_dns = kwargs.get('qemu_stream_dns', False) libvirt_stream = kwargs.get('libvirt_stream', False) cdrom_xml = self._get_cdrom_xml(libvirt_stream, qemu_stream_dns) @@ -292,3 +324,9 @@ class VMTemplate(object):

def _get_storage_path(self): return '' + + def _get_storage_type(self): + return '' + + def _get_available_volumes(self): + return []

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

On 01/30/2014 06:11 PM, Rodrigo Trujillo wrote:

On 01/30/2014 12:12 PM, Aline Manera wrote:

...

On 01/28/2014 07:19 PM, Rodrigo Trujillo wrote:

...

...

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

...

This patch implements basic routines to add a disk (scsi) to a new vm template. At this moment, the LUN that will be assigned to guest template will be the first and with more space found.

Which LUN used to assigned to a VM should be provided by user. As you suggested in RFC patches, we will show the user the LUNs and he will select which one he want to assign to vm The LUN can and cannot be passed through the JSON API when creating

On 01/25/2014 09:08 PM, Aline Manera wrote: the VM, if not passed, a LUN will be selected automatically

Nope! If user want to create a vm using a scsi pool he *need* to pass the LUN Otherwise, we are assuming much risk in selecting one for him

Ok, lets make LUN a requiments (if pool is SCSI).

...

...

...

...

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/model.py | 24 ++++++++++++++++++++++++ src/kimchi/vmtemplate.py | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 0eb8cc4..552f69d 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -1534,9 +1534,33 @@ class LibvirtVMTemplate(VMTemplate): xml = pool.XMLDesc(0) return xmlutils.xpath_get_text(xml, "/pool/target/path")[0]

+ def _get_storage_type(self): + pool = self._storage_validate() + xml = pool.XMLDesc(0) + return xmlutils.xpath_get_text(xml, "/pool/@type")[0] +

...

+ def _get_available_volumes(self): + pool = self._storage_validate() + vol_names = pool.listVolumes() + res = [] + for vol in vol_names: + # Considering a volume as available if there is not any format type + xml = pool.storageVolLookupByName(vol).XMLDesc(0) + if (xmlutils.xpath_get_text(xml, "/volume/target/format/@type")[0] + == 'unknown'): + res.append(( + xmlutils.xpath_get_text(xml, "/volume/target/path")[0], + int (xmlutils.xpath_get_text(xml, + "/volume/capacity")[0]) / 1024**3 ) + ) + res.sort(key=lambda x: x[1]) + return res +

Not sure we need this. The user may want to assign an existing volume to a VM. We should show all of them and he choice what he want to use.

This only happens when user has UI. I left the option to not pass any lun, when consuming the API strictly, so higher LUN will be selected automatically

Nope! As I said above

Ok, I am make LUN a required parameter (if scsi pool). But checking will be in vm creation (model) time. Need to figure a way to add this requirement in json schema checking

I don't think there is a any to make an optional parameter required in given scenario on jsonschema. On jsonschema, you can only add the pattern for the parameter. And on model.py you check it according to storage pool type.

...

...

...

...

def fork_vm_storage(self, vm_uuid): # Provision storage: # TODO: Rebase on the storage API once upstream

...

+ if self._get_storage_type() == 'scsi': + return []

You should not call fork_vm_storage() in that case. Move this condition to before it.

ACK

...

...

pool = self._storage_validate() vol_list = self.to_volume_list(vm_uuid) for v in vol_list: diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 58147e3..d6f807a 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -31,6 +31,7 @@ from kimchi import isoinfo from kimchi import osinfo from kimchi.exception import InvalidParameter, IsoFormatError from kimchi.isoinfo import IsoImage +from kimchi.utils import is_libvirt_version_lesser

QEMU_NAMESPACE = "xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'" @@ -180,6 +181,32 @@ class VMTemplate(object): graphics_xml = graphics_xml + spicevmc_xml return graphics_xml

+ def _get_scsi_disks_xml(self): + ret = "" + # Passthrough configuration + disk_xml = """ + <disk type='volume' device='lun'> + <driver name='qemu' type='raw'/> + <source dev='%(src)s'/> + <target dev='%(dev)s' bus='scsi'/> + </disk>""" + if is_libvirt_version_lesser('1.0.5'): + disk_xml = disk_xml.replace('volume','block') +

...

+ luns = self._get_available_volumes() + for i, disk in enumerate(self.info['disks']): + index = disk.get('index', i) + try: + # Get luns with more space firstly + src = luns.pop()[0] + except IndexError: + # Skip if there is not available luns + continue

That info is provided by user.

It should be:

def _get_scsi_vol_xml(self, vol)

The volume information provided by user is being modified in patch [6/7] The xml provides a DISK, just followed previous functions pattern.

...

...

+ dev = "sd%s" % string.lowercase[index] + params = {'src': src, 'dev': dev} + ret = ret + disk_xml % params + return ret + def to_volume_list(self, vm_uuid): storage_path = self._get_storage_path() ret = [] @@ -225,7 +252,6 @@ class VMTemplate(object): params = dict(self.info) params['name'] = vm_name params['uuid'] = vm_uuid - params['disks'] = self._get_disks_xml(vm_uuid) params['networks'] = self._get_networks_xml() params['qemu-namespace'] = '' params['cdroms'] = '' @@ -233,6 +259,12 @@ class VMTemplate(object): graphics = kwargs.get('graphics') params['graphics'] = self._get_graphics_xml(graphics)

...

+ # SCSI FC disk are different + if self._get_storage_type() == 'scsi': + params['disks'] = self._get_scsi_disks_xml() + else: + params['disks'] = self._get_disks_xml(vm_uuid) +

Then my vm only can have scsi volumes or file ones? never both?

At this moment, yes, as Kimchi only supports 1 disk.

But we need to change it to avoid rework in future

...

...

You can add a 'type' key to 'disks'

So we can have:

disks = [{index: 0, size: 10, volume: "/home/alinefm/my-disk.img", type: file}, {index:1, size: 0, volume: 'unit-0-0-1', type: lun}]

Then _get_disks_xml() get the correct xml according to file

I think this is another scope and another feature: "Support multiple disks". I would love code this, but not in the scope of this patch

Nope. Adjust it. As you can see the current code allow multiple disk from REST API. It is only blocked though UI.

Oh, maybe I expressed myself wrongly. We would need to support "Multiple disks, in multiple pools". Currently, a given Template is tied to one storage pool only. So all disks of the vm will be created inside that assigned pool. This structure does not allow us to mix different types of disks (like file and lun). We have to changes the template structure and other things, or even pass the storagepool name inside disk list. Thats why I said it another feature.

Got it. Thanks for the explanation. Keep doing it as you did. Then we can improve our template structure in future to allow multiples storage pools.

...

...

...

...

qemu_stream_dns = kwargs.get('qemu_stream_dns', False) libvirt_stream = kwargs.get('libvirt_stream', False) cdrom_xml = self._get_cdrom_xml(libvirt_stream, qemu_stream_dns) @@ -292,3 +324,9 @@ class VMTemplate(object):

def _get_storage_path(self): return '' + + def _get_storage_type(self): + return '' + + def _get_available_volumes(self): + return []

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

In order to implement support to SCSI/FC UI, it is necessary to retrieve node devices.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/control/devices.py | 40 ++++++++++++++++++++++++++++++++++++++++ src/kimchi/control/host.py | 3 +++ src/kimchi/model.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+) create mode 100644 src/kimchi/control/devices.py

diff --git a/src/kimchi/control/devices.py b/src/kimchi/control/devices.py new file mode 100644 index 0000000..d5ef8fe --- /dev/null +++ b/src/kimchi/control/devices.py @@ -0,0 +1,40 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014 +# +# Authors: +# Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + + +from kimchi.control.base import Collection, Resource + + +class Devices(Collection): + def __init__(self, model): + super(Devices, self).__init__(model) + self.resource = Device + + +class Device(Resource): + def __init__(self, model, id): + super(Device, self).__init__(model, id) + + @property + def data(self): + return self.info + diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py index 9b19577..82c8c13 100644 --- a/src/kimchi/control/host.py +++ b/src/kimchi/control/host.py @@ -24,6 +24,7 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

from kimchi.control.base import Collection, Resource +from kimchi.control.devices import Devices

class Host(Resource): @@ -36,6 +37,8 @@ class Host(Resource): self.stats.exposed = True self.partitions = Partitions(self.model) self.partitions.exposed = True + self.devices = Devices(self.model) + self.devices.exposed = True

If "Devices" is a sub-resource of host you should put the control/devices.py content in control/host.py The files in control/ are named according to uri.

@property def data(self): diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 552f69d..4a5f344 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -361,6 +361,34 @@ class Model(object): self.stats[vm_uuid].update({'disk_io': rate, 'max_disk_io': max_disk_io, 'diskRdKB': diskRdKB, 'diskWrKB': diskWrKB})

+ def device_lookup(self, nodedev_name): + return {'name': nodedev_name} + + def devices_get_list(self, _cap=None): + conn = self.conn.get() + if _cap == None: + dev_names = [name.name() for name in conn.listAllDevices(0)] + elif _cap == 'fc_host': + dev_names = self._get_devices_fc_host() + else: + # Get devices with required capability + dev_names = conn.listDevices(_cap,0) + return dev_names + + def _get_devices_fc_host(self): + conn = self.conn.get() + # Libvirt < 1.0.5 does not support fc_host capability + if is_libvirt_version_lesser('1.0.5'): + ret = [] + scsi_hosts = conn.listDevices('scsi_host',0) + for host in scsi_hosts: + xml = conn.nodeDeviceLookupByName(host).XMLDesc(0) + path = '/device/capability/capability/@type' + if 'fc_host' in xmlutils.xpath_get_text(xml, path): + ret.append(host) + return ret

What about the "else" code?

+ return conn.listDevices('fc_host',0) + def debugreport_lookup(self, name): path = config.get_debugreports_path() file_pattern = os.path.join(path, name)

On 01/25/2014 09:37 PM, Aline Manera wrote:

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

...

In order to implement support to SCSI/FC UI, it is necessary to retrieve node devices.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/control/devices.py | 40 ++++++++++++++++++++++++++++++++++++++++ src/kimchi/control/host.py | 3 +++ src/kimchi/model.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+) create mode 100644 src/kimchi/control/devices.py

diff --git a/src/kimchi/control/devices.py b/src/kimchi/control/devices.py new file mode 100644 index 0000000..d5ef8fe --- /dev/null +++ b/src/kimchi/control/devices.py @@ -0,0 +1,40 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014 +# +# Authors: +# Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + + +from kimchi.control.base import Collection, Resource + + +class Devices(Collection): + def __init__(self, model): + super(Devices, self).__init__(model) + self.resource = Device + + +class Device(Resource): + def __init__(self, model, id): + super(Device, self).__init__(model, id) + + @property + def data(self): + return self.info + diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py index 9b19577..82c8c13 100644 --- a/src/kimchi/control/host.py +++ b/src/kimchi/control/host.py @@ -24,6 +24,7 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

from kimchi.control.base import Collection, Resource +from kimchi.control.devices import Devices

class Host(Resource): @@ -36,6 +37,8 @@ class Host(Resource): self.stats.exposed = True self.partitions = Partitions(self.model) self.partitions.exposed = True + self.devices = Devices(self.model) + self.devices.exposed = True

If "Devices" is a sub-resource of host you should put the control/devices.py content in control/host.py The files in control/ are named according to uri.

Ok, makes sense, but I think we reached a Kimchi architectural decision here. Should devices be a sub-resource of Host or not ? While coding I thought more appropriated to add then under host, like volumes are under storagepool. But, in libvirt, 'nodedev' are not restricted to any resource ... they are listed and treated as Network or StoragePool. So, should the API be <kimchi>/devices ??

...

@property def data(self): diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 552f69d..4a5f344 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -361,6 +361,34 @@ class Model(object): self.stats[vm_uuid].update({'disk_io': rate, 'max_disk_io': max_disk_io, 'diskRdKB': diskRdKB, 'diskWrKB': diskWrKB})

+ def device_lookup(self, nodedev_name): + return {'name': nodedev_name} + + def devices_get_list(self, _cap=None): + conn = self.conn.get() + if _cap == None: + dev_names = [name.name() for name in conn.listAllDevices(0)] + elif _cap == 'fc_host': + dev_names = self._get_devices_fc_host() + else: + # Get devices with required capability + dev_names = conn.listDevices(_cap,0) + return dev_names + + def _get_devices_fc_host(self): + conn = self.conn.get() + # Libvirt < 1.0.5 does not support fc_host capability + if is_libvirt_version_lesser('1.0.5'): + ret = [] + scsi_hosts = conn.listDevices('scsi_host',0) + for host in scsi_hosts: + xml = conn.nodeDeviceLookupByName(host).XMLDesc(0) + path = '/device/capability/capability/@type' + if 'fc_host' in xmlutils.xpath_get_text(xml, path): + ret.append(host) + return ret

What about the "else" code?

This is code stile ... the else would have the return right below

...

+ return conn.listDevices('fc_host',0) + def debugreport_lookup(self, name): path = config.get_debugreports_path() file_pattern = os.path.join(path, name)

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

This patch modifies the storagepool add user interface in order to show all Fibre Channel scsi hosts found in the host system and let user to create a pool attached to this host (the LUNs will be the volumes). A second option to use and enable FC storages is when a LUN is assigned as a pool of FS type, hosting guest images. This second option will be implement in the future.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- ui/js/src/kimchi.api.js | 11 +++++++++ ui/js/src/kimchi.storagepool_add_main.js | 40 +++++++++++++++++++++++++++++++- ui/pages/i18n.html.tmpl | 1 + ui/pages/storagepool-add.html.tmpl | 12 ++++++++++ 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/ui/js/src/kimchi.api.js b/ui/js/src/kimchi.api.js index 63ddd88..66fc41e 100644 --- a/ui/js/src/kimchi.api.js +++ b/ui/js/src/kimchi.api.js @@ -731,5 +731,16 @@ var kimchi = { success : suc, error : err }); + }, + + listFCHosts : function(suc, err) { + kimchi.requestJSON({ + url : kimchi.url + 'host/devices?_cap=fc_host',

For older libvirt version this filter is not true. My suggestion: 1) Use /host/devices?_cap='scsi_host' 2) In backend: - Return all scsi_host devices and identify which are FC I think we will only be able to identify the FC scsi_host for newer libvirt versions. You need to investigate it.

+ type : 'GET', + contentType : 'application/json', + dataType : 'json', + success : suc, + error : err + }); } }; diff --git a/ui/js/src/kimchi.storagepool_add_main.js b/ui/js/src/kimchi.storagepool_add_main.js index e5922b3..8331076 100644 --- a/ui/js/src/kimchi.storagepool_add_main.js +++ b/ui/js/src/kimchi.storagepool_add_main.js @@ -40,7 +40,21 @@ kimchi.initStorageAddPage = function() { label : "iSCSI", value : "iscsi" } ]; - kimchi.listHostPartitions(function(data) { + kimchi.listFCHosts(function(data){ + if (data.length > 0) { + options.push( { + label : "SCSI Fibre Channel", + value : "scsi" + }); + } + var scsiFCHtml = $('#scsiFCTmpl').html(); + var scsiFCHostListHtml = ''; + $.each(data, function(index, value) { + scsiFCHostListHtml += kimchi.template(scsiFCHtml, value); + }); + $('.scsifc-hosts').html(scsiFCHostListHtml); + + kimchi.listHostPartitions(function(data) { if (data.length > 0) { options.push({ label : "LOGICAL", @@ -107,21 +121,31 @@ kimchi.initStorageAddPage = function() { $('.logical-section').addClass('tmpl-html'); $('.nfs-section').addClass('tmpl-html'); $('.iscsi-section').addClass('tmpl-html'); + $('.scsifc-section').addClass('tmpl-html'); } else if ($(this).val() === 'netfs') { $('.path-section').addClass('tmpl-html'); $('.logical-section').addClass('tmpl-html'); $('.nfs-section').removeClass('tmpl-html'); $('.iscsi-section').addClass('tmpl-html'); + $('.scsifc-section').addClass('tmpl-html'); } else if ($(this).val() === 'iscsi') { $('.path-section').addClass('tmpl-html'); $('.logical-section').addClass('tmpl-html'); $('.nfs-section').addClass('tmpl-html'); $('.iscsi-section').removeClass('tmpl-html'); + $('.scsifc-section').addClass('tmpl-html'); + } else if ($(this).val() === 'scsi') { + $('.path-section').addClass('tmpl-html'); + $('.logical-section').addClass('tmpl-html'); + $('.nfs-section').addClass('tmpl-html'); + $('.iscsi-section').addClass('tmpl-html'); + $('.scsifc-section').removeClass('tmpl-html'); } else if ($(this).val() === 'logical') { $('.path-section').addClass('tmpl-html'); $('.logical-section').removeClass('tmpl-html'); $('.nfs-section').addClass('tmpl-html'); $('.iscsi-section').addClass('tmpl-html'); + $('.scsifc-section').addClass('tmpl-html'); } }); $('#authId').click(function() { @@ -134,6 +158,7 @@ kimchi.initStorageAddPage = function() { $('#iscsiportId').keyup(function(event) { $(this).toggleClass("invalid-field",!/^[0-9]+$/.test($(this).val())); }); + }); }); };

@@ -154,6 +179,8 @@ kimchi.validateForm = function() { return kimchi.validateNfsForm(); } else if (poolType === "iscsi") { return kimchi.validateIscsiForm(); + } else if (poolType === "scsi") { + return kimchi.validateScsiFCForm(); } else if (poolType === "logical") { return kimchi.validateLogicalForm(); } else { @@ -204,6 +231,15 @@ kimchi.validateIscsiForm = function() { return true; };

+kimchi.validateScsiFCForm = function() { + var fcHost = $('input:radio[name=adapter_name]:checked').val(); + if (fcHost === undefined) { + kimchi.message.error(i18n['msg.validate.pool.edit.scsifchost']); + return false; + } + return true; +}; + kimchi.validateServer = function(serverField) { if ('' === serverField) { kimchi.message.error(i18n['msg.pool.edit.server.blank']); @@ -248,6 +284,8 @@ kimchi.addPool = function(event) { source.path = $('#nfspathId').val(); source.host = $('#nfsserverId').val(); formData.source = source; + } else if (poolType === 'scsi'){ + formData.adapter_type = 'fc_host'; } else if (poolType === 'iscsi') { var source = {}; source.target = $('#iscsiTargetId').val(); diff --git a/ui/pages/i18n.html.tmpl b/ui/pages/i18n.html.tmpl index d63d4e9..0d6550f 100644 --- a/ui/pages/i18n.html.tmpl +++ b/ui/pages/i18n.html.tmpl @@ -100,6 +100,7 @@ var i18n = { 'msg.validate.pool.edit.path':"$_("This is not a real linux path.")", 'msg.validate.pool.edit.nfspath':"$_("Invalid nfs mount path.")", 'msg.validate.pool.edit.logical.device':"$_("No logical device selected.")", + 'msg.validate.pool.edit.scsifchost':"$_("A Fibre Channel SCSI host must be selected.")", 'msg.kimchi.storage.pool.empty':"$_("This storage pool is empty.")", 'msg.kimchi.list.volume.fail':"$_("Failed to list the storage pool.")", 'msg.kimchi.storage.pool.not.active':"$_("The storage pool is not active now.")", diff --git a/ui/pages/storagepool-add.html.tmpl b/ui/pages/storagepool-add.html.tmpl index 14ef23a..c8d4114 100644 --- a/ui/pages/storagepool-add.html.tmpl +++ b/ui/pages/storagepool-add.html.tmpl @@ -104,6 +104,12 @@ <div class="host-partition"></div> </section> </div> + <div class="scsifc-section tmpl-html"> + <section class="form-section"> + <h2>3. $_("Select SCSI Fibre Channel Host")</h2> + <div class="scsifc-hosts"></div> + </section> + </div> <div class="iscsi-section tmpl-html"> <section class="form-section"> <h2>3. $_("iSCSI Server")</h2> @@ -154,5 +160,11 @@ <label>{path}</label> </div> </script> + <script id="scsiFCTmpl" type="html/text"> + <div class="field"> + <input type="radio" value="{name}" name="adapter_name"> + <label>{name}</label> + </div> + </script> </body> </html>

Am 23-01-2014 22:29, schrieb Rodrigo Trujillo:

- kimchi.listHostPartitions(function(data) { + kimchi.listFCHosts(function(data){ You need to pass another function to "listFCHosts" which will handle the error. Currently, if the fiber channel hosts cannot be listed, nothing will happen on the UI. @@ -154,5 +160,11 @@ <label>{path}</label> </div> </script> + <script id="scsiFCTmpl" type="html/text"> + <div class="field"> + <input type="radio" value="{name}" name="adapter_name"> + <label>{name}</label> + </div> + </script> </body> </html> Make the <label>'s associated with their corresponding <input>'s, so when the user clicks on the label, the corresponding radio button gets checked. Use the syntax <label for="radioID"> for each <label> element. For that, you need to give different IDs to each <input> element and match them in each <label>. I'd suggest to give the ID something like "fc-{name}".

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote:

If a user wants to create a VM using a SCSI Fibre Channel storagepool (where LUNs are the storagepool volumes), he can assign one of the LUNs as the new VM disk, passing the LUN name. This patch implements the backend of this feature.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- docs/API.md | 1 + src/kimchi/model.py | 8 +++++++- src/kimchi/vmtemplate.py | 14 +++++++++++--- 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/docs/API.md b/docs/API.md index d00cdd5..af32ec8 100644 --- a/docs/API.md +++ b/docs/API.md @@ -55,6 +55,7 @@ the following general conventions: Independent Computing Environments * null: Graphics is disabled or type not supported * listen: The network which the vnc/spice server listens on. + * volume *(optional)*: Fibre channel LUN name to be assigned as disk to VM

### Resource: Virtual Machine diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 4a5f344..5546c4f 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -682,7 +682,8 @@ class Model(object): xml = t.to_vm_xml(name, vm_uuid, libvirt_stream=libvirt_stream, qemu_stream_dns=self.qemu_stream_dns, - graphics=graphics) + graphics=graphics, + volume=params.get('volume')) try: dom = conn.defineXML(xml.encode('utf-8')) except libvirt.libvirtError as e: @@ -1567,6 +1568,11 @@ class LibvirtVMTemplate(VMTemplate): xml = pool.XMLDesc(0) return xmlutils.xpath_get_text(xml, "/pool/@type")[0]

+ def _get_volume_path(self, volume): + pool = self._storage_validate() + volxml = pool.storageVolLookupByName(volume).XMLDesc(0) + return xmlutils.xpath_get_text(volxml, "/volume/target/path")[0] + def _get_available_volumes(self): pool = self._storage_validate() vol_names = pool.listVolumes() diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index d6f807a..5bdaa67 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -181,7 +181,7 @@ class VMTemplate(object): graphics_xml = graphics_xml + spicevmc_xml return graphics_xml

- def _get_scsi_disks_xml(self): + def _get_scsi_disks_xml(self, volume=None): ret = "" # Passthrough configuration disk_xml = """ @@ -193,7 +193,12 @@ class VMTemplate(object): if is_libvirt_version_lesser('1.0.5'): disk_xml = disk_xml.replace('volume','block')

- luns = self._get_available_volumes() + # Get a list of available volumes or use which was passed + if volume == None: + luns = self._get_available_volumes()

If user doesn't pass any volume you need to raise an error

+ else: + luns = [(self._get_volume_path(volume),)] + for i, disk in enumerate(self.info['disks']): index = disk.get('index', i) try: @@ -261,7 +266,7 @@ class VMTemplate(object):

# SCSI FC disk are different if self._get_storage_type() == 'scsi': - params['disks'] = self._get_scsi_disks_xml() + params['disks'] = self._get_scsi_disks_xml(kwargs.get('volume')) else: params['disks'] = self._get_disks_xml(vm_uuid)

@@ -330,3 +335,6 @@ class VMTemplate(object):

def _get_available_volumes(self): return [] + + def _get_volume_path(self, volume): + return ''

On 01/25/2014 09:57 PM, Aline Manera wrote:

On 01/23/2014 10:30 PM, Rodrigo Trujillo wrote:

...

This patch implements the UI functions and API calls until show to user the list of volumes (LUNs) of a SCSI FC storagepools. The user can then select the LUN when creating a new VM. This patch is a draft and gives the steps of the functionality, missing only the final selection window and function to get the proper selected value from it.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- ui/js/src/kimchi.api.js | 13 +++++++++++++ ui/js/src/kimchi.guest_add_main.js | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)

diff --git a/ui/js/src/kimchi.api.js b/ui/js/src/kimchi.api.js index 66fc41e..4597c5d 100644 --- a/ui/js/src/kimchi.api.js +++ b/ui/js/src/kimchi.api.js @@ -155,6 +155,19 @@ var kimchi = { }); },

+ /* + * Retrieve the information of a storage pool by the given name. + */ + retrieveStoragePool : function(storagePoolName, suc, err) { + kimchi.requestJSON({ + url : kimchi.url + "storagepools/" + + encodeURIComponent(storagePoolName), + type : 'GET', + contentType : 'application/json', + dataType : 'json' + }).done(suc); + }, + /** * Retrieve the information of a template by the given name. */ diff --git a/ui/js/src/kimchi.guest_add_main.js b/ui/js/src/kimchi.guest_add_main.js index 2085562..ab731fb 100644 --- a/ui/js/src/kimchi.guest_add_main.js +++ b/ui/js/src/kimchi.guest_add_main.js @@ -65,6 +65,38 @@ kimchi.guest_add_main = function() { var addGuest = function(event) { var formData = $('#form-vm-add').serializeObject();

+ // Checking if need to ask LUN to user + var templateName = formData.template.substring(11); + kimchi.retrieveTemplate(templateName, function(templateInfo) { + if (templateInfo) {

Is there a case the template info won't be available?

From the UI, I don't think so. But, someone consuming the API could pass a wrong template name. Or the template could be removed. Just code resilience, this is not bad.

...

+ var poolName = templateInfo.storagepool.substring(14); + kimchi.retrieveStoragePool(poolName, function(poolInfo){ + if (poolInfo.type !== "scsi") {

It should be for scsi pools, right? We don't need to show anything for other pools

My bad! This was for testing purposes and forget to remove (should have removed extra spaces too)

...

+ kimchi.listStorageVolumes(poolInfo.name, function(lunsList) { + if (lunsList.length == 0) { + kimchi.message.error('There are not volumes for this pool'); + return false; + } + var popUpList = '<div class="field">'; + // TODO + // Implement a better UI and retrieve + // the selected LUN from a popup window + $.each(lunsList, function(index, value) { + popUpList += '<div class=field> <input type="radio" value="' + value.name + '" name="lun">' + + '<label>' + value.name + '</label></div>'; + }); + popUpList += '</div>'; + myhtml = $(popUpList); + myhtml.dialog();

I think we need to implement the UI, right? It will show a ugly window

Some UI export can look into it?

Rodrigo, if you want to do the UI go ahead but make it completely. Otherwise, remove this code and ask UI guys to do it.

I asked for help in the same day. Yu Xin Huo and Hongliang Wang kindly answered (thanks), I am going to complete this UI in next patch =)

...

+ // TODO / FIXME + // Retrieve the LUN name from here, should come from window + var lunName = "selectedLun"; + formData.volume = lunName; + }); + } + }); + } + }); kimchi.createVM(formData, function() { kimchi.listVmsAuto(); kimchi.window.close();

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel