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