Re: [Kimchi-devel] [PATCHv3 3/8] Change 'cdrom' to a optional param

On 07/20/2014 12:08 PM, lvroyce0210(a)gmail.com wrote: > From: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; > > Multiple files modification for change 'cdrom' to optional param. > > Signed-off-by: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; > --- > src/kimchi/control/templates.py | 2 +- > src/kimchi/i18n.py | 2 +- > src/kimchi/imageinfo.py | 19 ++++++++++++++++++- > src/kimchi/mockmodel.py | 27 +++++++++++++++++++++------ > src/kimchi/model/templates.py | 13 ++++++++----- > src/kimchi/vmtemplate.py | 37 +++++++++++++++++++++++++++---------- > tests/test_rest.py | 2 +- > tests/test_vmtemplate.py | 2 +- > 8 files changed, 78 insertions(+), 26 deletions(-) > > diff --git a/src/kimchi/control/templates.py > b/src/kimchi/control/templates.py > index 907929f..65b6805 100644 > --- a/src/kimchi/control/templates.py > +++ b/src/kimchi/control/templates.py > @@ -47,7 +47,7 @@ class Template(Resource): > 'os_version': self.info['os_version'], > 'cpus': self.info['cpus'], > 'memory': self.info['memory'], > - 'cdrom': self.info['cdrom'], > + 'cdrom': self.info.get('cdrom', None), > 'disks': self.info['disks'], > 'storagepool': self.info['storagepool'], > 'networks': self.info['networks'], > diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py > index 0c3aae5..f872bee 100644 > --- a/src/kimchi/i18n.py > +++ b/src/kimchi/i18n.py > @@ -118,7 +118,7 @@ messages = { > "KCHTMPL0013E": _("Amount of memory (MB) must be an integer greater > than 512"), > "KCHTMPL0014E": _("Template CDROM must be a local or remote ISO file"), > "KCHTMPL0015E": _("Invalid storage pool URI %(value)s specified for > template"), > - "KCHTMPL0016E": _("Specify an ISO image as CDROM to create a > template"), > + "KCHTMPL0016E": _("Specify an ISO image as CDROM or a base image to > create a template"), > "KCHTMPL0017E": _("All networks for the template must be specified in > a list."), > "KCHTMPL0018E": _("Must specify a volume to a template, when storage > pool is iscsi or scsi"), > "KCHTMPL0019E": _("The volume: %(volume)s in not in storage pool > %(pool)s"), > diff --git a/src/kimchi/imageinfo.py b/src/kimchi/imageinfo.py > index f874ece..f4c6356 100644 > --- a/src/kimchi/imageinfo.py > +++ b/src/kimchi/imageinfo.py > @@ -17,11 +17,28 @@ > # 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 json > import os > import sys > import guestfs > > -from kimchi.exception import ImageFormatError > +from kimchi.exception import ImageFormatError, TimeoutExpired > +from kimchi.utils import run_command, kimchi_log > + > + > +def probe_img_info(path): > + cmd = ["qemu-img", "info", "--output=json", path] > + info = dict() > + try: > + out = run_command(cmd, 10)[0] > + except TimeoutExpired: > + kimchi_log.warning("Cannot decide format of base img %s", path) > + return None > + > + info = json.loads(out) > + info['virtual-size'] = info['virtual-size'] >> 30 > + info['actual-size'] = info['actual-size'] >> 30 > + return info > > > def probe_image(image_path): > diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py > index 0e45d1e..ccaa4d8 100644 > --- a/src/kimchi/mockmodel.py > +++ b/src/kimchi/mockmodel.py > @@ -201,8 +201,10 @@ class MockModel(object): > index += 1 > > cdrom = "hd" + string.ascii_lowercase[index + 1] > - cdrom_params = {'dev': cdrom, 'path': t_info['cdrom'], 'type': > 'cdrom'} > - vm.storagedevices[cdrom] = MockVMStorageDevice(cdrom_params) > + if t_info.get('cdrom'): > + cdrom_params = { > + 'dev': cdrom, 'path': t_info['cdrom'], 'type': 'cdrom'} > + vm.storagedevices[cdrom] = MockVMStorageDevice(cdrom_params) > > self._mock_vms[name] = vm > return name > @@ -238,11 +240,24 @@ class MockModel(object): > > def templates_create(self, params): > name = params.get('name', '').strip() > + iso = params.get('cdrom') > if not name: > - iso = params['cdrom'] > - iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0] > - name = iso_name + str(int(time.time() * 1000)) > - params['name'] = name > + if iso: > + iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0] > + name = iso_name + str(int(time.time() * 1000)) > + params['name'] = name > + else: > + raise InvalidParameter("KCHTMPL0008E") > + If no ISO is specified you should get the template name based on "base" img I suggest change those 2 if's order base_tmpl = None if not iso: if 'disks' in params: for d in params['disks']: if 'base' in d: base_tmpl = d['base'] break if base_tmpl is None: raise MissingParameter() if not name: # get the name according to base_tmpl or iso > + base_tmpl = False > + if not iso: > + if 'disks' in params: > + for d in params['disks']: > + if 'base' in d: > + base_tmpl = True > + break > + if not base_tmpl: > + raise InvalidParameter("KCHTMPL0016E") MissingParameter() is better than InvalidParameter() > > if name in self._mock_templates: > raise InvalidOperation("KCHTMPL0001E", {'name': name}) > diff --git a/src/kimchi/model/templates.py > b/src/kimchi/model/templates.py > index 60f4de5..0e07a78 100644 > --- a/src/kimchi/model/templates.py > +++ b/src/kimchi/model/templates.py > @@ -40,9 +40,9 @@ class TemplatesModel(object): > > def create(self, params): > name = params.get('name', '').strip() > - iso = params['cdrom'] > + iso = params.get('cdrom') > # check search permission > - if iso.startswith('/') and os.path.isfile(iso): > + if iso and iso.startswith('/') and os.path.isfile(iso): > user = UserTests().probe_user() > ret, excp = probe_file_permission_as_user(iso, user) > if ret is False: > @@ -51,9 +51,12 @@ class TemplatesModel(object): > 'err': excp}) > > if not name: > - iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0] > - name = iso_name + str(int(time.time() * 1000)) > - params['name'] = name > + if iso: > + iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0] > + name = iso_name + str(int(time.time() * 1000)) > + params['name'] = name > + else: > + raise InvalidParameter('KCHTMPL0008E') The same I commented above. If no ISO is specified get the name from "base" > > conn = self.conn.get() > pool_uri = params.get(u'storagepool', '') > diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py > index 8d5217a..0903b7f 100644 > --- a/src/kimchi/vmtemplate.py > +++ b/src/kimchi/vmtemplate.py > @@ -24,7 +24,8 @@ import urlparse > > > from kimchi import osinfo > -from kimchi.exception import InvalidParameter, IsoFormatError > +from kimchi.exception import InvalidParameter, IsoFormatError, > ImageFormatError > +from kimchi.imageinfo import probe_image, probe_img_info > from kimchi.isoinfo import IsoImage > from kimchi.utils import check_url_path, pool_name_from_uri > from lxml import etree > @@ -54,6 +55,19 @@ class VMTemplate(object): > iso_distro = iso_version = 'unknown' > iso = args.get('cdrom', '') > > + # if ISO not specified and base disk image specified, > + # prevent cdrom from filling automatically > + if len(iso) == 0 and 'disks' in args: > + for d in args['disks']: > + if 'base' in d: > + try: > + distro, version = probe_image(d['base']) > + except ImageFormatError: > + pass > + if 'size' not in d: > + d['size'] = probe_img_info(d['base'])['virtual-size'] > + break > + > if scan and len(iso) > 0: > iso_distro, iso_version = self.get_iso_info(iso) > if not iso.startswith('/'): > @@ -84,6 +98,8 @@ class VMTemplate(object): > raise InvalidParameter("KCHISO0001E", {'filename': iso}) > > def _get_cdrom_xml(self, libvirt_stream_protocols, qemu_stream_dns): > + if 'cdrom' not in self.info: > + return None As this function should return a xml string, it is better to return "" instead of None Otherwise the xml will be filled wrong > bus = self.info['cdrom_bus'] > dev = "%s%s" % (self._bus_to_dev[bus], > string.lowercase[self.info['cdrom_index']]) > @@ -325,13 +341,14 @@ drive=drive-%(bus)s0-1-0,id=%(bus)s0-1-0'/> > libvirt_stream_protocols = kwargs.get('libvirt_stream_protocols', []) > cdrom_xml = self._get_cdrom_xml(libvirt_stream_protocols, > qemu_stream_dns) > - > - if not urlparse.urlparse(self.info['cdrom']).scheme in \ > - libvirt_stream_protocols and params.get('iso_stream', False): > - params['qemu-namespace'] = QEMU_NAMESPACE > - params['qemu-stream-cmdline'] = cdrom_xml > - else: > - params['cdroms'] = cdrom_xml > + if cdrom_xml: > + if not urlparse.urlparse(self.info['cdrom']).scheme in \ > + libvirt_stream_protocols and \ > + params.get('iso_stream', False): > + params['qemu-namespace'] = QEMU_NAMESPACE > + params['qemu-stream-cmdline'] = cdrom_xml > + else: > + params['cdroms'] = cdrom_xml If you return "" in _get_cdrom_xml() as I suggested you just need to change self.info['cdrom'] to self.info.get('cdrom', "") > xml = """ > <domain type='%(domain)s' %(qemu-namespace)s> > @@ -414,8 +431,8 @@ drive=drive-%(bus)s0-1-0,id=%(bus)s0-1-0'/> > > # validate iso integrity > # FIXME when we support multiples cdrom devices > - iso = self.info['cdrom'] > - if not (os.path.isfile(iso) or check_url_path(iso)): > + iso = self.info.get('cdrom') > + if iso and not (os.path.isfile(iso) or check_url_path(iso)): > invalid['cdrom'] = [iso] > > self.info['invalid'] = invalid > diff --git a/tests/test_rest.py b/tests/test_rest.py > index 54209ef..97668e7 100644 > --- a/tests/test_rest.py > +++ b/tests/test_rest.py > @@ -1068,7 +1068,7 @@ class RestTests(unittest.TestCase): > self.assertEquals(200, resp.status) > self.assertEquals(0, len(json.loads(resp.read()))) > > - # Create a template without cdrom fails with 400 > + # Create a template without cdrom and disk specified fails with 400 > t = {'name': 'test', 'os_distro': 'ImagineOS', > 'os_version': '1.0', 'memory': 1024, 'cpus': 1, > 'storagepool': '/storagepools/alt'} > diff --git a/tests/test_vmtemplate.py b/tests/test_vmtemplate.py > index 821ca24..319f250 100644 > --- a/tests/test_vmtemplate.py > +++ b/tests/test_vmtemplate.py > @@ -29,7 +29,7 @@ class VMTemplateTests(unittest.TestCase): > def test_minimal_construct(self): > fields = (('name', 'test'), ('os_distro', 'unknown'), > ('os_version', 'unknown'), ('cpus', 1), > - ('memory', 1024), ('cdrom', ''), ('networks', ['default']), > + ('memory', 1024), ('networks', ['default']), > ('disk_bus', 'ide'), ('nic_model', 'e1000'), > ('graphics', {'type': 'vnc', 'listen': '0.0.0.0'})) > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel