[Kimchi 0/4] Use single field to add installation media
This patch changes how templates are created. Instead of specifying cdrom and disks, add them into a single field and kimchi will identify it. Points to take noteL 1) UI support. Another task will need to change UI for a single entry. 2) Old support for cdrom and disk installation are still available. All tests are running under tests/test*. Ramon Medeiros (4): Create new field to create VM Method to retrieve stored templates at object store Fix checking duplicate template before creating it Identify installation media while creating template API.json | 5 +++ model/templates.py | 9 ++-- utils.py | 28 ++++++++++++ vmtemplate.py | 122 +++++++++++++++++++++++++++++++++++++++++++++++------ 4 files changed, 146 insertions(+), 18 deletions(-) -- 2.1.0
Create a single field to pass the installation media. Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- API.json | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/API.json b/API.json index 2b64d07..13fcfd3 100644 --- a/API.json +++ b/API.json @@ -460,6 +460,11 @@ "minimum": 512, "error": "KCHTMPL0013E" }, + "installation_media": { + "description": "Path for installation media (ISO, disk, remote ISO)", + "type" : "string", + "pattern" : "^[^ ]+( +[^ ]+)*$" + }, "cdrom": { "description": "Path for cdrom", "type": "string", -- 2.1.0
On 12/23/2015 01:12 PM, Ramon Medeiros wrote:
Create a single field to pass the installation media.
Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- API.json | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/API.json b/API.json index 2b64d07..13fcfd3 100644 --- a/API.json +++ b/API.json @@ -460,6 +460,11 @@ "minimum": 512, "error": "KCHTMPL0013E" }, + "installation_media": { + "description": "Path for installation media (ISO, disk, remote ISO)", + "type" : "string", + "pattern" : "^[^ ]+( +[^ ]+)*$" + }, Two things: 1- there will be a pattern checking here, when you create the template, so we need a error field. 2- what about the cdrom parameter below ? will it be discontinued ? Would you have to remove it and make all adjustments need in the code ?
"cdrom": { "description": "Path for cdrom", "type": "string",
On 12/24/2015 09:38 AM, Rodrigo Trujillo wrote:
On 12/23/2015 01:12 PM, Ramon Medeiros wrote:
Create a single field to pass the installation media.
Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- API.json | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/API.json b/API.json index 2b64d07..13fcfd3 100644 --- a/API.json +++ b/API.json @@ -460,6 +460,11 @@ "minimum": 512, "error": "KCHTMPL0013E" }, + "installation_media": { + "description": "Path for installation media (ISO, disk, remote ISO)", + "type" : "string", + "pattern" : "^[^ ]+( +[^ ]+)*$" + }, Two things: 1- there will be a pattern checking here, when you create the template, so we need a error field. 2- what about the cdrom parameter below ? will it be discontinued ? Would you have to remove it and make all adjustments need in the code ?
Like i said in the cover letter, i'm planning to remove this after the UI code being merged. But, if you think it's better to remove now, we can do.
"cdrom": { "description": "Path for cdrom", "type": "string",
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
-- Ramon Nunes Medeiros Kimchi Developer Linux Technology Center Brazil IBM Systems & Technology Group Phone : +55 19 2132 7878 ramonn@br.ibm.com
Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- utils.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/utils.py b/utils.py index 5a3f209..5f576f7 100644 --- a/utils.py +++ b/utils.py @@ -34,6 +34,34 @@ from wok.xmlutils.utils import xpath_get_text MAX_REDIRECTION_ALLOWED = 5 +def get_template_by_name(name): + + conn = sqlite3.connect(config.get_object_store(), timeout=10) + cursor = conn.cursor() + + # if * is passed: select all fields + sql = "SELECT json from objects where type == 'template' and id == '%s'" % name + if name == "*": + sql = "SELECT json from objects where type == 'template'" + + # execute and fetch results + cursor.execute(sql) + content = cursor.fetchall() + + # no record: return nothing + if len(content) == 0: + return {} + + # sqllite returns a tuple of strings + # iterate over it and return a list of dictonaries + if len(content[0]) == 1: + return eval(content[0][0]) + + result = [] + for dictonary in content[0]: + result.append(eval(dictonary)) + + return result def _uri_to_name(collection, uri): expr = '/plugins/kimchi/%s/(.*?)$' % collection -- 2.1.0
Template was being check if duplicated after calling for VMTemplate, which can make unecessary work. Check it before creating the class. Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- model/templates.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/model/templates.py b/model/templates.py index c9b11c3..6cef4e0 100644 --- a/model/templates.py +++ b/model/templates.py @@ -78,8 +78,11 @@ class TemplatesModel(object): raise InvalidParameter("KCHTMPL0003E", {'network': net_name, 'template': name}) # Creates the template class with necessary information - # Checkings will be done while creating this class, so any exception - # will be raised here + # template with the same name already exists: raise exception + with self.objstore as session: + if name in session.get_list('template'): + raise InvalidOperation("KCHTMPL0001E", {'name': params["name"]}) + t = LibvirtVMTemplate(params, scan=True, conn=self.conn) # Validate max memory @@ -98,8 +101,6 @@ class TemplatesModel(object): name = params['name'] try: with self.objstore as session: - if name in session.get_list('template'): - raise InvalidOperation("KCHTMPL0001E", {'name': name}) session.store('template', name, t.info, get_kimchi_version()) except InvalidOperation: -- 2.1.0
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@linux.vnet.ibm.com> --- 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) + + # 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 + + 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' -- 2.1.0
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@linux.vnet.ibm.com> --- 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.exception import InvalidOperation, 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.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.utils import check_url_path, get_template_by_name, pool_name_from_uri
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) + + # 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 + + 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'
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@linux.vnet.ibm.com> --- 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'
On 12/24/2015 11:20 AM, Rodrigo Trujillo wrote:
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@linux.vnet.ibm.com> --- 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
yes. As you can see, the next block will point to self.info: graphics = dict(self.info['graphics'])
+ + # 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 We are checking if:
os.path.exists Then, if not, we check if it's starts with http or ftp. This case will not match in any of the checkings
+ + 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@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
-- Ramon Nunes Medeiros Kimchi Developer Linux Technology Center Brazil IBM Systems & Technology Group Phone : +55 19 2132 7878 ramonn@br.ibm.com
make check-local errors: [danielhb@arthas kimchi]$ make check-local contrib/check_i18n.py ./i18n.py Checking for invalid i18n string... Checking for invalid i18n string successfully find . -path './.git' -prune -type f -o \ -name '*.py' -o -name '*.py.in' | xargs /bin/pyflakes | \ while read LINE; do echo "$LINE"; false; done ./vmtemplate.py:32: 'InvalidOperation' imported but unused Makefile:1035: recipe for target 'check-local' failed make: *** [check-local] Error 1 [danielhb@arthas kimchi]$ On 12/23/2015 01:12 PM, Ramon Medeiros wrote:
This patch changes how templates are created. Instead of specifying cdrom and disks, add them into a single field and kimchi will identify it.
Points to take noteL
1) UI support. Another task will need to change UI for a single entry.
2) Old support for cdrom and disk installation are still available. All tests are running under tests/test*.
Ramon Medeiros (4): Create new field to create VM Method to retrieve stored templates at object store Fix checking duplicate template before creating it Identify installation media while creating template
API.json | 5 +++ model/templates.py | 9 ++-- utils.py | 28 ++++++++++++ vmtemplate.py | 122 +++++++++++++++++++++++++++++++++++++++++++++++------ 4 files changed, 146 insertions(+), 18 deletions(-)
participants (3)
-
Daniel Henrique Barboza -
Ramon Medeiros -
Rodrigo Trujillo