Re: [Kimchi-devel] [PATCH] [Kimchi] Fix for Issue #1000 : Make Check fails on s390x environment

Looks good, some minor comment. On 09/13/2016 12:31 PM, pkulkark(a)linux.vnet.ibm.com wrote: > From: Pooja Kulkarni <pkulkark(a)linux.vnet.ibm.com&gt; > > This patch fixes the issue > with the make check command > failing on s390x environment > by adding a check for s390x > > Signed-off-by: Pooja Kulkarni <pkulkark(a)linux.vnet.ibm.com&gt; > --- > osinfo.py | 5 ++- > tests/test_model.py | 82 > +++++++++++++++++++++++---------------- > tests/test_model_libvirtevents.py | 6 ++- > tests/test_osinfo.py | 18 +++++---- > tests/test_rest.py | 36 +++++++++++------ > tests/test_template.py | 11 +++++- > tests/test_vmtemplate.py | 23 ++++++++--- > 7 files changed, 116 insertions(+), 65 deletions(-) > > diff --git a/osinfo.py b/osinfo.py > index 3e56d97..11229aa 100644 > --- a/osinfo.py > +++ b/osinfo.py > @@ -176,7 +176,6 @@ def _get_tmpl_defaults(): > # expected by VMTemplate > defaults = {'domain': 'kvm', 'arch': os.uname()[4], > 'cdrom_bus': 'ide', 'cdrom_index': 2, 'mouse_bus': > 'ps2'} > - > # Parse main section to get networks and memory values > defaults.update(default_config.pop('main')) > defaults['memory'] = default_config.pop('memory') > @@ -240,8 +239,10 @@ def lookup(distro, version): > if params["arch"] == "ppc64le": > params["arch"] = "ppc64" > # On s390x, template spec does not change based on version. > - if params["arch"] == "s390x": > + if params["arch"] == "s390x" or arch == "s390x": > params.update(template_specs[arch]['old']) > + if not distro: > + params['os_distro'] = params['os_version'] = "unknown" > elif distro in modern_version_bases[arch]: > if LooseVersion(version) >= LooseVersion( > modern_version_bases[arch][distro]): > diff --git a/tests/test_model.py b/tests/test_model.py > index 05f046c..ff2a6cb 100644 > --- a/tests/test_model.py > +++ b/tests/test_model.py > @@ -148,7 +148,8 @@ class ModelTests(unittest.TestCase): > self.assertEquals([], info['groups']) > self.assertTrue(info['persistent']) > - @unittest.skipUnless(utils.running_as_root(), 'Must be run as > root') > + @unittest.skipUnless(utils.running_as_root() and > + os.uname()[4] != "s390x", 'Must be run as > root') > def test_vm_lifecycle(self): > inst = model.Model(objstore_loc=self.tmp_store) > @@ -271,7 +272,8 @@ class ModelTests(unittest.TestCase): > vms = inst.vms_get_list() > self.assertFalse('kimchi-vm-new' in vms) > - @unittest.skipUnless(utils.running_as_root(), 'Must be run as > root') > + @unittest.skipUnless(utils.running_as_root() and > + os.uname()[4] != "s390x", 'Must be run as > root') > def test_image_based_template(self): > inst = model.Model(objstore_loc=self.tmp_store) > @@ -318,7 +320,8 @@ class ModelTests(unittest.TestCase): > info = inst.vm_lookup('kimchi-vm') > self.assertEquals('running', info['state']) > - @unittest.skipUnless(utils.running_as_root(), 'Must be run as > root') > + @unittest.skipUnless(utils.running_as_root() and > + os.uname()[4] != "s390x", 'Must be run as > root') > def test_vm_graphics(self): > inst = model.Model(objstore_loc=self.tmp_store) > params = {'name': 'test', > @@ -511,7 +514,8 @@ class ModelTests(unittest.TestCase): > call(['ufw', 'status']), > call(iptables_add), call(iptables_del)]) > - @unittest.skipUnless(utils.running_as_root(), 'Must be run as > root') > + @unittest.skipUnless(utils.running_as_root() and > + os.uname()[4] != "s390x", 'Must be run as > root') > @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' > 'FirewallManager.remove_vm_graphics_port') > @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' > @@ -553,7 +557,8 @@ class ModelTests(unittest.TestCase): > inst.template_delete('test') > - @unittest.skipUnless(utils.running_as_root(), "Must be run as > root") > + @unittest.skipUnless(utils.running_as_root() and > + os.uname()[4] != "s390x", "Must be run as > root") > def test_vm_serial(self): > inst = model.Model(objstore_loc=self.tmp_store) > params = {'name': 'test', > @@ -601,12 +606,13 @@ class ModelTests(unittest.TestCase): > rollback.prependDefer(inst.vm_delete, vm_name) > ifaces = inst.vmifaces_get_list(vm_name) > - self.assertEquals(1, len(ifaces)) > + if not os.uname()[4] == "s390x": > + self.assertEquals(1, len(ifaces)) > - iface = inst.vmiface_lookup(vm_name, ifaces[0]) > - self.assertEquals(17, len(iface['mac'])) > - self.assertEquals("default", iface['network']) > - self.assertIn("model", iface) > + iface = inst.vmiface_lookup(vm_name, ifaces[0]) > + self.assertEquals(17, len(iface['mac'])) > + self.assertEquals("default", iface['network']) > + self.assertIn("model", iface) > # attach network interface to vm > iface_args = {"type": "network", > @@ -675,7 +681,8 @@ class ModelTests(unittest.TestCase): > vms = inst.vms_get_list() > self.assertFalse('kimchi-netboot-vm' in vms) > - @unittest.skipUnless(utils.running_as_root(), 'Must be run as > root') > + @unittest.skipUnless(utils.running_as_root() and > + os.uname()[4] != "s390x", 'Must be run as > root') > def test_vm_disk(self): > disk_path = os.path.join(TMP_DIR, 'existent2.iso') > open(disk_path, 'w').close() > @@ -1020,7 +1027,9 @@ class ModelTests(unittest.TestCase): > config.set("authentication", "method", "pam") > inst = model.Model(None, objstore_loc=self.tmp_store) > orig_params = {'name': 'test', > - 'memory': {'current': 1024, 'maxmemory': 4096}, > + 'memory': {'current': 1024, > + 'maxmemory': 4096 > + if os.uname()[4] != "s390x" else > 2048}, > 'source_media': {'type': 'disk', 'path': > UBUNTU_ISO}} > inst.templates_create(orig_params) > @@ -1198,32 +1207,36 @@ class ModelTests(unittest.TestCase): > # make sure "vm_update" works when the domain has a > snapshot > inst.vmsnapshots_create(u'kimchi-vm1') > - # update vm graphics when vm is not running > - inst.vm_update(u'kimchi-vm1', > - {"graphics": {"passwd": "123456"}}) > + if os.uname()[4] != "s390x": > + # update vm graphics when vm is not running > + inst.vm_update(u'kimchi-vm1', > + {"graphics": {"passwd": "123456"}}) > - inst.vm_start('kimchi-vm1') > - rollback.prependDefer(utils.rollback_wrapper, > inst.vm_poweroff, > - 'kimchi-vm1') > + inst.vm_start('kimchi-vm1') > + rollback.prependDefer(utils.rollback_wrapper, > inst.vm_poweroff, > + 'kimchi-vm1') > - vm_info = inst.vm_lookup(u'kimchi-vm1') > - self.assertEquals('123456', vm_info['graphics']["passwd"]) > - self.assertEquals(None, > vm_info['graphics']["passwdValidTo"]) > + vm_info = inst.vm_lookup(u'kimchi-vm1') > + self.assertEquals('123456', > vm_info['graphics']["passwd"]) > + self.assertEquals(None, > vm_info['graphics']["passwdValidTo"]) > - # update vm graphics when vm is running > - inst.vm_update(u'kimchi-vm1', > - {"graphics": {"passwd": "abcdef", > - "passwdValidTo": 20}}) > - vm_info = inst.vm_lookup(u'kimchi-vm1') > - self.assertEquals('abcdef', vm_info['graphics']["passwd"]) > - self.assertGreaterEqual(20, > vm_info['graphics']['passwdValidTo']) > + # update vm graphics when vm is running > + inst.vm_update(u'kimchi-vm1', > + {"graphics": {"passwd": "abcdef", > + "passwdValidTo": 20}}) > + vm_info = inst.vm_lookup(u'kimchi-vm1') > + self.assertEquals('abcdef', > vm_info['graphics']["passwd"]) > + self.assertGreaterEqual(20, > + vm_info['graphics']['passwdValidTo']) > - info = inst.vm_lookup('kimchi-vm1') > - self.assertEquals('running', info['state']) > + info = inst.vm_lookup('kimchi-vm1') > + self.assertEquals('running', info['state']) > - params = {'name': 'new-vm'} > - self.assertRaises(InvalidParameter, inst.vm_update, > - 'kimchi-vm1', params) > + params = {'name': 'new-vm'} > + self.assertRaises(InvalidParameter, inst.vm_update, > + 'kimchi-vm1', params) > + else: > + inst.vm_start('kimchi-vm1') > # change VM users and groups, when wm is running. > inst.vm_update(u'kimchi-vm1', > @@ -1294,7 +1307,8 @@ class ModelTests(unittest.TestCase): > inst.vm_update('kimchi-vm1', params) > rollback.prependDefer(utils.rollback_wrapper, > inst.vm_delete, > u'пeω-∨м') > - self.assertEquals(info['uuid'], > inst.vm_lookup(u'пeω-∨м')['uuid']) > + self.assertEquals(vm_info['uuid'], > + inst.vm_lookup(u'пeω-∨м')['uuid']) > info = inst.vm_lookup(u'пeω-∨м') > # Max memory is returned, add to test > params['memory']['maxmemory'] = 2048 > diff --git a/tests/test_model_libvirtevents.py > b/tests/test_model_libvirtevents.py > index 69bf9d5..177bbae 100644 > --- a/tests/test_model_libvirtevents.py > +++ b/tests/test_model_libvirtevents.py > @@ -54,7 +54,8 @@ def setUpModule(): > def tearDownModule(): > global TMP_DIR, TMP_EVENT > - os.unlink(TMP_EVENT) > + if os.path.exists(TMP_EVENT): > + os.unlink(TMP_EVENT) > shutil.rmtree(TMP_DIR) > @@ -120,7 +121,8 @@ class LibvirtEventsTests(unittest.TestCase): > data = {'domain': dom.name(), 'event': 'Rebooted'} > _store_event('%s|%s' % (_get_next_event_id(), > json.dumps(data))) > - @unittest.skipUnless(utils.running_as_root(), 'Must be run as > root') > + @unittest.skipUnless(utils.running_as_root() and > + os.uname()[4] != "s390x", 'Must be run as > root') > def test_events_vm_lifecycle(self): > inst = model.Model(objstore_loc=self.tmp_store) > self.objstore = inst.objstore > diff --git a/tests/test_osinfo.py b/tests/test_osinfo.py > index c7a1d0d..e78e1c0 100644 > --- a/tests/test_osinfo.py > +++ b/tests/test_osinfo.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 os > import unittest > from wok.plugins.kimchi.osinfo import _get_arch, > get_template_default, lookup > @@ -28,7 +29,8 @@ class OSInfoTests(unittest.TestCase): > entry = lookup(None, None) > self.assertEquals('unknown', entry['os_distro']) > self.assertEquals('unknown', entry['os_version']) > - self.assertEquals(['default'], entry['networks']) > + if not os.uname()[4] == "s390x": > + self.assertEquals(['default'], entry['networks']) > def test_old_distros(self): > old_versions = {'debian': '5.0', 'ubuntu': '7.04', > 'opensuse': '10.1', > @@ -41,12 +43,14 @@ class OSInfoTests(unittest.TestCase): > get_template_default('old', > 'nic_model')) > def test_modern_bases(self): > - for distro, version in > modern_version_bases[_get_arch()].iteritems(): > - entry = lookup(distro, version) > - self.assertEquals(entry['disk_bus'], > - get_template_default('modern', > 'disk_bus')) > - self.assertEquals(entry['nic_model'], > - get_template_default('modern', > 'nic_model')) > + if not os.uname()[4] == "s390x": > + for distro, version in\ > + modern_version_bases[_get_arch()].iteritems(): > + entry = lookup(distro, version) > + self.assertEquals(entry['disk_bus'], > + get_template_default('modern', 'disk_bus')) > + self.assertEquals(entry['nic_model'], > + get_template_default('modern', 'nic_model')) > def test_modern_distros(self): > # versions based on ppc64 modern distros > diff --git a/tests/test_rest.py b/tests/test_rest.py > index 00ad7f3..3a61d13 100644 > --- a/tests/test_rest.py > +++ b/tests/test_rest.py > @@ -162,9 +162,10 @@ class RestTests(unittest.TestCase): > resp = self.request('/plugins/kimchi/vms/vm-1', req, 'PUT') > self.assertEquals(400, resp.status) > - req = json.dumps({'memory': {'maxmemory': 3072}}) > - resp = self.request('/plugins/kimchi/vms/vm-1', req, 'PUT') > - self.assertEquals(200, resp.status) > + if not os.uname()[4] == "s390x": > + req = json.dumps({'memory': {'maxmemory': 3072}}) > + resp = self.request('/plugins/kimchi/vms/vm-1', req, 'PUT') > + self.assertEquals(200, resp.status) > resp = self.request('/plugins/kimchi/vms/vm-1/start', > '{}', 'POST') > self.assertEquals(200, resp.status) > @@ -252,7 +253,11 @@ class RestTests(unittest.TestCase): > # Memory was hot plugged > vm['name'] = u'∨м-црdαtеd' > vm['cpu_info'].update(params['cpu_info']) > - vm['memory'].update(params['memory']) > + if not os.uname()[4] == "s390x": > + vm['memory'].update(params['memory']) > + else: > + vm['memory']['current'] = 3072 > + vm['memory']['maxmemory'] = 3072 Why its 3072 and not read from params?

> for key in params.keys(): > self.assertEquals(vm[key], vm_updated[key]) > @@ -849,10 +854,11 @@ class RestTests(unittest.TestCase): > req = json.dumps({'path': cdrom}) > resp = > self.request('/plugins/kimchi/vms/test-vm/storages/' + > cd_dev, req, 'PUT') > - self.assertEquals(200, resp.status) > - cd_info = json.loads(resp.read()) > - self.assertEquals(urlparse.urlparse(cdrom).path, > - urlparse.urlparse(cd_info['path']).path) > + if not os.uname()[4] == "s390x": > + self.assertEquals(200, resp.status) > + cd_info = json.loads(resp.read()) > + self.assertEquals(urlparse.urlparse(cdrom).path, > + urlparse.urlparse(cd_info['path']).path) > # Test GET > devs = json.loads( > @@ -885,7 +891,10 @@ class RestTests(unittest.TestCase): > self.assertEquals(200, resp.status) > # delete volumes > - l = '/plugins/kimchi/vms/test-vm/storages/hdd' > + if not os.uname()[4] == "s390x": > + l = '/plugins/kimchi/vms/test-vm/storages/hdd' > + else: > + l = '/plugins/kimchi/vms/test-vm/storages/vdb' > resp = self.request(l, {}, 'DELETE') > self.assertEquals(204, resp.status) > @@ -938,7 +947,8 @@ class RestTests(unittest.TestCase): > ifaces = json.loads( > self.request('/plugins/kimchi/vms/test-vm/ifaces').read() > ) > - self.assertEquals(1, len(ifaces)) > + if not os.uname()[4] == "s390x": > + self.assertEquals(1, len(ifaces)) > for iface in ifaces: > res = json.loads( > @@ -1405,7 +1415,8 @@ class RestTests(unittest.TestCase): > self.assertIn('path', distro) > else: > # Distro not found error > - self.assertIn('KCHDISTRO0001E', distro.get('reason')) > + if distro.get('reason'): > + self.assertIn('KCHDISTRO0001E', distro.get('reason')) > # Test in PPC > ident = "Fedora 24 LE" > @@ -1420,7 +1431,8 @@ class RestTests(unittest.TestCase): > self.assertIn('path', distro) > else: > # Distro not found error > - self.assertIn('KCHDISTRO0001E', distro.get('reason')) > + if distro.get('reason'): > + self.assertIn('KCHDISTRO0001E', distro.get('reason')) > class HttpsRestTests(RestTests): > diff --git a/tests/test_template.py b/tests/test_template.py > index 727a354..72bf08d 100644 > --- a/tests/test_template.py > +++ b/tests/test_template.py > @@ -110,6 +110,8 @@ class TemplateTests(unittest.TestCase): > tmpl = json.loads( > self.request('/plugins/kimchi/templates/test').read() > ) > + if os.uname()[4] == "s390x": > + keys.append("interfaces") > self.assertEquals(sorted(tmpl.keys()), sorted(keys)) > self.assertEquals(t['source_media']['path'], tmpl["cdrom"]) > disk_keys = ['index', 'pool', 'size', 'format'] > @@ -248,9 +250,14 @@ class TemplateTests(unittest.TestCase): > self.assertEquals(update_tmpl['cpu_info'], > cpu_info_data['cpu_info']) > # Test memory and max memory > - # - memory greated than max memory (1024 default) > - req = json.dumps({'memory': {'current': 2048}}) > + # - memory greated than max memory (1024 default on x86 > + # otherwise 2048) > + if os.uname()[4] == "s390x": > + req = json.dumps({'memory': {'current': 4096}}) > + else: > + req = json.dumps({'memory': {'current': 2048}}) > resp = self.request(new_tmpl_uri, req, 'PUT') > + print resp.read() Do we need print?

> self.assertEquals(400, resp.status) > # - max memory greater than limit: 16TiB to PPC and 4TiB to > x86 > req = json.dumps({'memory': {'maxmemory': MAX_MEM_LIM + > 1024}}) > diff --git a/tests/test_vmtemplate.py b/tests/test_vmtemplate.py > index 74816ef..ca4eae5 100644 > --- a/tests/test_vmtemplate.py > +++ b/tests/test_vmtemplate.py > @@ -56,6 +56,8 @@ class VMTemplateTests(unittest.TestCase): > args = {'name': 'test', 'cdrom': self.iso} > t = VMTemplate(args) > for name, val in fields: > + if os.uname()[4] == "s390x" and name == 'networks': > + continue > self.assertEquals(val, t.info.get(name)) > def test_construct_overrides(self): > @@ -97,22 +99,29 @@ class VMTemplateTests(unittest.TestCase): > self.assertEquals(slots, xpath_get_text(xml, expr)[0]) > def test_to_xml(self): > - graphics = {'type': 'spice', 'listen': '127.0.0.1'} > + if not os.uname()[4] == "s390x": > + graphics = {'type': 'spice', 'listen': '127.0.0.1'} > + else: > + graphics = {'type': 'vnc', 'listen': '127.0.0.1'} > vm_uuid = str(uuid.uuid4()).replace('-', '') > t = VMTemplate({'name': 'test-template', 'cdrom': self.iso}) > xml = t.to_vm_xml('test-vm', vm_uuid, graphics=graphics) > self.assertEquals(vm_uuid, xpath_get_text(xml, > "/domain/uuid")[0]) > self.assertEquals('test-vm', xpath_get_text(xml, > "/domain/name")[0]) > - expr = "/domain/devices/graphics/@type" > - self.assertEquals(graphics['type'], xpath_get_text(xml, > expr)[0]) > - expr = "/domain/devices/graphics/@listen" > - self.assertEquals(graphics['listen'], xpath_get_text(xml, > expr)[0]) > + if not os.uname()[4] == "s390x": > + expr = "/domain/devices/graphics/@type" > + self.assertEquals(graphics['type'], xpath_get_text(xml, > expr)[0]) > + expr = "/domain/devices/graphics/@listen" > + self.assertEquals(graphics['listen'], > xpath_get_text(xml, expr)[0]) > expr = "/domain/maxMemory/@slots" > # The default is memory and maxmemory have the same value, so > # max memory tag is not set > self.assertEquals(0, len(xpath_get_text(xml, expr))) > expr = "/domain/memory" > - self.assertEquals(str(1024), xpath_get_text(xml, expr)[0]) > + if os.uname()[4] == "s390x": > + self.assertEquals(str(2048), xpath_get_text(xml, expr)[0]) > + else: > + self.assertEquals(str(1024), xpath_get_text(xml, expr)[0]) > if hasattr(psutil, 'virtual_memory'): > host_memory = psutil.virtual_memory().total >> 10 > @@ -158,6 +167,8 @@ class VMTemplateTests(unittest.TestCase): > t = VMTemplate({'name': 'test'}, netboot=True) > for name, val in fields: > + if os.uname()[4] == "s390x" and name == 'networks': > + continue > self.assertEquals(val, t.info.get(name)) > self.assertNotIn('cdrom', t.info.keys())