[PATCH v3 1/4] Move configuration parsing to config.py
The configuration is also needed for other code except starting kimchi server. So it should be moved to a separate module, config.py. Then the configuration can be accessed directly by importing config module. Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/config.py.in | 24 ++++++++++++++++++++++++ src/kimchid.in | 20 +------------------- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index f3c408a..72308ff 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -27,6 +27,7 @@ import os import platform +from ConfigParser import SafeConfigParser from glob import iglob @@ -166,5 +167,28 @@ def get_plugin_tab_xml(name): return os.path.join(_get_plugin_ui_dir(name), 'config/tab-ext.xml') +def _get_config(): + config = SafeConfigParser() + config.add_section("server") + config.set("server", "host", "0.0.0.0") + config.set("server", "port", "8000") + config.set("server", "ssl_port", "8001") + config.set("server", "ssl_cert", "") + config.set("server", "ssl_key", "") + config.set("server", "environment", "development") + config.add_section("logging") + config.set("logging", "log_dir", get_default_log_dir()) + config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + + if os.path.exists(CONFIG_FILE): + config.read(CONFIG_FILE) + return config + + +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir() +DEFAULT_LOG_LEVEL = "debug" +config = _get_config() + + if __name__ == '__main__': print get_prefix() diff --git a/src/kimchid.in b/src/kimchid.in index 7865713..548fa52 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -33,31 +33,13 @@ import kimchi.config if kimchi.config.without_installation(): sys.path.append(kimchi.config.get_prefix()) -from ConfigParser import SafeConfigParser +from kimchi.config import config from optparse import OptionParser ACCESS_LOG = "kimchi-access.log" ERROR_LOG = "kimchi-error.log" -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir() -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir() -DEFAULT_LOG_LEVEL = "debug" def main(options): - config = SafeConfigParser() - config.add_section("server") - config.set("server", "host", "0.0.0.0") - config.set("server", "port", "8000") - config.set("server", "ssl_port", "8001") - config.set("server", "ssl_cert", "") - config.set("server", "ssl_key", "") - config.set("server", "environment", "development") - config.add_section("logging") - config.set("logging", "log_dir", DEFAULT_LOG_DIR) - config.set("logging", "log_level", DEFAULT_LOG_LEVEL) - - if os.path.exists(CONFIG_FILE): - config.read(CONFIG_FILE) - host = config.get("server", "host") port = config.get("server", "port") ssl_port = config.get("server", "ssl_port") -- 1.8.4.2
We are going to use one proxy instance to forward all vm's vnc traffics. The proxy instance listens on a fixed port instead of a random one. This patch makes the port configurable and exported to client by adding it to the response of '/config'. Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- docs/API.md | 1 + src/kimchi.conf.in | 4 ++++ src/kimchi/config.py.in | 2 ++ src/kimchi/control/config.py | 4 +++- 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/API.md b/docs/API.md index 0013e86..4d7bdff 100644 --- a/docs/API.md +++ b/docs/API.md @@ -432,6 +432,7 @@ Contains information about the application environment and configuration. * **GET**: Retrieve configuration information * http_port: The port number on which the server is listening + * vnc_proxy_port: Port for vnc's websocket proxy to listen on * **POST**: *See Configuration Actions* **Actions (POST):** diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index bf26c26..ac5a1f5 100644 --- a/src/kimchi.conf.in +++ b/src/kimchi.conf.in @@ -29,3 +29,7 @@ # Logging level: debug, info, warning, error or critical #log_level = debug + +[novnc] +# Port for vnc's websocket proxy to listen on +#vnc_proxy_port = 64667 diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index 72308ff..e1319e3 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -179,6 +179,8 @@ def _get_config(): config.add_section("logging") config.set("logging", "log_dir", get_default_log_dir()) config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + config.add_section("novnc") + config.set("novnc", "vnc_proxy_port", "64667") if os.path.exists(CONFIG_FILE): config.read(CONFIG_FILE) diff --git a/src/kimchi/control/config.py b/src/kimchi/control/config.py index 3630172..5186ddd 100644 --- a/src/kimchi/control/config.py +++ b/src/kimchi/control/config.py @@ -25,6 +25,7 @@ import cherrypy +from kimchi.config import config from kimchi.control.base import Collection, Resource @@ -38,7 +39,8 @@ class Config(Resource): @property def data(self): - return {'http_port': cherrypy.server.socket_port} + return {'http_port': cherrypy.server.socket_port, + 'vnc_proxy_port': config.get('novnc', 'vnc_proxy_port')} class Capabilities(Resource): -- 1.8.4.2
Currently, we start a new weksockify proxy on every vm's connect action. It causes trouble to allow the vnc traffic in firewall because we choose a random port for each vm. And it's also obversed that vnc connection fails because of race between client and proxy server. This patch changes to use websockify's target_cfg option instead of target_port. Then kimchi can dynamically add proxy configuration for a vm. It adds a map of target host/port and token pair to websockify's cfg on the connect request. With this change, all vnc traffic can be forwared in one tcp port, and therefore it's possible to add firewall rule for kimchi's vnc remote access. Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/model.py | 14 ++++++-------- src/kimchi/server.py | 5 +++++ src/kimchi/vnc.py | 41 +++++++++++++++++++++++------------------ ui/js/src/kimchi.api.js | 6 ++++-- 4 files changed, 38 insertions(+), 28 deletions(-) diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a21fcf7..10e6c3f 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -129,7 +129,6 @@ class Model(object): self.libvirt_uri = libvirt_uri or 'qemu:///system' self.conn = LibvirtConnection(self.libvirt_uri) self.objstore = ObjectStore(objstore_loc) - self.graphics_ports = {} self.next_taskid = 1 self.stats = {} self.host_stats = defaultdict(int) @@ -504,10 +503,7 @@ 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' - else None) + graphics_type, graphics_port = self._vm_get_graphics(name) try: if state == 'running': screenshot = self.vmscreenshot_lookup(name) @@ -567,6 +563,8 @@ class Model(object): with self.objstore as session: session.delete('vm', dom.UUIDString(), ignore_missing=True) + vnc.remove_proxy_token(name) + def vm_start(self, name): dom = self._get_vm(name) dom.create() @@ -596,10 +594,10 @@ class Model(object): 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 + vnc.add_proxy_token(name, port) else: - raise OperationFailed("Unable to find VNC port in %s" % name) + raise OperationFailed("Only able to connect to running vm's vnc " + "graphics.") def vms_create(self, params): conn = self.conn.get() diff --git a/src/kimchi/server.py b/src/kimchi/server.py index 1467c87..b820263 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -32,6 +32,7 @@ from kimchi import auth from kimchi import config from kimchi import model from kimchi import mockmodel +from kimchi import vnc from kimchi.root import Root from kimchi.utils import get_enabled_plugins, import_class @@ -188,6 +189,10 @@ class Server(object): else: model_instance = model.Model() + if isinstance(model_instance, model.Model): + vnc_ws_proxy = vnc.new_ws_proxy() + cherrypy.engine.subscribe('exit', vnc_ws_proxy.kill) + self.app = cherrypy.tree.mount(Root(model_instance, dev_env), config=self.configObj) self._load_plugins() diff --git a/src/kimchi/vnc.py b/src/kimchi/vnc.py index 089d64a..dc70b46 100644 --- a/src/kimchi/vnc.py +++ b/src/kimchi/vnc.py @@ -21,33 +21,38 @@ # 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 errno import os -import socket import subprocess -import sys -from contextlib import closing +from kimchi.config import config -from kimchi.exception import OperationFailed +WS_TOKENS_DIR = '/var/lib/kimchi/vnc-tokens' -def _getFreePort(): - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - with closing(sock): - try: - sock.bind(("0.0.0.0", 0)) - except: - raise OperationFailed("Could not find a free port") +def new_ws_proxy(): + try: + os.makedirs(WS_TOKENS_DIR, mode=0755) + except OSError as e: + if e.errno == errno.EEXIST: + pass - return sock.getsockname()[1] - -def new_ws_proxy(target_port): - src_port = _getFreePort() cmd = os.path.join(os.path.dirname(__file__), 'websockify.py') - args = ['python', cmd, str(src_port), '--timeout', '10', - '--idle-timeout', '10', 'localhost:%s' % target_port] + args = ['python', cmd, config.get('novnc', 'vnc_proxy_port'), + '--target-config', WS_TOKENS_DIR] p = subprocess.Popen(args, close_fds=True) + return p + + +def add_proxy_token(name, port): + with open(os.path.join(WS_TOKENS_DIR, name), 'w') as f: + f.write('%s: localhost:%s' % (name.encode('utf-8'), port)) + - return src_port +def remove_proxy_token(name): + try: + os.unlink(os.path.join(WS_TOKENS_DIR, name)) + except OSError: + pass diff --git a/ui/js/src/kimchi.api.js b/ui/js/src/kimchi.api.js index fbcf4a2..a4608ff 100644 --- a/ui/js/src/kimchi.api.js +++ b/ui/js/src/kimchi.api.js @@ -291,17 +291,19 @@ var kimchi = { dataType : 'json' }).done(function(data, textStatus, xhr) { http_port = data['http_port']; + proxy_port = data['vnc_proxy_port']; kimchi.requestJSON({ url : "/vms/" + encodeURIComponent(vm) + "/connect", type : "POST", dataType : "json" - }).done(function(data, textStatus, xhr) { + }).done(function() { /** * Due to problems with web sockets and self-signed * certificates, for now we will always redirect to http */ url = 'http://' + location.hostname + ':' + http_port; - url += "/vnc_auto.html?port=" + data.graphics.port; + url += "/vnc_auto.html?port=" + proxy_port; + url += "&path=?token=" + encodeURIComponent(vm); window.open(url); }); }).error(function() { -- 1.8.4.2
Mockmodel is used to test restapi's request validation and response. We don't need to start a real vnc server because we don't run functional test with mock model. Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 9488078..9ecef77 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -44,7 +44,6 @@ except ImportError: import kimchi.model from kimchi import config from kimchi import network as knetwork -from kimchi import vnc from kimchi.asynctask import AsyncTask from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter @@ -59,20 +58,8 @@ 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) - - if len(proc.stdout.readlines()) == 0: - p = subprocess.Popen(args, close_fds=True) - def get_capabilities(self): return {'libvirt_stream_protocols': ['http', 'https', 'ftp', 'ftps', 'tftp'], 'qemu_stream': True, @@ -85,7 +72,6 @@ class MockModel(object): self._mock_templates = {} self._mock_storagepools = {'default': MockStoragePool('default')} self._mock_networks = {'default': MockNetwork('default')} - self._mock_graphics_ports = {} self._mock_interfaces = self.dummy_interfaces() self.next_taskid = 1 self.storagepool_activate('default') @@ -121,7 +107,6 @@ 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) return vm.info def vm_delete(self, name): @@ -140,8 +125,7 @@ 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 + pass def vms_create(self, params): t_name = kimchi.model.template_name_from_uri(params['template']) -- 1.8.4.2
On 01/07/2014 06:56 AM, Mark Wu wrote:
The configuration is also needed for other code except starting kimchi server. So it should be moved to a separate module, config.py. Then the configuration can be accessed directly by importing config module.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/config.py.in | 24 ++++++++++++++++++++++++ src/kimchid.in | 20 +------------------- 2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index f3c408a..72308ff 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -27,6 +27,7 @@ import os import platform
+from ConfigParser import SafeConfigParser from glob import iglob
@@ -166,5 +167,28 @@ def get_plugin_tab_xml(name): return os.path.join(_get_plugin_ui_dir(name), 'config/tab-ext.xml')
+def _get_config(): + config = SafeConfigParser() + config.add_section("server") + config.set("server", "host", "0.0.0.0") + config.set("server", "port", "8000") + config.set("server", "ssl_port", "8001") + config.set("server", "ssl_cert", "") + config.set("server", "ssl_key", "") + config.set("server", "environment", "development") + config.add_section("logging") + config.set("logging", "log_dir", get_default_log_dir()) + config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + + if os.path.exists(CONFIG_FILE): + config.read(CONFIG_FILE) + return config +
+ +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir() +DEFAULT_LOG_LEVEL = "debug" +config = _get_config() +
We don't have a explicit rule for constants and I also don't know if there is one in PEP 8. But usually we put the constants in the begging of the file (after imports section).
+ if __name__ == '__main__': print get_prefix() diff --git a/src/kimchid.in b/src/kimchid.in index 7865713..548fa52 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -33,31 +33,13 @@ import kimchi.config if kimchi.config.without_installation(): sys.path.append(kimchi.config.get_prefix())
-from ConfigParser import SafeConfigParser +from kimchi.config import config from optparse import OptionParser
ACCESS_LOG = "kimchi-access.log" ERROR_LOG = "kimchi-error.log" -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir() -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir() -DEFAULT_LOG_LEVEL = "debug"
def main(options): - config = SafeConfigParser() - config.add_section("server") - config.set("server", "host", "0.0.0.0") - config.set("server", "port", "8000") - config.set("server", "ssl_port", "8001") - config.set("server", "ssl_cert", "") - config.set("server", "ssl_key", "") - config.set("server", "environment", "development") - config.add_section("logging") - config.set("logging", "log_dir", DEFAULT_LOG_DIR) - config.set("logging", "log_level", DEFAULT_LOG_LEVEL) - - if os.path.exists(CONFIG_FILE): - config.read(CONFIG_FILE) - host = config.get("server", "host") port = config.get("server", "port") ssl_port = config.get("server", "ssl_port")
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 01/07/2014 06:56 AM, Mark Wu wrote:
We are going to use one proxy instance to forward all vm's vnc traffics. The proxy instance listens on a fixed port instead of a random one. This patch makes the port configurable and exported to client by adding it to the response of '/config'.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- docs/API.md | 1 + src/kimchi.conf.in | 4 ++++ src/kimchi/config.py.in | 2 ++ src/kimchi/control/config.py | 4 +++- 4 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/docs/API.md b/docs/API.md index 0013e86..4d7bdff 100644 --- a/docs/API.md +++ b/docs/API.md @@ -432,6 +432,7 @@ Contains information about the application environment and configuration.
* **GET**: Retrieve configuration information * http_port: The port number on which the server is listening + * vnc_proxy_port: Port for vnc's websocket proxy to listen on * **POST**: *See Configuration Actions*
**Actions (POST):** diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index bf26c26..ac5a1f5 100644 --- a/src/kimchi.conf.in +++ b/src/kimchi.conf.in @@ -29,3 +29,7 @@
# Logging level: debug, info, warning, error or critical #log_level = debug + +[novnc] +# Port for vnc's websocket proxy to listen on +#vnc_proxy_port = 64667 diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index 72308ff..e1319e3 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -179,6 +179,8 @@ def _get_config(): config.add_section("logging") config.set("logging", "log_dir", get_default_log_dir()) config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + config.add_section("novnc") + config.set("novnc", "vnc_proxy_port", "64667")
if os.path.exists(CONFIG_FILE): config.read(CONFIG_FILE) diff --git a/src/kimchi/control/config.py b/src/kimchi/control/config.py index 3630172..5186ddd 100644 --- a/src/kimchi/control/config.py +++ b/src/kimchi/control/config.py @@ -25,6 +25,7 @@ import cherrypy
+from kimchi.config import config from kimchi.control.base import Collection, Resource
@@ -38,7 +39,8 @@ class Config(Resource):
@property def data(self): - return {'http_port': cherrypy.server.socket_port} + return {'http_port': cherrypy.server.socket_port, + 'vnc_proxy_port': config.get('novnc', 'vnc_proxy_port')}
class Capabilities(Resource):
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 01/07/2014 06:56 AM, Mark Wu wrote:
Currently, we start a new weksockify proxy on every vm's connect action. It causes trouble to allow the vnc traffic in firewall because we choose a random port for each vm. And it's also obversed that vnc connection fails because of race between client and proxy server. This patch changes to use websockify's target_cfg option instead of target_port. Then kimchi can dynamically add proxy configuration for a vm. It adds a map of target host/port and token pair to websockify's cfg on the connect request. With this change, all vnc traffic can be forwared in one tcp port, and therefore it's possible to add firewall rule for kimchi's vnc remote access.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/model.py | 14 ++++++-------- src/kimchi/server.py | 5 +++++ src/kimchi/vnc.py | 41 +++++++++++++++++++++++------------------ ui/js/src/kimchi.api.js | 6 ++++-- 4 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a21fcf7..10e6c3f 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -129,7 +129,6 @@ class Model(object): self.libvirt_uri = libvirt_uri or 'qemu:///system' self.conn = LibvirtConnection(self.libvirt_uri) self.objstore = ObjectStore(objstore_loc) - self.graphics_ports = {} self.next_taskid = 1 self.stats = {} self.host_stats = defaultdict(int) @@ -504,10 +503,7 @@ 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' - else None) + graphics_type, graphics_port = self._vm_get_graphics(name) try: if state == 'running': screenshot = self.vmscreenshot_lookup(name) @@ -567,6 +563,8 @@ class Model(object): with self.objstore as session: session.delete('vm', dom.UUIDString(), ignore_missing=True)
+ vnc.remove_proxy_token(name) + def vm_start(self, name): dom = self._get_vm(name) dom.create() @@ -596,10 +594,10 @@ class Model(object): 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 + vnc.add_proxy_token(name, port) else: - raise OperationFailed("Unable to find VNC port in %s" % name) + raise OperationFailed("Only able to connect to running vm's vnc " + "graphics.")
def vms_create(self, params): conn = self.conn.get() diff --git a/src/kimchi/server.py b/src/kimchi/server.py index 1467c87..b820263 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -32,6 +32,7 @@ from kimchi import auth from kimchi import config from kimchi import model from kimchi import mockmodel +from kimchi import vnc from kimchi.root import Root from kimchi.utils import get_enabled_plugins, import_class
@@ -188,6 +189,10 @@ class Server(object): else: model_instance = model.Model()
+ if isinstance(model_instance, model.Model): + vnc_ws_proxy = vnc.new_ws_proxy() + cherrypy.engine.subscribe('exit', vnc_ws_proxy.kill) + self.app = cherrypy.tree.mount(Root(model_instance, dev_env), config=self.configObj) self._load_plugins() diff --git a/src/kimchi/vnc.py b/src/kimchi/vnc.py index 089d64a..dc70b46 100644 --- a/src/kimchi/vnc.py +++ b/src/kimchi/vnc.py @@ -21,33 +21,38 @@ # 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 errno import os -import socket import subprocess -import sys
-from contextlib import closing +from kimchi.config import config
-from kimchi.exception import OperationFailed +WS_TOKENS_DIR = '/var/lib/kimchi/vnc-tokens'
-def _getFreePort(): - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - with closing(sock): - try: - sock.bind(("0.0.0.0", 0)) - except: - raise OperationFailed("Could not find a free port") +def new_ws_proxy(): + try: + os.makedirs(WS_TOKENS_DIR, mode=0755) + except OSError as e: + if e.errno == errno.EEXIST: + pass
- return sock.getsockname()[1] - -def new_ws_proxy(target_port): - src_port = _getFreePort() cmd = os.path.join(os.path.dirname(__file__), 'websockify.py') - args = ['python', cmd, str(src_port), '--timeout', '10', - '--idle-timeout', '10', 'localhost:%s' % target_port] + args = ['python', cmd, config.get('novnc', 'vnc_proxy_port'), + '--target-config', WS_TOKENS_DIR] p = subprocess.Popen(args, close_fds=True) + return p + + +def add_proxy_token(name, port): + with open(os.path.join(WS_TOKENS_DIR, name), 'w') as f: + f.write('%s: localhost:%s' % (name.encode('utf-8'), port)) +
- return src_port +def remove_proxy_token(name): + try: + os.unlink(os.path.join(WS_TOKENS_DIR, name)) + except OSError: + pass diff --git a/ui/js/src/kimchi.api.js b/ui/js/src/kimchi.api.js index fbcf4a2..a4608ff 100644 --- a/ui/js/src/kimchi.api.js +++ b/ui/js/src/kimchi.api.js @@ -291,17 +291,19 @@ var kimchi = { dataType : 'json' }).done(function(data, textStatus, xhr) { http_port = data['http_port']; + proxy_port = data['vnc_proxy_port']; kimchi.requestJSON({ url : "/vms/" + encodeURIComponent(vm) + "/connect", type : "POST", dataType : "json" - }).done(function(data, textStatus, xhr) { + }).done(function() { /** * Due to problems with web sockets and self-signed * certificates, for now we will always redirect to http */ url = 'http://' + location.hostname + ':' + http_port; - url += "/vnc_auto.html?port=" + data.graphics.port; + url += "/vnc_auto.html?port=" + proxy_port; + url += "&path=?token=" + encodeURIComponent(vm); window.open(url); }); }).error(function() {
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 01/07/2014 06:56 AM, Mark Wu wrote:
Mockmodel is used to test restapi's request validation and response. We don't need to start a real vnc server because we don't run functional test with mock model.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-)
diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 9488078..9ecef77 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -44,7 +44,6 @@ except ImportError: import kimchi.model from kimchi import config from kimchi import network as knetwork -from kimchi import vnc from kimchi.asynctask import AsyncTask from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter @@ -59,20 +58,8 @@ 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) - - if len(proc.stdout.readlines()) == 0: - p = subprocess.Popen(args, close_fds=True) - def get_capabilities(self): return {'libvirt_stream_protocols': ['http', 'https', 'ftp', 'ftps', 'tftp'], 'qemu_stream': True, @@ -85,7 +72,6 @@ class MockModel(object): self._mock_templates = {} self._mock_storagepools = {'default': MockStoragePool('default')} self._mock_networks = {'default': MockNetwork('default')} - self._mock_graphics_ports = {} self._mock_interfaces = self.dummy_interfaces() self.next_taskid = 1 self.storagepool_activate('default') @@ -121,7 +107,6 @@ 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) return vm.info
def vm_delete(self, name): @@ -140,8 +125,7 @@ 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 + pass
def vms_create(self, params): t_name = kimchi.model.template_name_from_uri(params['template'])
The configuration is also needed for other code except starting kimchi server. So it should be moved to a separate module, config.py. Then the configuration can be accessed directly by importing config module.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/config.py.in | 24 ++++++++++++++++++++++++ src/kimchid.in | 20 +------------------- 2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index f3c408a..72308ff 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -27,6 +27,7 @@ import os import platform
+from ConfigParser import SafeConfigParser from glob import iglob
@@ -166,5 +167,28 @@ def get_plugin_tab_xml(name): return os.path.join(_get_plugin_ui_dir(name), 'config/tab-ext.xml')
+def _get_config(): + config = SafeConfigParser() + config.add_section("server") + config.set("server", "host", "0.0.0.0") + config.set("server", "port", "8000") + config.set("server", "ssl_port", "8001") + config.set("server", "ssl_cert", "") + config.set("server", "ssl_key", "") + config.set("server", "environment", "development") + config.add_section("logging") + config.set("logging", "log_dir", get_default_log_dir()) + config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + + if os.path.exists(CONFIG_FILE): + config.read(CONFIG_FILE) + return config + + +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir() +DEFAULT_LOG_LEVEL = "debug" +config = _get_config() + + if __name__ == '__main__': print get_prefix() diff --git a/src/kimchid.in b/src/kimchid.in index 7865713..548fa52 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -33,31 +33,13 @@ import kimchi.config if kimchi.config.without_installation(): sys.path.append(kimchi.config.get_prefix())
-from ConfigParser import SafeConfigParser +from kimchi.config import config from optparse import OptionParser
ACCESS_LOG = "kimchi-access.log" ERROR_LOG = "kimchi-error.log" -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir() -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir() -DEFAULT_LOG_LEVEL = "debug"
def main(options): - config = SafeConfigParser() - config.add_section("server") - config.set("server", "host", "0.0.0.0") - config.set("server", "port", "8000") - config.set("server", "ssl_port", "8001") - config.set("server", "ssl_cert", "") - config.set("server", "ssl_key", "") - config.set("server", "environment", "development") - config.add_section("logging") - config.set("logging", "log_dir", DEFAULT_LOG_DIR) - config.set("logging", "log_level", DEFAULT_LOG_LEVEL) - - if os.path.exists(CONFIG_FILE): - config.read(CONFIG_FILE) - Starting here: host = config.get("server", "host") port = config.get("server", "port") ssl_port = config.get("server", "ssl_port") This may be a further refactor , but seems we can use set_defaults of
On 2014年01月07日 16:56, Mark Wu wrote: optionparser to set config dict as default value of option parser, so that the annoying dirty lines of reading every item of config can be removed. REF: http://docs.python.org/3.3/library/optparse.html#optparse.OptionParser.set_d...
Reviewed-by: Royce Lv<lvroyce@linux.vnet.ibm.com> On 2014年01月07日 16:56, Mark Wu wrote:
We are going to use one proxy instance to forward all vm's vnc traffics. The proxy instance listens on a fixed port instead of a random one. This patch makes the port configurable and exported to client by adding it to the response of '/config'.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- docs/API.md | 1 + src/kimchi.conf.in | 4 ++++ src/kimchi/config.py.in | 2 ++ src/kimchi/control/config.py | 4 +++- 4 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/docs/API.md b/docs/API.md index 0013e86..4d7bdff 100644 --- a/docs/API.md +++ b/docs/API.md @@ -432,6 +432,7 @@ Contains information about the application environment and configuration.
* **GET**: Retrieve configuration information * http_port: The port number on which the server is listening + * vnc_proxy_port: Port for vnc's websocket proxy to listen on * **POST**: *See Configuration Actions*
**Actions (POST):** diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index bf26c26..ac5a1f5 100644 --- a/src/kimchi.conf.in +++ b/src/kimchi.conf.in @@ -29,3 +29,7 @@
# Logging level: debug, info, warning, error or critical #log_level = debug + +[novnc] +# Port for vnc's websocket proxy to listen on +#vnc_proxy_port = 64667 diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index 72308ff..e1319e3 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -179,6 +179,8 @@ def _get_config(): config.add_section("logging") config.set("logging", "log_dir", get_default_log_dir()) config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + config.add_section("novnc") + config.set("novnc", "vnc_proxy_port", "64667")
if os.path.exists(CONFIG_FILE): config.read(CONFIG_FILE) diff --git a/src/kimchi/control/config.py b/src/kimchi/control/config.py index 3630172..5186ddd 100644 --- a/src/kimchi/control/config.py +++ b/src/kimchi/control/config.py @@ -25,6 +25,7 @@ import cherrypy
+from kimchi.config import config from kimchi.control.base import Collection, Resource
@@ -38,7 +39,8 @@ class Config(Resource):
@property def data(self): - return {'http_port': cherrypy.server.socket_port} + return {'http_port': cherrypy.server.socket_port, + 'vnc_proxy_port': config.get('novnc', 'vnc_proxy_port')}
class Capabilities(Resource):
Reviewed-by: Royce Lv<lvroyce@linux.vnet.ibm.com> On 2014年01月07日 16:56, Mark Wu wrote:
Currently, we start a new weksockify proxy on every vm's connect action. It causes trouble to allow the vnc traffic in firewall because we choose a random port for each vm. And it's also obversed that vnc connection fails because of race between client and proxy server. This patch changes to use websockify's target_cfg option instead of target_port. Then kimchi can dynamically add proxy configuration for a vm. It adds a map of target host/port and token pair to websockify's cfg on the connect request. With this change, all vnc traffic can be forwared in one tcp port, and therefore it's possible to add firewall rule for kimchi's vnc remote access.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/model.py | 14 ++++++-------- src/kimchi/server.py | 5 +++++ src/kimchi/vnc.py | 41 +++++++++++++++++++++++------------------ ui/js/src/kimchi.api.js | 6 ++++-- 4 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a21fcf7..10e6c3f 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -129,7 +129,6 @@ class Model(object): self.libvirt_uri = libvirt_uri or 'qemu:///system' self.conn = LibvirtConnection(self.libvirt_uri) self.objstore = ObjectStore(objstore_loc) - self.graphics_ports = {} self.next_taskid = 1 self.stats = {} self.host_stats = defaultdict(int) @@ -504,10 +503,7 @@ 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' - else None) + graphics_type, graphics_port = self._vm_get_graphics(name) try: if state == 'running': screenshot = self.vmscreenshot_lookup(name) @@ -567,6 +563,8 @@ class Model(object): with self.objstore as session: session.delete('vm', dom.UUIDString(), ignore_missing=True)
+ vnc.remove_proxy_token(name) + def vm_start(self, name): dom = self._get_vm(name) dom.create() @@ -596,10 +594,10 @@ class Model(object): 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 + vnc.add_proxy_token(name, port) else: - raise OperationFailed("Unable to find VNC port in %s" % name) + raise OperationFailed("Only able to connect to running vm's vnc " + "graphics.")
def vms_create(self, params): conn = self.conn.get() diff --git a/src/kimchi/server.py b/src/kimchi/server.py index 1467c87..b820263 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -32,6 +32,7 @@ from kimchi import auth from kimchi import config from kimchi import model from kimchi import mockmodel +from kimchi import vnc from kimchi.root import Root from kimchi.utils import get_enabled_plugins, import_class
@@ -188,6 +189,10 @@ class Server(object): else: model_instance = model.Model()
+ if isinstance(model_instance, model.Model): + vnc_ws_proxy = vnc.new_ws_proxy() + cherrypy.engine.subscribe('exit', vnc_ws_proxy.kill) + self.app = cherrypy.tree.mount(Root(model_instance, dev_env), config=self.configObj) self._load_plugins() diff --git a/src/kimchi/vnc.py b/src/kimchi/vnc.py index 089d64a..dc70b46 100644 --- a/src/kimchi/vnc.py +++ b/src/kimchi/vnc.py @@ -21,33 +21,38 @@ # 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 errno import os -import socket import subprocess -import sys
-from contextlib import closing +from kimchi.config import config
-from kimchi.exception import OperationFailed +WS_TOKENS_DIR = '/var/lib/kimchi/vnc-tokens'
-def _getFreePort(): - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - with closing(sock): - try: - sock.bind(("0.0.0.0", 0)) - except: - raise OperationFailed("Could not find a free port") +def new_ws_proxy(): + try: + os.makedirs(WS_TOKENS_DIR, mode=0755) + except OSError as e: + if e.errno == errno.EEXIST: + pass
- return sock.getsockname()[1] - -def new_ws_proxy(target_port): - src_port = _getFreePort() cmd = os.path.join(os.path.dirname(__file__), 'websockify.py') - args = ['python', cmd, str(src_port), '--timeout', '10', - '--idle-timeout', '10', 'localhost:%s' % target_port] + args = ['python', cmd, config.get('novnc', 'vnc_proxy_port'), + '--target-config', WS_TOKENS_DIR] p = subprocess.Popen(args, close_fds=True) + return p + + +def add_proxy_token(name, port): + with open(os.path.join(WS_TOKENS_DIR, name), 'w') as f: + f.write('%s: localhost:%s' % (name.encode('utf-8'), port)) +
- return src_port +def remove_proxy_token(name): + try: + os.unlink(os.path.join(WS_TOKENS_DIR, name)) + except OSError: + pass diff --git a/ui/js/src/kimchi.api.js b/ui/js/src/kimchi.api.js index fbcf4a2..a4608ff 100644 --- a/ui/js/src/kimchi.api.js +++ b/ui/js/src/kimchi.api.js @@ -291,17 +291,19 @@ var kimchi = { dataType : 'json' }).done(function(data, textStatus, xhr) { http_port = data['http_port']; + proxy_port = data['vnc_proxy_port']; kimchi.requestJSON({ url : "/vms/" + encodeURIComponent(vm) + "/connect", type : "POST", dataType : "json" - }).done(function(data, textStatus, xhr) { + }).done(function() { /** * Due to problems with web sockets and self-signed * certificates, for now we will always redirect to http */ url = 'http://' + location.hostname + ':' + http_port; - url += "/vnc_auto.html?port=" + data.graphics.port; + url += "/vnc_auto.html?port=" + proxy_port; + url += "&path=?token=" + encodeURIComponent(vm); window.open(url); }); }).error(function() {
On 2014年01月07日 16:56, Mark Wu wrote:
The configuration is also needed for other code except starting kimchi server. So it should be moved to a separate module, config.py. Then the configuration can be accessed directly by importing config module.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/config.py.in | 24 ++++++++++++++++++++++++ src/kimchid.in | 20 +------------------- 2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index f3c408a..72308ff 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -27,6 +27,7 @@ import os import platform
+from ConfigParser import SafeConfigParser from glob import iglob
@@ -166,5 +167,28 @@ def get_plugin_tab_xml(name): return os.path.join(_get_plugin_ui_dir(name), 'config/tab-ext.xml')
+def _get_config(): + config = SafeConfigParser() + config.add_section("server") + config.set("server", "host", "0.0.0.0") + config.set("server", "port", "8000") + config.set("server", "ssl_port", "8001") + config.set("server", "ssl_cert", "") + config.set("server", "ssl_key", "") + config.set("server", "environment", "development") + config.add_section("logging") + config.set("logging", "log_dir", get_default_log_dir()) + config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + + if os.path.exists(CONFIG_FILE): + config.read(CONFIG_FILE) + return config + + +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir() +DEFAULT_LOG_LEVEL = "debug" +config = _get_config() + + if __name__ == '__main__': print get_prefix() diff --git a/src/kimchid.in b/src/kimchid.in index 7865713..548fa52 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -33,31 +33,13 @@ import kimchi.config if kimchi.config.without_installation(): sys.path.append(kimchi.config.get_prefix())
-from ConfigParser import SafeConfigParser +from kimchi.config import config from optparse import OptionParser
ACCESS_LOG = "kimchi-access.log" ERROR_LOG = "kimchi-error.log" -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir() -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir() -DEFAULT_LOG_LEVEL = "debug"
def main(options): - config = SafeConfigParser() - config.add_section("server") - config.set("server", "host", "0.0.0.0") - config.set("server", "port", "8000") - config.set("server", "ssl_port", "8001") - config.set("server", "ssl_cert", "") - config.set("server", "ssl_key", "") - config.set("server", "environment", "development") - config.add_section("logging") - config.set("logging", "log_dir", DEFAULT_LOG_DIR) - config.set("logging", "log_level", DEFAULT_LOG_LEVEL) - - if os.path.exists(CONFIG_FILE): - config.read(CONFIG_FILE) - Starting here: host = config.get("server", "host") port = config.get("server", "port") ssl_port = config.get("server", "ssl_port") And don't forget to update config object if you want to use it else where, because the right configuration is processed by cmd args.In your
On 2014年01月08日 10:55, Royce Lv wrote: patch it won't cause problem because you don't accepted cmd args of vnc port, but if in the future if we want to use config object, that'll cause problem.
This may be a further refactor , but seems we can use set_defaults of optionparser to set config dict as default value of option parser, so that the annoying dirty lines of reading every item of config can be removed. REF: http://docs.python.org/3.3/library/optparse.html#optparse.OptionParser.set_d...
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 01/08/2014 10:55 AM, Royce Lv wrote:
The configuration is also needed for other code except starting kimchi server. So it should be moved to a separate module, config.py. Then the configuration can be accessed directly by importing config module.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/config.py.in | 24 ++++++++++++++++++++++++ src/kimchid.in | 20 +------------------- 2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index f3c408a..72308ff 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -27,6 +27,7 @@ import os import platform
+from ConfigParser import SafeConfigParser from glob import iglob
@@ -166,5 +167,28 @@ def get_plugin_tab_xml(name): return os.path.join(_get_plugin_ui_dir(name), 'config/tab-ext.xml')
+def _get_config(): + config = SafeConfigParser() + config.add_section("server") + config.set("server", "host", "0.0.0.0") + config.set("server", "port", "8000") + config.set("server", "ssl_port", "8001") + config.set("server", "ssl_cert", "") + config.set("server", "ssl_key", "") + config.set("server", "environment", "development") + config.add_section("logging") + config.set("logging", "log_dir", get_default_log_dir()) + config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + + if os.path.exists(CONFIG_FILE): + config.read(CONFIG_FILE) + return config + + +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir() +DEFAULT_LOG_LEVEL = "debug" +config = _get_config() + + if __name__ == '__main__': print get_prefix() diff --git a/src/kimchid.in b/src/kimchid.in index 7865713..548fa52 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -33,31 +33,13 @@ import kimchi.config if kimchi.config.without_installation(): sys.path.append(kimchi.config.get_prefix())
-from ConfigParser import SafeConfigParser +from kimchi.config import config from optparse import OptionParser
ACCESS_LOG = "kimchi-access.log" ERROR_LOG = "kimchi-error.log" -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir() -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir() -DEFAULT_LOG_LEVEL = "debug"
def main(options): - config = SafeConfigParser() - config.add_section("server") - config.set("server", "host", "0.0.0.0") - config.set("server", "port", "8000") - config.set("server", "ssl_port", "8001") - config.set("server", "ssl_cert", "") - config.set("server", "ssl_key", "") - config.set("server", "environment", "development") - config.add_section("logging") - config.set("logging", "log_dir", DEFAULT_LOG_DIR) - config.set("logging", "log_level", DEFAULT_LOG_LEVEL) - - if os.path.exists(CONFIG_FILE): - config.read(CONFIG_FILE) - Starting here: host = config.get("server", "host") port = config.get("server", "port") ssl_port = config.get("server", "ssl_port") This may be a further refactor , but seems we can use set_defaults of
On 2014年01月07日 16:56, Mark Wu wrote: optionparser to set config dict as default value of option parser, so that the annoying dirty lines of reading every item of config can be removed. REF: http://docs.python.org/3.3/library/optparse.html#optparse.OptionParser.set_d... Your idea sounds good. But it's not feasible. We still need get the value from config and then construct the dict for set_defaults.
On 01/08/2014 03:07 AM, Aline Manera wrote:
On 01/07/2014 06:56 AM, Mark Wu wrote:
The configuration is also needed for other code except starting kimchi server. So it should be moved to a separate module, config.py. Then the configuration can be accessed directly by importing config module.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/config.py.in | 24 ++++++++++++++++++++++++ src/kimchid.in | 20 +------------------- 2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index f3c408a..72308ff 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -27,6 +27,7 @@ import os import platform
+from ConfigParser import SafeConfigParser from glob import iglob
@@ -166,5 +167,28 @@ def get_plugin_tab_xml(name): return os.path.join(_get_plugin_ui_dir(name), 'config/tab-ext.xml')
+def _get_config(): + config = SafeConfigParser() + config.add_section("server") + config.set("server", "host", "0.0.0.0") + config.set("server", "port", "8000") + config.set("server", "ssl_port", "8001") + config.set("server", "ssl_cert", "") + config.set("server", "ssl_key", "") + config.set("server", "environment", "development") + config.add_section("logging") + config.set("logging", "log_dir", get_default_log_dir()) + config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + + if os.path.exists(CONFIG_FILE): + config.read(CONFIG_FILE) + return config +
+ +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir() +DEFAULT_LOG_LEVEL = "debug" +config = _get_config() +
We don't have a explicit rule for constants and I also don't know if there is one in PEP 8. But usually we put the constants in the begging of the file (after imports section).
ACK
+ if __name__ == '__main__': print get_prefix() diff --git a/src/kimchid.in b/src/kimchid.in index 7865713..548fa52 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -33,31 +33,13 @@ import kimchi.config if kimchi.config.without_installation(): sys.path.append(kimchi.config.get_prefix())
-from ConfigParser import SafeConfigParser +from kimchi.config import config from optparse import OptionParser
ACCESS_LOG = "kimchi-access.log" ERROR_LOG = "kimchi-error.log" -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir() -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir() -DEFAULT_LOG_LEVEL = "debug"
def main(options): - config = SafeConfigParser() - config.add_section("server") - config.set("server", "host", "0.0.0.0") - config.set("server", "port", "8000") - config.set("server", "ssl_port", "8001") - config.set("server", "ssl_cert", "") - config.set("server", "ssl_key", "") - config.set("server", "environment", "development") - config.add_section("logging") - config.set("logging", "log_dir", DEFAULT_LOG_DIR) - config.set("logging", "log_level", DEFAULT_LOG_LEVEL) - - if os.path.exists(CONFIG_FILE): - config.read(CONFIG_FILE) - host = config.get("server", "host") port = config.get("server", "port") ssl_port = config.get("server", "ssl_port")
On 2014年01月07日 16:56, Mark Wu wrote:
The configuration is also needed for other code except starting kimchi server. So it should be moved to a separate module, config.py. Then the configuration can be accessed directly by importing config module.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/config.py.in | 24 ++++++++++++++++++++++++ src/kimchid.in | 20 +------------------- 2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index f3c408a..72308ff 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -27,6 +27,7 @@ import os import platform
+from ConfigParser import SafeConfigParser from glob import iglob
@@ -166,5 +167,28 @@ def get_plugin_tab_xml(name): return os.path.join(_get_plugin_ui_dir(name), 'config/tab-ext.xml')
+def _get_config(): + config = SafeConfigParser() + config.add_section("server") + config.set("server", "host", "0.0.0.0") + config.set("server", "port", "8000") + config.set("server", "ssl_port", "8001") + config.set("server", "ssl_cert", "") + config.set("server", "ssl_key", "") + config.set("server", "environment", "development") + config.add_section("logging") + config.set("logging", "log_dir", get_default_log_dir()) + config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + + if os.path.exists(CONFIG_FILE): + config.read(CONFIG_FILE) + return config + + +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir() +DEFAULT_LOG_LEVEL = "debug" +config = _get_config() + + if __name__ == '__main__': print get_prefix() diff --git a/src/kimchid.in b/src/kimchid.in index 7865713..548fa52 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -33,31 +33,13 @@ import kimchi.config if kimchi.config.without_installation(): sys.path.append(kimchi.config.get_prefix())
-from ConfigParser import SafeConfigParser +from kimchi.config import config from optparse import OptionParser
ACCESS_LOG = "kimchi-access.log" ERROR_LOG = "kimchi-error.log" -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir() -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir() -DEFAULT_LOG_LEVEL = "debug"
def main(options): - config = SafeConfigParser() - config.add_section("server") - config.set("server", "host", "0.0.0.0") - config.set("server", "port", "8000") - config.set("server", "ssl_port", "8001") - config.set("server", "ssl_cert", "") - config.set("server", "ssl_key", "") - config.set("server", "environment", "development") - config.add_section("logging") - config.set("logging", "log_dir", DEFAULT_LOG_DIR) - config.set("logging", "log_level", DEFAULT_LOG_LEVEL) - - if os.path.exists(CONFIG_FILE): - config.read(CONFIG_FILE) - Starting here: host = config.get("server", "host") port = config.get("server", "port") ssl_port = config.get("server", "ssl_port") And don't forget to update config object if you want to use it else where, because the right configuration is processed by cmd args.In your patch it won't cause problem because you don't accepted cmd args of vnc port, but if in the future if we want to use config object,
On 2014年01月08日 10:55, Royce Lv wrote: that'll cause problem. I think we don't need update the config object with the values passed from the command lines. We could consider we have two kinds of configurations in the config.py: one could be overrode by the command line options and the other doesn't have corresponding
On 01/08/2014 01:51 PM, Royce Lv wrote: option in command line. For the first kind, kimchi server use the merged 'options' instead of directly getting config from config module. For the second case, it's not related to server and it doesn't make sense to pass the option from command line and pass around the option inside kimchi. The user module should be able to get its config directly. You could feel that it has some potential inconsistent. I think we can simply the command line options to resolve it. We could remove the options which are in the config and use one parameter: the path of configuration file instead. It will not cause any inconsistency. Are you fine to resolve it in a following up patch after we get agreement on this direction? Thanks! Mark.
This may be a further refactor , but seems we can use set_defaults of optionparser to set config dict as default value of option parser, so that the annoying dirty lines of reading every item of config can be removed. REF: http://docs.python.org/3.3/library/optparse.html#optparse.OptionParser.set_d...
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 2014年01月08日 17:42, Mark Wu wrote:
On 2014年01月07日 16:56, Mark Wu wrote:
The configuration is also needed for other code except starting kimchi server. So it should be moved to a separate module, config.py. Then the configuration can be accessed directly by importing config module.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/config.py.in | 24 ++++++++++++++++++++++++ src/kimchid.in | 20 +------------------- 2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index f3c408a..72308ff 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -27,6 +27,7 @@ import os import platform
+from ConfigParser import SafeConfigParser from glob import iglob
@@ -166,5 +167,28 @@ def get_plugin_tab_xml(name): return os.path.join(_get_plugin_ui_dir(name), 'config/tab-ext.xml')
+def _get_config(): + config = SafeConfigParser() + config.add_section("server") + config.set("server", "host", "0.0.0.0") + config.set("server", "port", "8000") + config.set("server", "ssl_port", "8001") + config.set("server", "ssl_cert", "") + config.set("server", "ssl_key", "") + config.set("server", "environment", "development") + config.add_section("logging") + config.set("logging", "log_dir", get_default_log_dir()) + config.set("logging", "log_level", DEFAULT_LOG_LEVEL) + + if os.path.exists(CONFIG_FILE): + config.read(CONFIG_FILE) + return config + + +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir() +DEFAULT_LOG_LEVEL = "debug" +config = _get_config() + + if __name__ == '__main__': print get_prefix() diff --git a/src/kimchid.in b/src/kimchid.in index 7865713..548fa52 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -33,31 +33,13 @@ import kimchi.config if kimchi.config.without_installation(): sys.path.append(kimchi.config.get_prefix())
-from ConfigParser import SafeConfigParser +from kimchi.config import config from optparse import OptionParser
ACCESS_LOG = "kimchi-access.log" ERROR_LOG = "kimchi-error.log" -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir() -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir() -DEFAULT_LOG_LEVEL = "debug"
def main(options): - config = SafeConfigParser() - config.add_section("server") - config.set("server", "host", "0.0.0.0") - config.set("server", "port", "8000") - config.set("server", "ssl_port", "8001") - config.set("server", "ssl_cert", "") - config.set("server", "ssl_key", "") - config.set("server", "environment", "development") - config.add_section("logging") - config.set("logging", "log_dir", DEFAULT_LOG_DIR) - config.set("logging", "log_level", DEFAULT_LOG_LEVEL) - - if os.path.exists(CONFIG_FILE): - config.read(CONFIG_FILE) - Starting here: host = config.get("server", "host") port = config.get("server", "port") ssl_port = config.get("server", "ssl_port") And don't forget to update config object if you want to use it else where, because the right configuration is processed by cmd args.In your patch it won't cause problem because you don't accepted cmd args of vnc port, but if in the future if we want to use config object,
On 2014年01月08日 10:55, Royce Lv wrote: that'll cause problem. I think we don't need update the config object with the values passed from the command lines. We could consider we have two kinds of configurations in the config.py: one could be overrode by the command line options and the other doesn't have corresponding
On 01/08/2014 01:51 PM, Royce Lv wrote: option in command line. For the first kind, kimchi server use the merged 'options' instead of directly getting config from config module. For the second case, it's not related to server and it doesn't make sense to pass the option from command line and pass around the option inside kimchi. The user module should be able to get its config directly.
You could feel that it has some potential inconsistent. I think we can simply the command line options to resolve it. We could remove the options which are in the config and use one parameter: the path of configuration file instead. It will not cause any inconsistency. Are you fine to resolve it in a following up patch after we get agreement on this direction? This idea is cool for me, so I think I have no complaints about this version then.
Thanks! Mark.
This may be a further refactor , but seems we can use set_defaults of optionparser to set config dict as default value of option parser, so that the annoying dirty lines of reading every item of config can be removed. REF: http://docs.python.org/3.3/library/optparse.html#optparse.OptionParser.set_d...
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (3)
-
Aline Manera -
Mark Wu -
Royce Lv