Re: [Kimchi-devel] [PATCH][Kimchi 4/6] Identify installation media while creating template

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.

> > - # 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' >