Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 02/13/2014 04:47 PM, Daniel Barboza wrote:

From: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

This patch changes API.md with new storage sub-collection/sub-resource information.

Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

Excluding hard drive support from docs/API.md

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- docs/API.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)

diff --git a/docs/API.md b/docs/API.md index 48a293f..c149ac7 100644 --- a/docs/API.md +++ b/docs/API.md @@ -115,6 +115,27 @@ Represents a snapshot of the Virtual Machine's primary monitor.

* **GET**: Redirect to the latest screenshot of a Virtual Machine in PNG format

+ +### Sub-collection: Virtual Machine storages +**URI:** /vms/*:name*/storages +* **GET**: Retrieve a summarized list of all storages of specified guest +* **POST**: Attach a new storage or virtual drive to specified virtual machine. + * dev: The name of the storage in the vm. + * type: The type of the storage (currently supports 'cdrom' only). + * path: Path of cdrom iso. + +### Sub-resource: storage +**URI:** /vms/*:name*/storages/*:dev* +* **GET**: Retrieve storage information + * dev: The name of the storage in the vm. + * type: The type of the storage ('cdrom' only for now). + * path: Path of cdrom iso. +* **PUT**: Update storage information + * path: Path of cdrom iso. Can not be blank. +* **DELETE**: Remove the storage. + + + ### Collection: Templates

**URI:** /templates

Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 02/13/2014 04:47 PM, Daniel Barboza wrote:

From: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

This patch adds the control classes for guest storages

Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

Removing hard disk support from API.json Changed API.json declarations to the new exception model

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- src/kimchi/API.json | 37 +++++++++++++++++++++++++++++ src/kimchi/control/vm/storages.py | 49 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 src/kimchi/control/vm/storages.py

diff --git a/src/kimchi/API.json b/src/kimchi/API.json index ba1619f..a1bdf64 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -340,6 +340,43 @@ "additionalProperties": false, "error": "KCHAPI0001E" }, + "storages_create": { + "type": "object", + "properties": { + "dev": { + "description": "The storage (cd-rom) device name", + "type": "string", + "pattern": "^hd[b-z]$", + "error": "KCHCDROM0001E" + }, + "type": { + "description": "The storage type", + "type": "string", + "pattern": "^cdrom$", + "required": true, + "error": "KCHCDROM0002E" + }, + "path": { + "description": "Path of iso image file or disk mount point", + "type": "string", + "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", + "required": true, + "error": "KCHCDROM0003E" + } + } + }, + "storage_update": { + "type": "object", + "properties": { + "path": { + "description": "Path of iso image file or disk mount point", + "type": "string", + "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", + "required": true, + "error": "KCHCDROM0003E" + } + } + }, "template_update": { "type": "object", "properties": { diff --git a/src/kimchi/control/vm/storages.py b/src/kimchi/control/vm/storages.py new file mode 100644 index 0000000..49f524c --- /dev/null +++ b/src/kimchi/control/vm/storages.py @@ -0,0 +1,49 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014 +# +# Authors: +# Rodrigo Trujilo <rodrigo.trujillo@linux.vnet.ibm.com> +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +from kimchi.control.base import Collection, Resource +from kimchi.control.utils import UrlSubNode + + +@UrlSubNode("storages") +class Storages(Collection): + def __init__(self, model, vm): + super(Storages, self).__init__(model) + self.resource = Storage + self.vm = vm + self.resource_args = [self.vm, ] + self.model_args = [self.vm, ] + + +class Storage(Resource): + def __init__(self, model, vm, ident): + super(Storage, self).__init__(model, ident) + self.vm = vm + self.ident = ident + self.info = {} + self.model_args = [self.vm, self.ident] + self.uri_fmt = '/vms/%s/storages/%s' + self.update_params = ['path'] + + @property + def data(self): + return self.info

On 02/13/2014 04:47 PM, Daniel Barboza wrote:

From: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

This patch adds the CREATE, LOOKUP, UPDATE and DELETE functionalities to guest storage devices support.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

Using lxml and objectify to parse the XML info. Pep8 adjustments in model/vms.py Removing hard disk support from model/vms.py Including xml creation using lxml in _get_storage_xml Including existing device name verification Changed model/vms.py to use the new exception model.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 173 +++++++++++++++++++++++++++++++++++++++++++++++- src/kimchi/xmlutils.py | 8 ++- 2 files changed, 178 insertions(+), 3 deletions(-)

diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 7a84122..e20dc38 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -21,12 +21,18 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

import os +import socket +import string import time +import urlparse import uuid from xml.etree import ElementTree

import libvirt +import lxml.etree as ET from cherrypy.process.plugins import BackgroundTask +from lxml import etree, objectify +from lxml.builder import E

from kimchi import vnc from kimchi import xmlutils @@ -36,7 +42,8 @@ from kimchi.model.config import CapabilitiesModel from kimchi.model.templates import TemplateModel from kimchi.model.utils import get_vm_name from kimchi.screenshot import VMScreenshot -from kimchi.utils import run_setfacl_set_attr, template_name_from_uri +from kimchi.utils import kimchi_log, run_setfacl_set_attr +from kimchi.utils import template_name_from_uri

DOM_STATE_MAP = {0: 'nostate', @@ -47,6 +54,10 @@ DOM_STATE_MAP = {0: 'nostate', 5: 'shutoff', 6: 'crashed'}

+DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', + 'block': 'dev', + 'dir': 'dir'} + GUESTS_STATS_INTERVAL = 5 VM_STATIC_UPDATE_PARAMS = {'name': './name'} VM_LIVE_UPDATE_PARAMS = {} @@ -222,7 +233,7 @@ class VMModel(object): dom = self.get_vm(name, self.conn) dom = self._static_vm_update(dom, params) self._live_vm_update(dom, params) - return dom.name().decode('utf-8') + return dom.name()

def _static_vm_update(self, dom, params): state = DOM_STATE_MAP[dom.info()[0]] @@ -453,3 +464,161 @@ class LibvirtVMScreenshot(VMScreenshot): stream.finish() finally: os.close(fd) + + +class StoragesModel(object): + def __init__(self, **kargs): + self.conn = kargs['conn'] + + def _get_storage_xml(self, params): + src_type = params.get('src_type') + disk = E.disk(type=src_type, device=params.get('type')) + disk.append(E.driver(name='qemu', type='raw')) + # Working with url paths + if src_type == 'network': + output = urlparse.urlparse(params.get('path')) + host = E.host(name=output.hostname, port= + output.port or socket.getservbyname(output.scheme)) + source = E.source(protocol=output.scheme, name=output.path) + source.append(host) + disk.append(source) + else: + # Fixing source attribute + source = E.source() + source.set(DEV_TYPE_SRC_ATTR_MAP[src_type], params.get('path')) + disk.append(source) + + disk.append(E.target(dev=params.get('dev'), bus='ide')) + return ET.tostring(disk) + + def create(self, vm_name, params): + # Use device name passed or pick next + dev_name = params.get('dev') + if not dev_name: + params['dev'] = self._get_storage_device_name(vm_name) + else: + devices = self.get_list(vm_name) + if dev_name in devices: + raise OperationFailed("KCHCDROM0004E", + {'dev_name': dev_name, + 'vm_name': vm_name}) + + # Path will never be blank due to API.json verification. + # There is no need to cover this case here. + path = params.get('path') + + # Checking if path exist, if not url + if path.startswith('/'): + if not os.path.exists(path): + raise InvalidParameter("KCHCDROM0005E") + elif path.endswith('.iso') or os.path.isfile(path): + params['src_type'] = 'file' + elif os.path.isdir(path): + raise InvalidParameter("KCHCDROM0006E") + else: + params['src_type'] = 'block' + else: + params['src_type'] = 'network'

Use chech_url_path() to check network cdrom if os.path.exists(path): if os.path.iffile(path) params['src_type'] = 'file' else: params['src_type'] = 'block' elif check_url_path(path): params['src_type'] = network else: raise InvalidParamter(<cdrom path is not a local or remote>)

+ + # Add device to VM + dev_xml = self._get_storage_xml(params) + try: + conn = self.conn.get() + dom = conn.lookupByName(vm_name) + dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) + except Exception as e: + raise OperationFailed("KCHCDROM0011E", {'error': e.message}) + return params['dev'] + + def _get_storage_device_name(self, vm_name): + dev_list = [dev for dev in self.get_list(vm_name) + if dev.startswith('hd')] + if len(dev_list) == 0: + return 'hda' + dev_list.sort() + last_dev = dev_list.pop() + # TODO: Improve to device names "greater then" hdz + next_dev_letter_pos = string.ascii_lowercase.index(last_dev[2]) + 1 + return 'hd' + string.ascii_lowercase[next_dev_letter_pos] + + def get_list(self, vm_name): + dom = VMModel.get_vm(vm_name, self.conn) + xml = dom.XMLDesc(0) + devices = objectify.fromstring(xml).devices + storages = [disk.target.attrib['dev'] + for disk in devices.xpath("./disk[@device='disk']")] + storages += [disk.target.attrib['dev'] + for disk in devices.xpath("./disk[@device='cdrom']")] + return storages + + +class StorageModel(object): + def __init__(self, **kargs): + self.conn = kargs['conn'] + self.kargs = kargs + + def _get_device_xml(self, vm_name, dev_name): + # Get VM xml and then devices xml + dom = VMModel.get_vm(vm_name, self.conn) + xml = dom.XMLDesc(0) + devices = objectify.fromstring(xml).devices + disk = devices.xpath("./disk/target[@dev='%s']/.." % dev_name) + if not disk: + return None + return disk[0] + + def lookup(self, vm_name, dev_name): + # Retrieve disk xml and format return dict + disk = self._get_device_xml(vm_name, dev_name) + if disk is None: + raise NotFoundError("KCHCDROM0007E", {'dev_name': dev_name, + 'vm_name': vm_name}) + source = disk.source + path = "" + if source is not None: + src_type = disk.attrib['type'] + if src_type == 'network': + host = source.host + path = (source.attrib['protocol'] + '://' + + host.attrib['name'] + ':' + + host.attrib['port'] + source.attrib['name']) + else: + path = source.attrib[DEV_TYPE_SRC_ATTR_MAP[src_type]] + dev_type = disk.attrib['device'] + return {'dev': dev_name, + 'type': dev_type, + 'path': path} + + def delete(self, vm_name, dev_name): + # Get storage device xml + disk = self._get_device_xml(vm_name, dev_name) + if disk is None: + raise NotFoundError("KCHCDROM0007E", + {'dev_name': dev_name, + 'vm_name': vm_name}) + try: + conn = self.conn.get() + dom = conn.lookupByName(vm_name) + dom.detachDeviceFlags(etree.tostring(disk), + libvirt.VIR_DOMAIN_AFFECT_CURRENT) + except Exception as e: + raise OperationFailed("KCHCDROM0010E", {'error': e.message}) + + def update(self, vm_name, dev_name, params): + info = self.lookup(vm_name, dev_name) + backup_params = info.copy() + try: + self.delete(vm_name, dev_name) + except Exception as e: + raise OperationFailed("KCHCDROM0009E", {'error': e.message}) + + info.update(params) + kargs = {'conn': self.conn} + stgModel = StoragesModel(**kargs) + try: + dev_name = stgModel.create(vm_name, info) + return dev_name + except Exception as e: + # Restoring previous device + dev_name = stgModel.create(vm_name, backup_params) + raise OperationFailed("KCHCDROM0009E", {'error': e.message}) diff --git a/src/kimchi/xmlutils.py b/src/kimchi/xmlutils.py index 51ff0ec..fd19da1 100644 --- a/src/kimchi/xmlutils.py +++ b/src/kimchi/xmlutils.py @@ -29,7 +29,7 @@ from xml.etree import ElementTree def xpath_get_text(xml, expr): doc = libxml2.parseDoc(xml) res = doc.xpathEval(expr) - ret = [None if x.children == None else x.children.content for x in res] + ret = [None if x.children is None else x.children.content for x in res]

doc.freeDoc() return ret @@ -40,3 +40,9 @@ def xml_item_update(xml, xpath, value): item = root.find(xpath) item.text = value return ElementTree.tostring(root, encoding="utf-8") +

+ +def xml_get_child(xml, xpath): + root = ElementTree.fromstring(xml) + item = root.find(xpath) + return ElementTree.tostring(item, encoding="utf-8")

This code can be removed as you are using lxml now

On 02/13/2014 04:47 PM, Daniel Barboza wrote:

From: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

This patch implements the mockmodel class to simulate lookup, add, remove, update of devices in a guest vm. Also, it adds test cases to test the rest API.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

Minor changes/improvements based on ML feedback Removing hard disk tests from tests/tests_rest.py

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 66 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/test_rest.py | 56 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)

diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index be0ae4b..934f66a 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -539,6 +539,51 @@ class MockModel(object): def networks_get_list(self): return sorted(self._mock_networks.keys())

+ def storages_create(self, vm_name, params): + path = params.get('path') + if path.startswith('/') and not os.path.exists(path): + msg = "Path specified for device is not valid" + raise InvalidParameter(msg) +

You need to change the error message to the refactor expection method

+ dom = self._get_vm(vm_name) + dev = params.get('dev', None) + if dev and dev in self.storages_get_list(vm_name): + return OperationFailed('Device name already in use.') + if not dev: + return OperationFailed('Must specify a device name.')

Same here and all cases you raise an exception

+ vmdev = MockVMStorageDevice(params) + dom.storagedevices[params['dev']] = vmdev + return params['dev'] + + def storages_get_list(self, vm_name): + dom = self._get_vm(vm_name) + return dom.storagedevices.keys() + + def storage_lookup(self, vm_name, dev_name): + dom = self._get_vm(vm_name) + if dev_name not in self.storages_get_list(vm_name): + msg = 'The storage device "%s" does not exist in the guest "%s"' \ + % (dev_name,vm_name) + raise NotFoundError(msg)

^

+ return dom.storagedevices.get('dev_name').info + + def storage_delete(self, vm_name, dev_name): + dom = self._get_vm(vm_name) + if dev_name not in self.storages_get_list(vm_name): + msg = 'The storage device "%s" does not exist in the guest "%s"' \ + % (dev_name,vm_name) + raise NotFoundError(msg)

^

+ dom.storagedevices.pop(dev_name) + + def storage_update(self, vm_name, dev_name, params): + try: + dom = self._get_vm(vm_name) + dom.storagedevices[dev_name].info.update(params) + except Exception as e: + msg = 'Was not possible to update storage device: %s' % e.message + raise OperationFailed(e.message)

^

+ return dev_name + def vmifaces_create(self, vm, params): if (params["type"] == "network" and params["network"] not in self.networks_get_list()): @@ -721,6 +766,24 @@ class MockVMTemplate(VMTemplate): return disk_paths

+class MockVMStorageDevice(object): + def __init__(self, params): + # Defaults + if params['dev'] == 'hda': + self.info = {'dev': params.get('dev'), + 'type': 'disk', + 'path': '/tmp/myimage.img'} + elif params['dev'] == 'hdc': + self.info = {'dev': params.get('dev'), + 'type': 'cdrom', + 'path': ''} + # New devices + else: + self.info = {'dev': params.get('dev'), + 'type': params.get('type'), + 'path': params.get('path')} + + class MockVMIface(object): counter = 0

@@ -746,6 +809,9 @@ class MockVM(object): self.disk_paths = [] self.networks = template_info['networks'] ifaces = [MockVMIface(net) for net in self.networks] + default_devices = [{'dev':'hda'}, {'dev':'hdc'}] + self.storagedevices = dict([(dev['dev'], MockVMStorageDevice(dev)) \ + for dev in default_devices]) self.ifaces = dict([(iface.info['mac'], iface) for iface in ifaces]) self.info = {'state': 'shutoff', 'stats': "{'cpu_utilization': 20, 'net_throughput' : 35, \ diff --git a/tests/test_rest.py b/tests/test_rest.py index b72ff0b..62973fa 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -341,6 +341,62 @@ class RestTests(unittest.TestCase): resp = self.request('/templates/test', '{}', 'DELETE') self.assertEquals(204, resp.status)

+ def test_vm_storage_devices(self): + + with RollbackContext() as rollback: + # Create a template as a base for our VMs + req = json.dumps({'name': 'test', 'cdrom': '/nonexistent.iso'}) + resp = self.request('/templates', req, 'POST') + self.assertEquals(201, resp.status) + # Delete the template + rollback.prependDefer(self.request, + '/templates/test', '{}', 'DELETE') + + # Create a VM with default args + req = json.dumps({'name': 'test-vm', + 'template': '/templates/test'}) + resp = self.request('/vms', req, 'POST') + self.assertEquals(201, resp.status) + # Delete the VM + rollback.prependDefer(self.request, + '/vms/test-vm', '{}', 'DELETE') + + # Attach cdrom with nonexistent iso + req = json.dumps({'dev': 'hdx', + 'type': 'cdrom', + 'path': '/tmp/nonexistent.iso'}) + resp = self.request('/vms/test-vm/storages', req, 'POST') + self.assertEquals(400, resp.status) + + # Attach a cdrom with existent dev name + open('/tmp/existent.iso', 'w').close() + req = json.dumps({'dev': 'hdx', + 'type': 'cdrom', + 'path': '/tmp/existent.iso'}) + resp = self.request('/vms/test-vm/storages', req, 'POST') + self.assertEquals(201, resp.status) + # Delete the file and cdrom + rollback.prependDefer(self.request, + '/vms/test-vm/storages/hdx', '{}', 'DELETE') + os.remove('/tmp/existent.iso') + + # Change path of storage cdrom + req = json.dumps({'path': 'http://myserver.com/myiso.iso'}) + resp = self.request('/vms/test-vm/storages/hdx', req, 'PUT') + self.assertEquals(200, resp.status) + + # Test GET + devs = json.loads(self.request('/vms/test-vm/storages').read()) + self.assertEquals(3, len(devs)) + + # Detach storage cdrom + resp = self.request('/vms/test-vm/storages/hdx', '{}', 'DELETE') + self.assertEquals(204, resp.status) + + # Test GET + devs = json.loads(self.request('/vms/test-vm/storages').read()) + self.assertEquals(2, len(devs)) + def test_vm_iface(self):

with RollbackContext() as rollback:

On 02/13/2014 10:23 PM, Daniel H Barboza wrote:

On 02/13/2014 10:19 PM, Aline Manera wrote:

...

On 02/13/2014 04:47 PM, Daniel Barboza wrote:

...

From: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

This patch implements the mockmodel class to simulate lookup, add, remove, update of devices in a guest vm. Also, it adds test cases to test the rest API.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com>

Minor changes/improvements based on ML feedback Removing hard disk tests from tests/tests_rest.py

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 66 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/test_rest.py | 56 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)

diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index be0ae4b..934f66a 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -539,6 +539,51 @@ class MockModel(object): def networks_get_list(self): return sorted(self._mock_networks.keys())

+ def storages_create(self, vm_name, params): + path = params.get('path') + if path.startswith('/') and not os.path.exists(path): + msg = "Path specified for device is not valid" + raise InvalidParameter(msg) +

You need to change the error message to the refactor expection method Yeah I forgot to check out mockmodel.py exceptions

I'll fix it in v5.

Actually I did this but it was on a different patch ... the one you said to "join it". I didn't notice that patch 4 and 5 changed the same file. Anyway, v5 will fix it :)

...

...

+ dom = self._get_vm(vm_name) + dev = params.get('dev', None) + if dev and dev in self.storages_get_list(vm_name): + return OperationFailed('Device name already in use.') + if not dev: + return OperationFailed('Must specify a device name.')

Same here and all cases you raise an exception

...

+ vmdev = MockVMStorageDevice(params) + dom.storagedevices[params['dev']] = vmdev + return params['dev'] + + def storages_get_list(self, vm_name): + dom = self._get_vm(vm_name) + return dom.storagedevices.keys() + + def storage_lookup(self, vm_name, dev_name): + dom = self._get_vm(vm_name) + if dev_name not in self.storages_get_list(vm_name): + msg = 'The storage device "%s" does not exist in the guest "%s"' \ + % (dev_name,vm_name) + raise NotFoundError(msg)

^

...

+ return dom.storagedevices.get('dev_name').info + + def storage_delete(self, vm_name, dev_name): + dom = self._get_vm(vm_name) + if dev_name not in self.storages_get_list(vm_name): + msg = 'The storage device "%s" does not exist in the guest "%s"' \ + % (dev_name,vm_name) + raise NotFoundError(msg)

^

...

+ dom.storagedevices.pop(dev_name) + + def storage_update(self, vm_name, dev_name, params): + try: + dom = self._get_vm(vm_name) + dom.storagedevices[dev_name].info.update(params) + except Exception as e: + msg = 'Was not possible to update storage device: %s' % e.message + raise OperationFailed(e.message)

^

...

+ return dev_name + def vmifaces_create(self, vm, params): if (params["type"] == "network" and params["network"] not in self.networks_get_list()): @@ -721,6 +766,24 @@ class MockVMTemplate(VMTemplate): return disk_paths

+class MockVMStorageDevice(object): + def __init__(self, params): + # Defaults + if params['dev'] == 'hda': + self.info = {'dev': params.get('dev'), + 'type': 'disk', + 'path': '/tmp/myimage.img'} + elif params['dev'] == 'hdc': + self.info = {'dev': params.get('dev'), + 'type': 'cdrom', + 'path': ''} + # New devices + else: + self.info = {'dev': params.get('dev'), + 'type': params.get('type'), + 'path': params.get('path')} + + class MockVMIface(object): counter = 0

@@ -746,6 +809,9 @@ class MockVM(object): self.disk_paths = [] self.networks = template_info['networks'] ifaces = [MockVMIface(net) for net in self.networks] + default_devices = [{'dev':'hda'}, {'dev':'hdc'}] + self.storagedevices = dict([(dev['dev'], MockVMStorageDevice(dev)) \ + for dev in default_devices]) self.ifaces = dict([(iface.info['mac'], iface) for iface in ifaces]) self.info = {'state': 'shutoff', 'stats': "{'cpu_utilization': 20, 'net_throughput' : 35, \ diff --git a/tests/test_rest.py b/tests/test_rest.py index b72ff0b..62973fa 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -341,6 +341,62 @@ class RestTests(unittest.TestCase): resp = self.request('/templates/test', '{}', 'DELETE') self.assertEquals(204, resp.status)

+ def test_vm_storage_devices(self): + + with RollbackContext() as rollback: + # Create a template as a base for our VMs + req = json.dumps({'name': 'test', 'cdrom': '/nonexistent.iso'}) + resp = self.request('/templates', req, 'POST') + self.assertEquals(201, resp.status) + # Delete the template + rollback.prependDefer(self.request, + '/templates/test', '{}', 'DELETE') + + # Create a VM with default args + req = json.dumps({'name': 'test-vm', + 'template': '/templates/test'}) + resp = self.request('/vms', req, 'POST') + self.assertEquals(201, resp.status) + # Delete the VM + rollback.prependDefer(self.request, + '/vms/test-vm', '{}', 'DELETE') + + # Attach cdrom with nonexistent iso + req = json.dumps({'dev': 'hdx', + 'type': 'cdrom', + 'path': '/tmp/nonexistent.iso'}) + resp = self.request('/vms/test-vm/storages', req, 'POST') + self.assertEquals(400, resp.status) + + # Attach a cdrom with existent dev name + open('/tmp/existent.iso', 'w').close() + req = json.dumps({'dev': 'hdx', + 'type': 'cdrom', + 'path': '/tmp/existent.iso'}) + resp = self.request('/vms/test-vm/storages', req, 'POST') + self.assertEquals(201, resp.status) + # Delete the file and cdrom + rollback.prependDefer(self.request, + '/vms/test-vm/storages/hdx', '{}', 'DELETE') + os.remove('/tmp/existent.iso') + + # Change path of storage cdrom + req = json.dumps({'path': 'http://myserver.com/myiso.iso'}) + resp = self.request('/vms/test-vm/storages/hdx', req, 'PUT') + self.assertEquals(200, resp.status) + + # Test GET + devs = json.loads(self.request('/vms/test-vm/storages').read()) + self.assertEquals(3, len(devs)) + + # Detach storage cdrom + resp = self.request('/vms/test-vm/storages/hdx', '{}', 'DELETE') + self.assertEquals(204, resp.status) + + # Test GET + devs = json.loads(self.request('/vms/test-vm/storages').read()) + self.assertEquals(2, len(devs)) + def test_vm_iface(self):

with RollbackContext() as rollback:

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

On 02/13/2014 04:47 PM, Daniel Barboza wrote:

From: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>

Adding model tests to verify the proper behavior of CDROM related model functions - add, remove and update. Changed mockmodel functions to use the new exception model.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 21 ++++++++----------- tests/test_model.py | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 12 deletions(-)

diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 934f66a..e4b94a4 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -542,15 +542,15 @@ class MockModel(object): def storages_create(self, vm_name, params): path = params.get('path') if path.startswith('/') and not os.path.exists(path): - msg = "Path specified for device is not valid" - raise InvalidParameter(msg) + raise InvalidParameter("KCHCDROM0005E")

dom = self._get_vm(vm_name) dev = params.get('dev', None) if dev and dev in self.storages_get_list(vm_name): - return OperationFailed('Device name already in use.') + return OperationFailed("KCHCDROM0004E", {'dev_name': dev, + 'vm_name': vm_name}) if not dev: - return OperationFailed('Must specify a device name.') + return OperationFailed("KCHCDROM0008E") vmdev = MockVMStorageDevice(params) dom.storagedevices[params['dev']] = vmdev return params['dev'] @@ -562,17 +562,15 @@ class MockModel(object): def storage_lookup(self, vm_name, dev_name): dom = self._get_vm(vm_name) if dev_name not in self.storages_get_list(vm_name): - msg = 'The storage device "%s" does not exist in the guest "%s"' \ - % (dev_name,vm_name) - raise NotFoundError(msg) + raise NotFoundError("KCHCDROM0007E", {'dev_name': dev_name, + 'vm_name': vm_name}) return dom.storagedevices.get('dev_name').info

def storage_delete(self, vm_name, dev_name): dom = self._get_vm(vm_name) if dev_name not in self.storages_get_list(vm_name): - msg = 'The storage device "%s" does not exist in the guest "%s"' \ - % (dev_name,vm_name) - raise NotFoundError(msg) + raise NotFoundError("KCHCDROM0007E", {'dev_name': dev_name, + 'vm_name': vm_name}) dom.storagedevices.pop(dev_name)

def storage_update(self, vm_name, dev_name, params): @@ -580,8 +578,7 @@ class MockModel(object): dom = self._get_vm(vm_name) dom.storagedevices[dev_name].info.update(params) except Exception as e: - msg = 'Was not possible to update storage device: %s' % e.message - raise OperationFailed(e.message) + raise OperationFailed("KCHCDROM0009E", {'error': e.message}) return dev_name

Ops... join it when you change the code at first time

def vmifaces_create(self, vm, params): diff --git a/tests/test_model.py b/tests/test_model.py index b374d2d..58a814b 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -163,6 +163,62 @@ class ModelTests(unittest.TestCase): self.assertEquals("virtio", iface["model"])

@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_vm_cdrom(self): + inst = model.Model(objstore_loc=self.tmp_store) + with RollbackContext() as rollback: + vm_name = 'kimchi-cdrom' + params = {'name': 'test', 'disks': []} + inst.templates_create(params) + rollback.prependDefer(inst.template_delete, 'test') + params = {'name': vm_name, 'template': '/templates/test'} + inst.vms_create(params) + rollback.prependDefer(inst.vm_delete, vm_name) + + # dummy .iso files + iso_path = '/tmp/existent.iso' + iso_path2 = '/tmp/existent2.iso' + open(iso_path, 'w').close() + open(iso_path2, 'w').close() + wrong_iso_path = '/nonexistent.iso' + rollback.prependDefer(os.remove, iso_path) + rollback.prependDefer(os.remove, iso_path2) + + # storage count before adding new cdroms + prev_count = len(inst.storages_get_list(vm_name)) + + # Create a cdrom + cdrom_args = {"type": "cdrom", + "path": iso_path} + cdrom_dev = inst.storages_create(vm_name, cdrom_args) + storage_list = inst.storages_get_list(vm_name) + self.assertEquals(prev_count + 1, len(storage_list)) + + # create a cdrom with existing dev_name + cdrom_args['dev'] = cdrom_dev + self.assertRaises(OperationFailed, inst.storages_create, + vm_name, cdrom_args) + + # update path of existing cd with + # non existent iso + self.assertRaises(OperationFailed, inst.storage_update, + vm_name, cdrom_dev, {'path': wrong_iso_path}) + + # update path of existing cd with + # existent iso + inst.storage_update(vm_name, cdrom_dev, {'path': iso_path2}) + cdrom_info = inst.storage_lookup(vm_name, cdrom_dev) + self.assertEquals(iso_path2, cdrom_info['path']) + + # removing non existent cdrom + self.assertRaises(NotFoundError, inst.storage_delete, vm_name, + "fakedev") + + # removing valid cdrom + inst.storage_delete(vm_name, cdrom_dev) + storage_list = inst.storages_get_list(vm_name) + self.assertEquals(prev_count, len(storage_list)) + + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_storage_provisioning(self): inst = model.Model(objstore_loc=self.tmp_store)

Join this patch when you use it at first time On 02/13/2014 04:47 PM, Daniel Barboza wrote:

From: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>

Externalized exception/error strings related to the CDROM Management backend.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 03d1052..6264e97 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -181,4 +181,15 @@ messages = { "KCHUTILS0001E": _("Invalid URI %(uri)s"), "KCHUTILS0002E": _("Timeout while running command '%(cmd)s' after %(seconds)s seconds"), "KCHUTILS0003E": _("Unable to choose a virutal machine name"), + "KCHCDROM0001E": _("Invalid cd-rom device name"), + "KCHCDROM0002E": _("Invalid storage type. Types supported: 'cdrom'"), + "KCHCDROM0003E": _("CDROM path must be a valid filesystem, http or ftp path"), + "KCHCDROM0004E": _("Device name %(dev_name)s already exists in vm %(vm_name)s"), + "KCHCDROM0005E": _("Path specified for the device is not valid"), + "KCHCDROM0006E": _("Can't specify a directory for a cd-rom device path"), + "KCHCDROM0007E": _("The storage device %(dev_name)s does not exist in the guest %(vm_name)s"), + "KCHCDROM0008E": _("Must specify a device name"), + "KCHCDROM0009E": _("Error while updating storage device: %(error)s"), + "KCHCDROM0010E": _("Error while removing storage device: %(error)s"), + "KCHCDROM0011E": _("Error while creating new storage device: %(error)s"), }