[PATCH V3 0/5] Add spice backend support for kimchi

v3-v2: 1. Rebase to upstream 2. Improve/simplify some code according to reviewers' advice. 3. Reimplement parameters validation using jsonschema. 4. Remove unnecessary test code in model.py. 5. Split patch to several smaller commits according to logic. v2-v1: Rebased to upstream and resend v1: Add spice backend support for kimchi apporc (5): Add spice backend support for kimchi Update mockmodel for spice support Validate graphics parameters input by users Update test case for graphics support Add graphics parameters description in API.md docs/API.md | 43 ++++++++++++++++++++++- src/kimchi/API.json | 33 ++++++++++++++++++ src/kimchi/controller.py | 14 +++++--- src/kimchi/mockmodel.py | 49 ++++++++++++++++++--------- src/kimchi/model.py | 41 +++++++++++++--------- src/kimchi/osinfo.py | 5 +-- src/kimchi/vmtemplate.py | 32 ++++++++++++++++-- tests/test_mockmodel.py | 2 ++ tests/test_model.py | 27 ++++++++++++++- tests/test_rest.py | 88 +++++++++++++++++++++++++++++++++++++++++++++--- tests/test_vmtemplate.py | 42 ++++++++++++++++++++--- 11 files changed, 325 insertions(+), 51 deletions(-) -- 1.8.1.2

1. Add spice support for kimchi template Now we can specify a spice parameter when creating a template and updating it. 2. Add spice support for kimchi vm When creating a vm, we have a chance to change the graphics configuration to override the value from the template. 3. Expose the real port for vnc/spice, so that users can connect vm with popular vnc/spice client tool. 4. Spice-vdagent support is added too, so that when users happended to install spice-tool/spice-vdagent in their vm, they will get improved spice graphics effect immediately. 5. Specifying graphics parameters when updating vm is not supported yet, it will be added separately in another patch. Signed-off-by: apporc <appleorchard2000@gmail.com> --- src/kimchi/controller.py | 10 +++++++--- src/kimchi/model.py | 41 +++++++++++++++++++++++++---------------- src/kimchi/osinfo.py | 5 +++-- src/kimchi/vmtemplate.py | 32 ++++++++++++++++++++++++++++++-- 4 files changed, 65 insertions(+), 23 deletions(-) diff --git a/src/kimchi/controller.py b/src/kimchi/controller.py index 2940278..dacaa6a 100644 --- a/src/kimchi/controller.py +++ b/src/kimchi/controller.py @@ -384,7 +384,10 @@ class VM(Resource): 'screenshot': self.info['screenshot'], 'icon': self.info['icon'], 'graphics': {'type': self.info['graphics']['type'], - 'port': self.info['graphics']['port']}} + 'listen': self.info['graphics']['listen'], + 'port': self.info['graphics']['port'], + 'real_port': self.info['graphics']['real_port']} + } class VMScreenShot(Resource): @@ -406,7 +409,7 @@ class Template(Resource): super(Template, self).__init__(model, ident) self.update_params = ["name", "folder", "icon", "os_distro", "storagepool", "os_version", "cpus", - "memory", "cdrom", "disks"] + "memory", "cdrom", "disks", "graphics"] self.uri_fmt = "/templates/%s" @property @@ -420,7 +423,8 @@ class Template(Resource): 'cdrom': self.info['cdrom'], 'disks': self.info['disks'], 'storagepool': self.info['storagepool'], - 'folder': self.info.get('folder', [])} + 'folder': self.info.get('folder', []), + 'graphics': self.info['graphics']} class Interfaces(Collection): diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 3bc5d6d..3ee949d 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -503,9 +503,10 @@ class Model(object): info = dom.info() state = Model.dom_state_map[info[0]] screenshot = None - graphics_type, _ = self._vm_get_graphics(name) # 'port' must remain None until a connect call is issued - graphics_port = (self.graphics_ports.get(name, None) if state == 'running' + graphics_type, graphics_listen, real_port = self._vm_get_graphics(name) + real_port = real_port if state == 'running' else None + graphics_port = (self.graphics_ports.get(name) if state == 'running' else None) try: if state == 'running': @@ -539,7 +540,12 @@ class Model(object): 'cpus': info[3], 'screenshot': screenshot, 'icon': icon, - 'graphics': {"type": graphics_type, "port": graphics_port}} + 'graphics': {"type": graphics_type, + "listen": graphics_listen, + "port": graphics_port, + "real_port": real_port + } + } def _vm_get_disk_paths(self, dom): xml = dom.XMLDesc(0) @@ -581,24 +587,23 @@ class Model(object): expr = "/domain/devices/graphics/@type" res = xmlutils.xpath_get_text(xml, expr) graphics_type = res[0] if res else None - port = None + expr = "/domain/devices/graphics/@listen" + res = xmlutils.xpath_get_text(xml, expr) + graphics_listen = res[0] if res else None + graphics_port = None if graphics_type: expr = "/domain/devices/graphics[@type='%s']/@port" % graphics_type res = xmlutils.xpath_get_text(xml, expr) - port = int(res[0]) if res else None - # FIX ME - # graphics_type should be 'vnc' or None. 'spice' should only be - # returned if we support it in the future. - graphics_type = None if graphics_type != "vnc" else graphics_type - return graphics_type, port + graphics_port = int(res[0]) if res else None + return graphics_type, graphics_listen, graphics_port def vm_connect(self, name): - graphics, port = self._vm_get_graphics(name) - if graphics == "vnc" and port != None: - port = vnc.new_ws_proxy(port) - self.graphics_ports[name] = port + graphics_type, graphics_listen, graphics_port \ + = self._vm_get_graphics(name) + if graphics_port is not None: + self.graphics_ports[name] = vnc.new_ws_proxy(graphics_port) else: - raise OperationFailed("Unable to find VNC port in %s" % name) + raise OperationFailed("Unable to find connection port in %s" % name) def vms_create(self, params): conn = self.conn.get() @@ -629,8 +634,12 @@ class Model(object): session.store('vm', vm_uuid, {'icon': icon}) libvirt_stream = False if len(self.libvirt_stream_protocols) == 0 else True + graphics = params.get('graphics') - xml = t.to_vm_xml(name, vm_uuid, libvirt_stream, self.qemu_stream_dns) + xml = t.to_vm_xml(name, vm_uuid, + libvirt_stream=libvirt_stream, + qemu_stream_dns=self.qemu_stream_dns, + graphics=graphics) try: dom = conn.defineXML(xml.encode('utf-8')) except libvirt.libvirtError as e: diff --git a/src/kimchi/osinfo.py b/src/kimchi/osinfo.py index 20a93a6..b8ec244 100644 --- a/src/kimchi/osinfo.py +++ b/src/kimchi/osinfo.py @@ -171,7 +171,8 @@ isolinks = { } defaults = {'network': 'default', 'storagepool': '/storagepools/default', - 'domain': 'kvm', 'arch': os.uname()[4] + 'domain': 'kvm', 'arch': os.uname()[4], + 'graphics':{'type': 'vnc', 'listen': '0.0.0.0'} } def lookup(distro, version): @@ -184,7 +185,7 @@ def lookup(distro, version): for name, entry in osinfo: # Test if this entry is a valid match if entry['version'](distro, version): - params = copy.copy(defaults) + params = copy.deepcopy(defaults) params['os_distro'] = distro params['os_version'] = version params.update(entry) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index dd43faa..6901e0a 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -25,6 +25,7 @@ import string import socket import urllib import urlparse +import copy from kimchi import isoinfo @@ -74,6 +75,11 @@ class VMTemplate(object): self.info.update(entry) # Override with the passed in parameters + graph_args = args.get('graphics') + if graph_args: + graphics = dict(self.info['graphics']) + graphics.update(graph_args) + args['graphics'] = graphics self.info.update(args) def _get_cdrom_xml(self, libvirt_stream, qemu_stream_dns): @@ -155,6 +161,24 @@ class VMTemplate(object): """ % params return ret + def _get_graphics_xml(self, params): + graphics_xml = """ + <graphics type='%(type)s' autoport='yes' listen='%(listen)s'> + </graphics> + """ + spicevmc_xml = """ + <channel type='spicevmc'> + <target type='virtio' name='com.redhat.spice.0'/> + </channel> + """ + graphics = dict(self.info['graphics']) + if params: + graphics.update(params) + graphics_xml = graphics_xml % graphics + if graphics['type'] == 'spice': + graphics_xml = graphics_xml + spicevmc_xml + return graphics_xml + def to_volume_list(self, vm_uuid): storage_path = self._get_storage_path() ret = [] @@ -182,7 +206,7 @@ class VMTemplate(object): ret.append(info) return ret - def to_vm_xml(self, vm_name, vm_uuid, libvirt_stream = False, qemu_stream_dns = False): + def to_vm_xml(self, vm_name, vm_uuid, **kwargs): params = dict(self.info) params['name'] = vm_name params['uuid'] = vm_uuid @@ -190,7 +214,11 @@ class VMTemplate(object): params['qemu-namespace'] = '' params['cdroms'] = '' params['qemu-stream-cmdline'] = '' + graphics = kwargs.get('graphics') + params['graphics'] = self._get_graphics_xml(graphics) + qemu_stream_dns = kwargs.get('qemu_stream_dns', False) + libvirt_stream = kwargs.get('libvirt_stream', False) cdrom_xml = self._get_cdrom_xml(libvirt_stream, qemu_stream_dns) if not libvirt_stream and params.get('iso_stream', False): params['qemu-namespace'] = QEMU_NAMESPACE @@ -224,7 +252,7 @@ class VMTemplate(object): <source network='%(network)s'/> <model type='%(nic_model)s'/> </interface> - <graphics type='vnc' /> + %(graphics)s <sound model='ich6' /> <memballoon model='virtio' /> </devices> -- 1.8.1.2

On 12/22/2013 06:43 AM, apporc wrote:
1. Add spice support for kimchi template Now we can specify a spice parameter when creating a template and updating it.
2. Add spice support for kimchi vm When creating a vm, we have a chance to change the graphics configuration to override the value from the template.
3. Expose the real port for vnc/spice, so that users can connect vm with popular vnc/spice client tool.
4. Spice-vdagent support is added too, so that when users happended to install spice-tool/spice-vdagent in their vm, they will get improved spice graphics effect immediately.
5. Specifying graphics parameters when updating vm is not supported yet, it will be added separately in another patch.
Signed-off-by: apporc <appleorchard2000@gmail.com> --- src/kimchi/controller.py | 10 +++++++--- src/kimchi/model.py | 41 +++++++++++++++++++++++++---------------- src/kimchi/osinfo.py | 5 +++-- src/kimchi/vmtemplate.py | 32 ++++++++++++++++++++++++++++++-- 4 files changed, 65 insertions(+), 23 deletions(-)
diff --git a/src/kimchi/controller.py b/src/kimchi/controller.py index 2940278..dacaa6a 100644 --- a/src/kimchi/controller.py +++ b/src/kimchi/controller.py @@ -384,7 +384,10 @@ class VM(Resource): 'screenshot': self.info['screenshot'], 'icon': self.info['icon'], 'graphics': {'type': self.info['graphics']['type'], - 'port': self.info['graphics']['port']}} + 'listen': self.info['graphics']['listen'], + 'port': self.info['graphics']['port'], + 'real_port': self.info['graphics']['real_port']} + }
class VMScreenShot(Resource): @@ -406,7 +409,7 @@ class Template(Resource): super(Template, self).__init__(model, ident) self.update_params = ["name", "folder", "icon", "os_distro", "storagepool", "os_version", "cpus", - "memory", "cdrom", "disks"] + "memory", "cdrom", "disks", "graphics"] self.uri_fmt = "/templates/%s"
@property @@ -420,7 +423,8 @@ class Template(Resource): 'cdrom': self.info['cdrom'], 'disks': self.info['disks'], 'storagepool': self.info['storagepool'], - 'folder': self.info.get('folder', [])} + 'folder': self.info.get('folder', []), + 'graphics': self.info['graphics']}
class Interfaces(Collection): diff --git a/src/kimchi/model.py b/src/kimchi/model.py index 3bc5d6d..3ee949d 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -503,9 +503,10 @@ class Model(object): info = dom.info() state = Model.dom_state_map[info[0]] screenshot = None - graphics_type, _ = self._vm_get_graphics(name) # 'port' must remain None until a connect call is issued - graphics_port = (self.graphics_ports.get(name, None) if state == 'running' + graphics_type, graphics_listen, real_port = self._vm_get_graphics(name) + real_port = real_port if state == 'running' else None
+ graphics_port = (self.graphics_ports.get(name) if state == 'running' else None)
Align it with (
try: if state == 'running': @@ -539,7 +540,12 @@ class Model(object): 'cpus': info[3], 'screenshot': screenshot, 'icon': icon, - 'graphics': {"type": graphics_type, "port": graphics_port}} + 'graphics': {"type": graphics_type, + "listen": graphics_listen, + "port": graphics_port, + "real_port": real_port + } + }
def _vm_get_disk_paths(self, dom): xml = dom.XMLDesc(0) @@ -581,24 +587,23 @@ class Model(object): expr = "/domain/devices/graphics/@type" res = xmlutils.xpath_get_text(xml, expr) graphics_type = res[0] if res else None - port = None + expr = "/domain/devices/graphics/@listen" + res = xmlutils.xpath_get_text(xml, expr) + graphics_listen = res[0] if res else None + graphics_port = None if graphics_type: expr = "/domain/devices/graphics[@type='%s']/@port" % graphics_type res = xmlutils.xpath_get_text(xml, expr) - port = int(res[0]) if res else None - # FIX ME - # graphics_type should be 'vnc' or None. 'spice' should only be - # returned if we support it in the future. - graphics_type = None if graphics_type != "vnc" else graphics_type - return graphics_type, port + graphics_port = int(res[0]) if res else None + return graphics_type, graphics_listen, graphics_port
def vm_connect(self, name): - graphics, port = self._vm_get_graphics(name) - if graphics == "vnc" and port != None: - port = vnc.new_ws_proxy(port) - self.graphics_ports[name] = port + graphics_type, graphics_listen, graphics_port \ + = self._vm_get_graphics(name) + if graphics_port is not None: + self.graphics_ports[name] = vnc.new_ws_proxy(graphics_port) else: - raise OperationFailed("Unable to find VNC port in %s" % name) + raise OperationFailed("Unable to find connection port in %s" % name)
def vms_create(self, params): conn = self.conn.get() @@ -629,8 +634,12 @@ class Model(object): session.store('vm', vm_uuid, {'icon': icon})
libvirt_stream = False if len(self.libvirt_stream_protocols) == 0 else True + graphics = params.get('graphics')
- xml = t.to_vm_xml(name, vm_uuid, libvirt_stream, self.qemu_stream_dns) + xml = t.to_vm_xml(name, vm_uuid, + libvirt_stream=libvirt_stream, + qemu_stream_dns=self.qemu_stream_dns, + graphics=graphics) try: dom = conn.defineXML(xml.encode('utf-8')) except libvirt.libvirtError as e: diff --git a/src/kimchi/osinfo.py b/src/kimchi/osinfo.py index 20a93a6..b8ec244 100644 --- a/src/kimchi/osinfo.py +++ b/src/kimchi/osinfo.py @@ -171,7 +171,8 @@ isolinks = { }
defaults = {'network': 'default', 'storagepool': '/storagepools/default', - 'domain': 'kvm', 'arch': os.uname()[4] + 'domain': 'kvm', 'arch': os.uname()[4], + 'graphics':{'type': 'vnc', 'listen': '0.0.0.0'} }
def lookup(distro, version): @@ -184,7 +185,7 @@ def lookup(distro, version): for name, entry in osinfo: # Test if this entry is a valid match if entry['version'](distro, version): - params = copy.copy(defaults) + params = copy.deepcopy(defaults) params['os_distro'] = distro params['os_version'] = version params.update(entry) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index dd43faa..6901e0a 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -25,6 +25,7 @@ import string import socket import urllib import urlparse
+import copy
It isn't being used
from kimchi import isoinfo @@ -74,6 +75,11 @@ class VMTemplate(object): self.info.update(entry)
# Override with the passed in parameters + graph_args = args.get('graphics') + if graph_args: + graphics = dict(self.info['graphics']) + graphics.update(graph_args) + args['graphics'] = graphics self.info.update(args)
def _get_cdrom_xml(self, libvirt_stream, qemu_stream_dns): @@ -155,6 +161,24 @@ class VMTemplate(object): """ % params return ret
+ def _get_graphics_xml(self, params): + graphics_xml = """ + <graphics type='%(type)s' autoport='yes' listen='%(listen)s'> + </graphics> + """ + spicevmc_xml = """ + <channel type='spicevmc'> + <target type='virtio' name='com.redhat.spice.0'/> + </channel> + """ + graphics = dict(self.info['graphics']) + if params: + graphics.update(params) + graphics_xml = graphics_xml % graphics + if graphics['type'] == 'spice': + graphics_xml = graphics_xml + spicevmc_xml + return graphics_xml + def to_volume_list(self, vm_uuid): storage_path = self._get_storage_path() ret = [] @@ -182,7 +206,7 @@ class VMTemplate(object): ret.append(info) return ret
- def to_vm_xml(self, vm_name, vm_uuid, libvirt_stream = False, qemu_stream_dns = False): + def to_vm_xml(self, vm_name, vm_uuid, **kwargs): params = dict(self.info) params['name'] = vm_name params['uuid'] = vm_uuid @@ -190,7 +214,11 @@ class VMTemplate(object): params['qemu-namespace'] = '' params['cdroms'] = '' params['qemu-stream-cmdline'] = '' + graphics = kwargs.get('graphics') + params['graphics'] = self._get_graphics_xml(graphics)
+ qemu_stream_dns = kwargs.get('qemu_stream_dns', False) + libvirt_stream = kwargs.get('libvirt_stream', False) cdrom_xml = self._get_cdrom_xml(libvirt_stream, qemu_stream_dns) if not libvirt_stream and params.get('iso_stream', False): params['qemu-namespace'] = QEMU_NAMESPACE @@ -224,7 +252,7 @@ class VMTemplate(object): <source network='%(network)s'/> <model type='%(nic_model)s'/> </interface> - <graphics type='vnc' /> + %(graphics)s <sound model='ich6' /> <memballoon model='virtio' /> </devices>

There is one patch in the mailling list from Markwu, which have code about vnc in mockmodel.py removed. But at the time i rebased this patch, that patch is not accepted yet, so i have to keep it here, so that the test can pass. See: [project-kimchi] [PATCH 4/4] Remove vnc related code in mockmodel Signed-off-by: apporc <appleorchard2000@gmail.com> --- src/kimchi/mockmodel.py | 49 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 348127a..66abec8 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -51,7 +51,6 @@ from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.objectstore import ObjectStore from kimchi.screenshot import VMScreenshot -from kimchi.utils import is_digit from kimchi.vmtemplate import VMTemplate @@ -59,19 +58,21 @@ class MockModel(object): def __init__(self, objstore_loc=None): self.reset() self.objstore = ObjectStore(objstore_loc) - self.vnc_port = 5999 self.distros = self._get_distros() # open vnc port # make it here to make sure it will be available on server startup - cmd = config.find_qemu_binary() - args = [cmd, "-vnc", ":99"] - - cmd = "ps aux | grep '%s' | grep -v grep" % " ".join(args) - proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE) + qemu = config.find_qemu_binary() + self.vnc_port = 5999 + self.spice_port = 5998 + vnc_args = [qemu, "-vnc", ":99"] + spice_args = [qemu, "-spice", "port=5998"] + cmd = 'netstat -tulnp|egrep "{0}.*qemu|{0}.*kvm"' + for item in [(vnc_args, self.vnc_port), (spice_args, self.spice_port)]: + proc = subprocess.Popen(cmd.format(item[1]), shell=True, stdout=subprocess.PIPE) if len(proc.stdout.readlines()) == 0: - p = subprocess.Popen(args, close_fds=True) + subprocess.Popen(item[0], close_fds=True) def get_capabilities(self): return {'libvirt_stream_protocols': ['http', 'https', 'ftp', 'ftps', 'tftp'], @@ -121,7 +122,14 @@ class MockModel(object): vm.info['screenshot'] = self.vmscreenshot_lookup(name) else: vm.info['screenshot'] = None - vm.info['graphics']['port'] = self._mock_graphics_ports.get(name, None) + vm.info['graphics']['port'] = self._mock_graphics_ports.get(name) + if vm.info['graphics']['type'] == 'vnc': + vm.info['graphics']['real_port'] = self.vnc_port + elif vm.info['graphics']['type'] == 'spice': + vm.info['graphics']['real_port'] = self.spice_port + else: + vm.info['graphics']['real_port'] = None + return vm.info def vm_delete(self, name): @@ -140,8 +148,12 @@ class MockModel(object): self._get_vm(name).info['state'] = 'shutoff' def vm_connect(self, name): - vnc_port = kimchi.vnc.new_ws_proxy(self.vnc_port) - self._mock_graphics_ports[name] = vnc_port + vm_info = self.vm_lookup(name) + if vm_info['grahpics']['type'] == 'vnc': + port = kimchi.vnc.new_ws_proxy(self.vnc_port) + else: + port = kimchi.vnc.new_ws_proxy(self.spice_port) + self._mock_graphics_ports[name] = port def vms_create(self, params): t_name = kimchi.model.template_name_from_uri(params['template']) @@ -159,8 +171,13 @@ class MockModel(object): t = self._get_template(t_name, vm_overrides) t.validate() - vm = MockVM(vm_uuid, name, t.info) - icon = t.info.get('icon') + t_info = copy.deepcopy(t.info) + graphics = params.get('graphics') + if graphics: + t_info.update({'graphics': graphics}) + + vm = MockVM(vm_uuid, name, t_info) + icon = t_info.get('icon') if icon: vm.info['icon'] = icon @@ -210,7 +227,6 @@ class MockModel(object): new_t.update(params) ident = name - new_storagepool = new_t.get(u'storagepool', '') try: self._get_storagepool(kimchi.model.pool_name_from_uri(new_storagepool)) @@ -628,7 +644,10 @@ class MockVM(object): 'memory': template_info['memory'], 'cpus': template_info['cpus'], 'icon': None, - 'graphics': {'type': 'vnc', 'port': None}} + 'graphics': {'type': 'vnc', 'listen': '0.0.0.0', 'port': None, + 'real_port': None} + } + self.info['graphics'].update(template_info['graphics']) class MockStoragePool(object): -- 1.8.1.2

1. Validate graphics parameters from rest requester, with newly added json schema in kimchi. 2. Because we are now using jsonschema.Draft3Validator in kimchi, which doesn't support 'oneOf' property, so i can not use this: "listen": { "type": "string", "oneOf": [ { "format": "ip-address" }, { "format": "ipv6"} As ipv6 is not urgent, i think just 'ipv4' validation should be enough by now. Which is : "type": { "enum": ["spice", "vnc"] }, "listen": { "type": "string", "format": "ip-address" } We can add ipv6 validation here when the os distribution we based on provided high versions of jsonschema, or when we are about to replace Draft3Validator with Draft4Validator. 3. To use "format" property, i need to hook format_checker for Draft3Validator in controller.py. Signed-off-by: apporc <appleorchard2000@gmail.com> --- src/kimchi/API.json | 33 +++++++++++++++++++++++++++++++++ src/kimchi/controller.py | 4 ++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..0f97f0e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -21,6 +21,17 @@ "description": "Assign a specefic Storage Pool to the new VM", "type": "string", "pattern": "^/storagepools/[^/]+/?$" + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } } }, @@ -129,6 +140,17 @@ "description": "Folder", "type": "array", "items": { "type": "string" } + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } }, "additionalProperties": false @@ -202,6 +224,17 @@ "description": "Folder", "type": "array", "items": { "type": "string" } + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } }, "additionalProperties": false diff --git a/src/kimchi/controller.py b/src/kimchi/controller.py index dacaa6a..003adf2 100644 --- a/src/kimchi/controller.py +++ b/src/kimchi/controller.py @@ -26,7 +26,7 @@ import urllib2 from functools import wraps -from jsonschema import Draft3Validator, ValidationError +from jsonschema import Draft3Validator, ValidationError, FormatChecker import kimchi.template @@ -94,7 +94,7 @@ def validate_params(params, instance, action): else: return operation = model_fn(instance, action) - validator = Draft3Validator(api_schema) + validator = Draft3Validator(api_schema, format_checker=FormatChecker()) request = {operation: params} try: validator.validate(request) -- 1.8.1.2

On 12/22/2013 06:43 AM, apporc wrote:
1. Validate graphics parameters from rest requester, with newly added json schema in kimchi.
2. Because we are now using jsonschema.Draft3Validator in kimchi, which doesn't support 'oneOf' property, so i can not use this: "listen": { "type": "string", "oneOf": [ { "format": "ip-address" }, { "format": "ipv6"} As ipv6 is not urgent, i think just 'ipv4' validation should be enough by now. Which is : "type": { "enum": ["spice", "vnc"] }, "listen": { "type": "string", "format": "ip-address" } We can add ipv6 validation here when the os distribution we based on provided high versions of jsonschema, or when we are about to replace Draft3Validator with Draft4Validator.
Not sure if it can help in this case: http://stackoverflow.com/questions/14550235/json-schema-away-to-specify-an-a...
3. To use "format" property, i need to hook format_checker for Draft3Validator in controller.py.
Signed-off-by: apporc <appleorchard2000@gmail.com> --- src/kimchi/API.json | 33 +++++++++++++++++++++++++++++++++ src/kimchi/controller.py | 4 ++-- 2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..0f97f0e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -21,6 +21,17 @@ "description": "Assign a specefic Storage Pool to the new VM", "type": "string", "pattern": "^/storagepools/[^/]+/?$" + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } } }, @@ -129,6 +140,17 @@ "description": "Folder", "type": "array", "items": { "type": "string" } + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } }, "additionalProperties": false @@ -202,6 +224,17 @@ "description": "Folder", "type": "array", "items": { "type": "string" } + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } }, "additionalProperties": false diff --git a/src/kimchi/controller.py b/src/kimchi/controller.py index dacaa6a..003adf2 100644 --- a/src/kimchi/controller.py +++ b/src/kimchi/controller.py @@ -26,7 +26,7 @@ import urllib2
from functools import wraps -from jsonschema import Draft3Validator, ValidationError +from jsonschema import Draft3Validator, ValidationError, FormatChecker
import kimchi.template @@ -94,7 +94,7 @@ def validate_params(params, instance, action): else: return operation = model_fn(instance, action) - validator = Draft3Validator(api_schema) + validator = Draft3Validator(api_schema, format_checker=FormatChecker()) request = {operation: params} try: validator.validate(request)

If I have not seen wrongly, the three graphic schemas are similar. Maybe you could write it once and reuse it with "$ref" keyword. More details here: http://spacetelescope.github.io/understanding-json-schema/structuring.html On 12/22/2013 06:43 AM, apporc wrote:
1. Validate graphics parameters from rest requester, with newly added json schema in kimchi.
2. Because we are now using jsonschema.Draft3Validator in kimchi, which doesn't support 'oneOf' property, so i can not use this: "listen": { "type": "string", "oneOf": [ { "format": "ip-address" }, { "format": "ipv6"} As ipv6 is not urgent, i think just 'ipv4' validation should be enough by now. Which is : "type": { "enum": ["spice", "vnc"] }, "listen": { "type": "string", "format": "ip-address" } We can add ipv6 validation here when the os distribution we based on provided high versions of jsonschema, or when we are about to replace Draft3Validator with Draft4Validator.
3. To use "format" property, i need to hook format_checker for Draft3Validator in controller.py.
Signed-off-by: apporc <appleorchard2000@gmail.com> --- src/kimchi/API.json | 33 +++++++++++++++++++++++++++++++++ src/kimchi/controller.py | 4 ++-- 2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..0f97f0e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -21,6 +21,17 @@ "description": "Assign a specefic Storage Pool to the new VM", "type": "string", "pattern": "^/storagepools/[^/]+/?$" + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } } }, @@ -129,6 +140,17 @@ "description": "Folder", "type": "array", "items": { "type": "string" } + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } }, "additionalProperties": false @@ -202,6 +224,17 @@ "description": "Folder", "type": "array", "items": { "type": "string" } + }, + "graphics": { + "description": "Configure graphics parameters for the new VM", + "type": "object", + "properties": { + "type": { "enum": ["spice", "vnc"] }, + "listen": { + "type": "string", + "format": "ip-address" + } + } } }, "additionalProperties": false diff --git a/src/kimchi/controller.py b/src/kimchi/controller.py index dacaa6a..003adf2 100644 --- a/src/kimchi/controller.py +++ b/src/kimchi/controller.py @@ -26,7 +26,7 @@ import urllib2
from functools import wraps -from jsonschema import Draft3Validator, ValidationError +from jsonschema import Draft3Validator, ValidationError, FormatChecker
import kimchi.template @@ -94,7 +94,7 @@ def validate_params(params, instance, action): else: return operation = model_fn(instance, action) - validator = Draft3Validator(api_schema) + validator = Draft3Validator(api_schema, format_checker=FormatChecker()) request = {operation: params} try: validator.validate(request)

Because all of params validation are moved to controller.py, i removed the test code about graphics data validation in model.py. Signed-off-by: apporc <appleorchard2000@gmail.com> --- tests/test_mockmodel.py | 2 ++ tests/test_model.py | 27 ++++++++++++++- tests/test_rest.py | 88 +++++++++++++++++++++++++++++++++++++++++++++--- tests/test_vmtemplate.py | 42 ++++++++++++++++++++--- 4 files changed, 149 insertions(+), 10 deletions(-) diff --git a/tests/test_mockmodel.py b/tests/test_mockmodel.py index b819172..a806d50 100644 --- a/tests/test_mockmodel.py +++ b/tests/test_mockmodel.py @@ -143,3 +143,5 @@ class MockModelTests(unittest.TestCase): self.assertEquals(1, info['cpus']) self.assertEquals('images/icon-vm.png', info['icon']) self.assertEquals(stats_keys, set(eval(info['stats']).keys())) + self.assertEquals('vnc', info['graphics']['type']) + self.assertEquals('0.0.0.0', info['graphics']['listen']) diff --git a/tests/test_model.py b/tests/test_model.py index fb7d6dd..4086550 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -62,7 +62,6 @@ class ModelTests(unittest.TestCase): self.assertEquals(2, info['cpus']) self.assertEquals(None, info['icon']) self.assertEquals(stats_keys, set(eval(info['stats']).keys())) - self.assertRaises(NotFoundError, inst.vm_lookup, 'nosuchvm') @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') @@ -91,6 +90,32 @@ class ModelTests(unittest.TestCase): self.assertFalse('kimchi-vm' in vms) @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_vm_graphics(self): + inst = kimchi.model.Model(objstore_loc=self.tmp_store) + params = {'name': 'test', 'disks': []} + inst.templates_create(params) + with utils.RollbackContext() as rollback: + params = {'name': 'kimchi-vnc', 'template': '/templates/test'} + inst.vms_create(params) + rollback.prependDefer(inst.vm_delete, 'kimchi-vnc') + + info = inst.vm_lookup('kimchi-vnc') + self.assertEquals('vnc', info['graphics']['type']) + self.assertEquals('0.0.0.0', info['graphics']['listen']) + + graphics = {'type': 'spice', 'listen': '127.0.0.1'} + params = {'name': 'kimchi-spice', 'template': '/templates/test', + 'graphics': graphics} + inst.vms_create(params) + rollback.prependDefer(inst.vm_delete, 'kimchi-spice') + + info = inst.vm_lookup('kimchi-spice') + self.assertEquals('spice', info['graphics']['type']) + self.assertEquals('127.0.0.1', info['graphics']['listen']) + + inst.template_delete('test') + + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_storage_provisioning(self): inst = kimchi.model.Model(objstore_loc=self.tmp_store) diff --git a/tests/test_rest.py b/tests/test_rest.py index f597796..2e62ce6 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -249,9 +249,73 @@ class RestTests(unittest.TestCase): resp = self.request('/vms/test-vm', '{}', 'DELETE') self.assertEquals(204, resp.status) + # Delete the Template + resp = self.request('/templates/test', '{}', 'DELETE') + self.assertEquals(204, resp.status) + # Verify the volume was deleted self.assertHTTPStatus(404, vol_uri) + def test_vm_graphics(self): + # Create a Template + req = json.dumps({'name': 'test', 'cdrom': '/nonexistent.iso'}) + resp = self.request('/templates', req, 'POST') + self.assertEquals(201, resp.status) + + # 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) + # Verify the VM + vm = json.loads(self.request('/vms/test-vm').read()) + self.assertEquals('0.0.0.0', vm['graphics']['listen']) + self.assertEquals('vnc', vm['graphics']['type']) + # Delete the VM + resp = self.request('/vms/test-vm', '{}', 'DELETE') + self.assertEquals(204, resp.status) + + # Create a VM with specified graphics type and listen + graphics = {'type': 'vnc', 'listen': '127.0.0.1'} + req = json.dumps({'name': 'test-vm', 'template': '/templates/test', 'graphics': graphics}) + resp = self.request('/vms', req, 'POST') + self.assertEquals(201, resp.status) + # Verify the VM + vm = json.loads(self.request('/vms/test-vm').read()) + self.assertEquals('127.0.0.1', vm['graphics']['listen']) + self.assertEquals('vnc', vm['graphics']['type']) + # Delete the VM + resp = self.request('/vms/test-vm', '{}', 'DELETE') + self.assertEquals(204, resp.status) + + # Create a VM with specified graphics type and default listen + graphics = {'type': 'spice'} + req = json.dumps({'name': 'test-vm', 'template': '/templates/test', 'graphics': graphics}) + resp = self.request('/vms', req, 'POST') + self.assertEquals(201, resp.status) + # Verify the VM + vm = json.loads(self.request('/vms/test-vm').read()) + self.assertEquals('0.0.0.0', vm['graphics']['listen']) + self.assertEquals('spice', vm['graphics']['type']) + # Delete the VM + resp = self.request('/vms/test-vm', '{}', 'DELETE') + self.assertEquals(204, resp.status) + + # Try to create a VM with invalid graphics type + graphics = {'type': 'invalid'} + req = json.dumps({'name': 'test-vm', 'template': '/templates/test', 'graphics': graphics}) + resp = self.request('/vms', req, 'POST') + self.assertEquals(400, resp.status) + + # Try to create a VM with invalid graphics listen + graphics = {'type': 'spice', 'listen': 'invalid'} + req = json.dumps({'name': 'test-vm', 'template': '/templates/test', 'graphics': graphics}) + resp = self.request('/vms', req, 'POST') + self.assertEquals(400, resp.status) + + # Delete the Template + resp = self.request('/templates/test', '{}', 'DELETE') + self.assertEquals(204, resp.status) + def test_vm_customise_storage(self): # Create a Template req = json.dumps({'name': 'test', 'cdrom': '/nonexistent.iso', @@ -565,9 +629,10 @@ class RestTests(unittest.TestCase): def test_templates(self): def verify_template(t, res): - for field in ('name', 'os_distro', - 'os_version', 'memory', 'cpus', 'storagepool'): - self.assertEquals(t[field], res[field]) + for field in ('name', 'os_distro', 'os_version', 'memory', + 'cpus', 'storagepool', 'graphics'): + if field in t: + self.assertEquals(t[field], res[field]) resp = self.request('/templates') self.assertEquals(200, resp.status) @@ -582,9 +647,11 @@ class RestTests(unittest.TestCase): self.assertEquals(400, resp.status) # Create a template + graphics = {'type': 'spice', 'listen': '127.0.0.1'} t = {'name': 'test', 'os_distro': 'ImagineOS', 'os_version': '1.0', 'memory': 1024, 'cpus': 1, - 'storagepool': '/storagepools/alt', 'cdrom': '/nonexistent.iso'} + 'storagepool': '/storagepools/alt', 'cdrom': '/nonexistent.iso', + 'graphics': graphics} req = json.dumps(t) resp = self.request('/templates', req, 'POST') self.assertEquals(201, resp.status) @@ -605,6 +672,7 @@ class RestTests(unittest.TestCase): # Update the template t['os_distro'] = 'Linux.ISO' t['os_version'] = '1.1' + t['graphics'] = {'type': 'vnc', 'listen': '0.0.0.0'} req = json.dumps(t) resp = self.request('/templates/%s' % t['name'], req, 'PUT') self.assertEquals(200, resp.status) @@ -663,6 +731,18 @@ class RestTests(unittest.TestCase): resp = self.request('/templates/%s' % tmpl_name, req, 'PUT') self.assertEquals(400, resp.status) + # Try to change template graphics type to invalid value + t['graphics'] = {'type': 'invalid'} + req = json.dumps(t) + resp = self.request('/templates/%s' % tmpl_name, req, 'PUT') + self.assertEquals(400, resp.status) + + # Try to change template graphics type to invalid listen + t['graphics'] = {'type': 'vnc', 'listen': 'invalid'} + req = json.dumps(t) + resp = self.request('/templates/%s' % tmpl_name, req, 'PUT') + self.assertEquals(400, resp.status) + # Try to clean up template cpus value t['cpus'] = ' ' req = json.dumps(t) diff --git a/tests/test_vmtemplate.py b/tests/test_vmtemplate.py index 81382c7..1386a97 100644 --- a/tests/test_vmtemplate.py +++ b/tests/test_vmtemplate.py @@ -31,7 +31,8 @@ class VMTemplateTests(unittest.TestCase): fields = (('name', 'test'), ('os_distro', 'unknown'), ('os_version', 'unknown'), ('cpus', 1), ('memory', 1024), ('cdrom', ''), ('network', 'default'), - ('disk_bus', 'ide'), ('nic_model', 'e1000')) + ('disk_bus', 'ide'), ('nic_model', 'e1000'), + ('graphics', {'type': 'vnc', 'listen': '0.0.0.0'})) args = {'name': 'test'} t = VMTemplate(args) @@ -39,27 +40,58 @@ class VMTemplateTests(unittest.TestCase): self.assertEquals(val, t.info.get(name)) def test_construct_overrides(self): - args = {'name': 'test', 'disks': [{'size': 10}, {'size': 20}]} + graphics = {'type': 'spice', 'listen': '127.0.0.1'} + args = {'name': 'test', 'disks': [{'size': 10}, {'size': 20}], + 'graphics': graphics} t = VMTemplate(args) self.assertEquals(2, len(t.info['disks'])) + self.assertEquals(graphics, t.info['graphics']) + + def test_specified_graphics(self): + # Test specified listen + graphics = {'type': 'vnc', 'listen': '127.0.0.1'} + args = {'name': 'test', 'disks': [{'size': 10}, {'size': 20}], + 'graphics': graphics} + t = VMTemplate(args) + self.assertEquals(graphics, t.info['graphics']) + + # Test specified type + graphics = {'type': 'spice', 'listen': '0.0.0.0'} + args['graphics'] = graphics + t = VMTemplate(args) + self.assertEquals(graphics, t.info['graphics']) + + # If no listen specified, test the default listen + graphics = {'type': 'vnc'} + args['graphics'] = graphics + t = VMTemplate(args) + self.assertEquals(graphics['type'], t.info['graphics']['type']) + self.assertEquals('0.0.0.0', t.info['graphics']['listen']) def test_to_xml(self): + graphics = {'type': 'spice', 'listen': '0.0.0.0'} vm_uuid = str(uuid.uuid4()).replace('-', '') t = VMTemplate({'name': 'test-template'}) - xml = t.to_vm_xml('test-vm', vm_uuid) + xml = t.to_vm_xml('test-vm', vm_uuid, graphics=graphics) self.assertEquals(vm_uuid, xpath_get_text(xml, "/domain/uuid")[0]) self.assertEquals('test-vm', xpath_get_text(xml, "/domain/name")[0]) + expr = "/domain/devices/graphics/@type" + self.assertEquals(graphics['type'], xpath_get_text(xml, expr)[0]) + expr = "/domain/devices/graphics/@listen" + self.assertEquals(graphics['listen'], xpath_get_text(xml, expr)[0]) def test_arg_merging(self): """ Make sure that default parameters from osinfo do not override user- provided parameters. """ + graphics = {'type': 'vnc', 'listen': '127.0.0.1'} args = {'name': 'test', 'os_distro': 'opensuse', 'os_version': '12.3', - 'cpus': 2, 'memory': 2048, 'network': 'foo', - 'cdrom': '/cd.iso'} + 'cpus': 2, 'memory': 2048, 'network': 'foo', 'cdrom': '/cd.iso', + 'graphics': graphics} t = VMTemplate(args) self.assertEquals(2, t.info.get('cpus')) self.assertEquals(2048, t.info.get('memory')) self.assertEquals('foo', t.info.get('network')) self.assertEquals('/cd.iso', t.info.get('cdrom')) + self.assertEquals(graphics, t.info.get('graphics')) -- 1.8.1.2

Add graphics parameters description in API.md Signed-off-by: apporc <appleorchard2000@gmail.com> --- docs/API.md | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/docs/API.md b/docs/API.md index 9edc551..a00d83b 100644 --- a/docs/API.md +++ b/docs/API.md @@ -47,6 +47,15 @@ the following general conventions: API. If omitted, a name will be chosen based on the template used. * template: The URI of a Template to use when building the VM * storagepool *(optional)*: Assign a specific Storage Pool to the new VM + * graphics *(optional)*: Specify the graphics paramenter for this vm + * type: The type of graphics, It can be VNC or spice or None. + * vnc: Graphical display using the Virtual Network + Computing protocol + * spice: Graphical display using the Simple Protocol for + Independent Computing Environments + * null: Graphics is disabled or type not supported + * listen: The network which the vnc/spice server listens on. + ### Resource: Virtual Machine @@ -75,14 +84,20 @@ the following general conventions: * screenshot: A link to a recent capture of the screen in PNG format * icon: A link to an icon that represents the VM * graphics: A dict to show detail of VM graphics. - * type: The type of graphics, It can be VNC or None. + * type: The type of graphics, It can be VNC or spice or None. * vnc: Graphical display using the Virtual Network Computing protocol + * spice: Graphical display using the Simple Protocol for + Independent Computing Environments * null: Graphics is disabled or type not supported + * listen: The network which the vnc/spice server listens on. * port: The port number of graphics. It will remain None until a connect call is issued. The port number exposed will support the websockets protocol and may support graphics type over plain TCP as well. + * real_port: The real port number of the graphics, vnc or spice. Users + can use this port to connect to the vm with general vnc/spice + clients. * **DELETE**: Remove the Virtual Machine * **PUT**: update the parameters of existed VM * name: New name for this VM (only applied for shutoff VM) @@ -92,6 +107,7 @@ the following general conventions: * start: Power on a VM * stop: Power off forcefully +* connect: Prepare the connection for spice or vnc ### Sub-resource: Virtual Machine Screenshot @@ -126,6 +142,14 @@ Represents a snapshot of the Virtual Machine's primary monitor. * size: The device size in GB * volume: A volume name that contains the initial disk contents + * graphics *(optional)*: The graphics paramenters of this template + * type: The type of graphics, It can be VNC or spice or None. + * vnc: Graphical display using the Virtual Network + Computing protocol + * spice: Graphical display using the Simple Protocol for + Independent Computing Environments + * null: Graphics is disabled or type not supported + * listen: The network which the vnc/spice server listens on. ### Resource: Template @@ -149,6 +173,15 @@ Represents a snapshot of the Virtual Machine's primary monitor. * index: The device index * size: The device size in GB * volume: A volume name that contains the initial disk contents + * graphcis: A dict of graphics paramenters of this template + * type: The type of graphics, It can be VNC or spice or None. + * vnc: Graphical display using the Virtual Network + Computing protocol + * spice: Graphical display using the Simple Protocol for + Independent Computing Environments + * null: Graphics is disabled or type not supported + * listen: The network which the vnc/spice server listens on. + * **DELETE**: Remove the Template * **POST**: *See Template Actions* * **PUT**: update the parameters of existed template @@ -167,6 +200,14 @@ Represents a snapshot of the Virtual Machine's primary monitor. * index: The device index * size: The device size in GB * volume: A volume name that contains the initial disk contents + * graphcis *(optional)*: A dict of graphics paramenters of this template + * type: The type of graphics, It can be VNC or spice or None. + * vnc: Graphical display using the Virtual Network + Computing protocol + * spice: Graphical display using the Simple Protocol for + Independent Computing Environments + * null: Graphics is disabled or type not supported + * listen: The network which the vnc/spice server listens on. **Actions (POST):** -- 1.8.1.2
participants (3)
-
Aline Manera
-
apporc
-
Rodrigo Trujillo