[PATCH 0/4] Improve VM CPU update code
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 | 3 ++- src/kimchi/mockmodel.py | 48 ++++++++++++++------------------------ src/kimchi/model/vms.py | 62 ++++++++++++++++++++++++++++++++++++++----------- tests/test_rest.py | 3 ++- 4 files changed, 71 insertions(+), 45 deletions(-) -- 2.4.3
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> --- 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 106e9bc..e3819dc 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -23,6 +23,7 @@ from lxml import etree, objectify import os import random import string +import threading import time import uuid from xml.etree import ElementTree @@ -75,6 +76,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): @@ -196,10 +200,16 @@ class VMModel(object): self.stats = {} def update(self, name, params): - dom = self.get_vm(name, self.conn) - dom = self._static_vm_update(dom, params) - self._live_vm_update(dom, params) - return dom.name().decode('utf-8') + 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) + dom = self._static_vm_update(dom, params) + self._live_vm_update(dom, params) + return dom.name().decode('utf-8') def clone(self, name): """Clone a virtual machine based on an existing one. -- 2.4.3
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> --- 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 aaf1af2..91278bd 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 e3819dc..c9ed5f6 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -60,8 +60,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" @@ -207,8 +207,8 @@ class VMModel(object): with lock: dom = self.get_vm(name, self.conn) - dom = self._static_vm_update(dom, params) self._live_vm_update(dom, params) + dom = self._static_vm_update(dom, params) return dom.name().decode('utf-8') def clone(self, name): @@ -731,8 +731,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) @@ -793,6 +792,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: -- 2.4.3
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> --- 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 c9ed5f6..374a5da 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -797,7 +797,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 -- 2.4.3
Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/i18n.py | 3 ++- src/kimchi/model/vms.py | 24 ++++++++++++++++++------ tests/test_rest.py | 3 ++- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index d2ffa34..c39fe96 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -75,7 +75,6 @@ messages = { "KCHVM0001E": _("Virtual machine %(name)s already exists"), "KCHVM0002E": _("Virtual machine %(name)s does not exist"), - "KCHVM0003E": _("Unable to rename virtual machine %(name)s. The name %(new_name)s is already in use or the virtual machine is not powered off."), "KCHVM0004E": _("Unable to retrieve screenshot for stopped virtual machine %(name)s"), "KCHVM0005E": _("Remote ISO image is not supported by this server."), "KCHVM0006E": _("Screenshot is not supported on virtual machine %(name)s"), @@ -118,6 +117,8 @@ messages = { "KCHVM0045E": _("There are not enough free slots of 1024 Mib in the guest."), "KCHVM0046E": _("Host's libvirt version does not support memory devices. Libvirt must be >= 1.2.14"), "KCHVM0047E": _("Error attaching memory device. Details: %(error)s"), + "KCHVM0048E": _("Unable to update the following parameters while the VM is offline: %(params)s"), + "KCHVM0049E": _("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 374a5da..a9159e8 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -61,9 +61,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' @@ -207,6 +212,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('KCHVM0048E', + {'params': ', '.join(ext_params)}) + else: + ext_params = set(params.keys()) - set(VM_ONLINE_UPDATE_PARAMS) + if len(ext_params) > 0: + raise InvalidParameter('KCHVM0049E', + {'params': ', '.join(ext_params)}) + self._live_vm_update(dom, params) dom = self._static_vm_update(dom, params) return dom.name().decode('utf-8') @@ -691,11 +708,6 @@ class VMModel(object): conn = self.conn.get() try: if 'name' in params: - state = DOM_STATE_MAP[dom.info()[0]] - if state != 'shutoff': - msg_args = {'name': vm_name, 'new_name': params['name']} - raise InvalidParameter("KCHVM0003E", msg_args) - lflags = libvirt.VIR_DOMAIN_SNAPSHOT_LIST_ROOTS dflags = (libvirt.VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | libvirt.VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) diff --git a/tests/test_rest.py b/tests/test_rest.py index c2d142f..945bf79 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"]) -- 2.4.3
Reviewed-by: Daniel Barboza <dhbarboza82@gmail.com> On 06/29/2015 02:21 PM, Crístian Deives wrote:
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 | 3 ++- src/kimchi/mockmodel.py | 48 ++++++++++++++------------------------ src/kimchi/model/vms.py | 62 ++++++++++++++++++++++++++++++++++++++----------- tests/test_rest.py | 3 ++- 4 files changed, 71 insertions(+), 45 deletions(-)
Sorry, this patch set got lost in the ML. Cristian, could you rebase and resend so I can apply it? Thanks, Aline Manera On 29/06/2015 14:21, Crístian Deives wrote:
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 | 3 ++- src/kimchi/mockmodel.py | 48 ++++++++++++++------------------------ src/kimchi/model/vms.py | 62 ++++++++++++++++++++++++++++++++++++++----------- tests/test_rest.py | 3 ++- 4 files changed, 71 insertions(+), 45 deletions(-)
participants (3)
-
Aline Manera -
Crístian Deives -
Daniel Henrique Barboza