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

A couple comments below. On 16-02-2016 00:32, 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" > + } > + }, What about define a single memory object to avoid duplication here? See cpu_info definition for an example.

> + "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."), > + "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"), > @@ -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: local var maxmem is not defined

> + 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']: > + 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 definitively > + 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 >