Xu He Jie has uploaded a new change for review.
Change subject: [WIP]Add text-based console support ......................................................................
[WIP]Add text-based console support
This patch use ssh protocol providing a text-based console for user. User can have another choice except spice and vnc. Text-based console consumes less bandwidth and generic ssh client can connect it. This patch try to keep same behaviour as vnc.
Two new params for VM creation: consoleEnable: if true, it will enable console support for this VM consolePort: the port used by ssh server. if -1 specified, the port will be allocated automatically. The allocated port will show at getVmStats().
setVmTicket: password: used to connect this console. ttl: after expired, the console will be closed. existingConnAction: if 'disconnect' specified, the current connection will be closed, and reopen with new params.
TODO: * Make console sharable. Current console only can connect by only one user * setTicket will be used by vnc and console seperately
Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Signed-off-by: Xu He Jie xuhj@linux.vnet.ibm.com --- M vdsm.spec.in M vdsm/Makefile.am A vdsm/consoleServer.py M vdsm/constants.py.in M vdsm/libvirtvm.py M vdsm/supervdsmServer.py M vdsm/utils.py M vdsm/vm.py 8 files changed, 377 insertions(+), 2 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/65/7165/1
diff --git a/vdsm.spec.in b/vdsm.spec.in index bf8684a..33c8d6f 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -545,6 +545,7 @@ %{_datadir}/%{vdsm_name}/blkid.py* %{_datadir}/%{vdsm_name}/caps.py* %{_datadir}/%{vdsm_name}/clientIF.py* +%{_datadir}/%{vdsm_name}/consoleServer.py* %{_datadir}/%{vdsm_name}/API.py* %{_datadir}/%{vdsm_name}/hooking.py* %{_datadir}/%{vdsm_name}/hooks.py* diff --git a/vdsm/Makefile.am b/vdsm/Makefile.am index 80befdb..dbb4dc3 100644 --- a/vdsm/Makefile.am +++ b/vdsm/Makefile.am @@ -31,6 +31,7 @@ caps.py \ clientIF.py \ configNetwork.py \ + consoleServer.py \ debugPluginClient.py \ guestIF.py \ hooking.py \ diff --git a/vdsm/consoleServer.py b/vdsm/consoleServer.py new file mode 100755 index 0000000..172fcae --- /dev/null +++ b/vdsm/consoleServer.py @@ -0,0 +1,279 @@ +#! /usr/bin/python +# +# Copyright (C) 2012, IBM Corporation +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# + +import sys +import logging +import logging.config +import socket +import select +import os +import subprocess + +import paramiko + +from vdsm import constants +from vdsm import utils + + +class PortAllocator(): + + def __init__(self, minPort, maxPort): + self._minPort = minPort + self._maxPort = maxPort + self._bitmap = utils.Bitmap(self._maxPort - self._minPort) + self._addrs = {} + + def getPort(self): + for i in range(0, self._maxPort - self._minPort): + if self._bitmap[i] == 0x0: + servSock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + servSock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + + try: + servSock.bind(("", i + self._minPort)) + except socket.error: + continue + finally: + servSock.close() + + self._bitmap[i] = 0x1 + return i + self._minPort + + raise Exception("Can not free port on address: %s", addr) + + def freePort(self, port): + print("free", port - self._minPort) + self._bitmap[port - self._minPort] = 0x0 + +portAllocator = PortAllocator(constants.SSH_CONSOLE_MIN_PORT, + constants.SSH_CONSOLE_MAX_PORT) + + +class SSHConsole(object): + AUTO_PORT = -1 + logger = logging.getLogger("console") + + def __init__(self, pty="", addr="", port=AUTO_PORT, + passwd="", timeout=-1, hostKeyFile="", + portAlloc=portAllocator): + self._prog = None + self._pty = pty + self._addr = addr + + self._portAlloc = portAllocator + self._port = port + if self._port == self.AUTO_PORT: + self._port = self._portAlloc.getPort() + + self._passwd = passwd + self._hostKeyFile = hostKeyFile + self._timeout = timeout + + def setPty(self, pty): + self._pty = pty + + def setPasswd(self, passwd): + self._passwd = passwd + + def setTimeout(self, timeout): + self._timeout = timeout + + def getPort(self): + return self._port + + def open(self): + self.logger.info("open console with: %s", {"pty": self._pty, + "addr": self._addr, + "port": self._port, + "timeout": self._timeout, + "hostKeyFile": self._hostKeyFile}) + f = open(self._pty, "rwa+") + self._prog = subprocess.Popen([constants.EXT_PYTHON, + os.path.join(constants.P_VDSM, __file__), + self._addr, + str(self._port), + self._passwd, + str(self._timeout), + self._hostKeyFile], + stdin=f, + stdout=f, + stderr=subprocess.STDOUT) + + def close(self): + if self._prog: + self._prog.kill() + self._prog.wait() + self._portAlloc.freePort(self._port) + + def isOpen(self): + return self._prog != None and self._prog.poll() == None + + +class ParamikoServerInf(paramiko.ServerInterface): + + def __init__(self, passwd): + self._passwd = passwd + + def check_channel_request(self, kind, chanid): + if kind == 'session': + return paramiko.OPEN_SUCCEEDED + return paramiko.OPEN_FAILED_UNKNOWN_CHANNEL_TYPE + + def check_auth_password(self, username, password): + if password != self._passwd: + return paramiko.AUTH_FAILED + return paramiko.AUTH_SUCCESSFUL + + def check_channel_shell_request(self, channel): + return True + + def check_channel_pty_request(self, channel, term, width, height, + pixelwidth, pixelheight, modes): + return True + + def check_auth_none(self, username): + return paramiko.AUTH_FAILED + + +class SSHConsoleServer(object): + + def __init__(self, logger, inputf, outputf, + addr, port, passwd, timeout, hostKeyFile): + self._logger = logger + self._output = outputf + self._input = inputf + self._addr = addr + self._port = port + self._passwd = passwd + self._timeout = timeout + self._hostKeyFile = hostKeyFile + + def start(self): + try: + hostKey = paramiko.RSAKey(filename=self._hostKeyFile) + except (paramiko.SSHException, IOError) as e: + self._logger.error("Can not load key from %s", + self._hostKeyFile, + exc_info=True) + return + + try: + servSock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + servSock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + except socket.error as e: + self._logger.error("create socket failed: %s", + e.message, exc_info=True) + return + + try: + servSock.bind((self._addr, self._port)) + servSock.listen(1) + except socket.error as e: + self._logger.error("failed binding address on %s:%s", + self._addr, self._port, exc_info=True) + servSock.close() + return + + while True: + self._logger.info("ssh console server waiting for connection") + + rd, wr, er = select.select([servSock, self._input], + [], [], self._timeout) + + if len(rd) == 0: + self._logger.info("connection timeout") + break + elif self._input in rd: + # if 0 returned means input was closed + if len(self._input.read()) == 0: + self._logger.info("console closed") + break + + clientSock, addr = servSock.accept() + self._logger.info("accept connection from %s", addr) + + trans = paramiko.Transport(clientSock) + trans.add_server_key(hostKey) + serverInf = ParamikoServerInf(self._passwd) + + try: + trans.start_server(server=serverInf) + except (paramiko.SSHException, EOFError) as e: + self._logger.error("start ssh server failed: %s", e.message) + clientSock.close() + continue + + chan = trans.accept() + + if chan is None: + self._logger.error("get SSH channel failed") + continue + + while True: + try: + rd, wr, er = select.select([chan, self._output], [], []) + + if chan in rd: + buf = chan.recv(1024) + + if len(buf) == 0: + self._logger.info("ssh channel closed") + break + + os.write(self._input.fileno(), buf) + elif self._output in rd: + buf = os.read(self._output.fileno(), 1) + + if chan.send(buf) == 0: + self._logger.info("ssh channel closed") + break + else: + self._logger.error("could not be happened", + exc_info=True) + + except IOError as e: + self._logger.error("got io error", exc_info=True) + break + + self._logger.info("ssh closed") + chan.close() + clientSock.close() + trans.close() + + servSock.close() + +if __name__ == '__main__': + logging.config.fileConfig(constants.P_VDSM_CONF + 'logger.conf') + logger = logging.getLogger("consoleServer") + + try: + addr = sys.argv[1] + port = int(sys.argv[2]) + passwd = sys.argv[3] + timeout = int(sys.argv[4]) + hostKey = sys.argv[5] + + logger.info("start a ssh server") + s = SSHConsoleServer(logger, sys.stdin, sys.stdout, + addr, port, passwd, timeout, hostKey) + s.start() + except Exception as e: + logger.error("ssh server failed: %s: %s", e.message, type(e)) diff --git a/vdsm/constants.py.in b/vdsm/constants.py.in index 3a5fdce..53ae56f 100644 --- a/vdsm/constants.py.in +++ b/vdsm/constants.py.in @@ -167,3 +167,9 @@ max_fds 4096 } """ + +# +# ssh console server port constants +# +SSH_CONSOLE_MAX_PORT = 65535 +SSH_CONSOLE_MIN_PORT = 12200 diff --git a/vdsm/libvirtvm.py b/vdsm/libvirtvm.py index ddd030e..ea3b1b8 100644 --- a/vdsm/libvirtvm.py +++ b/vdsm/libvirtvm.py @@ -24,6 +24,7 @@ import time import threading import json +import os.path
import vm from vdsm.define import ERROR, doneCode, errCode @@ -37,6 +38,7 @@ import caps from vdsm import netinfo import supervdsm +import consoleServer
_VMCHANNEL_DEVICE_NAME = 'com.redhat.rhevm.vdsm'
@@ -1232,6 +1234,7 @@ self._getUnderlyingVideoDeviceInfo() self._getUnderlyingControllerDeviceInfo() self._getUnderlyingBalloonDeviceInfo() + self._getUnderlyingConsoleDeviceInfo() # Obtain info of all unknown devices. Must be last! self._getUnderlyingUnknownDeviceInfo()
@@ -1260,6 +1263,23 @@ self.guestAgent = guestIF.GuestAgent(self._guestSocketFile, self.cif.channelListener, self.log, connect=utils.tobool(self.conf.get('vmchannel', 'true'))) + + if utils.tobool(self.conf.get("enableConsole", False)): + self.log.info("vm console enabled") + + supervdsm.getProxy().changePtyOwner( + int(os.path.basename(self.conf["consolePath"]))) + + self._console = consoleServer.SSHConsole( + self.conf['consolePath'], + "", + int(self.conf.get("consolePort", -1)), + "", + -1, + os.path.join( + constants.P_VDSM_KEYS, 'vdsmkey.pem')) + + self.conf["consolePort"] = self._console.getPort()
self._guestCpuRunning = self._dom.info()[0] == libvirt.VIR_DOMAIN_RUNNING if self.lastStatus not in ('Migration Destination', @@ -1931,6 +1951,21 @@ graphics.setAttribute('passwdValidTo', validto) if graphics.getAttribute('type') == 'spice': graphics.setAttribute('connected', connAct) + + if self._console: + self.log.debug("setticket for console") + if connAct == "disconnect": + if self._console.isOpen(): + self._console.close() + self._console.setPasswd(otp) + self._console.setTimeout(seconds) + self._console.open() + else: + if not self._console.isOpen(): + self._console.setPasswd(otp) + self._console.setTimeout(seconds) + self._console.open() + hooks.before_vm_set_ticket(self._lastXMLDesc, self.conf, params) self._dom.updateDeviceFlags(graphics.toxml(), 0) hooks.after_vm_set_ticket(self._lastXMLDesc, self.conf, params) @@ -2041,6 +2076,8 @@ self._vmStats.stop() if self.guestAgent: self.guestAgent.stop() + if self._console: + self._console.close() if self._dom: try: self._dom.destroyFlags(libvirt.VIR_DOMAIN_DESTROY_GRACEFUL) @@ -2219,6 +2256,17 @@ dev['address'] = address dev['alias'] = alias
+ def _getUnderlyingConsoleDeviceInfo(self): + """ + Obtain console device info from libvirt + """ + consolexml = xml.dom.minidom.parseString(self._lastXMLDesc) \ + .childNodes[0].getElementsByTagName('devices')[0] \ + .getElementsByTagName('console')[0] + + self.conf["consolePath"] = str(consolexml.getElementsByTagName( + 'source')[0].getAttribute('path')) + def _getUnderlyingVideoDeviceInfo(self): """ Obtain video devices info from libvirt. diff --git a/vdsm/supervdsmServer.py b/vdsm/supervdsmServer.py index 8d9ada4..e3966be 100755 --- a/vdsm/supervdsmServer.py +++ b/vdsm/supervdsmServer.py @@ -43,8 +43,8 @@ from supervdsm import _SuperVdsmManager, PIDFILE, ADDRESS from storage.fileUtils import chown, resolveGid, resolveUid from storage.fileUtils import validateAccess as _validateAccess -from vdsm.constants import METADATA_GROUP, METADATA_USER, EXT_UDEVADM, \ - DISKIMAGE_USER, DISKIMAGE_GROUP, P_LIBVIRT_VMCHANNELS +from vdsm.constants import METADATA_GROUP, METADATA_USER, EXT_CHOWN, \ + EXT_UDEVADM, DISKIMAGE_USER, DISKIMAGE_GROUP, P_LIBVIRT_VMCHANNELS from storage.devicemapper import _removeMapping, _getPathsStatus import storage.misc import configNetwork @@ -268,6 +268,18 @@ def removeFs(self, path): return mkimage.removeFs(path)
+ @logDecorator + def changePtyOwner(self, ptyNum): + if type(ptyNum) != int: + raise TypeError("ptyNum must be integer") + + cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)] + rc, out, err = storage.misc.execCmd(cmd, sudo=False) + if rc: + raise OSError(errno.EINVAL, "can not change own of /dev/pts/%d, " \ + "out: %s\nerr: %s" % + (ptyNum, out, err)) +
def __pokeParent(parentPid): try: diff --git a/vdsm/utils.py b/vdsm/utils.py index 5e2d4e5..f0ebf79 100644 --- a/vdsm/utils.py +++ b/vdsm/utils.py @@ -36,6 +36,7 @@ import fcntl import functools import stat +from array import array
import ethtool
@@ -829,3 +830,27 @@
def __unicode__(self): return unicode(self.cmd) + + +class Bitmap(object): + + def __init__(self, size): + self._size = size + self._array = array('B', [0] * ((size / 8) + 1)) + + def __len__(self): + return self._size + + def __getitem__(self, key): + if key >= self._size or key < 0: + raise IndexError + return (self._array[key / 8] >> (key % 8)) & 0x1 + + def __setitem__(self, key, value): + if key >= self._size or key < 0: + raise IndexError + p = key / 8 + if value == 0x1: + self._array[p] = self._array[p] | (0x1 << (key % 8)) + else: + self._array[p] = self._array[p] & ~(0x1 << (key % 8)) \ No newline at end of file diff --git a/vdsm/vm.py b/vdsm/vm.py index 44796c3..f2b8e04 100644 --- a/vdsm/vm.py +++ b/vdsm/vm.py @@ -311,6 +311,7 @@ SOUND_DEVICES: [], VIDEO_DEVICES: [], CONTROLLER_DEVICES: [], GENERAL_DEVICES: [], BALLOON_DEVICES: [], REDIR_DEVICES: []} + self._console = None
def _get_lastStatus(self): SHOW_PAUSED_STATES = ('Powering down', 'RebootInProgress', 'Up') @@ -971,6 +972,8 @@ stats['cdrom'] = self.conf['cdrom'] if 'boot' in self.conf: stats['boot'] = self.conf['boot'] + if utils.tobool(self.conf.get('enableConsole', False)): + stats['consolePort'] = self.conf['consolePort']
decStats = {} try:
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1:
Build Failed
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/445/ : ABORTED
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Adam Litke has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1: (8 inline comments)
.................................................... File vdsm/consoleServer.py Line 54: finally: Line 55: servSock.close() Line 56: Line 57: self._bitmap[i] = 0x1 Line 58: return i + self._minPort Have you tested how this algorithm works when ports in the range are in use by other applications? Does the bind fail quickly or does it hang? I wonder if this added complexity is worth it since you are just as likely to conflict with another application as you are to conflict with another open vm console. Line 59: Line 60: raise Exception("Can not free port on address: %s", addr) Line 61: Line 62: def freePort(self, port):
Line 64: self._bitmap[port - self._minPort] = 0x0 Line 65: Line 66: portAllocator = PortAllocator(constants.SSH_CONSOLE_MIN_PORT, Line 67: constants.SSH_CONSOLE_MAX_PORT) Line 68: I wonder if the above should go into utils.py so others can use it in the future. Line 69: Line 70: class SSHConsole(object): Line 71: AUTO_PORT = -1 Line 72: logger = logging.getLogger("console")
Line 126: def isOpen(self): Line 127: return self._prog != None and self._prog.poll() == None Line 128: Line 129: Line 130: class ParamikoServerInf(paramiko.ServerInterface): I think this should be named: ParamikoServerIf Line 131: Line 132: def __init__(self, passwd): Line 133: self._passwd = passwd Line 134:
Line 226: if chan is None: Line 227: self._logger.error("get SSH channel failed") Line 228: continue Line 229: Line 230: while True: Nested while loop. Please try to encapsulate this into a function to make the code more readable. Line 231: try: Line 232: rd, wr, er = select.select([chan, self._output], [], []) Line 233: Line 234: if chan in rd:
.................................................... File vdsm/libvirtvm.py Line 1259 Line 1260 Line 1261 Line 1262 Line 1263 Don't forget to update the VMFullDefinition data type in vdsm_api/vdsmapi-schema.json for any additions you make to the VM configuration parameters.
Line 1963: else: Line 1964: if not self._console.isOpen(): Line 1965: self._console.setPasswd(otp) Line 1966: self._console.setTimeout(seconds) Line 1967: self._console.open() There are three connAct modes: 'disconnect', 'keep', 'fail'. You should explicitly handle 'keep' and 'fail' differently. Line 1968: Line 1969: hooks.before_vm_set_ticket(self._lastXMLDesc, self.conf, params) Line 1970: self._dom.updateDeviceFlags(graphics.toxml(), 0) Line 1971: hooks.after_vm_set_ticket(self._lastXMLDesc, self.conf, params)
.................................................... File vdsm.spec.in Line 544: %{_datadir}/%{vdsm_name}/alignmentScan.py* Line 545: %{_datadir}/%{vdsm_name}/blkid.py* Line 546: %{_datadir}/%{vdsm_name}/caps.py* Line 547: %{_datadir}/%{vdsm_name}/clientIF.py* Line 548: %{_datadir}/%{vdsm_name}/consoleServer.py* We are now dependent on python-paramiko. Make sure to add that package dependency. Line 549: %{_datadir}/%{vdsm_name}/API.py* Line 550: %{_datadir}/%{vdsm_name}/hooking.py* Line 551: %{_datadir}/%{vdsm_name}/hooks.py* Line 552: %{_datadir}/%{vdsm_name}/libvirtev.py*
.................................................... File vdsm/vm.py Line 972: stats['cdrom'] = self.conf['cdrom'] Line 973: if 'boot' in self.conf: Line 974: stats['boot'] = self.conf['boot'] Line 975: if utils.tobool(self.conf.get('enableConsole', False)): Line 976: stats['consolePort'] = self.conf['consolePort'] Another friendly reminder to update the vdsmapi-schema.json file :) Line 977: Line 978: decStats = {} Line 979: try: Line 980: if self._vmStats:
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Xu He Jie has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1: (8 inline comments)
Adam, thanks for your review
.................................................... File vdsm/consoleServer.py Line 54: finally: Line 55: servSock.close() Line 56: Line 57: self._bitmap[i] = 0x1 Line 58: return i + self._minPort I have tested this situation. 'bitmap' can check the port used by vdsm-self. 'bind' can check the port used by other application. If port used by other application, 'bind' will failed quickly. Line 59: Line 60: raise Exception("Can not free port on address: %s", addr) Line 61: Line 62: def freePort(self, port):
Line 64: self._bitmap[port - self._minPort] = 0x0 Line 65: Line 66: portAllocator = PortAllocator(constants.SSH_CONSOLE_MIN_PORT, Line 67: constants.SSH_CONSOLE_MAX_PORT) Line 68: Done Line 69: Line 70: class SSHConsole(object): Line 71: AUTO_PORT = -1 Line 72: logger = logging.getLogger("console")
Line 126: def isOpen(self): Line 127: return self._prog != None and self._prog.poll() == None Line 128: Line 129: Line 130: class ParamikoServerInf(paramiko.ServerInterface): Done Line 131: Line 132: def __init__(self, passwd): Line 133: self._passwd = passwd Line 134:
Line 226: if chan is None: Line 227: self._logger.error("get SSH channel failed") Line 228: continue Line 229: Line 230: while True: Done Line 231: try: Line 232: rd, wr, er = select.select([chan, self._output], [], []) Line 233: Line 234: if chan in rd:
.................................................... File vdsm/libvirtvm.py Line 1259 Line 1260 Line 1261 Line 1262 Line 1263 Done
Line 1963: else: Line 1964: if not self._console.isOpen(): Line 1965: self._console.setPasswd(otp) Line 1966: self._console.setTimeout(seconds) Line 1967: self._console.open() Done Line 1968: Line 1969: hooks.before_vm_set_ticket(self._lastXMLDesc, self.conf, params) Line 1970: self._dom.updateDeviceFlags(graphics.toxml(), 0) Line 1971: hooks.after_vm_set_ticket(self._lastXMLDesc, self.conf, params)
.................................................... File vdsm.spec.in Line 544: %{_datadir}/%{vdsm_name}/alignmentScan.py* Line 545: %{_datadir}/%{vdsm_name}/blkid.py* Line 546: %{_datadir}/%{vdsm_name}/caps.py* Line 547: %{_datadir}/%{vdsm_name}/clientIF.py* Line 548: %{_datadir}/%{vdsm_name}/consoleServer.py* Done Line 549: %{_datadir}/%{vdsm_name}/API.py* Line 550: %{_datadir}/%{vdsm_name}/hooking.py* Line 551: %{_datadir}/%{vdsm_name}/hooks.py* Line 552: %{_datadir}/%{vdsm_name}/libvirtev.py*
.................................................... File vdsm/vm.py Line 972: stats['cdrom'] = self.conf['cdrom'] Line 973: if 'boot' in self.conf: Line 974: stats['boot'] = self.conf['boot'] Line 975: if utils.tobool(self.conf.get('enableConsole', False)): Line 976: stats['consolePort'] = self.conf['consolePort'] done. :) thanks for your review Line 977: Line 978: decStats = {} Line 979: try: Line 980: if self._vmStats:
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Adam Litke has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/consoleServer.py Line 54: finally: Line 55: servSock.close() Line 56: Line 57: self._bitmap[i] = 0x1 Line 58: return i + self._minPort In that case, why not drop the dependency on this Bitmap and just store the last used port and round-robin within that port range? It would be simpler and bind will catch any failures. Line 59: Line 60: raise Exception("Can not free port on address: %s", addr) Line 61: Line 62: def freePort(self, port):
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Xu He Jie has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1: (2 inline comments)
.................................................... File vdsm/consoleServer.py Line 53: continue Line 54: finally: Line 55: servSock.close() Line 56: Line 57: self._bitmap[i] = 0x1 there is a race problem Line 58: return i + self._minPort Line 59: Line 60: raise Exception("Can not free port on address: %s", addr) Line 61:
Line 54: finally: Line 55: servSock.close() Line 56: Line 57: self._bitmap[i] = 0x1 Line 58: return i + self._minPort SSHConsoleServer is started after setTicket. so the port is used after setTicket. If there is another VM created between the port allocation and setTicket, It can't check the port had already allocated to other VM by 'bind'.
But there still have problem if the port used by other application between port allocation and setTicket, SSHConsoleServer will startup failed. I am trying to find a way to resolve it. Line 59: Line 60: raise Exception("Can not free port on address: %s", addr) Line 61: Line 62: def freePort(self, port):
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/consoleServer.py Line 237: if len(buf) == 0: Line 238: self._logger.info("ssh channel closed") Line 239: break Line 240: Line 241: os.write(self._input.fileno(), buf) In "__main__", sys.stdin is given as "inputf" in "__init__", then "inputf" is assigned to "self._input". So write to "self._input" here is actually write to "sys.stdin". Why we write to "sys.stdin"?
After reading the code, I find that "sys.stdin" and "sys.stdout" are both redirected to the console pty, so they are interchangeable, but I still think the naming is a bit confused to me. Line 242: elif self._output in rd: Line 243: buf = os.read(self._output.fileno(), 1) Line 244: Line 245: if chan.send(buf) == 0:
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Xu He Jie has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/consoleServer.py Line 237: if len(buf) == 0: Line 238: self._logger.info("ssh channel closed") Line 239: break Line 240: Line 241: os.write(self._input.fileno(), buf) If we won't use stdin and stdout as the source, we don't need change all the hardcode sys.stdin. you just need change the argument that you passed to SSHConsoleServer.
And I plan won't use sys.stdin and std.stdout in next verison :)
Thanks for your review! Line 242: elif self._output in rd: Line 243: buf = os.read(self._output.fileno(), 1) Line 244: Line 245: if chan.send(buf) == 0:
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(I haven't looked at the code yet.. just do not forget to update the API schema)
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
ShaoHe Feng has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1: I would prefer that you didn't submit this
why not use fix port? such as ssh vmuuid@adrress -p port. or ssh vmname@adrress -p port
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: [WIP]Add text-based console support ......................................................................
Patch Set 1:
shaohef, it's a good idea to use fixed port. I think both dynamic port like VNC and fixed port is good.
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Xu He Jie has posted comments on this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2:
patchset2 changlog: 1. support console sharable 2. setTicket can be used by vnc|spice and console seperately 3. startup consoleServer process when vm startup 4. drop the bitmap for port auto-allocated 5. use crabRPC as rpc utils 6. update vdsm api schema
Royce, Shaohe, Zheng Sheng, Mark, thanks for your sugguestion.
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/654/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2:
The design looks good. In the commit message you introduce some alternative design. I would like +1 for the fixed port design and use uuid as ssh user name. I think we can have one thread for each VM. The thread can be OS thread or green thread from some other Python library.
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Mark Wu has posted comments on this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2: (15 inline comments)
Some small suggestions. I didn't do a full review on the consoleServer implementation because you're going to change it to the way of fix port. I also think the idea is cooler than the current one. Look forward to your next patch!
.................................................... File vdsm/consoleServer.py Line 26: import select Line 27: import os Line 28: import threading Line 29: import errno Line 30: intentional blank line? Line 31: import paramiko Line 32: Line 33: from vdsm import constants Line 34: from vdsm.betterPopen import BetterPopen
Line 28: import threading Line 29: import errno Line 30: Line 31: import paramiko Line 32: and here Line 33: from vdsm import constants Line 34: from vdsm.betterPopen import BetterPopen Line 35: from storage import misc Line 36: from storage.remoteFileHandler import CrabRPCProxy, CrabRPCServer
Line 76: sock.close() Line 77: continue Line 78: return p, sock Line 79: Line 80: raise Exception("can not allocated any port on %s", self._addr) You should never raise a general exception. I learn it from Adam :) Line 81: Line 82: def setTicket(self, passwd, timeout, connAct): Line 83: try: Line 84: return self._crabRPCProxy.callCrabRPCFunction(10, "setTicket",
Line 107: str(self._rpcProxyRd), Line 108: str(self._rpcServerWr), Line 109: self._hostKeyFile], Line 110: close_fds=False, env=env) Line 111: finally: No error handling for BetterPopen failure? Line 112: pty.close() Line 113: Line 114: def isOpen(self): Line 115: return self._prog != None and self._prog.poll() == None
Line 111: finally: Line 112: pty.close() Line 113: Line 114: def isOpen(self): Line 115: return self._prog != None and self._prog.poll() == None I don't understand what the poll function checks here. Line 116: Line 117: def close(self): Line 118: if self.isOpen(): Line 119: self._prog.terminate()
.................................................... File vdsm/constants.py.in Line 170: # Line 171: # ssh console server port constants Line 172: # Line 173: SSH_CONSOLE_MAX_PORT = 65535 Line 174: SSH_CONSOLE_MIN_PORT = 12200 it's not welcomed to put the definition here because it's only used by console server. Probably you will remove it in your next implementation. Just let you know.
.................................................... File vdsm/libvirtvm.py Line 1283: self.guestAgent = guestIF.GuestAgent(self._guestSocketFile, Line 1284: self.cif.channelListener, self.log, Line 1285: connect=utils.tobool(self.conf.get('vmchannel', 'true'))) Line 1286: Line 1287: if utils.tobool(self.conf.get("enableConsole", False)): is 'textConsole' better than 'enableConsole' ? Line 1288: self.log.info("vm console enabled") Line 1289: Line 1290: supervdsm.getProxy().changePtyOwner( Line 1291: int(os.path.basename(self.conf["consolePath"])))
Line 1989: hooks.before_vm_set_ticket(self._lastXMLDesc, self.conf, params) Line 1990: Line 1991: if actOn == "graphics": Line 1992: graphics = xml.dom.minidom.parseString(self._dom.XMLDesc(0)) \ Line 1993: .childNodes[0].getElementsByTagName('graphics')[0] Why change _domParseStr to xml.dom.minidom.parseString? Is it intentional? Line 1994: graphics.setAttribute('passwd', otp) Line 1995: if int(seconds) > 0: Line 1996: validto = time.strftime('%Y-%m-%dT%H:%M:%S', Line 1997: time.gmtime(time.time() + float(seconds)))
Line 2007: return {'status': Line 2008: {'code': errCode['ticketErr']['status']['code'], Line 2009: 'message': 'setTicket failed on console'}} Line 2010: else: Line 2011: self.log.error("console was disable") was disabled. Maybe 'was not enabled' is better. Line 2012: return {'status': {'code': Line 2013: errCode['ticketErr']['status']['code'], Line 2014: 'message': 'console was disable'}} Line 2015: else:
Line 2010: else: Line 2011: self.log.error("console was disable") Line 2012: return {'status': {'code': Line 2013: errCode['ticketErr']['status']['code'], Line 2014: 'message': 'console was disable'}} the same problem as line 2011 Line 2015: else: Line 2016: self.log.error("unsupported actOn argument: %s", actOn) Line 2017: return {'status': {'code': errCode['ticketErr']['status']['code'], Line 2018: 'message': 'the value of actOn was unsupported'}}
Line 2014: 'message': 'console was disable'}} Line 2015: else: Line 2016: self.log.error("unsupported actOn argument: %s", actOn) Line 2017: return {'status': {'code': errCode['ticketErr']['status']['code'], Line 2018: 'message': 'the value of actOn was unsupported'}} 'actOn' type %s was unsupported' % actOn more informative? Line 2019: Line 2020: hooks.after_vm_set_ticket(self._lastXMLDesc, self.conf, params) Line 2021: return {'status': doneCode} Line 2022:
Line 2309: def _getUnderlyingConsoleDeviceInfo(self): Line 2310: """ Line 2311: Obtain console device info from libvirt Line 2312: """ Line 2313: consolexml = xml.dom.minidom.parseString(self._lastXMLDesc) \ The same problem as line 1993. Line 2314: .childNodes[0].getElementsByTagName('devices')[0] \ Line 2315: .getElementsByTagName('console')[0] Line 2316: Line 2317: self.conf["consolePath"] = str(consolexml.getElementsByTagName(
.................................................... File vdsm/Makefile.am Line 30: blkid.py \ Line 31: caps.py \ Line 32: clientIF.py \ Line 33: configNetwork.py \ Line 34: consoleServer.py \ please also add it to pep8 whitelist Line 35: debugPluginClient.py \ Line 36: guestIF.py \ Line 37: hooking.py \ Line 38: hooks.py \
.................................................... File vdsm/supervdsmServer.py Line 282: def changePtyOwner(self, ptyNum): Line 283: if type(ptyNum) != int: Line 284: raise TypeError("ptyNum must be integer") Line 285: Line 286: cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)] Use the macro VDSM_USER and VDSM_GROUP instead? Line 287: rc, out, err = storage.misc.execCmd(cmd, sudo=False) Line 288: if rc: Line 289: raise OSError(errno.EINVAL, "can not change own of /dev/pts/%d, " \ Line 290: "out: %s\nerr: %s" %
Line 285: Line 286: cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)] Line 287: rc, out, err = storage.misc.execCmd(cmd, sudo=False) Line 288: if rc: Line 289: raise OSError(errno.EINVAL, "can not change own of /dev/pts/%d, " \ EINVAL could not reflect all cases, how about using rc as error code?
Please change 'own' to 'owner' Line 290: "out: %s\nerr: %s" % Line 291: (ptyNum, out, err)) Line 292: Line 293:
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2: (2 inline comments)
.................................................... File vdsm/consoleServer.py Line 26: import select Line 27: import os Line 28: import threading Line 29: import errno Line 30: I guess He Jie is following PEP8 http://www.python.org/dev/peps/pep-0008/#imports
Imports should be grouped in the following order: standard library imports related third party imports local application/library specific imports You should put a blank line between each group of imports.
Since paramiko is third party lib we add blank lines around this group. Line 31: import paramiko Line 32: Line 33: from vdsm import constants Line 34: from vdsm.betterPopen import BetterPopen
Line 76: sock.close() Line 77: continue Line 78: return p, sock Line 79: Line 80: raise Exception("can not allocated any port on %s", self._addr) Agree Line 81: Line 82: def setTicket(self, passwd, timeout, connAct): Line 83: try: Line 84: return self._crabRPCProxy.callCrabRPCFunction(10, "setTicket",
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Xu He Jie has posted comments on this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2: (9 inline comments)
Thanks for both of your review!
.................................................... File vdsm/consoleServer.py Line 26: import select Line 27: import os Line 28: import threading Line 29: import errno Line 30: zheng sheng, thanks for your explain! Line 31: import paramiko Line 32: Line 33: from vdsm import constants Line 34: from vdsm.betterPopen import BetterPopen
Line 76: sock.close() Line 77: continue Line 78: return p, sock Line 79: Line 80: raise Exception("can not allocated any port on %s", self._addr) Done Line 81: Line 82: def setTicket(self, passwd, timeout, connAct): Line 83: try: Line 84: return self._crabRPCProxy.callCrabRPCFunction(10, "setTicket",
Line 107: str(self._rpcProxyRd), Line 108: str(self._rpcServerWr), Line 109: self._hostKeyFile], Line 110: close_fds=False, env=env) Line 111: finally: just raise the exception to up-level, There is nothing to do for it's failure. Line 112: pty.close() Line 113: Line 114: def isOpen(self): Line 115: return self._prog != None and self._prog.poll() == None
Line 111: finally: Line 112: pty.close() Line 113: Line 114: def isOpen(self): Line 115: return self._prog != None and self._prog.poll() == None poll is used for checking subprocess is alive or not with non-block. if the process is dead, poll will get return code. Line 116: Line 117: def close(self): Line 118: if self.isOpen(): Line 119: self._prog.terminate()
.................................................... File vdsm/libvirtvm.py Line 1283: self.guestAgent = guestIF.GuestAgent(self._guestSocketFile, Line 1284: self.cif.channelListener, self.log, Line 1285: connect=utils.tobool(self.conf.get('vmchannel', 'true'))) Line 1286: Line 1287: if utils.tobool(self.conf.get("enableConsole", False)): I prefer 'consoleEnable' or 'textConsoleEnable' now. When sort the attr in schema, ahead console will be easy to find. Line 1288: self.log.info("vm console enabled") Line 1289: Line 1290: supervdsm.getProxy().changePtyOwner( Line 1291: int(os.path.basename(self.conf["consolePath"])))
Line 1989: hooks.before_vm_set_ticket(self._lastXMLDesc, self.conf, params) Line 1990: Line 1991: if actOn == "graphics": Line 1992: graphics = xml.dom.minidom.parseString(self._dom.XMLDesc(0)) \ Line 1993: .childNodes[0].getElementsByTagName('graphics')[0] I guess rebase error. Line 1994: graphics.setAttribute('passwd', otp) Line 1995: if int(seconds) > 0: Line 1996: validto = time.strftime('%Y-%m-%dT%H:%M:%S', Line 1997: time.gmtime(time.time() + float(seconds)))
Line 2007: return {'status': Line 2008: {'code': errCode['ticketErr']['status']['code'], Line 2009: 'message': 'setTicket failed on console'}} Line 2010: else: Line 2011: self.log.error("console was disable") Done Line 2012: return {'status': {'code': Line 2013: errCode['ticketErr']['status']['code'], Line 2014: 'message': 'console was disable'}} Line 2015: else:
.................................................... File vdsm/supervdsmServer.py Line 282: def changePtyOwner(self, ptyNum): Line 283: if type(ptyNum) != int: Line 284: raise TypeError("ptyNum must be integer") Line 285: Line 286: cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)] Done Line 287: rc, out, err = storage.misc.execCmd(cmd, sudo=False) Line 288: if rc: Line 289: raise OSError(errno.EINVAL, "can not change own of /dev/pts/%d, " \ Line 290: "out: %s\nerr: %s" %
Line 285: Line 286: cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)] Line 287: rc, out, err = storage.misc.execCmd(cmd, sudo=False) Line 288: if rc: Line 289: raise OSError(errno.EINVAL, "can not change own of /dev/pts/%d, " \ Done Line 290: "out: %s\nerr: %s" % Line 291: (ptyNum, out, err)) Line 292: Line 293:
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2: (2 inline comments)
(just a glimpse)
.................................................... File vdsm/supervdsmServer.py Line 278: def removeFs(self, path): Line 279: return mkimage.removeFs(path) Line 280: Line 281: @logDecorator Line 282: def changePtyOwner(self, ptyNum): can we somehow limit this to ptys connected to qemu processes? Line 283: if type(ptyNum) != int: Line 284: raise TypeError("ptyNum must be integer") Line 285: Line 286: cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)]
Line 282: def changePtyOwner(self, ptyNum): Line 283: if type(ptyNum) != int: Line 284: raise TypeError("ptyNum must be integer") Line 285: Line 286: cmd = [EXT_CHOWN, "vdsm:kvm", "/dev/pts/" + str(ptyNum)] os.chown() would be more concise. Line 287: rc, out, err = storage.misc.execCmd(cmd, sudo=False) Line 288: if rc: Line 289: raise OSError(errno.EINVAL, "can not change own of /dev/pts/%d, " \ Line 290: "out: %s\nerr: %s" %
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Saggi Mizrahi has posted comments on this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(19 inline comments)
Also, a thread per console is wasteful. Always use non-blocking pipes. Use poll and have all connections multiplexed in the same thread.
Run pep8 on everything!
.................................................... File vdsm/API.py Line 511: Used only by QA and might be discontinued in next version. Line 512: """ Line 513: return errCode['noimpl'] Line 514: Line 515: def setTicket(self, password, ttl, existingConnAction, params, actOn): update the docs
Also actOn should be something more descriptive. 'interface' maybe? Line 516: """ Line 517: Set the ticket (password) to be used to connect to a VM display Line 518: Line 519: :param vmId: specify the VM whos ticket is to be changed.
.................................................... File vdsm/consoleServer.py Line 125: os.close(self._rpcServerWr) Line 126: self._sock.close() Line 127: Line 128: Line 129: class _ParamikoServerIf(paramiko.ServerInterface): Please choose a more descriptive name Line 130: Line 131: def __init__(self): Line 132: self._passwd = None Line 133:
Line 130: Line 131: def __init__(self): Line 132: self._passwd = None Line 133: Line 134: def setPasswd(self, passwd): Saving two chars isn't worth obfuscating the interface. Line 135: self._passwd = passwd Line 136: Line 137: def clearPasswd(self): Line 138: self._passwd = None
Line 134: def setPasswd(self, passwd): Line 135: self._passwd = passwd Line 136: Line 137: def clearPasswd(self): Line 138: self._passwd = None 'clear' Seems redundant. Line 139: Line 140: def check_channel_request(self, kind, chanid): Line 141: if kind == 'session': Line 142: return paramiko.OPEN_SUCCEEDED
Line 165: def fileno(self): Line 166: return self._fd Line 167: Line 168: Line 169: class _ClientThread(threading.Thread): Never inherit from thread. It makes no sense.
You are not making a specific type of thread. Line 170: Line 171: def __init__(self, logger, pty, clientSock, hostKey, serverIf, addr): Line 172: threading.Thread.__init__(self) Line 173: self._logger = logger
Line 169: class _ClientThread(threading.Thread): Line 170: Line 171: def __init__(self, logger, pty, clientSock, hostKey, serverIf, addr): Line 172: threading.Thread.__init__(self) Line 173: self._logger = logger We generally use self._log not self._logger Line 174: self._pty = pty Line 175: self._clientSock = clientSock Line 176: self._hostKey = hostKey Line 177: self._serverIf = serverIf
Line 176: self._hostKey = hostKey Line 177: self._serverIf = serverIf Line 178: self._addr = addr Line 179: self._isTerminated = False Line 180: self._pipeRd, self._pipeWr = os.pipe() Don't forget to add a desctructor to clear these. Line 181: misc.setNonBlocking(self._pipeRd) Line 182: misc.setNonBlocking(self._pipeWr) Line 183: Line 184: def fileno(self):
Line 188: def send(self, data): Line 189: try: Line 190: os.write(self._pipeWr, data) Line 191: except OSError as e: Line 192: if e.errno not in (errno.EAGAIN, errno.EINTR): retry? The use will assume the method succeeded even though it failed Line 193: raise Line 194: Line 195: def terminate(self): Line 196: self._logger.info("terminate client thread: %s", self._addr)
Line 209: if chan == None: Line 210: self._logger.error("get SSH channel failed") Line 211: return Line 212: Line 213: pipeRdFdw = _FdWraper(self._pipeRd) Seems redundant. Everything in python accepts a raw fd Line 214: Line 215: while True: Line 216: rd, wr, er = select.select([chan, pipeRdFdw], [], []) Line 217:
Line 212: Line 213: pipeRdFdw = _FdWraper(self._pipeRd) Line 214: Line 215: while True: Line 216: rd, wr, er = select.select([chan, pipeRdFdw], [], []) never use select. It's broken (doesn't support FDs above 1024) and slow Use poll or epoll Line 217: Line 218: if self._isTerminated: Line 219: break Line 220:
Line 218: if self._isTerminated: Line 219: break Line 220: Line 221: if chan in rd: Line 222: buf = chan.recv(4096) self._bufferSize
Also, change the FD to non-blocking if you are using a select loop. Line 223: Line 224: if len(buf) == 0: Line 225: self._logger.info("ssh channel was closed %s", Line 226: self._addr)
Line 225: self._logger.info("ssh channel was closed %s", Line 226: self._addr) Line 227: break Line 228: Line 229: os.write(self._pty.fileno(), buf) When you change to non-blocking make sure everything gets written Line 230: elif pipeRdFdw in rd: Line 231: try: Line 232: buf = os.read(self._pipeRd, 4096) Line 233: if len(buf) == 0:
Line 228: Line 229: os.write(self._pty.fileno(), buf) Line 230: elif pipeRdFdw in rd: Line 231: try: Line 232: buf = os.read(self._pipeRd, 4096) again Line 233: if len(buf) == 0: Line 234: raise IOError(errno.EIO, "pipe closed") Line 235: except OSError as e: Line 236: if e.errno not in (errno.EAGAIN, errno.EINTR):
Line 229: os.write(self._pty.fileno(), buf) Line 230: elif pipeRdFdw in rd: Line 231: try: Line 232: buf = os.read(self._pipeRd, 4096) Line 233: if len(buf) == 0: Doesn't belong inside the try block Line 234: raise IOError(errno.EIO, "pipe closed") Line 235: except OSError as e: Line 236: if e.errno not in (errno.EAGAIN, errno.EINTR): Line 237: self._logger.error("PIPE read error", exc_info=True)
Line 232: buf = os.read(self._pipeRd, 4096) Line 233: if len(buf) == 0: Line 234: raise IOError(errno.EIO, "pipe closed") Line 235: except OSError as e: Line 236: if e.errno not in (errno.EAGAIN, errno.EINTR): These (espscially EAGAIN) pop up when the system is stressed. No need to freak out and break. retry Line 237: self._logger.error("PIPE read error", exc_info=True) Line 238: break Line 239: Line 240: if chan.send(buf) == 0:
Line 251: Line 252: class SSHConsoleServer(object): Line 253: Line 254: def __init__(self, logger, pty, sock, rpcRd, rpcWr, hostKeyFile): Line 255: self._logger = logger _log Line 256: self._pty = _FdWraper(pty) Line 257: self._sock = sock Line 258: self._rpcRd = rpcRd Line 259: self._rpcWr = rpcWr
Line 270: # if 0 returned means input was closed Line 271: buf = os.read(self._pty.fileno(), 4096) Line 272: Line 273: if len(buf) == 0: Line 274: raise IOError(errno.EIO, "console was closed") When a file is closed the errno is EBADF Line 275: Line 276: for client in self._clients: Line 277: client.send(buf) Line 278:
Line 284: self._serverIf, Line 285: addr) Line 286: Line 287: self._clients.append(newClient) Line 288: newClient.start() setDaemon(True) Line 289: Line 290: def _closeClient(self, client): Line 291: client.terminate() Line 292: client.join()
Line 327: rpcRd = _FdWraper(self._rpcRd) Line 328: Line 329: try: Line 330: while True: Line 331: rd, wr, er = select.select([self._sock, Too much indentation select is evil Line 332: self._pty, Line 333: rpcRd] + self._clients, Line 334: [], [], self._timeout) Line 335:
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Xu He Jie has abandoned this change.
Change subject: [WIP] Add text-based console support ......................................................................
Patch Set 2: Abandoned
-- To view, visit http://gerrit.ovirt.org/7165 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon Gerrit-Change-Id: I69904bf7aafd4f764a256d4075c9bf71d988e7c5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org