From: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; This patch adds the documentation and i18n changes for the new Virt-Viewer launcher API. URI: plugins/kimchi/vms/*:name*/virtviewerfile 'Retrieve the Virt-Viewer launcher file to connect to the graphics of this virtual machine. Virtual machine must be running.' Signed-off-by: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; --- docs/API.md | 6 ++++++ i18n.py | 2 ++ 2 files changed, 8 insertions(+) diff --git a/docs/API.md b/docs/API.md index 84ef7c9..78c0574 100644 --- a/docs/API.md +++ b/docs/API.md @@ -286,6 +286,12 @@ Represents a snapshot of the Virtual Machine's primary monitor. **URI:** /plugins/kimchi/vms/*:name*/snapshots/current * **GET**: Retrieve current snapshot information for the virtual machine. +### Sub-resource: Virt-Viewer File +**URI:** /plugins/kimchi/vms/*:name*/virtviewerfile +* **GET**: Retrieve the Virt-Viewer launcher file to connect to the + graphics of this virtual machine. Virtual machine must + be running. + ### Collection: Templates **URI:** /plugins/kimchi/templates diff --git a/i18n.py b/i18n.py index 08a3ac8..deb0f8c 100644 --- a/i18n.py +++ b/i18n.py @@ -131,6 +131,8 @@ messages = { "KCHVM0080E": _("Cannot update Maximum Memory when guest is running."), "KCHVM0081E": _("Impossible to create %(dir)s directory."), "KCHVM0082E": _("Either the guest %(name)s did not start to listen to the serial or it is not configured to use the serial console."), + "KCHVM0083E": _("Unable to retrieve virt viever file for stopped virtual machine %(name)s"), + "KCHVM0084E": _("Error occured while retrieving the virt viewer file for virtual machine %(name)s : %(err)s"), "KCHVMHDEV0001E": _("VM %(vmid)s does not contain directly assigned host device %(dev_name)s."), "KCHVMHDEV0002E": _("The host device %(dev_name)s is not allowed to directly assign to VM."), -- 2.5.5

On 07/07/2016 10:45 AM, Lucio Correia wrote: Will fix these in v2

From: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; control/vms.py: - added a new resource called VMVirtViewerFile that returns an URL to download the virt-viewer launcher file. - added a new URL called 'virtviewerfile' to allow access to the new VMVirtViewerFile resource. model/vms.py: - turned get_graphics() into a staticmethod to be used outside of the vms.py module. Signed-off-by: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; --- control/vms.py | 12 ++++++++++++ model/vms.py | 9 +++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/control/vms.py b/control/vms.py index bbf0fa4..645cb40 100644 --- a/control/vms.py +++ b/control/vms.py @@ -65,6 +65,7 @@ class VM(Resource): super(VM, self).__init__(model, ident) self.role_key = 'guests' self.screenshot = VMScreenShot(model, ident) + self.virtviewerfile = VMVirtViewerFile(model, ident) self.uri_fmt = '/vms/%s' for ident, node in sub_nodes.items(): setattr(self, ident, node(model, self.ident)) @@ -103,3 +104,14 @@ class VMScreenShot(Resource): self.lookup() internal_uri = self.info.replace('plugins/kimchi', '') raise internal_redirect(internal_uri) + + +class VMVirtViewerFile(Resource): + def __init__(self, model, ident): + super(VMVirtViewerFile, self).__init__(model, ident) + self.role_key = 'guests' + + @property + def data(self): + internal_uri = self.info.replace('plugins/kimchi', '') + raise internal_redirect(internal_uri) diff --git a/model/vms.py b/model/vms.py index 6a309fc..e0b0a37 100644 --- a/model/vms.py +++ b/model/vms.py @@ -1154,7 +1154,7 @@ class VMModel(object): state = DOM_STATE_MAP[info[0]] screenshot = None # (type, listen, port, passwd, passwdValidTo) - graphics = self._vm_get_graphics(name) + graphics = self.get_graphics(name, self.conn) graphics_port = graphics[2] graphics_port = graphics_port if state == 'running' else None try: @@ -1426,8 +1426,9 @@ class VMModel(object): return True - def _vm_get_graphics(self, name): - dom = self.get_vm(name, self.conn) + @staticmethod + def get_graphics(name, conn): + dom = VMModel.get_vm(name, conn) xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_SECURE) expr = "/domain/devices/graphics/@type" @@ -1492,7 +1493,7 @@ class VMModel(object): def connect(self, name): # (type, listen, port, passwd, passwdValidTo) - graphics_port = self._vm_get_graphics(name)[2] + graphics_port = self.get_graphics(name, self.conn)[2] if graphics_port is not None: websocket.add_proxy_token(name.encode('utf-8'), graphics_port) else: -- 2.5.5

On 07-07-2016 09:57, dhbarboza82(a)gmail.com wrote: Why 2 dicts here? Shouldn't it be only one dict? -- Lucio Correia Software Engineer IBM LTC Brazil

From: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; This patch adds 3 new unit tests in test_model.py to test the new VirtViewerModel. Signed-off-by: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; --- tests/test_model.py | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 1 deletion(-) diff --git a/tests/test_model.py b/tests/test_model.py index 9a6a1aa..2fc1d38 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -18,15 +18,20 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +import __builtin__ as builtins + import grp import lxml.etree as ET import os import pwd +import mock import re import shutil import time import unittest +from mock import call, mock_open, patch + import tests.utils as utils import wok.objectstore @@ -43,6 +48,7 @@ from wok.plugins.kimchi import osinfo from wok.plugins.kimchi.config import kimchiPaths as paths from wok.plugins.kimchi.model import model from wok.plugins.kimchi.model.libvirtconnection import LibvirtConnection +from wok.plugins.kimchi.model.virtviewerfile import VMVirtViewerFileModel from wok.plugins.kimchi.model.vms import VMModel import iso_gen @@ -108,7 +114,8 @@ class ModelTests(unittest.TestCase): # test_rest or test_mockmodel to avoid overriding problems LibvirtConnection._connections['test:///default'] = {} - os.unlink(self.tmp_store) + if os.path.isfile(self.tmp_store): + os.unlink(self.tmp_store) def test_vm_info(self): inst = model.Model('test:///default', self.tmp_store) @@ -338,6 +345,94 @@ class ModelTests(unittest.TestCase): inst.template_delete('test') + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_vm_virtviewerfile_vmnotrunning(self): + inst = model.Model(objstore_loc=self.tmp_store) + params = {'name': 'test', + 'source_media': {'type': 'disk', 'path': UBUNTU_ISO}} + inst.templates_create(params) + + with RollbackContext() as rollback: + params = {'name': 'kimchi-vnc', + 'template': '/plugins/kimchi/templates/test'} + task1 = inst.vms_create(params) + inst.task_wait(task1['id']) + rollback.prependDefer(inst.vm_delete, 'kimchi-vnc') + + expected_error_msg = "KCHVM0083E" + with self.assertRaisesRegexp(NotFoundError, expected_error_msg): + vvmodel = VMVirtViewerFileModel(conn=inst.conn) + vvmodel.lookup('kimchi-vnc') + + inst.template_delete('test') + + @mock.patch('wok.plugins.kimchi.model.virtviewerfile._get_request_host') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'VMModel.get_graphics') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'VMVirtViewerFileModel._check_if_vm_running') + def test_vm_virtviewerfile_vnc(self, mock_vm_running, mock_get_graphics, + mock_get_host): + + mock_get_host.return_value = 'kimchi-test-host' + mock_get_graphics.return_value = ['vnc', 'listen', '5999', None] + mock_vm_running.return_value = True + + vvmodel = VMVirtViewerFileModel(conn=None) + + open_ = mock_open(read_data='') + with patch.object(builtins, 'open', open_): + vvfilepath = vvmodel.lookup('kimchi-vm') + + self.assertEqual( + vvfilepath, + 'plugins/kimchi/data/virtviewerfiles/kimchi-vm-access.vv' + ) + + expected_write_content = "[virt-viewer]\ntype=vnc\n"\ + "host=kimchi-test-host\nport=5999\n" + self.assertEqual( + open_().write.mock_calls, [call(expected_write_content)] + ) + + mock_get_graphics.assert_called_once_with('kimchi-vm', None) + mock_vm_running.assert_called_once_with('kimchi-vm') + + @mock.patch('wok.plugins.kimchi.model.virtviewerfile._get_request_host') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'VMModel.get_graphics') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'VMVirtViewerFileModel._check_if_vm_running') + def test_vm_virtviewerfile_spice_passwd(self, mock_vm_running, + mock_get_graphics, + mock_get_host): + + mock_get_host.return_value = 'kimchi-test-host' + mock_get_graphics.return_value = [ + 'spice', 'listen', '6660', 'spicepasswd' + ] + mock_vm_running.return_value = True + + vvmodel = VMVirtViewerFileModel(conn=None) + + open_ = mock_open(read_data='') + with patch.object(builtins, 'open', open_): + vvfilepath = vvmodel.lookup('kimchi-vm') + + self.assertEqual( + vvfilepath, + 'plugins/kimchi/data/virtviewerfiles/kimchi-vm-access.vv' + ) + + expected_write_content = "[virt-viewer]\ntype=spice\n"\ + "host=kimchi-test-host\nport=6660\npassword=spicepasswd\n" + self.assertEqual( + open_().write.mock_calls, [call(expected_write_content)] + ) + + mock_get_graphics.assert_called_once_with('kimchi-vm', None) + mock_vm_running.assert_called_once_with('kimchi-vm') + @unittest.skipUnless(utils.running_as_root(), "Must be run as root") def test_vm_serial(self): inst = model.Model(objstore_loc=self.tmp_store) -- 2.5.5

This is very nice code and IMHO looks like a server feature, to be added to Wok. So my suggestion is to split this patch between Wok and Kimchi, keeping on Kimchi, as an extension of Wok class, only: Also, isn't necessary a --reload after por is opened by firewall_cmd? On 07-07-2016 09:57, dhbarboza82(a)gmail.com wrote: -- Lucio Correia Software Engineer IBM LTC Brazil

Lucio, after doing some thinking I believe that this firewall class should remain in Kimchi for now. There is no immediate gain into adding the FirewallManager somewhere else while keeping this patch set waiting - specially considering that this is the only feature in all WoK plug-ins that is using it. Note that this does not mean that we can't start a RFC proposing that the FirewallManager should be hosted in WoK (perhaps Gingerbase?). We can send a RFC with a more robust proposal that can also deal with firewall zones configuration (Ginger has a request for this specific feature at https://github.com/kimchi-project/ginger/issues/107) and with more generic rules (adding udp ports for example). After this feature is discussed and contributed we can go back to this code and change it to use this new firewall backend. Daniel On 07/07/2016 02:44 PM, Daniel Henrique Barboza wrote:

From: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; This patch adds unit tests for the FirewallManager class. Signed-off-by: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; --- tests/test_model.py | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/tests/test_model.py b/tests/test_model.py index 2fc1d38..1162d2c 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -48,6 +48,7 @@ from wok.plugins.kimchi import osinfo from wok.plugins.kimchi.config import kimchiPaths as paths from wok.plugins.kimchi.model import model from wok.plugins.kimchi.model.libvirtconnection import LibvirtConnection +from wok.plugins.kimchi.model.virtviewerfile import FirewallManager from wok.plugins.kimchi.model.virtviewerfile import VMVirtViewerFileModel from wok.plugins.kimchi.model.vms import VMModel @@ -433,6 +434,76 @@ class ModelTests(unittest.TestCase): mock_get_graphics.assert_called_once_with('kimchi-vm', None) mock_vm_running.assert_called_once_with('kimchi-vm') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.run_command') + def test_firewall_provider_firewallcmd(self, mock_run_cmd): + mock_run_cmd.side_effect = [ + ['', '', 0], ['', '', 0], ['', '', 0] + ] + + fw_manager = FirewallManager() + fw_manager.add_vm_graphics_port('vm-name', 5905) + self.assertEqual(fw_manager.opened_ports, {'vm-name': 5905}) + + fw_manager.remove_vm_graphics_port('vm-name') + self.assertEqual(fw_manager.opened_ports, {}) + + mock_run_cmd.assert_has_calls( + [ + call(['firewall-cmd', '--state', '-q']), + call(['firewall-cmd', '--add-port=5905/tcp']), + call(['firewall-cmd', '--remove-port=5905/tcp']) + ] + ) + + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.run_command') + def test_firewall_provider_ufw(self, mock_run_cmd): + mock_run_cmd.side_effect = [ + ['', '', 1], ['', '', 0], ['', '', 0], ['', '', 0] + ] + + fw_manager = FirewallManager() + fw_manager.add_vm_graphics_port('vm-name', 5905) + self.assertEqual(fw_manager.opened_ports, {'vm-name': 5905}) + + fw_manager.remove_vm_graphics_port('vm-name') + self.assertEqual(fw_manager.opened_ports, {}) + + mock_run_cmd.assert_has_calls( + [ + call(['firewall-cmd', '--state', '-q']), + call(['ufw', 'status']), + call(['ufw', 'allow', '5905/tcp']), + call(['ufw', 'deny', '5905/tcp']) + ] + ) + + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.run_command') + def test_firewall_provider_iptables(self, mock_run_cmd): + mock_run_cmd.side_effect = [ + ['', '', 1], ['', '', 1], ['', '', 0], ['', '', 0] + ] + + fw_manager = FirewallManager() + fw_manager.add_vm_graphics_port('vm-name', 5905) + self.assertEqual(fw_manager.opened_ports, {'vm-name': 5905}) + + fw_manager.remove_vm_graphics_port('vm-name') + self.assertEqual(fw_manager.opened_ports, {}) + + iptables_add = ['iptables', '-I', 'INPUT', '-p', 'tcp', '--dport', + 5905, '-j', 'ACCEPT'] + + iptables_del = ['iptables', '-D', 'INPUT', '-p', 'tcp', '--dport', + 5905, '-j', 'ACCEPT'] + + mock_run_cmd.assert_has_calls( + [ + call(['firewall-cmd', '--state', '-q']), + call(['ufw', 'status']), + call(iptables_add), call(iptables_del) + ] + ) + @unittest.skipUnless(utils.running_as_root(), "Must be run as root") def test_vm_serial(self): inst = model.Model(objstore_loc=self.tmp_store) -- 2.5.5

From: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; This patch changes existing tests and add a new test to assert that the latest changes with FirewallManager and libvirt event listening in virtviewerfile.py is working properly. Signed-off-by: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; --- tests/test_model.py | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/tests/test_model.py b/tests/test_model.py index 1162d2c..44358a5 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -371,8 +371,13 @@ class ModelTests(unittest.TestCase): @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' 'VMModel.get_graphics') @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'FirewallManager.add_vm_graphics_port') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'VMVirtViewerFileModel.handleVMShutdownPowerOff') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' 'VMVirtViewerFileModel._check_if_vm_running') - def test_vm_virtviewerfile_vnc(self, mock_vm_running, mock_get_graphics, + def test_vm_virtviewerfile_vnc(self, mock_vm_running, mock_handleVMOff, + mock_add_port, mock_get_graphics, mock_get_host): mock_get_host.return_value = 'kimchi-test-host' @@ -398,13 +403,21 @@ class ModelTests(unittest.TestCase): mock_get_graphics.assert_called_once_with('kimchi-vm', None) mock_vm_running.assert_called_once_with('kimchi-vm') + mock_handleVMOff.assert_called_once_with('kimchi-vm') + mock_add_port.assert_called_once_with('kimchi-vm', '5999') @mock.patch('wok.plugins.kimchi.model.virtviewerfile._get_request_host') @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' 'VMModel.get_graphics') @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'FirewallManager.add_vm_graphics_port') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'VMVirtViewerFileModel.handleVMShutdownPowerOff') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' 'VMVirtViewerFileModel._check_if_vm_running') def test_vm_virtviewerfile_spice_passwd(self, mock_vm_running, + mock_handleVMOff, + mock_add_port, mock_get_graphics, mock_get_host): @@ -433,6 +446,8 @@ class ModelTests(unittest.TestCase): mock_get_graphics.assert_called_once_with('kimchi-vm', None) mock_vm_running.assert_called_once_with('kimchi-vm') + mock_handleVMOff.assert_called_once_with('kimchi-vm') + mock_add_port.assert_called_once_with('kimchi-vm', '6660') @mock.patch('wok.plugins.kimchi.model.virtviewerfile.run_command') def test_firewall_provider_firewallcmd(self, mock_run_cmd): @@ -504,6 +519,42 @@ class ModelTests(unittest.TestCase): ] ) + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'FirewallManager.remove_vm_graphics_port') + @mock.patch('wok.plugins.kimchi.model.virtviewerfile.' + 'FirewallManager.add_vm_graphics_port') + def test_vm_virtviewerfile_vmlifecycle(self, mock_add_port, + mock_remove_port): + + inst = model.Model(objstore_loc=self.tmp_store) + params = {'name': 'test', + 'source_media': {'type': 'disk', 'path': UBUNTU_ISO}} + inst.templates_create(params) + + with RollbackContext() as rollback: + params = {'name': 'kimchi-vnc', + 'template': '/plugins/kimchi/templates/test'} + task1 = inst.vms_create(params) + inst.task_wait(task1['id']) + rollback.prependDefer(inst.vm_delete, 'kimchi-vnc') + + inst.vm_start('kimchi-vnc') + + graphics_info = VMModel.get_graphics('kimchi-vnc', inst.conn) + graphics_port = graphics_info[2] + + vvmodel = VMVirtViewerFileModel(conn=inst.conn) + vvmodel.lookup('kimchi-vnc') + + inst.vm_poweroff('kimchi-vnc') + time.sleep(5) + + mock_add_port.assert_called_once_with('kimchi-vnc', graphics_port) + mock_remove_port.assert_called_once_with('kimchi-vnc') + + inst.template_delete('test') + @unittest.skipUnless(utils.running_as_root(), "Must be run as root") def test_vm_serial(self): inst = model.Model(objstore_loc=self.tmp_store) -- 2.5.5

On 07/07/2016 12:25 PM, Lucio Correia wrote: I'll edit this unit test to use non-ASCII chars in the VM name.