[Kimchi-devel] [PATCH V2 3/7] (WIP) Storagepool SCSI/FC: Assign SCSI fibre channel LUN as disk to a new guest
Aline Manera
alinefm at linux.vnet.ibm.com
Thu Jan 30 14:12:37 UTC 2014
On 01/28/2014 07:19 PM, Rodrigo Trujillo wrote:
> On 01/25/2014 09:08 PM, Aline Manera 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
> 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 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.
> 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 at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>
More information about the Kimchi-devel
mailing list