On 10/13/2014 11:40 AM, Christy Perez wrote:

On 10/13/2014 04:57 AM, Royce Lv wrote:
On 2014年10月10日 05:20, Christy Perez wrote:
In order to allow a guest to use SMT/hyperthreading, we should
enable passing in of the sockets, cores, and threads values when
creating a template.

All three values must be specified, as per the topology descr at
http://libvirt.org/formatdomain.html#elementsCPU

v2->v3:
- Set vcpus based on topology, if specified.
- Move the update cpu+topology validation out to a function
   for redability
- Add a minimum value of 1 for topology values
- Leave new English error msg as empty string
- Update the API documentation on cpu defaults

v1->v2:
- Added a check to make sure that vcpus = sockets*cores*threads
- Set individual topoology params to required in API.json
- Change the topology object types from string to integer
- Always return cpu_info from templates lookup()
- Removed check for cpu_info in to_vm_xml
- Build cpu_info xml using lxml.builder instead of string
- CPU and topology verification on template update

Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com>
---
  docs/API.md                     | 13 ++++++++++-
  po/en_US.po                     |  3 +++
  po/pt_BR.po                     |  3 +++
  po/zh_CN.po                     |  3 +++
  src/kimchi/API.json             | 33 ++++++++++++++++++++++++--
  src/kimchi/control/templates.py | 13 ++++++++---
  src/kimchi/i18n.py              |  1 +
  src/kimchi/model/templates.py   | 52
+++++++++++++++++++++++++++++++++++++++++
  src/kimchi/osinfo.py            |  2 +-
  src/kimchi/vmtemplate.py        | 15 ++++++++++++
  10 files changed, 131 insertions(+), 7 deletions(-)

diff --git a/docs/API.md b/docs/API.md
index cc438cc..7f1a8c2 100644
--- a/docs/API.md
+++ b/docs/API.md
@@ -179,7 +179,9 @@ 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
-    * cpus *(optional)*: The number of CPUs assigned to the VM.
Default is 1.
+    * cpus *(optional)*: The number of CPUs assigned to the VM.
+          Default is 1, unlees specifying a cpu topology. In that
case, cpus
unlees-->unless? :)
+          will default to a product of the topology values (see
cpu_info).
      * memory *(optional)*: The amount of memory assigned to the VM.
        Default is 1024M.
      * cdrom *(optional)*: A volume name or URI to an ISO image.
@@ -201,6 +203,15 @@ Represents a snapshot of the Virtual Machine's
primary monitor.
                       Independent Computing Environments
              * null: Graphics is disabled or type not supported
          * listen: The network which the vnc/spice server listens on.
+    * cpu_info *(optional)*: CPU-specific information.
+        * topology: Specify sockets, threads, and cores to run the
virtual CPU
+            threads on.
+            All three are required in order to specify cpu topology.
+            * sockets - The number of sockets to use.
+            * cores   - The number of cores per socket.
+            * threads - The number of threads per core.
+            If specifying both cpus and CPU topology, make sure cpus is
+            equal to the product of sockets, cores, and threads.
    ### Sub-Collection: Virtual Machine Network Interfaces
  diff --git a/po/en_US.po b/po/en_US.po
index eb571ca..a88675c 100644
--- a/po/en_US.po
+++ b/po/en_US.po
@@ -371,6 +371,9 @@ msgstr ""
  msgid "Cannot identify base image %(path)s format"
  msgstr ""
  +msgid "When specifying CPU topology, VCPUs must be a product of
sockets, cores, and threads."
+msgstr ""
+
  #, python-format
  msgid "Storage pool %(name)s already exists"
  msgstr ""
diff --git a/po/pt_BR.po b/po/pt_BR.po
index f2fba64..1bebe30 100644
--- a/po/pt_BR.po
+++ b/po/pt_BR.po
@@ -424,6 +424,9 @@ msgstr "Imagem base do modelo deve ser um arquivo
de imagem local válido"
  msgid "Cannot identify base image %(path)s format"
  msgstr "Não foi possível identificar o formato da imagem base %(path)s"
  +msgid "When specifying CPU topology, VCPUs must be a product of
sockets, cores, and threads."
+msgstr ""
+
  #, python-format
  msgid "Storage pool %(name)s already exists"
  msgstr "Storage pool %(name)s já existe"
diff --git a/po/zh_CN.po b/po/zh_CN.po
index f77e405..f944b8d 100644
--- a/po/zh_CN.po
+++ b/po/zh_CN.po
@@ -392,6 +392,9 @@ msgstr "模板基础镜像必须为一个有效的本地镜像文件"
  msgid "Cannot identify base image %(path)s format"
  msgstr "未能识别基础镜像%(path)s格式"
  +msgid "When specifying CPU topology, VCPUs must be a product of
sockets, cores, and threads."
+msgstr ""
+
  #, python-format
  msgid "Storage pool %(name)s already exists"
  msgstr "存储池%(name)s已经存在"
diff --git a/src/kimchi/API.json b/src/kimchi/API.json
index 5b752dc..fe3df72 100644
--- a/src/kimchi/API.json
+++ b/src/kimchi/API.json
@@ -26,6 +26,33 @@
                          ]
                  }
              }
+        },
+        "cpu_info": {
+            "description": "Configure CPU specifics for a VM.",
+            "type": "object",
+            "properties": {
+                "topology": {
+                    "description": "Configure the guest CPU topology.",
+                    "type": "object",
+                    "properties": {
+                        "sockets": {
+                            "type": "integer",
+                            "required": true,
+                            "minimum": 1
+                        },
+                        "cores": {
+                            "type": "integer",
+                            "required": true,
+                            "minimum": 1
+                        },
+                        "threads": {
+                            "type": "integer",
+                            "required": true,
+                            "minimum": 1
+                        }
+                    }
+                }
+            }
          }
      },
      "properties": {
@@ -448,7 +475,8 @@
                      "type": "array",
                      "items": { "type": "string" }
                  },
-                "graphics": { "$ref": "#/kimchitype/graphics" }
+                "graphics": { "$ref": "#/kimchitype/graphics" },
+                "cpu_info": { "$ref": "#/kimchitype/cpu_info" }
              },
              "additionalProperties": false,
              "error": "KCHAPI0001E"
@@ -612,7 +640,8 @@
                      "type": "array",
                      "items": { "type": "string" }
                  },
-                "graphics": { "$ref": "#/kimchitype/graphics" }
+                "graphics": { "$ref": "#/kimchitype/graphics" },
+                "cpu_info": { "$ref": "#/kimchitype/cpu_info" }
              },
              "additionalProperties": false,
              "error": "KCHAPI0001E"
diff --git a/src/kimchi/control/templates.py
b/src/kimchi/control/templates.py
index e17fa54..54caa92 100644
--- a/src/kimchi/control/templates.py
+++ b/src/kimchi/control/templates.py
@@ -38,13 +38,14 @@ def __init__(self, model, ident):
          self.update_params = ["name", "folder", "icon", "os_distro",
                                "storagepool", "os_version", "cpus",
                                "memory", "cdrom", "disks", "networks",
-                              "graphics"]
+                              "graphics", "cpu_info"]
          self.uri_fmt = "/templates/%s"
          self.clone = self.generate_action_handler('clone')
        @property
      def data(self):
-        return {'name': self.ident,
+        return_data = {
+                'name': self.ident,
                  'icon': self.info['icon'],
                  'invalid': self.info['invalid'],
                  'os_distro': self.info['os_distro'],
@@ -56,4 +57,10 @@ def data(self):
                  'storagepool': self.info['storagepool'],
                  'networks': self.info['networks'],
                  'folder': self.info.get('folder', []),
-                'graphics': self.info['graphics']}
+                'graphics': self.info['graphics']
+                }
+        if (self.info.get('cpu_info')):
+            return_data['cpu_info'] = self.info['cpu_info']
+        else:
+            return_data['cpu_info'] = ''
+        return return_data
diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
index 1b543ce..9ce8e86 100644
--- a/src/kimchi/i18n.py
+++ b/src/kimchi/i18n.py
@@ -131,6 +131,7 @@
      "KCHTMPL0022E": _("Disk size must be an integer greater than
1GB."),
      "KCHTMPL0023E": _("Template base image must be a valid local
image file"),
      "KCHTMPL0024E": _("Cannot identify base image %(path)s format"),
+    "KCHTMPL0025E": _("When specifying CPU topology, VCPUs must be a
product of sockets, cores, and threads."),
        "KCHPOOL0001E": _("Storage pool %(name)s already exists"),
      "KCHPOOL0002E": _("Storage pool %(name)s does not exist"),
diff --git a/src/kimchi/model/templates.py
b/src/kimchi/model/templates.py
index 9278cdc..04d2af9 100644
--- a/src/kimchi/model/templates.py
+++ b/src/kimchi/model/templates.py
@@ -48,6 +48,22 @@ def create(self, params):
                                         {'filename': iso, 'user': user,
                                          'err': excp})
  +        cpu_info = params.get('cpu_info')
+        if cpu_info:
+            cpu_info = dict(params['cpu_info'])
+            topology = cpu_info.get('topology')
+            # Check, even though currently only topology
+            #   is supported.
+            if topology:
+                sockets = topology['sockets']
+                cores = topology['cores']
+                threads = topology['threads']
+                vcpus = params.get('cpus')
+                if vcpus is None:
+                    params['cpus'] = sockets * cores * threads
+                elif sockets * cores * threads != vcpus:
+                    raise InvalidParameter("KCHTMPL0025E")
+
          conn = self.conn.get()
          pool_uri = params.get(u'storagepool', '')
          if pool_uri:
@@ -154,6 +170,9 @@ def delete(self, name):
        def update(self, name, params):
          old_t = self.lookup(name)
+        if not self._verify_updated_cpu_params(params, old_t):
+            raise InvalidParameter('KCHTMPL0025E')
+
          new_t = copy.copy(old_t)
          new_t.update(params)
          ident = name
@@ -187,6 +206,39 @@ def update(self, name, params):
              raise
          return ident
  +    def _verify_updated_cpu_params(self, params, old_t):
+        # Note: cpu_info is the parent of topology. cpus is vcpus
+        # Keep two check_ variables for what we'll verify at the end to
+        #   keep from duplicating code.
+        check_cpu = None
+        check_topology = None
+        # First see what parameters were passed in.
+        new_vcpus = params.get('cpus')
+        new_cpu_info = params.get('cpu_info')
+        new_topology = ''
+        if new_cpu_info:
+            new_topology = new_cpu_info.get('topology')
+        # Now figure out what needs to be verified.
+        if new_vcpus and new_topology:
+            check_cpu = new_vcpus
+            check_topology = new_topology
+        elif new_vcpus:
+            old_cpu_info = old_t.get('cpu_info')
+            if old_cpu_info:
+                old_topology = old_cpu_info.get('topology')
+                if old_topology:
+                    check_cpu = new_vcpus
+                    check_topology = old_topology
+        elif new_topology:
+            check_cpu = old_t.get('cpus')
+            check_topology = new_topology
+        # Now verify the cpu and topoloy parameters
+        if check_cpu and  (check_cpu != check_topology['sockets'] * \
+            check_topology['cores'] * check_topology['threads']):
+            return False
+
+        return True
+
    class LibvirtVMTemplate(VMTemplate):
      def __init__(self, args, scan=False, conn=None):
diff --git a/src/kimchi/osinfo.py b/src/kimchi/osinfo.py
index 6ee5e48..10d0ab0 100644
--- a/src/kimchi/osinfo.py
+++ b/src/kimchi/osinfo.py
@@ -32,7 +32,7 @@
                     'power': ('ppc', 'ppc64')}
    -common_spec = {'cpus': 1, 'cpu_cores': 1, 'cpu_threads': 1,
'memory': 1024,
+common_spec = {'cpus': 1, 'cpu_sockets': '1', 'cpu_cores': 1,
'cpu_threads': 1, 'memory': 1024,
As we use common_spec to create template, we need to put it in dict
"cpu_info", I suppose put it here won't get expected default value, right?
In osinfo.py:

def lookup(distro, version):
    ....

    if distro in modern_version_bases[arch]:
        if LooseVersion(version) >= LooseVersion(
                modern_version_bases[arch][distro]):
            params.update(template_specs[arch]['modern'])
        else:
            params.update(template_specs[arch]['old'])
    else:
        params['os_distro'] = params['os_version'] = "unknown"
        params.update(template_specs[arch]['old'])
   ....

    return params

In vmtemplate.py:
        ...
        entry = osinfo.lookup(os_distro, os_version)
        self.info.update(entry)

And in def _get_cpu_xml(self), you used

cpu_info = self.info.get('cpu_info')

So we need to define default value in 'cpu_info' dict instead.
I think the default value for this should be to leave it out from the
backend standpoint.

When we implement the UI, I think the default value should be to have
SMT/threads checked, and make the user uncheck it. But, that's another
discussion.

I'm not sure why the cpu_info information was put into the dict like it
was. Maybe we should just take those elements out?

No, those values must be there.

Let me try to clarify the relation between osinfo.py -> VMTemplate (vmtemplate.py) -> Template (model/templates.py)

First, keep in mind that you only need a ISO or Image file to create a Template.

When creating a Template, Kimchi will scan the ISO or Image file to get its OS and OS version.
Based on OS information we use the osinfo.py to get the default configuration,

On vmtemplate.py (__init__)

self.info = {}
entry = osinfo.lookup(os_distro, os_version)
self.info.update(entry)

(...)

# update the dict with user inputs
self.info.update(args) 

You can think VMTemplate like a dict of data.

if you add to common_specs:

cpu_info: {topology: {sockets: 1, cores: 1, threads: 1}}

All VMTemplate instances will have this configuration.
And it will be only override by user inputs (if he/she wants to)

Template is the Kimchi resource. Its data is just a copy of VMTemplate.info data.

Does that make sense?

In a next step, we will have a new default dict based on host architecture.
For example, for Power 8, we should use SMT8

Something like:

power_specs = {8: {cpu_info: {topology: {sockets: 1, cores: 2, threads: 4}}}},
                          ....
                         }

That way, user does not care about the topology values. Kimchi will automatically handle it according to host architecture and guest OS.



        
                 'disks': [{'index': 0, 'size': 10}], 'cdrom_bus': 'ide',
                 'cdrom_index': 2, 'mouse_bus': 'ps2'}
  diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py
index 5f22db9..34720de 100644
--- a/src/kimchi/vmtemplate.py
+++ b/src/kimchi/vmtemplate.py
@@ -358,6 +358,19 @@ def _get_input_output_xml(self):
              input_output += sound % self.info
          return input_output
  +    def _get_cpu_xml(self):
+
+        cpu_info = self.info.get('cpu_info')
+        if cpu_info is not None:
+            cpu_topo = cpu_info.get('topology')
+            if cpu_topo is not None:
+                return etree.tostring(E.cpu(E.topology(
+                    sockets=str(cpu_topo['sockets']),
+                    cores=str(cpu_topo['cores']),
+                    threads=str(cpu_topo['threads']))))
+
+        return ""
+
      def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
          params = dict(self.info)
          params['name'] = vm_name
@@ -369,6 +382,7 @@ def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
          params['qemu-stream-cmdline'] = ''
          graphics = kwargs.get('graphics')
          params['graphics'] = self._get_graphics_xml(graphics)
+        params['cpu_info'] = self._get_cpu_xml()
            # Current implementation just allows to create disk in one
single
          # storage pool, so we cannot mix the types (scsi volumes vs
img file)
@@ -400,6 +414,7 @@ def to_vm_xml(self, vm_name, vm_uuid, **kwargs):
            <uuid>%(uuid)s</uuid>
            <memory unit='MiB'>%(memory)s</memory>
            <vcpu>%(cpus)s</vcpu>
+          %(cpu_info)s
            <os>
              <type arch='%(arch)s'>hvm</type>
              <boot dev='hd'/>

      
_______________________________________________
Kimchi-devel mailing list
Kimchi-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/kimchi-devel