Re: [Kimchi-devel] [PATCH][Kimchi 1/3] Add support to edit max memory in Templates

On 02/16/2016 10:43 AM, Paulo Ricardo Paz Vital wrote: > > On 02/16/2016 12:32 AM, Rodrigo Trujillo wrote: >> This patch changes the 'memory' parameter in API of Templates to: >> memory: {current: XXX, maxmemory: YYY} >> Other changes include: >> * enable maxmemory edition >> * remove max memory tests and limits >> * keeps max memory limit to 1TiB >> * changes templates.conf to suport max memory >> * set default memory and maxmemory to 1024 >> >> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo(a)linux.vnet.ibm.com&gt; >> --- >> API.json | 44 ++++++++++++++++++++++++++------ >> docs/API.md | 18 ++++++++++--- >> i18n.py | 11 ++++++-- >> model/templates.py | 58 +++++++++++++++++++++++++++++++++++++++--- >> model/vms.py | 10 ++++++-- >> osinfo.py | 16 ++++++++---- >> template.conf | 10 +++++--- >> vmtemplate.py | 74 >> ++++++++++++++++++++++-------------------------------- >> 8 files changed, 169 insertions(+), 72 deletions(-) >> >> diff --git a/API.json b/API.json >> index 294be64..fcde123 100644 >> --- a/API.json >> +++ b/API.json >> @@ -453,10 +453,24 @@ >> "error": "KCHTMPL0011E" >> }, >> "memory": { >> - "description": "Memory (MB) for the template", >> - "type": "integer", >> - "minimum": 512, >> - "error": "KCHTMPL0013E" >> + "description": "Current memory and maximum >> memory values", >> + "type": "object", >> + "properties": { >> + "current": { >> + "description": "Memory (MB) for the >> template", >> + "type": "integer", >> + "minimum": 512, >> + "error": "KCHTMPL0013E" >> + }, >> + "maxmemory": { >> + "description": "Maximum memory (MB) for >> the template", >> + "type": "integer", >> + "minimum": 512, >> + "error": "KCHTMPL0013E" >> + } >> + }, >> + "additionalProperties": false, >> + "error": "KCHTMPL0030E" >> }, >> "cdrom": { >> "description": "Path for cdrom", >> @@ -630,10 +644,24 @@ >> "error": "KCHTMPL0011E" >> }, >> "memory": { >> - "description": "Memory (MB) for the template", >> - "type": "integer", >> - "minimum": 512, >> - "error": "KCHTMPL0013E" >> + "description": "Current memory and maximum >> memory values", >> + "type": "object", >> + "properties": { >> + "current": { >> + "description": "Memory (MB) for the >> template", >> + "type": "integer", >> + "minimum": 512, >> + "error": "KCHTMPL0013E" >> + }, >> + "maxmemory": { >> + "description": "Maximum memory (MB) for >> the template", >> + "type": "integer", >> + "minimum": 512, >> + "error": "KCHTMPL0013E" >> + } >> + }, >> + "additionalProperties": false, >> + "error": "KCHTMPL0030E" >> }, >> "cdrom": { >> "description": "Path for cdrom", >> diff --git a/docs/API.md b/docs/API.md >> index a46e80e..33d51ac 100644 >> --- a/docs/API.md >> +++ b/docs/API.md >> @@ -292,8 +292,11 @@ Represents a snapshot of the Virtual Machine's >> primary monitor. >> * name: The name of the Template. Used to identify the >> Template in this API >> * os_distro *(optional)*: The operating system distribution >> * os_version *(optional)*: The version of the operating system >> distribution >> - * memory *(optional)*: The amount of memory assigned to the VM. >> - Default is 1024M. >> + * memory *(optional)*: The memory parameters of the template, >> specify one >> + or both. Default values are 1024MiB: >> + * current: The amount of memory that will be assigned to >> the VM. >> + * maxmemory: The maximum total of memory that the VM can >> have. Amount >> + over current will be used exclusively for memory hotplug >> * cdrom *(optional)*: A volume name or URI to an ISO image. >> * networks *(optional)*: list of networks will be assigned to >> the new VM. >> Default is '[default]' >> @@ -396,7 +399,11 @@ A interface represents available network >> interface on VM. >> * icon: A URI to a PNG image representing this template >> * os_distro: The operating system distribution >> * os_version: The version of the operating system distribution >> - * memory: The amount of memory assigned to the VM in the unit >> of MB >> + * memory: The memory parameters of the template, that will be >> assigned to >> + the VM in the unit of MiB. >> + * current: The amount of memory that will be assigned to >> the VM. >> + * maxmemory: The maximum total of memory that the VM can >> have. Amount >> + over current will be used exclusively for memory hotplug >> * cdrom: A volume name or URI to an ISO image >> * storagepool: URI of the storagepool where template allocates >> vm storage. >> * networks *(optional)*: list of networks will be assigned to >> the new VM. >> @@ -439,7 +446,10 @@ A interface represents available network >> interface on VM. >> * icon: A URI to a PNG image representing this template >> * os_distro: The operating system distribution >> * os_version: The version of the operating system distribution >> - * memory: The amount of memory assigned to the VM >> + * memory: The memory parameters of the template, specify one or >> both of: >> + * current: The amount of memory that will be assigned to >> the VM. >> + * maxmemory: The maximum total of memory that the VM can >> have. Amount >> + over current will be used exclusively for memory hotplug >> * cdrom: A volume name or URI to an ISO image >> * networks *(optional)*: list of networks will be assigned to >> the new VM. >> * disks: An array of requested disks with the following >> optional fields >> diff --git a/i18n.py b/i18n.py >> index 59e75f7..5395879 100644 >> --- a/i18n.py >> +++ b/i18n.py >> @@ -127,9 +127,14 @@ messages = { >> "KCHVM0068E": _("Unable to setup password-less login at remote >> host %(host)s using user %(user)s. Error: %(error)s"), >> "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."), >> + "KCHVM0071E": _("%(param)s value (%(mem)sMiB) must be aligned >> to %(alignment)sMiB."), >> "KCHVM0073E": _("Unable to update the following parameters >> while the VM is offline: %(params)s"), >> "KCHVM0074E": _("Unable to update the following parameters >> while the VM is online: %(params)s"), >> + "KCHVM0075E": _("Maximum memory requested is higher than amount >> supported by the host: %(memHost)sMiB."), >> + "KCHVM0076E": _("Maximum memory requested is higher than >> maximum amount recommended: 1TiB"), >> + "KCHVM0077E": _("Cannot update maximum memory when guest is >> running."), > KCHVM0076E and KCHVM0077E already exist and were not modified. See below > >> + "KCHVM0078E": _("Maximum memory requested is higher than amount >> supported by the host: %(memHost)sMiB."), >> + "KCHVM0079E": _("Memory requested is higher than maximum amount >> recommended: 1TiB"), >> >> "KCHVM0076E": _("VM %(name)s must have serial and console >> defined to open a web serial console"), >> "KCHVM0077E": _("Impossible to get the serial console of >> %(name)s"), > They are here ^^^ ACK > >> @@ -165,7 +170,7 @@ messages = { >> "KCHTMPL0010E": _("Template distribution must be a string"), >> "KCHTMPL0011E": _("Template distribution version must be a >> string"), >> "KCHTMPL0012E": _("The number of CPUs must be an integer >> greater than 0"), >> - "KCHTMPL0013E": _("Amount of memory (MB) must be an integer >> greater than 512"), >> + "KCHTMPL0013E": _("Amount of memory and maximum memory (MB) >> must be an integer greater than 512"), >> "KCHTMPL0014E": _("Template CDROM must be a local or remote >> ISO file"), >> "KCHTMPL0015E": _("Invalid storage pool URI %(value)s >> specified for template"), >> "KCHTMPL0016E": _("Specify an ISO image as CDROM or a base >> image to create a template"), >> @@ -181,6 +186,8 @@ messages = { >> "KCHTMPL0027E": _("Invalid disk image format. Valid formats: >> bochs, cloop, cow, dmg, qcow, qcow2, qed, raw, vmdk, vpc."), >> "KCHTMPL0028E": _("When setting template disks, following >> parameters are required: 'index', 'pool name', 'format', 'size' or >> 'volume' (for scsi/iscsi pools)"), >> "KCHTMPL0029E": _("Disk format must be 'raw', for logical, >> iscsi, and scsi pools."), >> + "KCHTMPL0030E": _("Memory expects an object with one or both >> parameters: 'current' and 'maxmemory'"), >> + "KCHTMPL0031E": _("Memory value (%(mem)sMiB) must be equal or >> lesser than maximum memory value (%(maxmem)sMiB)"), >> >> "KCHPOOL0001E": _("Storage pool %(name)s already exists"), >> "KCHPOOL0002E": _("Storage pool %(name)s does not exist"), >> diff --git a/model/templates.py b/model/templates.py >> index 8a29e02..6138592 100644 >> --- a/model/templates.py >> +++ b/model/templates.py >> @@ -20,6 +20,8 @@ >> import copy >> import libvirt >> import os >> +import platform >> +import psutil >> import stat >> >> from wok.exception import InvalidOperation, InvalidParameter >> @@ -34,6 +36,12 @@ from wok.plugins.kimchi.utils import >> pool_name_from_uri >> from wok.plugins.kimchi.vmtemplate import VMTemplate >> >> >> +# In PowerPC, memories must be aligned to 256 MiB >> +PPC_MEM_ALIGN = 256 >> +# Max memory 1TB, in KiB >> +MAX_MEM_LIM = 1073741824 >> + >> + >> class TemplatesModel(object): >> def __init__(self, **kargs): >> self.objstore = kargs['objstore'] >> @@ -69,10 +77,8 @@ class TemplatesModel(object): >> # Validate cpu info >> t.cpuinfo_validate() >> >> - # Validate max memory >> - maxMem = (t._get_max_memory(t.info.get('memory')) >> 10) >> - if t.info.get('memory') > maxMem: >> - raise OperationFailed("KCHVM0041E", {'maxmem': >> str(maxMem)}) >> + # Validate memory >> + self._validate_memory(t.info.get('memory')) >> >> # Validate volumes >> for disk in t.info.get('disks'): >> @@ -115,6 +121,44 @@ class TemplatesModel(object): >> raise InvalidParameter("KCHTMPL0019E", {'pool': >> pool_name, >> 'volume': volume}) >> >> + def _validate_memory(self, memory): >> + current = memory.get('current') >> + maxmem = memory.get('maxmemory') >> + >> + # Memories must be lesser than 1TB and the Host memory limit >> + if maxmem > (MAX_MEM_LIM >> 10): >> + raise InvalidParameter("KCHVM0076E") >> + if current > (MAX_MEM_LIM >> 10): >> + raise InvalidParameter("KCHVM0079E") >> + >> + if hasattr(psutil, 'virtual_memory'): >> + host_memory = psutil.virtual_memory().total >> 10 >> 10 >> + else: >> + host_memory = psutil.TOTAL_PHYMEM >> 10 >> 10 >> + if maxmem > host_memory: >> + raise InvalidParameter("KCHVM0075E", {'memHost': >> host_memory}) >> + if current > host_memory: >> + raise InvalidParameter("KCHVM0078E", {'memHost': >> host_memory}) >> + >> + if current > maxmem: >> + raise InvalidParameter("KCHTMPL0031E", >> + {'mem': str(current), >> + 'maxmem': str(maxmem)}) >> + >> + # make sure memory and Maxmemory are alingned in 256MiB in >> PowerPC >> + distro, _, _ = platform.linux_distribution() >> + if distro == "IBM_PowerKVM": >> + if current % PPC_MEM_ALIGN != 0: >> + raise InvalidParameter('KCHVM0071E', >> + {'param': "Memory", >> + 'mem': str(current), >> + 'alignment': >> str(PPC_MEM_ALIGN)}) >> + elif maxmem % PPC_MEM_ALIGN != 0: >> + raise InvalidParameter('KCHVM0071E', >> + {'param': "Maximum Memory", >> + 'mem': str(maxmem), >> + 'alignment': >> str(PPC_MEM_ALIGN)}) >> + >> >> class TemplateModel(object): >> def __init__(self, **kargs): >> @@ -180,6 +224,12 @@ class TemplateModel(object): >> cpu_info.update(new_cpu_info) >> params['cpu_info'] = cpu_info >> >> + # Fix memory values, because method update does not work >> recursively >> + new_mem = params.get('memory') >> + if new_mem: >> + params['memory'] = copy.copy(old_t.get('memory')) >> + params['memory'].update(new_mem) >> + >> new_t.update(params) >> >> for net_name in params.get(u'networks', []): >> diff --git a/model/vms.py b/model/vms.py >> index 23e0df9..400ef46 100644 >> --- a/model/vms.py >> +++ b/model/vms.py >> @@ -53,13 +53,13 @@ from wok.plugins.kimchi.model.config import >> CapabilitiesModel >> from wok.plugins.kimchi.model.cpuinfo import CPUInfoModel >> from wok.plugins.kimchi.model.featuretests import FeatureTests >> from wok.plugins.kimchi.model.templates import TemplateModel >> +from wok.plugins.kimchi.model.templates import MAX_MEM_LIM, >> PPC_MEM_ALIGN >> 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.screenshot import VMScreenshot >> from wok.plugins.kimchi.utils import template_name_from_uri >> -from wok.plugins.kimchi.vmtemplate import MAX_MEM_LIM, PPC_MEM_ALIGN >> from wok.plugins.kimchi.xmlutils.cpu import get_cpu_xml, get_numa_xml >> from wok.plugins.kimchi.xmlutils.cpu import get_topology_xml >> from wok.plugins.kimchi.xmlutils.disk import get_vm_disk_info, >> get_vm_disks >> @@ -256,7 +256,13 @@ class VMModel(object): >> if 'memory' in params and distro == "IBM_PowerKVM": >> if params['memory'] % PPC_MEM_ALIGN != 0: >> raise InvalidParameter('KCHVM0071E', >> - {'mem': >> str(params['memory']), >> + {'param': "Memory", >> + 'mem': str(memory), >> + 'alignment': >> str(PPC_MEM_ALIGN)}) >> + elif maxmem % PPC_MEM_ALIGN != 0: >> + raise InvalidParameter('KCHVM0071E', >> + {'param': "Maximum Memory", >> + 'mem': str(maxmem), >> 'alignment': >> str(PPC_MEM_ALIGN)}) >> >> dom = self.get_vm(name, self.conn) >> diff --git a/osinfo.py b/osinfo.py >> index 2ec5c3e..8102548 100644 >> --- a/osinfo.py >> +++ b/osinfo.py >> @@ -110,11 +110,13 @@ def _get_tmpl_defaults(): >> ConfigObj returns a dict like below when no changes were made >> in the >> template configuration file (template.conf) >> >> - {'main': {}, 'storage': {'disk.0': {}}, 'processor': {}, >> 'graphics': {}} >> + {'main': {}, 'memory': {}, 'storage': {'disk.0': {}}, >> 'processor': {}, >> + 'graphics': {}} >> >> The default values should be like below: >> >> - {'main': {'networks': ['default'], 'memory': '1024'}, >> + {'main': {'networks': ['default']}, >> + 'memory': {'current': 1024, 'maxmemory': 1024}, >> 'storage': { 'disk.0': {'format': 'qcow2', 'size': '10', >> 'pool': >> '/plugins/kimchi/storagepools/default'}}, >> 'processor': {'vcpus': '1', 'maxvcpus': 1}, >> @@ -123,7 +125,8 @@ def _get_tmpl_defaults(): >> # Create dict with default values >> tmpl_defaults = defaultdict(dict) >> tmpl_defaults['main']['networks'] = ['default'] >> - tmpl_defaults['main']['memory'] = _get_default_template_mem() >> + tmpl_defaults['memory'] = {'current': _get_default_template_mem(), >> + 'maxmemory': >> _get_default_template_mem()} >> tmpl_defaults['storage']['disk.0'] = {'size': 10, 'format': >> 'qcow2', >> 'pool': 'default'} >> tmpl_defaults['processor']['vcpus'] = 1 >> @@ -145,8 +148,11 @@ def _get_tmpl_defaults(): >> 'cdrom_bus': 'ide', 'cdrom_index': 2, 'mouse_bus': >> 'ps2'} >> >> # Parse main section to get networks and memory values >> - main_section = default_config.pop('main') >> - defaults.update(main_section) >> + defaults.update(default_config.pop('main')) >> + defaults['memory'] = default_config.pop('memory') >> + >> + defaults['memory']['current'] = int(defaults['memory']['current']) >> + defaults['memory']['maxmemory'] = >> int(defaults['memory']['maxmemory']) >> >> # Parse storage section to get disks values >> storage_section = default_config.pop('storage') >> diff --git a/template.conf b/template.conf >> index 3839be4..c4598f1 100644 >> --- a/template.conf >> +++ b/template.conf >> @@ -3,13 +3,17 @@ >> # >> >> [main] >> -# Memory in MB >> -#memory = 1024 >> - >> # List of networks separated by comma >> # Represents the virtual network interfaces to be assigned to guest >> #networks = default, >> >> +[memory] >> +# Memory in MB >> +# current = 1024 >> + >> +# Maximum value of memory to be assigned to guest in MB >> +# maxmemory = 1024 >> + >> [storage] >> >> # Specify multiple [[disk.X]] sub-sections to add multiples disks >> to guest >> diff --git a/vmtemplate.py b/vmtemplate.py >> index ef17ff6..ab6d580 100644 >> --- a/vmtemplate.py >> +++ b/vmtemplate.py >> @@ -19,7 +19,6 @@ >> >> import os >> import platform >> -import psutil >> import stat >> import time >> import urlparse >> @@ -42,12 +41,6 @@ from wok.plugins.kimchi.xmlutils.qemucmdline >> import get_qemucmdline_xml >> from wok.plugins.kimchi.xmlutils.serial import get_serial_xml >> >> >> -# In PowerPC, memories must be aligned to 256 MiB >> -PPC_MEM_ALIGN = 256 >> -# Max memory 1TB, in KiB >> -MAX_MEM_LIM = 1073741824 >> - >> - >> class VMTemplate(object): >> def __init__(self, args, scan=False): >> """ >> @@ -84,6 +77,14 @@ class VMTemplate(object): >> args['graphics'] = graphics >> >> default_disk = self.info['disks'][0] >> + >> + # Complete memory args, because dict method update is not >> recursive >> + if 'memory' in args: >> + if 'current' not in args['memory']: > After apply your patch and execute a GET on templates, to list my > current templates, I got the following error: > > File > "/home/pvital/Projects/tmp/wok_test/src/wok/plugins/kimchi/vmtemplate.py", > > line 83, in __init__ > if 'current' not in args['memory']: > TypeError: argument of type 'int' is not iterable > > > the line executed was: > > $ curl -k -u test -H "Content-Type: application/json" -H "Accept: > application/json" 'https://localhost:8001/plugins/kimchi/templates' > -X GET > > So, what happens if I already have templates in the old model (the > current one today) after I apply your patches? I don't have the new > structure of memory. Yes, templates must be updated. I will add a script to do this. Thanks

>> + args['memory']['current'] = self.info['memory']['current'] >> + if 'maxmemory' not in args['memory']: >> + args['memory']['maxmemory'] = >> self.info['memory']['maxmemory'] >> + >> # Override template values according to 'args' >> self.info.update(args) >> disks = self.info.get('disks') >> @@ -325,29 +326,8 @@ class VMTemplate(object): >> def _get_cpu_xml(self): >> # Include CPU topology, if provided >> cpu_topo = self.info.get('cpu_info', {}).get('topology', {}) >> - >> - return get_cpu_xml(0, self.info.get('memory') << 10, cpu_topo) >> - >> - def _get_max_memory(self, guest_memory): >> - # Setting maxMemory of the VM, which will be lesser value >> between: >> - # 1TB, (Template Memory * 4), Host Physical Memory. >> - max_memory = MAX_MEM_LIM >> - if hasattr(psutil, 'virtual_memory'): >> - host_memory = psutil.virtual_memory().total >> 10 >> - else: >> - host_memory = psutil.TOTAL_PHYMEM >> 10 >> - if host_memory < max_memory: >> - max_memory = host_memory >> - if (((guest_memory * 4) << 10) < max_memory): >> - max_memory = (guest_memory * 4) << 10 >> - >> - # set up arch to ppc64 instead of ppc64le due to libvirt >> compatibility >> - if self.info["arch"] == "ppc64": >> - # in Power, memory must be aligned in 256MiB >> - if (max_memory >> 10) % PPC_MEM_ALIGN != 0: >> - alignment = max_memory % (PPC_MEM_ALIGN << 10) >> - max_memory -= alignment >> - return max_memory >> + return get_cpu_xml(0, >> (self.info.get('memory').get('current')) << 10, >> + cpu_topo) >> >> def to_vm_xml(self, vm_name, vm_uuid, **kwargs): >> params = dict(self.info) >> @@ -375,27 +355,33 @@ class VMTemplate(object): >> else: >> params['cdroms'] = cdrom_xml >> >> - # max memory >> - params['max_memory'] = self._get_max_memory(params['memory']) >> - >> # Setting maximum number of slots to avoid errors when >> hotplug memory >> # Number of slots are the numbers of chunks of 1GB that >> fit inside >> # the max_memory of the host minus memory assigned to the >> VM. It >> # cannot have more than 32 slots in Power. >> - params['slots'] = ((params['max_memory'] >> 10) - >> - params['memory']) >> 10 >> - if params['slots'] < 0: >> + memory = self.info['memory'].get('current') >> + maxmemory = self.info['memory'].get('maxmemory') >> + >> + slots = (maxmemory - memory) >> 10 >> + if slots < 0: >> raise OperationFailed("KCHVM0041E", >> - {'maxmem': >> str(params['max_memory'] >> 10)}) >> - elif params['slots'] == 0: >> - params['slots'] = 1 >> - elif params['slots'] > 32: >> + {'maxmem': str(maxmemory)}) >> + elif slots == 0: >> + slots = 1 >> + elif slots > 32: >> distro, _, _ = platform.linux_distribution() >> if distro == "IBM_PowerKVM": >> - params['slots'] = 32 >> + slots = 32 >> + >> + # Rearrange memory parameters >> + params['memory'] = self.info['memory'].get('current') >> + params['max_memory'] = "" >> + if memory != maxmemory: >> + maxmem_xml = "<maxMemory slots='%s' >> unit='MiB'>%s</maxMemory>" >> + params['max_memory'] = maxmem_xml % (slots, maxmemory) >> >> # set a hard limit using max_memory + 1GiB >> - params['hard_limit'] = params['max_memory'] + (1024 << 10) >> + params['hard_limit'] = maxmemory + 1024 >> >> # vcpu element >> cpus = params['cpu_info']['vcpus'] >> @@ -411,9 +397,9 @@ class VMTemplate(object): >> <name>%(name)s</name> >> <uuid>%(uuid)s</uuid> >> <memtune> >> - <hard_limit unit='KiB'>%(hard_limit)s</hard_limit> >> + <hard_limit unit='MiB'>%(hard_limit)s</hard_limit> >> </memtune> >> - <maxMemory slots='%(slots)s' >> unit='KiB'>%(max_memory)s</maxMemory> >> + %(max_memory)s >> <memory unit='MiB'>%(memory)s</memory> >> %(vcpus_xml)s >> %(cpu_info_xml)s >> > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel