On 04/16/2014 03:30 PM, Rodrigo Trujillo wrote:

This patch apply necessary changes in: - documentation (API.md): adds cpu and memory in update parameter - data validation (API.json: validate given cpu and memory numbers - error message (i18n.py): this new feature uses the same messages from Templates update, but a minor change was necessary in cpu text.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- docs/API.md | 3 +++ src/kimchi/API.json | 12 ++++++++++++ src/kimchi/i18n.py | 3 ++- 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/API.md b/docs/API.md index 716c983..845867c 100644 --- a/docs/API.md +++ b/docs/API.md @@ -103,6 +103,9 @@ the following general conventions: * name: New name for this VM (only applied for shutoff VM) * users: New list of system users. * groups: New list of system groups. + * cpus: New number of virtual cpus for this VM (only applied for shutoff VM) + * memory: New amount of memory (MB) for this VM (only applied for shutoff + VM)

I guess we are able to update those values in a running VM but the new values only will take effect in next boot.

* **POST**: *See Virtual Machine Actions*

**Actions (POST):** diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 3360a9c..38e9607 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -223,6 +223,18 @@ "type": "string", "error": "KCHVM0026E" } + }, + "cpus": { + "description": "The new number of virtual CPUs for the VM", + "type": "integer", + "minimum": 1, + "error": "KCHTMPL0012E" + }, + "memory": { + "description": "The new amount (MB) of memory for the VM", + "type": "integer", + "minimum": 512, + "error": "KCHTMPL0013E" } } }, diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 3fc3013..15cfd16 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -86,6 +86,7 @@ messages = { "KCHVM0026E": _("Group name must be a string"), "KCHVM0027E": _("User %(user)s does not exist"), "KCHVM0028E": _("Group %(group)s does not exist"), + "KCHVM0029E": _("Unable to update virtual machine in running state. You must power it off before."),

"KCHVMIF0001E": _("Interface %(iface)s does not exist in virtual machine %(name)s"), "KCHVMIF0002E": _("Network %(network)s specified for virtual machine %(name)s does not exist"), @@ -106,7 +107,7 @@ messages = { "KCHTMPL0009E": _("Template icon must be a path to the image"), "KCHTMPL0010E": _("Template distribution must be a string"), "KCHTMPL0011E": _("Template distribution version must be a string"), - "KCHTMPL0012E": _("The number of CPUs must be a integer"), + "KCHTMPL0012E": _("The number of CPUs must be an integer greater than 0"), "KCHTMPL0013E": _("Amount of 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"),

On 04/16/2014 03:30 PM, Rodrigo Trujillo wrote:

This patch changes vm control file in order to allow user to change cpus and memory. It also changes vms model in order to change xml properly and test if vm is running.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/control/vms.py | 17 ++--------------- src/kimchi/i18n.py | 1 - src/kimchi/model/vms.py | 24 +++++++++++++++--------- 3 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index ea810e4..77e6d8b 100644 --- a/src/kimchi/control/vms.py +++ b/src/kimchi/control/vms.py @@ -32,7 +32,7 @@ class VMs(Collection): class VM(Resource): def __init__(self, model, ident): super(VM, self).__init__(model, ident) - self.update_params = ["name", "users", "groups"] + self.update_params = ["name", "users", "groups", "cpus", "memory"] self.screenshot = VMScreenShot(model, ident) self.uri_fmt = '/vms/%s' for ident, node in sub_nodes.items(): @@ -44,20 +44,7 @@ class VM(Resource):

@property def data(self): - return {'name': self.ident, - 'uuid': self.info['uuid'], - 'stats': self.info['stats'], - 'memory': self.info['memory'], - 'cpus': self.info['cpus'], - 'state': self.info['state'], - 'screenshot': self.info['screenshot'], - 'icon': self.info['icon'], - 'graphics': {'type': self.info['graphics']['type'], - 'listen': self.info['graphics']['listen'], - 'port': self.info['graphics']['port']}, - 'users': self.info['users'], - 'groups': self.info['groups'] - } + return self.info

class VMScreenShot(Resource): diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 15cfd16..5207183 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -86,7 +86,6 @@ messages = { "KCHVM0026E": _("Group name must be a string"), "KCHVM0027E": _("User %(user)s does not exist"), "KCHVM0028E": _("Group %(group)s does not exist"),

- "KCHVM0029E": _("Unable to update virtual machine in running state. You must power it off before."),

You added this message in previous patch. Why are you removing it now?

"KCHVMIF0001E": _("Interface %(iface)s does not exist in virtual machine %(name)s"), "KCHVMIF0002E": _("Network %(network)s specified for virtual machine %(name)s does not exist"), diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 90e9537..a694517 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -51,7 +51,9 @@ DOM_STATE_MAP = {0: 'nostate', 6: 'crashed'}

GUESTS_STATS_INTERVAL = 5 -VM_STATIC_UPDATE_PARAMS = {'name': './name'} +VM_STATIC_UPDATE_PARAMS = {'name': './name', + 'cpus': './vcpu', + 'memory': './memory'} VM_LIVE_UPDATE_PARAMS = {}

stats = {} @@ -255,7 +257,6 @@ class VMModel(object):

def _static_vm_update(self, dom, params): state = DOM_STATE_MAP[dom.info()[0]] - old_xml = new_xml = dom.XMLDesc(0)

metadata_xpath = "/domain/metadata/kimchi/access/%s" @@ -275,6 +276,12 @@ class VMModel(object): groups = val else: if key in VM_STATIC_UPDATE_PARAMS: + if key == 'memory': + # Libvirt saves memory in KiB. Retrieved xml has memory + # in KiB too, so new valeu must be in KiB here + val = val * 1024

+ if type(val) == int: + val = str(val)

Why do you need this? If it is because the following function (xmlutils.xml_item_update) expected a string you should always do this convertion new_xml = xmlutils.xml_item_update(new_xml, xpath, str(val))

xpath = VM_STATIC_UPDATE_PARAMS[key] new_xml = xmlutils.xml_item_update(new_xml, xpath, val)

@@ -285,21 +292,19 @@ class VMModel(object): msg_args = {'name': dom.name(), 'new_name': params['name']} raise InvalidParameter("KCHVM0003E", msg_args)

- # Undefine old vm and create a new one with updated values + # Undefine old vm, only if name is going to change dom.undefine() - conn = self.conn.get() - dom = conn.defineXML(new_xml)

- # Update metadata element root = ET.fromstring(new_xml) + root.remove(root.find('.currentMemory')) + # Update metadata element current_metadata = root.find('metadata') new_metadata = self._get_metadata_node(users, groups) if current_metadata is not None: root.replace(current_metadata, new_metadata) else: root.append(new_metadata) - dom = conn.defineXML(ET.tostring(root)) - + dom = conn.defineXML(ET.tostring(root, encoding="utf-8")) except libvirt.libvirtError as e: dom = conn.defineXML(old_xml) raise OperationFailed("KCHVM0008E", {'name': dom.name(), @@ -350,7 +355,8 @@ class VMModel(object): users = xpath_get_text(xml, "/domain/metadata/kimchi/access/user") groups = xpath_get_text(xml, "/domain/metadata/kimchi/access/group")

- return {'state': state, + return {'name': name, + 'state': state, 'stats': res, 'uuid': dom.UUIDString(), 'memory': info[2] >> 10,

Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 04/16/2014 03:30 PM, Rodrigo Trujillo wrote:

This patch changes the mockmodel in order to allow update of vm cpu and memory. It also implements new tests for the updates and errors.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 27 +++++++++++++++------------ tests/test_mockmodel.py | 6 ++++-- tests/test_model.py | 14 ++++++++++---- tests/test_rest.py | 32 +++++++++++++++++++++++++++++--- 4 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 8c7d7bb..e602a54 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -87,16 +87,16 @@ class MockModel(object): def _static_vm_update(self, dom, params): state = dom.info['state']

- if 'name' in params: - if state == 'running' or params['name'] in self.vms_get_list(): - msg_args = {'name': dom.name, 'new_name': params['name']} - raise InvalidParameter("KCHVM0003E", msg_args) - else: - del self._mock_vms[dom.name] - dom.name = params['name'] - self._mock_vms[dom.name] = dom - for key, val in params.items(): + if key == 'name': + if state == 'running' or params['name'] in self.vms_get_list(): + msg_args = {'name': dom.name, 'new_name': params['name']} + raise InvalidParameter("KCHVM0003E", msg_args) + else: + del self._mock_vms[dom.name] + dom.name = params['name'] + self._mock_vms[dom.name] = dom + if key in dom.info: dom.info[key] = val

@@ -954,6 +954,8 @@ class MockVM(object): def __init__(self, uuid, name, template_info): self.uuid = uuid self.name = name + self.memory = template_info['memory'] + self.cpus = template_info['cpus'] self.disk_paths = [] self.networks = template_info['networks'] ifaces = [MockVMIface(net) for net in self.networks] @@ -965,11 +967,12 @@ class MockVM(object): 'net_throughput_peak': 100, 'io_throughput': 45, 'io_throughput_peak': 100} - self.info = {'state': 'shutoff', + self.info = {'name': self.name, + 'state': 'shutoff', 'stats': stats, 'uuid': self.uuid, - 'memory': template_info['memory'], - 'cpus': template_info['cpus'], + 'memory': self.memory, + 'cpus': self.cpus, 'icon': None, 'graphics': {'type': 'vnc', 'listen': '0.0.0.0', 'port': None}, diff --git a/tests/test_mockmodel.py b/tests/test_mockmodel.py index 9e258ce..8dd6a03 100644 --- a/tests/test_mockmodel.py +++ b/tests/test_mockmodel.py @@ -134,8 +134,9 @@ class MockModelTests(unittest.TestCase): self.assertEquals(1, len(vms)) self.assertEquals(u'test', vms[0])

- keys = set(('state', 'stats', 'uuid', 'memory', 'cpus', 'screenshot', - 'icon', 'graphics', 'users', 'groups')) + keys = set(('name', 'state', 'stats', 'uuid', 'memory', 'cpus', + 'screenshot', 'icon', 'graphics', 'users', 'groups')) + stats_keys = set(('cpu_utilization', 'net_throughput', 'net_throughput_peak', 'io_throughput', 'io_throughput_peak')) @@ -143,6 +144,7 @@ class MockModelTests(unittest.TestCase): info = model.vm_lookup(u'test') self.assertEquals(keys, set(info.keys())) self.assertEquals('shutoff', info['state']) + self.assertEquals('test', info['name']) self.assertEquals(1024, info['memory']) self.assertEquals(1, info['cpus']) self.assertEquals('images/icon-vm.png', info['icon']) diff --git a/tests/test_model.py b/tests/test_model.py index 357d969..eee20f0 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -62,14 +62,16 @@ class ModelTests(unittest.TestCase): self.assertEquals(1, len(vms)) self.assertEquals('test', vms[0])

- keys = set(('state', 'stats', 'uuid', 'memory', 'cpus', 'screenshot', - 'icon', 'graphics', 'users', 'groups')) + keys = set(('name', 'state', 'stats', 'uuid', 'memory', 'cpus', + 'screenshot', 'icon', 'graphics', 'users', 'groups')) + stats_keys = set(('cpu_utilization', 'net_throughput', 'net_throughput_peak', 'io_throughput', 'io_throughput_peak')) info = inst.vm_lookup('test') self.assertEquals(keys, set(info.keys())) self.assertEquals('running', info['state']) + self.assertEquals('test', info['name']) self.assertEquals(2048, info['memory']) self.assertEquals(2, info['cpus']) self.assertEquals(None, info['icon']) @@ -602,13 +604,17 @@ class ModelTests(unittest.TestCase): 'kimchi-vm1', params)

inst.vm_poweroff('kimchi-vm1') - params = {'name': u'пeω-∨м'} self.assertRaises(OperationFailed, inst.vm_update, 'kimchi-vm1', {'name': 'kimchi-vm2'}) + + params = {'name': u'пeω-∨м', 'cpus': 4, 'memory': 2048} inst.vm_update('kimchi-vm1', params) - self.assertEquals(info['uuid'], inst.vm_lookup(u'пeω-∨м')['uuid']) rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, u'пeω-∨м') + self.assertEquals(info['uuid'], inst.vm_lookup(u'пeω-∨м')['uuid']) + info = inst.vm_lookup(u'пeω-∨м') + for key in params.keys(): + self.assertEquals(params[key], info[key])

# change only VM users - groups are not changed (default is empty) users = ['root'] diff --git a/tests/test_rest.py b/tests/test_rest.py index a40ba93..4cba5a4 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -203,6 +203,14 @@ class RestTests(unittest.TestCase): resp = self.request('/vms/vm-1', req, 'PUT') self.assertEquals(400, resp.status)

+ req = json.dumps({'cpus': 3}) + resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(200, resp.status) + + req = json.dumps({'memory': 2048}) + resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(200, resp.status) + resp = self.request('/vms/vm-1/poweroff', '{}', 'POST') self.assertEquals(200, resp.status)

@@ -214,15 +222,33 @@ class RestTests(unittest.TestCase): resp = self.request('/vms/vm-1', req, 'PUT') self.assertEquals(400, resp.status)

- req = json.dumps({'name': 'new-name', 'cpus': 5}) + req = json.dumps({'cpus': -2}) + resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(400, resp.status) + + req = json.dumps({'cpus': 'four'}) + resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(400, resp.status) + + req = json.dumps({'memory': 100}) + resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(400, resp.status) + + req = json.dumps({'memory': 'ten gigas'}) + resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(400, resp.status) + + req = json.dumps({'name': 'new-name', 'cpus': 5, 'UUID': 'notallowed'}) resp = self.request('/vms/vm-1', req, 'PUT') self.assertEquals(405, resp.status)

- req = json.dumps({'name': u'∨м-црdαtеd'}) + params = {'name': u'∨м-црdαtеd', 'cpus': 5, 'memory': 4096} + req = json.dumps(params) resp = self.request('/vms/vm-1', req, 'PUT') self.assertEquals(303, resp.status) vm = json.loads(self.request('/vms/∨м-црdαtеd', req).read()) - self.assertEquals(u'∨м-црdαtеd', vm['name']) + for key in params.keys(): + self.assertEquals(params[key], vm[key])

# change only VM users - groups are not changed (default is empty) req = json.dumps({'users': ['root']})