Change in vdsm[master]: [WIP]Add text-based console support

agl at us.ibm.com agl at us.ibm.com
Tue Aug 14 13:42:32 UTC 2012


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 at linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Ryan Harper <ryanh at us.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list