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

On 12/23/2015 01:12 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(a)linux.vnet.ibm.com&gt; > --- > vmtemplate.py | 122 > +++++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 108 insertions(+), 14 deletions(-) > > diff --git a/vmtemplate.py b/vmtemplate.py > index d629226..a209578 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 psutil > @@ -28,12 +29,14 @@ from lxml import etree > from lxml.builder import E > > from wok.exception import InvalidParameter, ImageFormatError, > IsoFormatError > -from wok.exception import MissingParameter, OperationFailed > +from wok.exception import MissingParameter, OperationFailed, > InvalidOperation > > +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.utils import check_url_path, pool_name_from_uri > +from wok.plugins.kimchi.model.libvirtconnection import > LibvirtConnection > +from wok.plugins.kimchi.utils import check_url_path, > pool_name_from_uri, 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 > @@ -41,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" > > # In PowerPC, memories must be aligned to 256 MiB > PPC_MEM_ALIGN = 256 > @@ -61,6 +67,36 @@ class VMTemplate(object): > self.info = {} > self.fc_host_support = args.get('fc_host_support') > > + # search for template with the same name > + template = get_template_by_name(args.get("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) Is this else + self.info.update(args) necessary ? I am asking because there is another info.update right below

> + > + # Merge graphics settings > + graph_args = args.get('graphics') > + if graph_args: > + graphics = dict(self.info['graphics']) > + graphics.update(graph_args) > + args['graphics'] = graphics > + > + # validate disks > + default_disk = self.info['disks'][0] > + self.info.update(args) > + 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) > @@ -69,23 +105,16 @@ class VMTemplate(object): > 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.name = args['name'] > - > - # Merge graphics settings > - graph_args = args.get('graphics') > - if graph_args: > - graphics = dict(self.info['graphics']) > - graphics.update(graph_args) > - args['graphics'] = graphics > + self.info["name"] = args['name'] > > - default_disk = self.info['disks'][0] > - # Override template values according to 'args' > - self.info.update(args) > + def _validate_disks(self, default_disk): > disks = self.info.get('disks') > > basic_disk = ['index', 'format', 'pool', 'size'] > @@ -94,7 +123,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} > @@ -117,6 +145,72 @@ class VMTemplate(object): > disk_info['index'] = disk_info.get('index', index) > self.info['disks'][index] = disk_info > > + def _identify_installation_media(self, args): > + path = args.get("installation_media") > + > + # user did not passed installation media: return > + if path == 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") == None: > + args["disks"] = [] > + > + # append disk to the list > + args["disks"].insert(0, {"base": path, > + "format": > DISK_TYPE[types], > + "index": 0 > + }) > + # get storage pool > + pool = > self._get_storagepool_from_storagevolume(path) > + args["disks"][0]['pool'] = pool > + return > + > + # not local image: set as remote ISO > + elif path.startswith("http") or path.startswith("ftp"): > + args["cdrom"] = path > + return I think you must add an ELSE statement here, because, I am not sure what is going to happen if path is something strange, like "43242/4343/232" Have you searched if there is something similar in other parts ? Or another way to get the same result ? If there is not , then I am ok with this function, however , I think you would have to move it to another place, because I was told that we should not use Libvirt or call libvirt in vmtemplates.py

> + > + 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] > + for storagevolume in sorted(map(lambda x: > x.decode('utf-8'), pool.listVolumes())): > + > + # path does not exists: continue to the next > + if not os.path.exists("%s/%s" % (spath, > storagevolume)): > + continue > + > + # storagepool found: return > + if os.path.samefile(storagevolume_path,"%s/%s" % > (spath, storagevolume)): > + return {"name": > "/plugins/kimchi/storagepools/%s" % storagepool, > + "type": pool_type > + } > + return {} > + > def _get_os_info(self, args, scan): > distro = version = 'unknown' > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel