[PATCH v2 0/4] Improve VM CPU update code
v2: - patch rebased This patchset refactors part of the code related to updating VM CPUs. I am aware of two tests which are failing with this patchset. They'll be worked on before the final version. Crístian Deives (4): Use locks to prevent concurrent updates to VMs Update VCPU by using libvirt function Set max VCPU count to the max supported value Check if the VM update params are valid for the current state src/kimchi/i18n.py | 4 +++- src/kimchi/mockmodel.py | 48 ++++++++++++++++------------------------- src/kimchi/model/vms.py | 57 ++++++++++++++++++++++++++++++++++++++++++------- tests/test_rest.py | 3 ++- 4 files changed, 72 insertions(+), 40 deletions(-) -- 1.9.1
From: Crístian Deives <cristiandeives@gmail.com> If several users try to update the same VM at the same time, an error may happen due to concurrent access. Define a dictionary containing one lock per each VM, and use it when updating the VM: the lock is acquired before the update operation begins and it is released when the operation finishes (whether by success or failure). Signed-off-by: Crístian Deives <cristiandeives@gmail.com> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 8463287..b579712 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -22,6 +22,7 @@ import lxml.etree as ET import os import random import string +import threading import time import uuid @@ -78,6 +79,9 @@ XPATH_DOMAIN_UUID = '/domain/uuid' XPATH_NUMA_CELL = './cpu/numa/cell' +# key: VM name; value: lock object +vm_locks = {} + class VMsModel(object): def __init__(self, **kargs): @@ -215,10 +219,16 @@ class VMModel(object): self.stats = {} def update(self, name, params): - dom = self.get_vm(name, self.conn) - vm_name, dom = self._static_vm_update(name, dom, params) - self._live_vm_update(dom, params) - return vm_name + lock = vm_locks.get(name) + if lock is None: + lock = threading.Lock() + vm_locks[name] = lock + + with lock: + dom = self.get_vm(name, self.conn) + vm_name, dom = self._static_vm_update(name, dom, params) + self._live_vm_update(dom, params) + return vm_name def clone(self, name): """Clone a virtual machine based on an existing one. -- 1.9.1
From: Crístian Deives <cristiandeives@gmail.com> Currently, the VCPU count is updated by editing the configuration XML (tag: <vcpu>). However, in some cases, editing that tag will only update the maximum VCPU count, not the current one. For example, if the VM has the following XML tag: <vcpu current='2'>8</vcpu> and the user updates the VCPU value to 10, Kimchi will update the XML tag to: <vcpu current='2'>10</vcpu> which is probably not what the user wanted. Use the libvirt function "setVcpusFlags" to update the current and the maximum VCPU values to the one specified by the user. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> Signed-off-by: Crístian Deives <cristiandeives@gmail.com> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 48 ++++++++++++++++++------------------------------ src/kimchi/model/vms.py | 23 ++++++++++++++++++----- 2 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 34bcfc9..3080868 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -25,6 +25,7 @@ import time import kimchi.model.cpuinfo +from collections import defaultdict from lxml import objectify from lxml.builder import E @@ -54,10 +55,9 @@ mockmodel_defaults = {'storagepool': '/storagepools/default-pool', class MockModel(Model): - _mock_vms = {} + _mock_vms = defaultdict(list) _mock_snapshots = {} _XMLDesc = libvirt.virDomain.XMLDesc - _defineXML = libvirt.virConnect.defineXML _undefineDomain = libvirt.virDomain.undefine _libvirt_get_vol_path = LibvirtVMTemplate._get_volume_path @@ -76,7 +76,6 @@ class MockModel(Model): kimchi.model.cpuinfo.get_topo_capabilities = \ MockModel.get_topo_capabilities - libvirt.virConnect.defineXML = MockModel.domainDefineXML libvirt.virDomain.XMLDesc = MockModel.domainXMLDesc libvirt.virDomain.undefine = MockModel.undefineDomain libvirt.virDomain.attachDeviceFlags = MockModel.attachDeviceFlags @@ -119,7 +118,7 @@ class MockModel(Model): imageinfo.probe_image = self._probe_image def reset(self): - MockModel._mock_vms = {} + MockModel._mock_vms = defaultdict(list) MockModel._mock_snapshots = {} self._mock_swupdate = MockSoftwareUpdate() self._mock_repositories = MockRepositories() @@ -154,21 +153,15 @@ class MockModel(Model): return ET.fromstring(xml) @staticmethod - def domainDefineXML(conn, xml): - name = objectify.fromstring(xml).name.text - try: - dom = conn.lookupByName(name) - if not dom.isActive(): - MockModel._mock_vms[name] = xml - except: - pass + def domainXMLDesc(dom, flags=0): + xml = MockModel._XMLDesc(dom, flags) + root = objectify.fromstring(xml) - return MockModel._defineXML(conn, xml) + for dev_xml in MockModel._mock_vms.get(dom.name(), []): + dev = objectify.fromstring(dev_xml) + root.devices.append(dev) - @staticmethod - def domainXMLDesc(dom, flags=0): - return MockModel._mock_vms.get(dom.name(), - MockModel._XMLDesc(dom, flags)) + return ET.tostring(root, encoding="utf-8") @staticmethod def undefineDomain(dom): @@ -179,12 +172,7 @@ class MockModel(Model): @staticmethod def attachDeviceFlags(dom, xml, flags=0): - old_xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_SECURE) - root = objectify.fromstring(old_xml) - dev = objectify.fromstring(xml) - root.devices.append(dev) - - MockModel._mock_vms[dom.name()] = ET.tostring(root, encoding="utf-8") + MockModel._mock_vms[dom.name()].append(xml) @staticmethod def _get_device_node(dom, xml): @@ -210,16 +198,16 @@ class MockModel(Model): @staticmethod def detachDeviceFlags(dom, xml, flags=0): - root, dev = MockModel._get_device_node(dom, xml) - root.devices.remove(dev) - - MockModel._mock_vms[dom.name()] = ET.tostring(root, encoding="utf-8") + if xml in MockModel._mock_vms[dom.name()]: + MockModel._mock_vms[dom.name()].remove(xml) @staticmethod def updateDeviceFlags(dom, xml, flags=0): - root, old_dev = MockModel._get_device_node(dom, xml) - root.devices.replace(old_dev, objectify.fromstring(xml)) - MockModel._mock_vms[dom.name()] = ET.tostring(root, encoding="utf-8") + _, old_dev = MockModel._get_device_node(dom, xml) + old_xml = ET.tostring(old_dev, encoding="utf-8") + if old_xml in MockModel._mock_vms[dom.name()]: + MockModel._mock_vms[dom.name()].remove(old_xml) + MockModel._mock_vms[dom.name()].append(xml) @staticmethod def volResize(vol, size, flags=0): diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index b579712..0dd8099 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -63,8 +63,8 @@ DOM_STATE_MAP = {0: 'nostate', 6: 'crashed', 7: 'pmsuspended'} -VM_STATIC_UPDATE_PARAMS = {'name': './name', - 'cpus': './vcpu'} +VM_STATIC_UPDATE_PARAMS = {'name': './name'} + VM_LIVE_UPDATE_PARAMS = {} XPATH_DOMAIN_DISK = "/domain/devices/disk[@device='disk']/source/@file" @@ -226,8 +226,8 @@ class VMModel(object): with lock: dom = self.get_vm(name, self.conn) - vm_name, dom = self._static_vm_update(name, dom, params) self._live_vm_update(dom, params) + vm_name, dom = self._static_vm_update(name, dom, params) return vm_name def clone(self, name): @@ -751,8 +751,7 @@ class VMModel(object): vcpus = params.get('cpus') if numa_mem == []: if vcpus is None: - vcpus = int(xpath_get_text(xml, - VM_STATIC_UPDATE_PARAMS['cpus'])[0]) + vcpus = int(xpath_get_text(xml, 'vcpu')[0]) cpu = root.find('./cpu') if cpu is None: cpu = get_cpu_xml(vcpus, params['memory'] << 10) @@ -813,6 +812,20 @@ class VMModel(object): if 'memory' in params and dom.isActive(): self._update_memory_live(dom, params) + if 'cpus' in params: + cpus = params['cpus'] + + try: + # set maximum VCPU count + dom.setVcpusFlags(cpus, libvirt.VIR_DOMAIN_AFFECT_CONFIG | + libvirt.VIR_DOMAIN_VCPU_MAXIMUM) + + # set current VCPU count + dom.setVcpusFlags(cpus, libvirt.VIR_DOMAIN_AFFECT_CONFIG) + except libvirt.libvirtError, e: + raise OperationFailed('KCHVM0008E', {'name': dom.name(), + 'err': e.message}) + def _update_memory_live(self, dom, params): # Check if host supports memory device if not self.caps.mem_hotplug_support: -- 1.9.1
From: Crístian Deives <cristiandeives@gmail.com> When updating the CPU count of a VM, its maximum CPU count is also updated to the same value. This wouldn't allow for CPU hotplug afterwards. Update the maximum CPU count to the maximum value supported by libvirt when the user updates the VM CPU count. Signed-off-by: Crístian Deives <cristiandeives@gmail.com> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 0dd8099..6e4579d 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -817,7 +817,8 @@ class VMModel(object): try: # set maximum VCPU count - dom.setVcpusFlags(cpus, libvirt.VIR_DOMAIN_AFFECT_CONFIG | + max_vcpus = self.conn.get().getMaxVcpus('kvm') + dom.setVcpusFlags(max_vcpus, libvirt.VIR_DOMAIN_AFFECT_CONFIG | libvirt.VIR_DOMAIN_VCPU_MAXIMUM) # set current VCPU count -- 1.9.1
From: Crístian Deives <cristiandeives@gmail.com> Signed-off-by: Crístian Deives <cristiandeives@gmail.com> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 2 ++ src/kimchi/model/vms.py | 19 ++++++++++++++++++- tests/test_rest.py | 3 ++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 1004aa4..5efeabe 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -122,6 +122,8 @@ messages = { "KCHVM0049E": _("Cannot power off %(name)s. Virtual machine is shut off."), "KCHVM0050E": _("Cannot shutdown %(name)s. Virtual machine is shut off."), "KCHVM0051E": _("Cannot reset %(name)s. Virtual machine is already shut off."), + "KCHVM0052E": _("Unable to update the following parameters while the VM is offline: %(params)s"), + "KCHVM0053E": _("Unable to update the following parameters while the VM is online: %(params)s"), "KCHVMHDEV0001E": _("VM %(vmid)s does not contain directly assigned host device %(dev_name)s."), "KCHVMHDEV0002E": _("The host device %(dev_name)s is not allowed to directly assign to VM."), diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 6e4579d..f4d15af 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -64,9 +64,14 @@ DOM_STATE_MAP = {0: 'nostate', 7: 'pmsuspended'} VM_STATIC_UPDATE_PARAMS = {'name': './name'} - VM_LIVE_UPDATE_PARAMS = {} +# update parameters which are updatable when the VM is online +VM_ONLINE_UPDATE_PARAMS = ['graphics', 'groups', 'memory', 'users'] +# update parameters which are updatable when the VM is offline +VM_OFFLINE_UPDATE_PARAMS = ['cpus', 'graphics', 'groups', 'memory', 'name', + 'users'] + XPATH_DOMAIN_DISK = "/domain/devices/disk[@device='disk']/source/@file" XPATH_DOMAIN_DISK_BY_FILE = "./devices/disk[@device='disk']/source[@file='%s']" XPATH_DOMAIN_NAME = '/domain/name' @@ -226,6 +231,18 @@ class VMModel(object): with lock: dom = self.get_vm(name, self.conn) + + if DOM_STATE_MAP[dom.info()[0]] == 'shutoff': + ext_params = set(params.keys()) - set(VM_OFFLINE_UPDATE_PARAMS) + if len(ext_params) > 0: + raise InvalidParameter('KCHVM0052E', + {'params': ', '.join(ext_params)}) + else: + ext_params = set(params.keys()) - set(VM_ONLINE_UPDATE_PARAMS) + if len(ext_params) > 0: + raise InvalidParameter('KCHVM0053E', + {'params': ', '.join(ext_params)}) + self._live_vm_update(dom, params) vm_name, dom = self._static_vm_update(name, dom, params) return vm_name diff --git a/tests/test_rest.py b/tests/test_rest.py index 7e726b0..0eb43f8 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -154,7 +154,7 @@ class RestTests(unittest.TestCase): req = json.dumps({'cpus': 3}) resp = self.request('/vms/vm-1', req, 'PUT') - self.assertEquals(200, resp.status) + self.assertEquals(400, resp.status) # Check if there is support to memory hotplug, once vm is running resp = self.request('/config/capabilities').read() @@ -168,6 +168,7 @@ class RestTests(unittest.TestCase): req = json.dumps({"graphics": {'passwd': "abcdef"}}) resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(200, resp.status) info = json.loads(resp.read()) self.assertEquals('abcdef', info["graphics"]["passwd"]) self.assertEquals(None, info["graphics"]["passwdValidTo"]) -- 1.9.1
participants (1)
-
Jose Ricardo Ziviani