[Kimchi-devel] [PATCH V2 3/7] (WIP) Storagepool SCSI/FC: Assign SCSI fibre channel LUN as disk to a new guest

[Aline Manera]

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 at 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 []