[Kimchi-devel] [PATCH][Kimchi 4/6] Identify installation media while creating template
Ramon Medeiros
ramonn at linux.vnet.ibm.com
Thu Mar 24 19:30:01 UTC 2016
On 03/23/2016 05:21 PM, Aline Manera wrote:
>
>
> On 03/22/2016 04:20 PM, Ramon Medeiros wrote:
>> First, improve functions inside class. This will improve code reading,
>> also, make more functions to split procedures like: validating disks,
>> graphics xml and distro checking.
>>
>> Add separate function to identify the installation media.
>>
>> Signed-off-by: Ramon Medeiros <ramonn at linux.vnet.ibm.com>
>> ---
>> model/templates.py | 4 ++
>> vmtemplate.py | 142
>> +++++++++++++++++++++++++++++++++++++++++++++++------
>> 2 files changed, 132 insertions(+), 14 deletions(-)
>>
>> diff --git a/model/templates.py b/model/templates.py
>> index b49ac50..a03a7cf 100644
>> --- a/model/templates.py
>> +++ b/model/templates.py
>> @@ -196,6 +196,10 @@ class TemplateModel(object):
>> params['memory'].update(new_mem)
>> validate_memory(params['memory'])
>
>
>> + # params are cdrom or disk: add it to source_media
>> + if "cdrom" in params:
>> + params["source_media"] = params.get("cdrom")
>> +
>
> Is it 'source_media' ou 'installation_media'?
>
> I prefer 'source_media' but if so, you need to update API.json (first
> patch) to reflect it.
>
> Also, if the user is already using the 'cdrom' or base disk as
> parameter, why do you need to check the media type?
>
>> new_t.update(params)
>>
>> for net_name in params.get(u'networks', []):
>> diff --git a/vmtemplate.py b/vmtemplate.py
>> index 4bcf324..8572dfe 100644
>> --- a/vmtemplate.py
>> +++ b/vmtemplate.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 magic
>> import os
>> import platform
>> import stat
>> @@ -29,10 +30,13 @@ from lxml.builder import E
>> from wok.exception import InvalidParameter, ImageFormatError,
>> IsoFormatError
>> from wok.exception import MissingParameter, OperationFailed
>>
>> +from wok.xmlutils.utils import xpath_get_text
>> from wok.plugins.kimchi import imageinfo
>> from wok.plugins.kimchi import osinfo
>> from wok.plugins.kimchi.isoinfo import IsoImage
>
>> +from wok.plugins.kimchi.model.libvirtconnection import
>> LibvirtConnection
>
> VMTemplate must be independent of Model. Instead of doing that, you
> should pass the data you want as parameter to the function.
>
>> from wok.plugins.kimchi.utils import check_url_path,
>> pool_name_from_uri
>> +from wok.plugins.kimchi.utils import get_template_by_name
>> from wok.plugins.kimchi.xmlutils.cpu import get_cpu_xml
>> from wok.plugins.kimchi.xmlutils.disk import get_disk_xml
>> from wok.plugins.kimchi.xmlutils.graphics import get_graphics_xml
>> @@ -40,6 +44,9 @@ from wok.plugins.kimchi.xmlutils.interface import
>> get_iface_xml
>> from wok.plugins.kimchi.xmlutils.qemucmdline import
>> get_qemucmdline_xml
>> from wok.plugins.kimchi.xmlutils.serial import get_serial_xml
>>
>> +DISK_TYPE = {"QEMU QCOW Image": "qcow2",
>> + "data": "raw"}
>> +ISO_TYPE = "ISO 9660 CD-ROM"
>>
>> class VMTemplate(object):
>> def __init__(self, args, scan=False):
>> @@ -54,20 +61,16 @@ class VMTemplate(object):
>> self.info = {}
>> self.fc_host_support = args.get('fc_host_support')
>>
>> - # Fetch defaults based on the os distro and version
>> - try:
>> - distro, version = self._get_os_info(args, scan)
>> - except ImageFormatError as e:
>> - raise OperationFailed('KCHTMPL0020E', {'err': e.message})
>> - os_distro = args.get('os_distro', distro)
>> - os_version = args.get('os_version', version)
>> - entry = osinfo.lookup(os_distro, os_version)
>> - self.info.update(entry)
>
>> + # search for template with the same name
>> + template = get_template_by_name(args.get("name"))
>
> You are mixing the Template resource provided by model/templates.py to
> VMTemplate.
> VMTemplate is a dict of information. At this point, there is no
> information stored anywhere.
>
>
> I think you are doing it to do no scan the source_media multiple
> times, but for that the 'scan' parameter was used. You should continue
> to use it.
>
i created two functions to improve code reading. Instead of that. we
will have a 100 lines function.
>>
>> - # Auto-generate a template name if no one is passed
>> - if 'name' not in args or args['name'] == '':
>> - args['name'] = self._gen_name(distro, version)
>> - self.name = args['name']
>> + # no record found: create template
>> + if len(template) == 0:
>> + self._create_template(args, scan)
>> +
>> + # template already exists: load it
>> + else:
>> + self.info.update(args)
>>
>> # Merge graphics settings
>> graph_args = args.get('graphics')
>> @@ -87,6 +90,35 @@ class VMTemplate(object):
>>
>> # Override template values according to 'args'
>> self.info.update(args)
>> +
>> + # validate disks
>> + self._validate_disks(default_disk)
>> +
>> + def _create_template(self, args, scan=False):
>> + """
>> + Creates a new template
>> + """
>> + # identify installation media
>> + self._identify_installation_media(args)
>> +
>> + # Fetch defaults based on the os distro and version
>> + try:
>> + distro, version = self._get_os_info(args, scan)
>> + except ImageFormatError as e:
>> + raise OperationFailed('KCHTMPL0020E', {'err': e.message})
>> + os_distro = args.get('os_distro', distro)
>> + os_version = args.get('os_version', version)
>> + entry = osinfo.lookup(os_distro, os_version)
>> +
>> + # update info
>> + self.info.update(entry)
>> +
>> + # Auto-generate a template name if no one is passed
>> + if 'name' not in args or args['name'] == '':
>> + args['name'] = self._gen_name(distro, version)
>> + self.info["name"] = args['name']
>> +
>> + def _validate_disks(self, default_disk):
>> disks = self.info.get('disks')
>>
>> basic_disk = ['index', 'format', 'pool', 'size']
>> @@ -95,7 +127,6 @@ class VMTemplate(object):
>>
>> for index, disk in enumerate(disks):
>> disk_info = dict(default_disk)
>> -
>> pool_type = self._get_storage_type(disk['pool']['name'])
>> if pool_type in ['iscsi', 'scsi']:
>> disk_info = {'index': 0, 'format': 'raw', 'volume':
>> None}
>> @@ -118,6 +149,89 @@ class VMTemplate(object):
>> disk_info['index'] = disk_info.get('index', index)
>> self.info['disks'][index] = disk_info
>>
>> + def _identify_installation_media(self, args):
>
> Why a new function is needed? Can't we only use the existing functions
> to identify an ISO or image?
>
>> + path = args.get("installation_media")
>> +
>
> You are mixing 'installation_media' with 'source_media'
>
>> + # user did not passed installation media: return
>> + if path is None:
>> + return
>> +
>> + # create magic object to discover file type
>> + file_type = magic.open(magic.MAGIC_NONE)
>> + file_type.load()
>> +
>> + # file is local and exists: check if it's disk or cdrom
>> + if path.startswith("/") and os.path.exists(path):
>> + ftype = file_type.file(path)
>> +
>> + # cdrom
>> + if ISO_TYPE in ftype:
>> + args["cdrom"] = path
>> + return
>> +
>> + # disk
>> + for types in DISK_TYPE.keys():
>> + if types in ftype:
>> +
>> + # no disk list: create it
>> + if args.get("disks") is None:
>> + args["disks"] = []
>> +
>> + # append disk to the list
>> + args["disks"].insert(0,
>> + {"base": path,
>> + "format": DISK_TYPE[types],
>> + "index": 0,
>> + "pool": {"name":
>> + "/plugins/kimchi/storagepools/default",
>> + "type":"dir"}})
>> +
>> + # get storage pool
>> + pool =
>> self._get_storagepool_from_storagevolume(path)
>> +
>> + # image not in any pool: get default pool
>> + if len(pool) == 0:
>> + os_info = osinfo.lookup("unknow",
>> "unknow")["disks"][0]["pool"]
>> +
>> + # os_info has default pool: get it
>> + if os_info != None:
>> + args["disks"][0]["pool"] = os_info
>> + return
>> +
>> + # pool available
>> + args["disks"][0]["pool"] = pool
>> +
>
>> + # not local image: set as remote ISO
>> + elif path.startswith("http") or path.startswith("ftp"):
>> + args["cdrom"] = path
>> + return
>> +
>
> There is a function on utils.py for that - check_url_path()
>
>> + def _get_storagepool_from_storagevolume(self, storagevolume_path):
>> + conn = LibvirtConnection('qemu:///system').get()
>> + storagepools = conn.listStoragePools()
>> + storagepools += conn.listDefinedStoragePools()
>> +
>> + # discover storagepool
>> + for storagepool in storagepools:
>> + pool = conn.storagePoolLookupByName(storagepool)
>> + spath = xpath_get_text(pool.XMLDesc(),
>> "/pool/target/path")[0]
>> + pool_type = xpath_get_text(pool.XMLDesc(),
>> "/pool/@type")[0]
>> + storagevolumes = sorted(map(lambda x: x.decode('utf-8'),
>> + pool.listVolumes()))
>> + for storagevolume in storagevolumes:
>> +
>> + # path does not exists: continue to the next
>> + if not os.path.exists("%s/%s" % (spath,
>> storagevolume)):
>> + continue
>> +
>> + # storagepool found: return
>> + volume = "%s/%s" % (spath, storagevolume)
>> + if os.path.samefile(storagevolume_path, volume):
>> + pool_uri = "/plugins/kimchi/storagepools/%s" %
>> storagepool
>> + return {"name": pool_uri,
>> + "type": pool_type}
>> + return {}
>> +
>
> All that should not be on VMTemplate.
> As I said before VMTemplate is independent of Model.
> Any information you want should go through Model via parameter.
>
>> def _get_os_info(self, args, scan):
>> distro = version = 'unknown'
>>
>
--
Ramon Nunes Medeiros
Kimchi Developer
Linux Technology Center Brazil
IBM Systems & Technology Group
Phone : +55 19 2132 7878
ramonn at br.ibm.com
More information about the Kimchi-devel
mailing list