Re: [Kimchi-devel] [PATCH] Update VCPU by using libvirt function

There is a problem that this patch is not fixing: If you try to edit a Guest (offline) and pass: a good CPU value (2), and a wrong Memory value (4324243224), you are going to receive an error about the memory. But, the CPU is going to be updated. Offline updates should be atomic. Rodrigo Trujillo On 11/26/2015 02:16 PM, Jose Ricardo Ziviani wrote: > From: Crístian Deives <cristiandeives(a)gmail.com&gt; > > 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> > > Use the libvirt function "setVcpusFlags" to update the current and the > maximum VCPU values to the one specified by the user. > > * This patch also changes vcpu setting when guest is being > created. Now 'current' is always set and is the total of cpus > from the Template and maxVcpu is the >= 255 or (sockets * cores > * threads) in PPC. In x86 it is the value of libvirt getMaxVcpu. > > * Change NUMA management. Sets all cpus to 0. There is not > impact to the guest. > > Signed-off-by: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; > Signed-off-by: Crístian Deives <cristiandeives(a)gmail.com&gt; > Signed-off-by: Jose Ricardo Ziviani <joserz(a)linux.vnet.ibm.com&gt; > Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo(a)linux.vnet.ibm.com&gt; > --- > src/wok/plugins/kimchi/i18n.py | 3 + > src/wok/plugins/kimchi/mockmodel.py | 51 +++++------ > src/wok/plugins/kimchi/model/vms.py | 144 > +++++++++++++++++++++++++++--- > src/wok/plugins/kimchi/tests/test_rest.py | 10 ++- > src/wok/plugins/kimchi/vmtemplate.py | 26 +++++- > 5 files changed, 186 insertions(+), 48 deletions(-) > > diff --git a/src/wok/plugins/kimchi/i18n.py > b/src/wok/plugins/kimchi/i18n.py > index de7962a..29a7222 100644 > --- a/src/wok/plugins/kimchi/i18n.py > +++ b/src/wok/plugins/kimchi/i18n.py > @@ -128,6 +128,9 @@ messages = { > "KCHVM0069E": _("Password field must be a string."), > "KCHVM0070E": _("Error creating local host ssh rsa key of user > 'root'."), > "KCHVM0071E": _("Memory value %(mem)s must be aligned to > %(alignment)sMiB."), > + "KCHVM0072E": _("Unable to update the following parameters while > the VM is offline: %(params)s"), > + "KCHVM0073E": _("Unable to update the following parameters while > the VM is online: %(params)s"), > + "KCHVM0074E": _("Cannot change VCPU value because '%(vm)s' has a > topology defined - sockets: %(sockets)s, cores: %(cores)s, threads: > %(threads)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/wok/plugins/kimchi/mockmodel.py > b/src/wok/plugins/kimchi/mockmodel.py > index 9186f78..cca38e5 100644 > --- a/src/wok/plugins/kimchi/mockmodel.py > +++ b/src/wok/plugins/kimchi/mockmodel.py > @@ -21,6 +21,8 @@ import libvirt > import lxml.etree as ET > import os > import time > + > +from collections import defaultdict > from lxml import objectify > from lxml.builder import E > @@ -60,10 +62,9 @@ storagevolumes.VALID_RAW_CONTENT = ['dos/mbr > boot sector', > 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 > @@ -81,7 +82,6 @@ class MockModel(Model): > cpuinfo.get_topo_capabilities = > MockModel.get_topo_capabilities > vmifaces.getDHCPLeases = MockModel.getDHCPLeases > - libvirt.virConnect.defineXML = MockModel.domainDefineXML > libvirt.virDomain.XMLDesc = MockModel.domainXMLDesc > libvirt.virDomain.undefine = MockModel.undefineDomain > libvirt.virDomain.attachDeviceFlags = > MockModel.attachDeviceFlags > @@ -124,7 +124,7 @@ class MockModel(Model): > imageinfo.probe_image = self._probe_image > def reset(self): > - MockModel._mock_vms = {} > + MockModel._mock_vms = defaultdict(list) > MockModel._mock_snapshots = {} > if hasattr(self, 'objstore'): > @@ -157,21 +157,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): > @@ -182,12 +176,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): > @@ -213,16 +202,18 @@ 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") > + node = ET.fromstring(xml) > + xml = ET.tostring(node, encoding="utf-8", pretty_print=True) > + 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", > pretty_print=True) > + 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/wok/plugins/kimchi/model/vms.py > b/src/wok/plugins/kimchi/model/vms.py > index 7e12b91..a67044d 100644 > --- a/src/wok/plugins/kimchi/model/vms.py > +++ b/src/wok/plugins/kimchi/model/vms.py > @@ -56,6 +56,7 @@ from wok.plugins.kimchi.model.utils import > get_ascii_nonascii_name, get_vm_name > from wok.plugins.kimchi.model.utils import get_metadata_node > from wok.plugins.kimchi.model.utils import remove_metadata_node > from wok.plugins.kimchi.model.utils import set_metadata_node > +from wok.plugins.kimchi.model.cpuinfo import CPUInfoModel > from wok.plugins.kimchi.screenshot import VMScreenshot > from wok.plugins.kimchi.utils import template_name_from_uri > from wok.plugins.kimchi.xmlutils.cpu import get_cpu_xml, get_numa_xml > @@ -71,10 +72,15 @@ 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 = {} > +# 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' > @@ -84,9 +90,12 @@ XPATH_DOMAIN_MAC_BY_ADDRESS = > "./devices/interface[@type='network']/"\ > XPATH_DOMAIN_MEMORY = '/domain/memory' > XPATH_DOMAIN_MEMORY_UNIT = '/domain/memory/@unit' > XPATH_DOMAIN_UUID = '/domain/uuid' > +XPATH_DOMAIN_DEV_CPU_ID = '/domain/devices/spapr-cpu-socket/@id' > XPATH_NUMA_CELL = './cpu/numa/cell' > +XPATH_TOPOLOGY = './cpu/topology' > + > # key: VM name; value: lock object > vm_locks = {} > @@ -98,6 +107,17 @@ class VMsModel(object): > self.caps = CapabilitiesModel(**kargs) > self.task = TaskModel(**kargs) > + def _get_host_maxcpu(self): > + if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']: > + cpu_model = CPUInfoModel(conn=self.conn) > + max_vcpu_val = (cpu_model.cores_available * > + cpu_model.threads_per_core) > + if max_vcpu_val > 255: > + max_vcpu_val = 255 > + else: > + max_vcpu_val = self.conn.get().getMaxVcpus('kvm') > + return max_vcpu_val > + > def create(self, params): > t_name = template_name_from_uri(params['template']) > vm_list = self.get_list() > @@ -163,7 +183,8 @@ class VMsModel(object): > xml = t.to_vm_xml(name, vm_uuid, > libvirt_stream_protocols=stream_protocols, > graphics=graphics, > - volumes=vol_list) > + volumes=vol_list, > + max_vcpus=self._get_host_maxcpu()) > cb('Defining new VM') > try: > @@ -231,6 +252,30 @@ class VMModel(object): > self.vmsnapshots = cls(**kargs) > self.stats = {} > + def has_topology(self, dom): > + xml = dom.XMLDesc(0) > + sockets = xpath_get_text(xml, XPATH_TOPOLOGY + '/@sockets') > + cores = xpath_get_text(xml, XPATH_TOPOLOGY + '/@cores') > + threads = xpath_get_text(xml, XPATH_TOPOLOGY + '/@threads') > + return sockets and cores and threads > + > + def get_vm_max_sockets(self, dom): > + return int(xpath_get_text(dom.XMLDesc(0), > + XPATH_TOPOLOGY + '/@sockets')[0]) > + > + def get_vm_sockets(self, dom): > + current_vcpu = > dom.vcpusFlags(libvirt.VIR_DOMAIN_AFFECT_CURRENT) > + return (current_vcpu / self.get_vm_cores(dom) / > + self.get_vm_threads(dom)) > + > + def get_vm_cores(self, dom): > + return int(xpath_get_text(dom.XMLDesc(0), > + XPATH_TOPOLOGY + '/@cores')[0]) > + > + def get_vm_threads(self, dom): > + return int(xpath_get_text(dom.XMLDesc(0), > + XPATH_TOPOLOGY + '/@threads')[0]) > + > def update(self, name, params): > lock = vm_locks.get(name) > if lock is None: > @@ -247,8 +292,20 @@ class VMModel(object): > 'alignment': > str(PPC_MEM_ALIGN)}) > dom = self.get_vm(name, self.conn) > - vm_name, dom = self._static_vm_update(name, dom, params) > + > + 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('KCHVM0072E', > + {'params': ', > '.join(ext_params)}) > + else: > + ext_params = set(params.keys()) - > set(VM_ONLINE_UPDATE_PARAMS) > + if len(ext_params) > 0: > + raise InvalidParameter('KCHVM0073E', > + {'params': ', > '.join(ext_params)}) > + > self._live_vm_update(dom, params) > + vm_name, dom = self._static_vm_update(name, dom, params) > return vm_name > def clone(self, name): > @@ -713,23 +770,34 @@ class VMModel(object): > params['name'], nonascii_name = > get_ascii_nonascii_name(name) > for key, val in params.items(): > + change_numa = True > if key in VM_STATIC_UPDATE_PARAMS: > if type(val) == int: > val = str(val) > xpath = VM_STATIC_UPDATE_PARAMS[key] > - new_xml = xml_item_update(new_xml, xpath, val) > + attrib = None > + if key == 'cpus': > + if self.has_topology(dom) or dom.isActive(): > + change_numa = False > + continue > + # Update maxvcpu firstly > + new_xml = xml_item_update(new_xml, xpath, > + str(self._get_host_maxcpu()), > + attrib) > + # Update current vcpu > + attrib = 'current' > + new_xml = xml_item_update(new_xml, xpath, val, attrib) > # Updating memory and NUMA if necessary, if vm is offline > if not dom.isActive(): > if 'memory' in params: > new_xml = self._update_memory_config(new_xml, params) > - elif 'cpus' in params and \ > + elif 'cpus' in params and change_numa and \ > (xpath_get_text(new_xml, XPATH_NUMA_CELL + > '/@memory') != []): > - vcpus = params['cpus'] > new_xml = xml_item_update( > new_xml, > XPATH_NUMA_CELL, > - value='0-' + str(vcpus - 1) if vcpus > 1 else '0', > + value='0', > attr='cpus') > if 'graphics' in params: > @@ -764,6 +832,8 @@ class VMModel(object): > raise OperationFailed("KCHVM0008E", {'name': vm_name, > 'err': > e.get_error_message()}) > + if name is not None: > + vm_name = name > return (nonascii_name if nonascii_name is not None else > vm_name, dom) > def _update_memory_config(self, xml, params): > @@ -775,21 +845,20 @@ 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) > + cpu = get_cpu_xml(0, params['memory'] << 10) > root.insert(0, ET.fromstring(cpu)) > else: > - numa_element = get_numa_xml(vcpus, params['memory'] > << 10) > + numa_element = get_numa_xml(0, params['memory'] << 10) > cpu.insert(0, ET.fromstring(numa_element)) > else: > if vcpus is not None: > xml = xml_item_update( > xml, > XPATH_NUMA_CELL, > - value='0-' + str(vcpus - 1) if vcpus > 1 else '0', > + value='0', > attr='cpus') > root = ET.fromstring(xml_item_update(xml, XPATH_NUMA_CELL, > str(params['memory'] << 10), > @@ -853,11 +922,60 @@ class VMModel(object): > return new_xml > return ET.tostring(root, encoding="utf-8") > + def _get_host_maxcpu(self): > + if os.uname()[4] in ['ppc', 'ppc64', 'ppc64le']: > + cpu_model = CPUInfoModel(conn=self.conn) > + max_vcpu_val = (cpu_model.cores_available * > + cpu_model.threads_per_core) > + if max_vcpu_val > 255: > + max_vcpu_val = 255 > + else: > + max_vcpu_val = self.conn.get().getMaxVcpus('kvm') > + return max_vcpu_val > + > def _live_vm_update(self, dom, params): > self._vm_update_access_metadata(dom, params) > if 'memory' in params and dom.isActive(): > self._update_memory_live(dom, params) > + if 'cpus' in params: > + cpus = params['cpus'] > + > + if DOM_STATE_MAP[dom.info()[0]] == 'shutoff': > + > + # user cannot change vcpu if topology is defined. In > this case > + # vcpu must always be sockets * cores * threads. > + current_vcpu = > dom.vcpusFlags(libvirt.VIR_DOMAIN_VCPU_MAXIMUM) > + > + if self.has_topology(dom): > + if current_vcpu != cpus: > + raise InvalidOperation('KCHVM0074E', > + {'vm': dom.name(), > + 'sockets': > + self.get_vm_max_sockets(), > + 'cores': > + self.get_vm_cores(), > + 'threads': > + self.get_vm_threads()}) > + > + # do not need to update vcpu if the value edit > did not > + # change > + else: > + return > + > + try: > + # set maximum VCPU count > + max_vcpus = self._get_host_maxcpu() > + dom.setVcpusFlags(max_vcpus, > + 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: > diff --git a/src/wok/plugins/kimchi/tests/test_rest.py > b/src/wok/plugins/kimchi/tests/test_rest.py > index 544f2e6..9fa8c8d 100644 > --- a/src/wok/plugins/kimchi/tests/test_rest.py > +++ b/src/wok/plugins/kimchi/tests/test_rest.py > @@ -144,6 +144,10 @@ class RestTests(unittest.TestCase): > vm = > json.loads(self.request('/plugins/kimchi/vms/vm-1').read()) > self.assertEquals('vm-1', vm['name']) > + req = json.dumps({'cpus': 3}) > + 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) > @@ -155,9 +159,10 @@ class RestTests(unittest.TestCase): > resp = self.request('/plugins/kimchi/vms/vm-1', req, 'PUT') > self.assertEquals(400, resp.status) > - req = json.dumps({'cpus': 3}) > + # Unable to do CPU hotplug > + req = json.dumps({'cpus': 5}) > resp = self.request('/plugins/kimchi/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('/plugins/kimchi/config/capabilities').read() > @@ -171,6 +176,7 @@ class RestTests(unittest.TestCase): > req = json.dumps({"graphics": {'passwd': "abcdef"}}) > resp = self.request('/plugins/kimchi/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"]) > diff --git a/src/wok/plugins/kimchi/vmtemplate.py > b/src/wok/plugins/kimchi/vmtemplate.py > index 3097b66..8d669db 100644 > --- a/src/wok/plugins/kimchi/vmtemplate.py > +++ b/src/wok/plugins/kimchi/vmtemplate.py > @@ -298,7 +298,7 @@ class VMTemplate(object): > cpu_info = self.info.get('cpu_info') > if cpu_info is not None: > cpu_topo = cpu_info.get('topology') > - return get_cpu_xml(self.info.get('cpus'), > + return get_cpu_xml(0, > self.info.get('memory') << 10, > cpu_topo) > @@ -311,7 +311,6 @@ class VMTemplate(object): > params['qemu-namespace'] = '' > params['cdroms'] = '' > params['qemu-stream-cmdline'] = '' > - params['cpu_info'] = self._get_cpu_xml() > params['disks'] = self._get_disks_xml(vm_uuid) > params['serial'] = get_serial_xml(params) > @@ -322,6 +321,8 @@ class VMTemplate(object): > libvirt_stream_protocols = > kwargs.get('libvirt_stream_protocols', []) > cdrom_xml = self._get_cdrom_xml(libvirt_stream_protocols) > + max_vcpus = kwargs.get('max_vcpus', 1) > + > if not urlparse.urlparse(self.info.get('cdrom', "")).scheme > in \ > libvirt_stream_protocols and \ > params.get('iso_stream', False): > @@ -344,6 +345,25 @@ class VMTemplate(object): > if distro == "IBM_PowerKVM": > params['slots'] = 32 > + cpu_topo = self.info.get('cpu_info').get('topology') > + if (cpu_topo is not None): > + sockets = int(max_vcpus / (cpu_topo['cores'] * > + cpu_topo['threads'])) > + self.info['cpu_info']['topology']['sockets'] = sockets > + > + # Reduce maxvcpu to fit number of sockets if necessary > + total_max_vcpu = sockets * cpu_topo['cores'] * > cpu_topo['threads'] > + if total_max_vcpu != max_vcpus: > + max_vcpus = total_max_vcpu > + > + params['vcpus'] = "<vcpu current='%s'>%d</vcpu>" % \ > + (params['cpus'], max_vcpus) > + else: > + params['vcpus'] = "<vcpu current='%s'>%d</vcpu>" % \ > + (params['cpus'], max_vcpus) > + > + params['cpu_info'] = self._get_cpu_xml() > + > xml = """ > <domain type='%(domain)s'> > %(qemu-stream-cmdline)s > @@ -351,7 +371,7 @@ class VMTemplate(object): > <uuid>%(uuid)s</uuid> > <maxMemory slots='%(slots)s' > unit='KiB'>%(max_memory)s</maxMemory> > <memory unit='MiB'>%(memory)s</memory> > - <vcpu>%(cpus)s</vcpu> > + %(vcpus)s > %(cpu_info)s > <os> > <type arch='%(arch)s'>hvm</type> _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel