Re: [Kimchi-devel] [PATCH V1] [Kimchi] Only on s390x add default networks to template if template.conf has default network configuration uncommented.

This patch broke the following unit tests: ====================================================================== FAIL: test_storagevolume_action (test_model_storagevolume.StorageVolumeTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "test_model_storagevolume.py", line 274, in test_storagevolume_action _do_volume_test(self, model, host, ssl_port, 'default') File "test_model_storagevolume.py", line 244, in _do_volume_test self.assertEquals(202, resp.status) AssertionError: 202 != 400 ---------------------------------------------------------------------- Ran 2 tests in 63.746s FAILED (failures=1) More comments inline: On 08/17/2016 09:38 AM, archus(a)linux.vnet.ibm.com wrote: > From: Archana Singh <archus(a)linux.vnet.ibm.com&gt; > > Below are the changes done for s390x architecture to only add default > networks > to template if template.conf has network configuration uncommented > otherwise do not add any network by default. > > 1) Only on s390x, osinfo._get_tmpl_defaults() does not add > default network in case template.conf has commented out network > configuration. > 2) Only on s390x, NetworksModel._check_default_networks() does > not throw key error if networks not found in tmpl_defaults. > 3) Only on s390x, VMTemplate.validate_integrity() does not throw > key error if networks not found in the template. > 4) Only on s390x, LibvirtVMTemplate._network_validate() does not > throw key error on s390x if networks not found in tmpl_default, as on > s390x networks is optional. > 5) Only on s390x, VMTemplate._get_network_xml() does > return empty string and does not throw key error if networks not > found in template object. > 6) Only on s390x, control/templates.py gets networks as [] if not found > self.info. > > Signed-off-by: Archana Singh <archus(a)linux.vnet.ibm.com&gt; > --- > control/templates.py | 2 +- > model/networks.py | 7 ++++++- > model/templates.py | 5 ++++- > osinfo.py | 14 +++++++++++++- > vmtemplate.py | 15 +++++++++++++-- > 5 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/control/templates.py b/control/templates.py > index bb2e068..2dd8601 100644 > --- a/control/templates.py > +++ b/control/templates.py > @@ -67,7 +67,7 @@ class Template(Resource): > 'memory': self.info['memory'], > 'cdrom': self.info.get('cdrom', None), > 'disks': self.info['disks'], > - 'networks': self.info['networks'], > + 'networks': self.info.get('networks', []), > 'folder': self.info.get('folder', []), > 'graphics': self.info['graphics'], > 'cpu_info': self.info.get('cpu_info') > diff --git a/model/networks.py b/model/networks.py > index 35431d4..c9db979 100644 > --- a/model/networks.py > +++ b/model/networks.py > @@ -21,6 +21,7 @@ import copy > import ipaddr > import libvirt > import time > +import os wrong import order > from libvirt import VIR_INTERFACE_XML_INACTIVE > from wok.exception import InvalidOperation, InvalidParameter > @@ -55,7 +56,11 @@ class NetworksModel(object): > self.caps = CapabilitiesModel(**kargs) > def _check_default_networks(self): > - networks = list(set(tmpl_defaults['networks'])) > + if os.uname()[4] in ['s390x']: This call os.uname()[4] in ['s390x'] Is used 4 times in this patch alone. Another similar verification is made in osinfo.py I believe this warrants a function is_running_in_s390x() in utils.py to avoid code repetition. > + networks = list(set(tmpl_defaults.get('networks', []))) > + else: > + networks = list(set(tmpl_defaults['networks'])) > + > conn = self.conn.get() > for net_name in networks: > diff --git a/model/templates.py b/model/templates.py > index a299c85..877bea1 100644 > --- a/model/templates.py > +++ b/model/templates.py > @@ -358,7 +358,10 @@ class LibvirtVMTemplate(VMTemplate): > return sorted(map(lambda x: x.decode('utf-8'), names)) > def _network_validate(self): > - names = self.info['networks'] > + if os.uname()[4] in ['s390x']: > + names = self.info.get('networks', []) > + else: > + names = self.info['networks'] > for name in names: > try: > conn = self.conn.get() > diff --git a/osinfo.py b/osinfo.py > index 2c59312..af37c72 100644 > --- a/osinfo.py > +++ b/osinfo.py > @@ -138,10 +138,22 @@ def _get_tmpl_defaults(): > 'pool': > '/plugins/kimchi/storagepools/default'}}, > 'processor': {'vcpus': '1', 'maxvcpus': 1}, > 'graphics': {'type': 'spice', 'listen': '127.0.0.1'}} > + > + The default values on s390x architecture: > + > + {'memory': {'current': 1024, 'maxmemory': 1024}, > + 'storage': { 'disk.0': {'format': 'qcow2', 'size': '10', > + 'pool': > '/plugins/kimchi/storagepools/default'}}, > + 'processor': {'vcpus': '1', 'maxvcpus': 1}, > + 'graphics': {'type': 'spice', 'listen': '127.0.0.1'}} > """ > # Create dict with default values > tmpl_defaults = defaultdict(dict) > - tmpl_defaults['main']['networks'] = ['default'] > + > + host_arch = _get_arch() > + if host_arch not in ['s390x']: > + tmpl_defaults['main']['networks'] = ['default'] > + > tmpl_defaults['memory'] = {'current': _get_default_template_mem(), > 'maxmemory': > _get_default_template_mem()} > tmpl_defaults['storage']['disk.0'] = {'size': 10, 'format': > 'qcow2', > diff --git a/vmtemplate.py b/vmtemplate.py > index dc81fe2..5a45123 100644 > --- a/vmtemplate.py > +++ b/vmtemplate.py > @@ -288,7 +288,13 @@ class VMTemplate(object): > networks = "" > params = {'type': 'network', > 'model': self.info['nic_model']} > - for nw in self.info['networks']: > + > + if os.uname()[4] in ['s390x']: > + info_networks = self.info.get('networks', []) > + else: > + info_networks = self.info['networks'] > + > + for nw in info_networks: > params['network'] = nw > networks += get_iface_xml(params, self.info['arch'], > self.info['os_distro'], > @@ -484,7 +490,12 @@ class VMTemplate(object): > def validate_integrity(self): > invalid = {} > # validate networks integrity > - invalid_networks = list(set(self.info['networks']) - > + if os.uname()[4] in ['s390x']: > + networks = self.info.get('networks', []) > + else: > + networks = self.info['networks'] > + > + invalid_networks = list(set(networks) - > set(self._get_all_networks_name())) > if invalid_networks: > invalid['networks'] = invalid_networks _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel