[kimchi-devel][PATCH 0/2] Fix vcpu updating
From: Royce Lv <lvroyce@linux.vnet.ibm.com> vcpu validation need to be checked when both updating cpu topology and cpu count only. This patch fix this case and its tests. Royce Lv (2): Fix update vcpu count: Check available vcpu count before update Ignore cpu test case when exceed host cpu count src/kimchi/model/cpuinfo.py | 8 +++++-- src/kimchi/model/templates.py | 7 +++--- tests/test_model.py | 54 ++++++++++++++++++++++++++++--------------- tests/utils.py | 5 ++++ 4 files changed, 51 insertions(+), 23 deletions(-) -- 1.9.1
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Currently we just check vcpu count does not exceed host available count when topology specified. Actually vcpu count also need to be checked when only vcpu specified. Move this check outside topology check and add it to param update. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/cpuinfo.py | 8 ++++++-- src/kimchi/model/templates.py | 7 ++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/kimchi/model/cpuinfo.py b/src/kimchi/model/cpuinfo.py index 3411ef5..54bfad4 100644 --- a/src/kimchi/model/cpuinfo.py +++ b/src/kimchi/model/cpuinfo.py @@ -114,11 +114,15 @@ class CPUInfoModel(object): cores = topology['cores'] threads = topology['threads'] + self.check_vcpus(vcpus) + if not self.guest_threads_enabled: raise InvalidOperation("KCHCPUINF0003E") if vcpus != sockets * cores * threads: raise InvalidParameter("KCHCPUINF0002E") - if vcpus > self.cores_available * self.threads_per_core: - raise InvalidParameter("KCHCPUINF0001E") if threads > self.threads_per_core: raise InvalidParameter("KCHCPUINF0002E") + + def check_vcpus(self, vcpus): + if vcpus > self.cores_available * self.threads_per_core: + raise InvalidParameter("KCHCPUINF0001E") diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py index 85f0839..9f5b645 100644 --- a/src/kimchi/model/templates.py +++ b/src/kimchi/model/templates.py @@ -49,7 +49,7 @@ class TemplatesModel(object): {'filename': iso, 'user': user, 'err': excp}) - cpu_info = params.get('cpu_info') + cpu_info = params.setdefault('cpu_info', dict()) if cpu_info: topology = cpu_info.get('topology') # Check, even though currently only topology @@ -64,8 +64,9 @@ class TemplatesModel(object): # exception if a topology is invalid. CPUInfoModel(conn=self.conn).\ check_topology(params['cpus'], topology) - else: - params['cpu_info'] = dict() + if params.get('cpus'): + CPUInfoModel(conn=self.conn).check_vcpus(params['cpus']) + conn = self.conn.get() pool_uri = params.get(u'storagepool', '') -- 1.9.1
From: Royce Lv <lvroyce@linux.vnet.ibm.com> If vpu given exceed host cpu count, ignore corresponding test. Because test model does not support host cpu topology query, using real model instead of test model in cpu related tests. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 54 +++++++++++++++++++++++++++++++++++------------------ tests/utils.py | 5 +++++ 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index f80f1c9..c3f5595 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -622,7 +622,7 @@ class ModelTests(unittest.TestCase): @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_create(self): - inst = model.Model('test:///default', + inst = model.Model('qemu:///system', objstore_loc=self.tmp_store) # Test non-exist path raises InvalidParameter params = {'name': 'test', @@ -664,7 +664,7 @@ class ModelTests(unittest.TestCase): @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_integrity(self): - inst = model.Model('test:///default', + inst = model.Model('qemu:///system', objstore_loc=self.tmp_store) with RollbackContext() as rollback: @@ -723,6 +723,37 @@ class ModelTests(unittest.TestCase): for key in clone_temp.keys(): self.assertEquals(clone_temp[key], orig_temp[key]) + @unittest.skipUnless(utils.allow_vcpu_change(2), 'Host does not have enough CPUs') + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_template_update_vcpu(self): + params = {'name': 'new-test', 'memory': 512, 'cpus': 2} + inst.template_update('new-test', params) + rollback.prependDefer(inst.template_delete, 'new-test') + + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + + params = {'name': 'new-test', 'memory': 1024, 'cpus': 1, + 'networks': ['default', 'test-network', u'kīмсhī-пet']} + inst.template_update('new-test', params) + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + + # test cpu_info + # new-test has 1 cpu, so this should fail: + params['cpu_info'] = {"topology": + {"sockets": 1, "cores": 1, "threads": 2}} + self.assertRaises(InvalidParameter, inst.template_update, + 'new-test', params) + + params['cpus'] = 2 + inst.template_update('new-test', params) + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_update(self): inst = model.Model(None, @@ -754,7 +785,7 @@ class ModelTests(unittest.TestCase): self.assertEquals('new-test', inst.template_update('test', params)) self.assertRaises(NotFoundError, inst.template_delete, 'test') - params = {'name': 'new-test', 'memory': 512, 'cpus': 2} + params = {'name': 'new-test', 'memory': 512} inst.template_update('new-test', params) rollback.prependDefer(inst.template_delete, 'new-test') @@ -770,19 +801,6 @@ class ModelTests(unittest.TestCase): for key in params.keys(): self.assertEquals(params[key], info[key]) - # test cpu_info - # new-test has 1 cpu, so this should fail: - params['cpu_info'] = {"topology": - {"sockets": 1, "cores": 1, "threads": 2}} - self.assertRaises(InvalidParameter, inst.template_update, - 'new-test', params) - - params['cpus'] = 2 - inst.template_update('new-test', params) - info = inst.template_lookup('new-test') - for key in params.keys(): - self.assertEquals(params[key], info[key]) - # test update with non-existent network params = {'networks': ["no-exist"]} self.assertRaises(InvalidParameter, inst.template_update, @@ -798,10 +816,10 @@ class ModelTests(unittest.TestCase): def test_vm_edit(self): config.set("authentication", "method", "pam") - inst = model.Model(None, + inst = model.Model("qemu:///system", objstore_loc=self.tmp_store) - orig_params = {'name': 'test', 'memory': '1024', 'cpus': '1', + orig_params = {'name': 'test', 'memory': '1024', 'cpus': 1, 'cdrom': self.kimchi_iso} inst.templates_create(orig_params) diff --git a/tests/utils.py b/tests/utils.py index 2a8929f..7650433 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -23,6 +23,7 @@ import cherrypy import grp import httplib import json +import multiprocessing import os import socket import sys @@ -135,6 +136,10 @@ def running_as_root(): return os.geteuid() == 0 +def allow_vcpu_change(vcpu_cnt): + return multiprocessing.cpu_count() >= vcpu_cnt + + def _request(conn, path, data, method, headers): if headers is None: headers = {'Content-Type': 'application/json', -- 1.9.1
I can not apply it. Please, rebase and resend. I will apply only the first patch. On 09/03/2015 10:52, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
If vpu given exceed host cpu count, ignore corresponding test. Because test model does not support host cpu topology query, using real model instead of test model in cpu related tests.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 54 +++++++++++++++++++++++++++++++++++------------------ tests/utils.py | 5 +++++ 2 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/tests/test_model.py b/tests/test_model.py index f80f1c9..c3f5595 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -622,7 +622,7 @@ class ModelTests(unittest.TestCase):
@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_create(self): - inst = model.Model('test:///default', + inst = model.Model('qemu:///system', objstore_loc=self.tmp_store) # Test non-exist path raises InvalidParameter params = {'name': 'test', @@ -664,7 +664,7 @@ class ModelTests(unittest.TestCase):
@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_integrity(self): - inst = model.Model('test:///default', + inst = model.Model('qemu:///system', objstore_loc=self.tmp_store)
with RollbackContext() as rollback: @@ -723,6 +723,37 @@ class ModelTests(unittest.TestCase): for key in clone_temp.keys(): self.assertEquals(clone_temp[key], orig_temp[key])
+ @unittest.skipUnless(utils.allow_vcpu_change(2), 'Host does not have enough CPUs') + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_template_update_vcpu(self): + params = {'name': 'new-test', 'memory': 512, 'cpus': 2} + inst.template_update('new-test', params) + rollback.prependDefer(inst.template_delete, 'new-test') + + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + + params = {'name': 'new-test', 'memory': 1024, 'cpus': 1, + 'networks': ['default', 'test-network', u'kīмсhī-пet']} + inst.template_update('new-test', params) + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + + # test cpu_info + # new-test has 1 cpu, so this should fail: + params['cpu_info'] = {"topology": + {"sockets": 1, "cores": 1, "threads": 2}} + self.assertRaises(InvalidParameter, inst.template_update, + 'new-test', params) + + params['cpus'] = 2 + inst.template_update('new-test', params) + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_update(self): inst = model.Model(None, @@ -754,7 +785,7 @@ class ModelTests(unittest.TestCase): self.assertEquals('new-test', inst.template_update('test', params)) self.assertRaises(NotFoundError, inst.template_delete, 'test')
- params = {'name': 'new-test', 'memory': 512, 'cpus': 2} + params = {'name': 'new-test', 'memory': 512} inst.template_update('new-test', params) rollback.prependDefer(inst.template_delete, 'new-test')
@@ -770,19 +801,6 @@ class ModelTests(unittest.TestCase): for key in params.keys(): self.assertEquals(params[key], info[key])
- # test cpu_info - # new-test has 1 cpu, so this should fail: - params['cpu_info'] = {"topology": - {"sockets": 1, "cores": 1, "threads": 2}} - self.assertRaises(InvalidParameter, inst.template_update, - 'new-test', params) - - params['cpus'] = 2 - inst.template_update('new-test', params) - info = inst.template_lookup('new-test') - for key in params.keys(): - self.assertEquals(params[key], info[key]) - # test update with non-existent network params = {'networks': ["no-exist"]} self.assertRaises(InvalidParameter, inst.template_update, @@ -798,10 +816,10 @@ class ModelTests(unittest.TestCase):
def test_vm_edit(self): config.set("authentication", "method", "pam") - inst = model.Model(None, + inst = model.Model("qemu:///system", objstore_loc=self.tmp_store)
- orig_params = {'name': 'test', 'memory': '1024', 'cpus': '1', + orig_params = {'name': 'test', 'memory': '1024', 'cpus': 1, 'cdrom': self.kimchi_iso} inst.templates_create(orig_params)
diff --git a/tests/utils.py b/tests/utils.py index 2a8929f..7650433 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -23,6 +23,7 @@ import cherrypy import grp import httplib import json +import multiprocessing import os import socket import sys @@ -135,6 +136,10 @@ def running_as_root(): return os.geteuid() == 0
+def allow_vcpu_change(vcpu_cnt): + return multiprocessing.cpu_count() >= vcpu_cnt + + def _request(conn, path, data, method, headers): if headers is None: headers = {'Content-Type': 'application/json',
On 11/03/2015 11:26, Aline Manera wrote:
I can not apply it. Please, rebase and resend. I will apply only the first patch.
Ops... seems I can't apply only the first one. ====================================================================== ERROR: test_vm_edit (test_model.ModelTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "test_model.py", line 568, in test_vm_edit inst.templates_create(orig_params) File "/home/alinefm/kimchi/src/kimchi/model/templates.py", line 68, in create CPUInfoModel(conn=self.conn).check_vcpus(params['cpus']) File "/home/alinefm/kimchi/src/kimchi/model/cpuinfo.py", line 128, in check_vcpus raise InvalidParameter("KCHCPUINF0001E") InvalidParameter: KCHCPUINF0001E: The number of vCPUs is too large for this system. ----------------------------------------------------------------------
On 09/03/2015 10:52, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
If vpu given exceed host cpu count, ignore corresponding test. Because test model does not support host cpu topology query, using real model instead of test model in cpu related tests.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 54 +++++++++++++++++++++++++++++++++++------------------ tests/utils.py | 5 +++++ 2 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/tests/test_model.py b/tests/test_model.py index f80f1c9..c3f5595 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -622,7 +622,7 @@ class ModelTests(unittest.TestCase): @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_create(self): - inst = model.Model('test:///default', + inst = model.Model('qemu:///system', objstore_loc=self.tmp_store) # Test non-exist path raises InvalidParameter params = {'name': 'test', @@ -664,7 +664,7 @@ class ModelTests(unittest.TestCase): @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_integrity(self): - inst = model.Model('test:///default', + inst = model.Model('qemu:///system', objstore_loc=self.tmp_store) with RollbackContext() as rollback: @@ -723,6 +723,37 @@ class ModelTests(unittest.TestCase): for key in clone_temp.keys(): self.assertEquals(clone_temp[key], orig_temp[key]) + @unittest.skipUnless(utils.allow_vcpu_change(2), 'Host does not have enough CPUs') + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_template_update_vcpu(self): + params = {'name': 'new-test', 'memory': 512, 'cpus': 2} + inst.template_update('new-test', params) + rollback.prependDefer(inst.template_delete, 'new-test') + + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + + params = {'name': 'new-test', 'memory': 1024, 'cpus': 1, + 'networks': ['default', 'test-network', u'kīмсhī-пet']} + inst.template_update('new-test', params) + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + + # test cpu_info + # new-test has 1 cpu, so this should fail: + params['cpu_info'] = {"topology": + {"sockets": 1, "cores": 1, "threads": 2}} + self.assertRaises(InvalidParameter, inst.template_update, + 'new-test', params) + + params['cpus'] = 2 + inst.template_update('new-test', params) + info = inst.template_lookup('new-test') + for key in params.keys(): + self.assertEquals(params[key], info[key]) + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_update(self): inst = model.Model(None, @@ -754,7 +785,7 @@ class ModelTests(unittest.TestCase): self.assertEquals('new-test', inst.template_update('test', params)) self.assertRaises(NotFoundError, inst.template_delete, 'test') - params = {'name': 'new-test', 'memory': 512, 'cpus': 2} + params = {'name': 'new-test', 'memory': 512} inst.template_update('new-test', params) rollback.prependDefer(inst.template_delete, 'new-test') @@ -770,19 +801,6 @@ class ModelTests(unittest.TestCase): for key in params.keys(): self.assertEquals(params[key], info[key]) - # test cpu_info - # new-test has 1 cpu, so this should fail: - params['cpu_info'] = {"topology": - {"sockets": 1, "cores": 1, "threads": 2}} - self.assertRaises(InvalidParameter, inst.template_update, - 'new-test', params) - - params['cpus'] = 2 - inst.template_update('new-test', params) - info = inst.template_lookup('new-test') - for key in params.keys(): - self.assertEquals(params[key], info[key]) - # test update with non-existent network params = {'networks': ["no-exist"]} self.assertRaises(InvalidParameter, inst.template_update, @@ -798,10 +816,10 @@ class ModelTests(unittest.TestCase): def test_vm_edit(self): config.set("authentication", "method", "pam") - inst = model.Model(None, + inst = model.Model("qemu:///system", objstore_loc=self.tmp_store) - orig_params = {'name': 'test', 'memory': '1024', 'cpus': '1', + orig_params = {'name': 'test', 'memory': '1024', 'cpus': 1, 'cdrom': self.kimchi_iso} inst.templates_create(orig_params) diff --git a/tests/utils.py b/tests/utils.py index 2a8929f..7650433 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -23,6 +23,7 @@ import cherrypy import grp import httplib import json +import multiprocessing import os import socket import sys @@ -135,6 +136,10 @@ def running_as_root(): return os.geteuid() == 0 +def allow_vcpu_change(vcpu_cnt): + return multiprocessing.cpu_count() >= vcpu_cnt + + def _request(conn, path, data, method, headers): if headers is None: headers = {'Content-Type': 'application/json',
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 09/03/2015 10:52, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
vcpu validation need to be checked when both updating cpu topology and cpu count only. This patch fix this case and its tests.
Royce Lv (2): Fix update vcpu count: Check available vcpu count before update Ignore cpu test case when exceed host cpu count
src/kimchi/model/cpuinfo.py | 8 +++++-- src/kimchi/model/templates.py | 7 +++--- tests/test_model.py | 54 ++++++++++++++++++++++++++++--------------- tests/utils.py | 5 ++++ 4 files changed, 51 insertions(+), 23 deletions(-)
Reviewed-by: Daniel Barboza <dhbarboza82@gmail.com> On 03/09/2015 10:52 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
vcpu validation need to be checked when both updating cpu topology and cpu count only. This patch fix this case and its tests.
Royce Lv (2): Fix update vcpu count: Check available vcpu count before update Ignore cpu test case when exceed host cpu count
src/kimchi/model/cpuinfo.py | 8 +++++-- src/kimchi/model/templates.py | 7 +++--- tests/test_model.py | 54 ++++++++++++++++++++++++++++--------------- tests/utils.py | 5 ++++ 4 files changed, 51 insertions(+), 23 deletions(-)
participants (3)
-
Aline Manera -
Daniel Henrique Barboza -
lvroyce＠linux.vnet.ibm.com