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

Wed Jul 23 02:29:23 UTC 2014

Will fix all in next release
On 2014年07月22日 02:10, Aline Manera wrote:
>
> On 07/20/2014 12:08 PM, lvroyce0210 at gmail.com wrote:
>> From: Royce Lv <lvroyce at linux.vnet.ibm.com>
>>
>> Multiple files modification for change 'cdrom' to optional param.
>>
>> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
>> ---
>> 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 at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>