On Fri, 2014-04-11 at 15:38 -0300, 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 143c70c..822e551 100644 --- a/docs/API.md +++ b/docs/API.md @@ -97,6 +97,9 @@ the following general conventions: * **DELETE**: Remove the Virtual Machine * **PUT**: update the parameters of existed VM * name: New name for this VM (only applied for shutoff VM) + * 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) * **POST**: *See Virtual Machine Actions*

**Actions (POST):** diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 5ca94e3..f95bddf 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -201,6 +201,18 @@ "type": "string", "minLength": 1, "error": "KCHVM0011E" + }, + "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 ae8d24e..341700c 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -80,6 +80,7 @@ messages = { "KCHVM0019E": _("Unable to start virtual machine %(name)s. Details: %(err)s"), "KCHVM0020E": _("Unable to power off virtual machine %(name)s. Details: %(err)s"), "KCHVM0021E": _("Unable to delete virtual machine %(name)s. Details: %(err)s"), + "KCHVM0022E": _("Unable to update virtual machine in running state. You must power it off before."),

About do you think about "The changes will take effect after the next guest shutdown." ? The rest of patch is OK for me!

"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"), @@ -100,7 +101,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"),

Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 04/11/2014 03:38 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 143c70c..822e551 100644 --- a/docs/API.md +++ b/docs/API.md @@ -97,6 +97,9 @@ the following general conventions: * **DELETE**: Remove the Virtual Machine * **PUT**: update the parameters of existed VM * name: New name for this VM (only applied for shutoff VM) + * 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) * **POST**: *See Virtual Machine Actions*

**Actions (POST):** diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 5ca94e3..f95bddf 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -201,6 +201,18 @@ "type": "string", "minLength": 1, "error": "KCHVM0011E" + }, + "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 ae8d24e..341700c 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -80,6 +80,7 @@ messages = { "KCHVM0019E": _("Unable to start virtual machine %(name)s. Details: %(err)s"), "KCHVM0020E": _("Unable to power off virtual machine %(name)s. Details: %(err)s"), "KCHVM0021E": _("Unable to delete virtual machine %(name)s. Details: %(err)s"), + "KCHVM0022E": _("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"), @@ -100,7 +101,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"),

-- Reviewed-by: Paulo Vital <pvital@linux.vnet.ibm.com> On Fri, 2014-04-11 at 15:38 -0300, 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 | 4 ++-- src/kimchi/model/vms.py | 43 +++++++++++++++++++++++++++---------------- 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index e75b27f..0964fb7 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"] + self.update_params = ["name", "cpus", "memory"] self.screenshot = VMScreenShot(model, ident) self.uri_fmt = '/vms/%s' for ident, node in sub_nodes.items(): @@ -43,7 +43,7 @@ class VM(Resource):

@property def data(self): - return {'name': self.ident, + return {'name': self.info['name'], 'uuid': self.info['uuid'], 'stats': self.info['stats'], 'memory': self.info['memory'], diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 2425a43..35fbf3e 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -47,7 +47,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 = {} @@ -235,33 +237,41 @@ class VMModel(object):

def update(self, name, params): dom = self.get_vm(name, self.conn) + # Change memory unit from MB to KB dom = self._static_vm_update(dom, params) self._live_vm_update(dom, params) return dom.name().decode('utf-8')

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

for key, val in params.items(): if key in VM_STATIC_UPDATE_PARAMS: + # Machine must be powered off + if state == 'running': + raise InvalidOperation("KCHVM0022E") xpath = VM_STATIC_UPDATE_PARAMS[key] + if key == 'memory': + val = val * 1024 + if type(val) == int: + val = str(val) new_xml = xmlutils.xml_item_update(new_xml, xpath, val)

- try: - if 'name' in params: - if state == 'running': - msg_args = {'name': dom.name(), 'new_name': params['name']} - raise InvalidParameter("KCHVM0003E", msg_args) - else: - dom.undefine() - conn = self.conn.get() - dom = conn.defineXML(new_xml) - except libvirt.libvirtError as e: - dom = conn.defineXML(old_xml) - raise OperationFailed("KCHVM0008E", {'name': dom.name(), - 'err': e.get_error_message()}) + if not new_xml == old_xml: + # Remove currentMemory element + root = ElementTree.fromstring(new_xml) + root.remove(root.find('.currentMemory')) + new_xml = ElementTree.tostring(root, encoding="utf-8") + try: + conn = self.conn.get() + dom.undefine() + dom = conn.defineXML(new_xml) + except libvirt.libvirtError as e: + dom = conn.defineXML(old_xml) + raise OperationFailed("KCHVM0008E", + {'name': dom.name(), + 'err': e.get_error_message()}) return dom

def _live_vm_update(self, dom, params): @@ -304,7 +314,8 @@ class VMModel(object): res['io_throughput'] = vm_stats.get('disk_io', 0) res['io_throughput_peak'] = vm_stats.get('max_disk_io', 100)

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

On 04/14/2014 05:15 PM, Aline Manera wrote:

On 04/11/2014 03:38 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 | 4 ++-- src/kimchi/model/vms.py | 43 +++++++++++++++++++++++++++---------------- 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index e75b27f..0964fb7 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"] + self.update_params = ["name", "cpus", "memory"] self.screenshot = VMScreenShot(model, ident) self.uri_fmt = '/vms/%s' for ident, node in sub_nodes.items(): @@ -43,7 +43,7 @@ class VM(Resource):

@property def data(self): - return {'name': self.ident, + return {'name': self.info['name'], 'uuid': self.info['uuid'], 'stats': self.info['stats'], 'memory': self.info['memory'],

Could we return the whole self.info object instead of listing item by item?

@property def data(self): return self.info

Or there is something in self.info that should not be returned?

Yes, I think it is possible to return only info. I am going to check

...

diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 2425a43..35fbf3e 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -47,7 +47,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 = {} @@ -235,33 +237,41 @@ class VMModel(object):

def update(self, name, params): dom = self.get_vm(name, self.conn)

...

+ # Change memory unit from MB to KB

Why?

Wrong comment I forgot to remove

...

dom = self._static_vm_update(dom, params) self._live_vm_update(dom, params) return dom.name().decode('utf-8')

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

for key, val in params.items(): if key in VM_STATIC_UPDATE_PARAMS: + # Machine must be powered off + if state == 'running': + raise InvalidOperation("KCHVM0022E") xpath = VM_STATIC_UPDATE_PARAMS[key]

...

+ if key == 'memory': + val = val * 1024

Could we save the memory in MB? This memory parameter comes from VM xml, which are being created using KB. Yes, it is possible to save in MB, but we need to change in all VM create API. Do you know why KB was chosen in the beginning of the project ?

...

+ if type(val) == int: + val = str(val) new_xml = xmlutils.xml_item_update(new_xml, xpath, val)

- try: - if 'name' in params: - if state == 'running': - msg_args = {'name': dom.name(), 'new_name': params['name']} - raise InvalidParameter("KCHVM0003E", msg_args) - else: - dom.undefine() - conn = self.conn.get() - dom = conn.defineXML(new_xml) - except libvirt.libvirtError as e: - dom = conn.defineXML(old_xml) - raise OperationFailed("KCHVM0008E", {'name': dom.name(), - 'err': e.get_error_message()}) + if not new_xml == old_xml: + # Remove currentMemory element + root = ElementTree.fromstring(new_xml) + root.remove(root.find('.currentMemory')) + new_xml = ElementTree.tostring(root, encoding="utf-8") + try: + conn = self.conn.get() + dom.undefine() + dom = conn.defineXML(new_xml) + except libvirt.libvirtError as e: + dom = conn.defineXML(old_xml) + raise OperationFailed("KCHVM0008E", + {'name': dom.name(), + 'err': e.get_error_message()}) return dom

def _live_vm_update(self, dom, params): @@ -304,7 +314,8 @@ class VMModel(object): res['io_throughput'] = vm_stats.get('disk_io', 0) res['io_throughput_peak'] = vm_stats.get('max_disk_io', 100)

- 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/11/2014 03:38 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 | 30 +++++++++++++++++------------- tests/test_mockmodel.py | 5 +++-- tests/test_model.py | 24 +++++++++++++++++++----- tests/test_rest.py | 32 +++++++++++++++++++++++++++++--- 4 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index bf17975..2f92a7a 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -88,17 +88,18 @@ 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 in VM_STATIC_UPDATE_PARAMS and key in dom.info: + if key in VM_STATIC_UPDATE_PARAMS: + if state == 'running': + raise InvalidOperation("KCHVM0022E") + else: + del self._mock_vms[dom.name] + dom.name = params['name'] + dom.cpus = params['cpus'] + dom.memory = params['memory'] + self._mock_vms[dom.name] = dom + + if key in dom.info: dom.info[key] = val

def _live_vm_update(self, dom, params): @@ -951,6 +952,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] @@ -962,11 +965,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 0798701..a0cb882 100644 --- a/tests/test_mockmodel.py +++ b/tests/test_mockmodel.py @@ -134,8 +134,8 @@ 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')) + keys = set(('name', 'state', 'stats', 'uuid', 'memory', 'cpus', + 'screenshot', 'icon', 'graphics')) stats_keys = set(('cpu_utilization', 'net_throughput', 'net_throughput_peak', 'io_throughput', 'io_throughput_peak')) @@ -143,6 +143,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 3041196..62960ce 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -60,14 +60,15 @@ class ModelTests(unittest.TestCase): self.assertEquals(1, len(vms)) self.assertEquals('test', vms[0])

- keys = set(('state', 'stats', 'uuid', 'memory', 'cpus', 'screenshot', - 'icon', 'graphics')) + keys = set(('name', 'state', 'stats', 'uuid', 'memory', 'cpus', + 'screenshot', 'icon', 'graphics')) 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']) @@ -594,17 +595,29 @@ class ModelTests(unittest.TestCase): self.assertEquals('running', info['state'])

params = {'name': 'new-vm'} - self.assertRaises(InvalidParameter, inst.vm_update, + self.assertRaises(InvalidOperation, inst.vm_update, + 'kimchi-vm1', params) + + params = {'cpus': 3} + self.assertRaises(InvalidOperation, inst.vm_update, + 'kimchi-vm1', params) + + params = {'memory': 2048} + self.assertRaises(InvalidOperation, inst.vm_update, '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])

@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_network(self): @@ -806,6 +819,7 @@ class ModelTests(unittest.TestCase): return

@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + @unittest.skip(True) def test_delete_running_vm(self): inst = model.Model(objstore_loc=self.tmp_store)

diff --git a/tests/test_rest.py b/tests/test_rest.py index cd40d55..25f839e 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -201,6 +201,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(400, resp.status) + + req = json.dumps({'memory': 2048}) + resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(400, resp.status) + resp = self.request('/vms/vm-1/poweroff', '{}', 'POST') self.assertEquals(200, resp.status)

@@ -212,15 +220,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])

def test_vm_lifecycle(self): # Create a Template