Re: [Kimchi-devel] [PATCH] Centralize graphics information

On 2014年08月25日 06:43, Aline Manera wrote: > Recently the ticket information was added to the vm to report the > graphics password and its expiration time. > That way, graphics information was found in 2 different data structuce: > some information under "graphics" and other ones under "ticket". > > { > "graphics":{ > "type":"vnc", > "port":null, > "listen":"127.0.0.1" > }, > "ticket":{ > "passwd":null, > "expire":null > }, > "name":"Ubuntu14.04", > "uuid":"8522d82d-d51b-436f-88ff-aff1fed27613", > "access":"full", > "state":"shutoff", > "memory":1024 > ... > } > > As "ticket" data structure only contains graphics information it makes > more sense to centralize it under "graphics" and also use the same > terminology used by libvirt (passwd and passwdValidTo) > > "graphics":{ > "type":"vnc", > "port":null, > "listen":"127.0.0.1" > "passwd":null, > "passwdValidTo":null > }, > > Signed-off-by: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; > --- > docs/API.md | 14 +++--- > src/kimchi/API.json | 12 ++--- > src/kimchi/control/vms.py | 2 +- > src/kimchi/i18n.py | 4 +- > src/kimchi/mockmodel.py | 38 +++++++++------- > src/kimchi/model/vms.py | 112 > ++++++++++++++++++++++------------------------ > tests/test_mockmodel.py | 2 +- > tests/test_model.py | 19 ++++---- > tests/test_rest.py | 13 +++--- > 9 files changed, 111 insertions(+), 105 deletions(-) > > diff --git a/docs/API.md b/docs/API.md > index d75c55f..7d5e804 100644 > --- a/docs/API.md > +++ b/docs/API.md > @@ -94,9 +94,8 @@ the following general conventions: > * 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. > - * ticket: A ticket represents credentials to access VM. > - * password: the password of a ticket. > - * expire: lifetime of a ticket. > + * passwd: console password > + * passwdValidTo: lifetime for the console password. > * users: A list of system users who have permission to access > the VM. > Default is: empty (i.e. only root-users may access). > * groups: A list of system groups whose users have permission > to access > @@ -110,9 +109,12 @@ the following general conventions: > will take effect in next reboot) > * memory: New amount of memory (MB) for this VM (if VM is > running, new > value will take effect in next reboot) > - * ticket: A ticket represents credentials to access VM. > - * password *(optional)*: the password of a ticket. > - * expire *(optional)*: lifetime of a ticket. > + * graphics: A dict to show detail of VM graphics. > + * passwd *(optional)*: console password. When omitted a > random password > + willbe generated. > + * passwdValidTo *(optional)*: lifetime for the console > password. When > + omitted the password will be > valid just > + for 30 seconds. > > * **POST**: *See Virtual Machine Actions* > > diff --git a/src/kimchi/API.json b/src/kimchi/API.json > index c3fc5e3..67612f2 100644 > --- a/src/kimchi/API.json > +++ b/src/kimchi/API.json > @@ -235,17 +235,17 @@ > "error": "KCHVM0026E" > } > }, > - "ticket": { > - "description": "A ticket represents credentials > to access VM", > + "graphics": { > + "description": "Graphics information from guest", > "type": "object", > "properties": { > - "password": { > - "description": "the password of a ticket.", > + "passwd": { > + "description": "New graphics password.", > "type": "string", > "error": "KCHVM0031E" > }, > - "size": { > - "description": "lifetime of a ticket.", > + "passwdValidTo": { > + "description": "Life time for the > graphics password.", > "type": "number", > "error": "KCHVM0032E" > } > diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py > index 6632e52..88d8a81 100644 > --- a/src/kimchi/control/vms.py > +++ b/src/kimchi/control/vms.py > @@ -36,7 +36,7 @@ class VM(Resource): > super(VM, self).__init__(model, ident) > self.role_key = 'guests' > self.update_params = ["name", "users", "groups", "cpus", > "memory", > - "ticket"] > + "graphics"] > self.screenshot = VMScreenShot(model, ident) > self.uri_fmt = '/vms/%s' > for ident, node in sub_nodes.items(): > diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py > index 2eae7e8..c276b38 100644 > --- a/src/kimchi/i18n.py > +++ b/src/kimchi/i18n.py > @@ -94,8 +94,8 @@ messages = { > "KCHVM0028E": _("Group(s) '%(groups)s' do not exist"), > "KCHVM0029E": _("Unable to shutdown virtual machine %(name)s. > Details: %(err)s"), > "KCHVM0030E": _("Unable to get access metadata of virtual > machine %(name)s. Details: %(err)s"), > - "KCHVM0031E": _("password of a vm ticket must be a string."), > - "KCHVM0032E": _("expire of a vm ticket must be a number."), > + "KCHVM0031E": _("The guest console password must be a string."), > + "KCHVM0032E": _("The life time for the guest console password > must be a number."), > > "KCHVMIF0001E": _("Interface %(iface)s does not exist in > virtual machine %(name)s"), > "KCHVMIF0002E": _("Network %(network)s specified for virtual > machine %(name)s does not exist"), > diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py > index fc474de..1322def 100644 > --- a/src/kimchi/mockmodel.py > +++ b/src/kimchi/mockmodel.py > @@ -91,15 +91,6 @@ class MockModel(object): > def _static_vm_update(self, dom, params): > state = dom.info['state'] > > - if 'ticket' in params: > - ticket = params.pop('ticket') > - passwd = ticket.get('passwd') > - dom.ticket["passwd"] = passwd if passwd is not None else > "".join( > - random.sample(string.ascii_letters + string.digits, 8)) > - expire = ticket.get('expire') > - dom.ticket["ValidTo"] = expire if expire is None else > round( > - time.time()) + expire > - > for key, val in params.items(): > if key == 'name': > if state == 'running' or params['name'] in > self.vms_get_list(): > @@ -126,7 +117,21 @@ class MockModel(object): > dom.info[key] = val > > def _live_vm_update(self, dom, params): > - pass > + if 'graphics' not in params: > + return > + > + graphics = params.pop('graphics') > + passwd = graphics.get('passwd') > + if passwd is None: > + passwd = "".join(random.sample(string.ascii_letters + > + string.digits, 8)) > + > + expire = graphics.get('passwdValidTo') > + if expire is not None: > + expire = round(time.time()) + expire > + > + dom.info['graphics']["passwd"] = passwd > + dom.info['graphics']["passwdValidTo"] = expire > > def vm_update(self, name, params): > dom = self._get_vm(name) > @@ -141,10 +146,11 @@ class MockModel(object): > vm.info['screenshot'] = self.vmscreenshot_lookup(name) > else: > vm.info['screenshot'] = None > - vm.info['ticket'] = {"passwd": vm.ticket["passwd"]} > - validTo = vm.ticket["ValidTo"] > - vm.info['ticket']["expire"] = (validTo - round(time.time()) > - if validTo is not None else > None) > + > + validTo = vm.info['graphics']['passwdValidTo'] > + validTo = (validTo - round(time.time()) if validTo is not None > + else None) > + vm.info['graphics']['passwdValidTo'] = validTo > return vm.info > > def vm_delete(self, name): > @@ -1050,7 +1056,6 @@ class MockVM(object): > self.networks = template_info['networks'] > ifaces = [MockVMIface(net) for net in self.networks] > self.storagedevices = {} > - self.ticket = {"passwd": "123456", "ValidTo": None} > self.ifaces = dict([(iface.info['mac'], iface) for iface in > ifaces]) > > stats = {'cpu_utilization': 20, > @@ -1066,7 +1071,8 @@ class MockVM(object): > 'cpus': self.cpus, > 'icon': None, > 'graphics': {'type': 'vnc', 'listen': > '127.0.0.1', > - 'port': None}, > + 'port': None, 'passwd': '123456', > + 'passwdValidTo': None}, > 'users': ['user1', 'user2', 'root'], > 'groups': ['group1', 'group2', 'admin'], > 'access': 'full' > diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py > index 476e4ac..0a05b76 100644 > --- a/src/kimchi/model/vms.py > +++ b/src/kimchi/model/vms.py > @@ -251,12 +251,6 @@ class VMModel(object): > self.groups = > import_class('kimchi.model.host.GroupsModel')(**kargs) > > def update(self, name, params): > - if 'ticket' in params: > - password = params['ticket'].get("passwd") > - password = password if password is not None else "".join( > - random.sample(string.ascii_letters + string.digits, 8)) > - params['ticket']['passwd'] = password > - > dom = self.get_vm(name, self.conn) > dom = self._static_vm_update(dom, params) > self._live_vm_update(dom, params) > @@ -315,14 +309,15 @@ class VMModel(object): > os_elem = E.os({"distro": distro, "version": version}) > set_metadata_node(dom, os_elem) > > - def _set_ticket(self, xml, params, flag=0): > + def _set_graphics_passwd(self, xml, params, flag=0): > DEFAULT_VALID_TO = 30 > password = params.get("passwd") > - expire = params.get("expire") > + expire = params.get("passwdValidTo") > root = objectify.fromstring(xml) > graphic = root.devices.find("graphics") > if graphic is None: > return None > + > graphic.attrib['passwd'] = password > to = graphic.attrib.get('passwdValidTo') > if to is not None: > @@ -337,27 +332,27 @@ class VMModel(object): > > return root if flag == 0 else graphic > > - def _set_persistent_ticket(self, dom, xml, params): > - if 'ticket' not in params: > - # store the password for copy > - ticket = self._get_ticket(dom) > - if ticket['passwd'] is None: > - return xml > - else: > - params['ticket'] = ticket > - node = self._set_ticket(xml, params['ticket']) > - return xml if node is None else ET.tostring(node, > encoding="utf-8") > + def _update_graphics(self, dom, xml, params): > + if 'graphics' not in params: > + return xml > > - def _set_live_ticket(self, dom, params): > - if 'ticket' not in params: > - return > - if dom.isActive(): > - xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_SECURE) > - flag = libvirt.VIR_DOMAIN_XML_SECURE > - node = self._set_ticket(xml, params['ticket'], flag) > - if node is not None: > - dom.updateDeviceFlags(etree.tostring(node), > - libvirt.VIR_DOMAIN_AFFECT_LIVE) > + password = params['graphics'].get("passwd") > + if password is None: > + password = "".join(random.sample(string.ascii_letters + > + string.digits, 8)) What if user just want to extend expire time of the passwd without setting a password?

> + params['graphics']['passwd'] = password > + > + if not dom.isActive(): > + node = self._set_graphics_passwd(xml, params['graphics']) > + return xml if node is None else ET.tostring(node, > encoding="utf-8") > + > + xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_SECURE) > + flag = libvirt.VIR_DOMAIN_XML_SECURE > + node = self._set_graphics_passwd(xml, params['graphics'], flag) > + if node is not None: > + dom.updateDeviceFlags(etree.tostring(node), > + libvirt.VIR_DOMAIN_AFFECT_LIVE) > + return xml > > def _static_vm_update(self, dom, params): > state = DOM_STATE_MAP[dom.info()[0]] > @@ -374,7 +369,7 @@ class VMModel(object): > xpath = VM_STATIC_UPDATE_PARAMS[key] > new_xml = xmlutils.xml_item_update(new_xml, xpath, > val) > > - new_xml = self._set_persistent_ticket(dom, new_xml, params) > + new_xml = self._update_graphics(dom, new_xml, params) > > conn = self.conn.get() > try: > @@ -397,35 +392,19 @@ class VMModel(object): > > def _live_vm_update(self, dom, params): > self._vm_update_access_metadata(dom, params) > - self._set_live_ticket(dom, params) > > def _has_video(self, dom): > dom = ElementTree.fromstring(dom.XMLDesc(0)) > return dom.find('devices/video') is not None > > - def _get_ticket(self, dom): > - xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_SECURE) > - root = objectify.fromstring(xml) > - graphic = root.devices.find("graphics") > - if graphic is None: > - return {"passwd": None, "expire": None} > - passwd = graphic.attrib.get('passwd') > - ticket = {"passwd": passwd} > - valid_to = graphic.attrib.get('passwdValidTo') > - ticket['expire'] = None > - if valid_to is not None: > - to = time.mktime(time.strptime(valid_to, > '%Y-%m-%dT%H:%M:%S')) > - ticket['expire'] = to - time.mktime(time.gmtime()) > - > - return ticket > - > def lookup(self, name): > dom = self.get_vm(name, self.conn) > info = dom.info() > state = DOM_STATE_MAP[info[0]] > screenshot = None > + # (type, listen, port, passwd, passwdValidTo) > graphics = self._vm_get_graphics(name) > - graphics_type, graphics_listen, graphics_port = graphics > + graphics_port = graphics[2] > graphics_port = graphics_port if state == 'running' else None > try: > if state == 'running' and self._has_video(dom): > @@ -465,10 +444,12 @@ class VMModel(object): > 'cpus': info[3], > 'screenshot': screenshot, > 'icon': icon, > - 'graphics': {"type": graphics_type, > - "listen": graphics_listen, > - "port": graphics_port}, > - 'ticket': self._get_ticket(dom), > + # (type, listen, port, passwd, passwdValidTo) > + 'graphics': {"type": graphics[0], > + "listen": graphics[1], > + "port": graphics_port, > + "passwd": graphics[3], > + "passwdValidTo": graphics[4]}, > 'users': users, > 'groups': groups, > 'access': 'full' > @@ -567,23 +548,38 @@ class VMModel(object): > > def _vm_get_graphics(self, name): > dom = self.get_vm(name, self.conn) > - xml = dom.XMLDesc(0) > + xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_SECURE) > + > expr = "/domain/devices/graphics/@type" > res = xmlutils.xpath_get_text(xml, expr) > graphics_type = res[0] if res else None > + > expr = "/domain/devices/graphics/@listen" > res = xmlutils.xpath_get_text(xml, expr) > graphics_listen = res[0] if res else None > - graphics_port = None > + > + graphics_port = graphics_passwd = graphics_passwdValidTo = None > if graphics_type: > - expr = "/domain/devices/graphics[@type='%s']/@port" % > graphics_type > - res = xmlutils.xpath_get_text(xml, expr) > + expr = "/domain/devices/graphics[@type='%s']/@port" > + res = xmlutils.xpath_get_text(xml, expr % graphics_type) > graphics_port = int(res[0]) if res else None > - return graphics_type, graphics_listen, graphics_port > + > + expr = "/domain/devices/graphics[@type='%s']/@passwd" > + res = xmlutils.xpath_get_text(xml, expr % graphics_type) > + graphics_passwd = res[0] if res else None > + > + expr = > "/domain/devices/graphics[@type='%s']/@passwdValidTo" > + res = xmlutils.xpath_get_text(xml, expr % graphics_type) > + if res: > + to = time.mktime(time.strptime(res[0], > '%Y-%m-%dT%H:%M:%S')) > + graphics_passwdValidTo = to - > time.mktime(time.gmtime()) > + > + return (graphics_type, graphics_listen, graphics_port, > + graphics_passwd, graphics_passwdValidTo) > > def connect(self, name): > - graphics = self._vm_get_graphics(name) > - graphics_type, graphics_listen, graphics_port = graphics > + # (type, listen, port, passwd, passwdValidTo) > + graphics_port = self._vm_get_graphics(name)[2] > if graphics_port is not None: > vnc.add_proxy_token(name, graphics_port) > else: > diff --git a/tests/test_mockmodel.py b/tests/test_mockmodel.py > index b7e6a48..8de7ce9 100644 > --- a/tests/test_mockmodel.py > +++ b/tests/test_mockmodel.py > @@ -143,7 +143,7 @@ class MockModelTests(unittest.TestCase): > > keys = set(('name', 'state', 'stats', 'uuid', 'memory', > 'cpus', > 'screenshot', 'icon', 'graphics', 'users', > 'groups', > - 'access', 'ticket')) > + 'access')) > > stats_keys = set(('cpu_utilization', > 'net_throughput', 'net_throughput_peak', > diff --git a/tests/test_model.py b/tests/test_model.py > index f5b13d0..5090395 100644 > --- a/tests/test_model.py > +++ b/tests/test_model.py > @@ -65,7 +65,7 @@ class ModelTests(unittest.TestCase): > > keys = set(('name', 'state', 'stats', 'uuid', 'memory', > 'cpus', > 'screenshot', 'icon', 'graphics', 'users', > 'groups', > - 'access', 'ticket')) > + 'access')) > > stats_keys = set(('cpu_utilization', > 'net_throughput', 'net_throughput_peak', > @@ -764,24 +764,25 @@ class ModelTests(unittest.TestCase): > vms = inst.vms_get_list() > self.assertTrue('kimchi-vm1' in vms) > > - # update vm ticket when vm is not running > + # update vm graphics when vm is not running > inst.vm_update(u'kimchi-vm1', > - {"ticket": {"passwd": "123456"}}) > + {"graphics": {"passwd": "123456"}}) > > inst.vm_start('kimchi-vm1') > rollback.prependDefer(self._rollback_wrapper, > inst.vm_poweroff, > 'kimchi-vm1') > > vm_info = inst.vm_lookup(u'kimchi-vm1') > - self.assertEquals('123456', vm_info['ticket']["passwd"]) > - self.assertEquals(None, vm_info['ticket']["expire"]) > + self.assertEquals('123456', vm_info['graphics']["passwd"]) > + self.assertEquals(None, > vm_info['graphics']["passwdValidTo"]) > > - # update vm ticket when vm is running > + # update vm graphics when vm is running > inst.vm_update(u'kimchi-vm1', > - {"ticket": {"passwd": "abcdef", "expire": > 20}}) > + {"graphics": {"passwd": "abcdef", > + "passwdValidTo": 20}}) > vm_info = inst.vm_lookup(u'kimchi-vm1') > - self.assertEquals('abcdef', vm_info['ticket']["passwd"]) > - self.assertGreaterEqual(20, vm_info['ticket']['expire']) > + self.assertEquals('abcdef', vm_info['graphics']["passwd"]) > + self.assertGreaterEqual(20, > vm_info['graphics']['passwdValidTo']) > > info = inst.vm_lookup('kimchi-vm1') > self.assertEquals('running', info['state']) > diff --git a/tests/test_rest.py b/tests/test_rest.py > index 2117399..0bf5997 100644 > --- a/tests/test_rest.py > +++ b/tests/test_rest.py > @@ -221,20 +221,21 @@ class RestTests(unittest.TestCase): > resp = self.request('/vms/vm-1', req, 'PUT') > self.assertEquals(200, resp.status) > > - req = json.dumps({"ticket": {'passwd': "abcdef"}}) > + req = json.dumps({"graphics": {'passwd': "abcdef"}}) > resp = self.request('/vms/vm-1', req, 'PUT') > info = json.loads(resp.read()) > - self.assertEquals('abcdef', info["ticket"]["passwd"]) > - self.assertEquals(None, info["ticket"]["expire"]) > + self.assertEquals('abcdef', info["graphics"]["passwd"]) > + self.assertEquals(None, info["graphics"]["passwdValidTo"]) > > resp = self.request('/vms/vm-1/poweroff', '{}', 'POST') > self.assertEquals(200, resp.status) > > - req = json.dumps({"ticket": {'passwd': "123456", "expire": > 20}}) > + req = json.dumps({"graphics": {'passwd': "123456", > + 'passwdValidTo': 20}}) > resp = self.request('/vms/vm-1', req, 'PUT') > info = json.loads(resp.read()) > - self.assertEquals('123456', info["ticket"]["passwd"]) > - self.assertGreaterEqual(20, info["ticket"]["expire"]) > + self.assertEquals('123456', info["graphics"]["passwd"]) > + self.assertGreaterEqual(20, info["graphics"]["passwdValidTo"]) > > req = json.dumps({'name': 12}) > resp = self.request('/vms/vm-1', req, 'PUT')