I'm sorry, the MessageDispatcher class is not used in the refactorized
version, but I forgot to remove it from repo and from commit message. :-/
The next patch should be final.
Sorry again,
Jiri
On 07/27/2015 08:43 AM, Ondrej Lichtner wrote:
On Fri, Jul 24, 2015 at 01:05:14PM +0200, Jan Tluka wrote:
> See comments inline. The patch looks good however there are still some minor
> issues around.
>
> Tue, Jul 21, 2015 at 11:13:11AM CEST, jprochaz(a)redhat.com wrote:
>> MessageDispatcherLite.py
>> + Remove unused import
>> + Make pep8 compliant
>> + Rewrite one error message
> Why we need it? Since Wizard class now communicates with lnst-slave
> directly we don't need it. Nack, unless there's valid reason to keep the
> DispatcherLite.
I agree... MessageDispatcherLite should be removed.
>> lnst-pool-wizard
>> + Make pep8 compliant
>> + Print error messages on stderr
>> + Changes correspoding with Wizard.py refactorization
>>
>> Wizard.py
>> + Make pep8 compliant
>> + Convert public methods to private
>> + Most of the methods reworked to be more clean and readable
>> + Setup for implementation of support for virtual machines
>>
>> Closes #129
>>
>> Signed-off-by: Jiri Prochazka <jprochaz(a)redhat.com>
>> ---
>> lnst-pool-wizard | 55 +++--
>> lnst/Controller/MessageDispatcherLite.py | 9 +-
>> lnst/Controller/Wizard.py | 349 ++++++++++++++++++++-----------
>> 3 files changed, 264 insertions(+), 149 deletions(-)
>>
>> diff --git a/lnst-pool-wizard b/lnst-pool-wizard
>> index c788688..d17d23a 100755
>> --- a/lnst-pool-wizard
>> +++ b/lnst-pool-wizard
>> @@ -17,48 +17,67 @@ import getopt
>> RETVAL_PASS = 0
>> RETVAL_ERR = 1
>>
>> -def help(retval = 0):
>> +
>> +def help(retval=0):
>> print "Usage:\n"\
>> " lnst-pool-wizard [mode] [hostname[:port]]\n"\
>> "\n"\
>> "Modes:\n"\
>> - " -h, --help display this help text and
exit\n"\
>> - " -i, --interactive start wizard in interactive mode
(this "\
>> - "is default mode)\n"\
>> - " -n, --noninteractive start wizard in noninteractive
mode\n"\
>> + " -h, --help display this help text and
exit\n"\
>> + " -p, --pool_dir <directory> set the pool dir (works both
in) "\
>> + "interactive and noninteractive mode\n"\
>> + " -i, --interactive start wizard in interactive mode
(this "\
>> + "is default mode)\n"\
>> + " -n, --noninteractive start wizard in noninteractive
mode\n"\
>> "Examples:\n"\
>> " lnst-pool-wizard --interactive\n"\
>> " lnst-pool-wizard --noninteractive 192.168.122.2\n"\
>> - " lnst-pool-wizard -n 192.168.122.2:8888 192.168.122.3:9999
192.168.122.4\n"
>> + " lnst-pool-wizard -n 192.168.122.2:8888 192.168.122.4\n"\
>> + " lnst-pool-wizard -p \".pool/\" -n 192.168.1.1:8877
192.168.122.4\n"
>> sys.exit(retval)
>>
>>
>> def main():
>> try:
>> opts, args = getopt.getopt(
>> - sys.argv[1:],
>> - "hin",
>> - ["help", "interactive", "noninteractive"]
>> + sys.argv[1:],
>> + "hinp:",
>> + ["help", "interactive",
"noninteractive", "pool_dir="]
>> )
>> except getopt.GetoptError as err:
>> - print str(err)
>> + sys.stderr.write(str(err))
>> help(RETVAL_ERR)
>> - wizard = Wizard()
>> - for opt, arg in opts:
>> +
>> + pool_dir= None
>> + mode = "interactive"
>> +
>> + for opt, arg in opts:
>> if opt in ("-h", "--help"):
>> help()
>> elif opt in ("-i", "--interactive"):
>> - wizard.interactive()
>> - sys.exit(RETVAL_PASS)
>> + mode = "interactive"
>> elif opt in ("-n", "--noninteractive"):
>> if not args:
>> - print "No hostname entered!"
>> + sys.stderr.write("No hostnames entered\n")
>> return RETVAL_ERR
>> - wizard.noninteractive(args)
>> - sys.exit(RETVAL_PASS)
>> + else:
>> + hostlist = args
>> + mode = "noninteractive"
>> + elif opt in ("-p", "--pool_dir"):
>> + if not arg:
>> + sys.stderr.write("No pool directory specified\n")
>> + else:
>> + pool_dir = arg
>> else:
>> help(RETVAL_ERR)
>> - wizard.interactive()
>> +
>> + w = Wizard()
>> +
>> + if mode == "noninteractive":
>> + w.noninteractive(hostlist, pool_dir)
>> + else:
>> + w.interactive(pool_dir)
>> +
>> sys.exit(RETVAL_PASS)
>>
>>
>> diff --git a/lnst/Controller/MessageDispatcherLite.py
b/lnst/Controller/MessageDispatcherLite.py
>> index 2973763..623c232 100644
>> --- a/lnst/Controller/MessageDispatcherLite.py
>> +++ b/lnst/Controller/MessageDispatcherLite.py
>> @@ -15,7 +15,9 @@ from lnst.Common.ConnectionHandler import send_data
>> from lnst.Common.ConnectionHandler import ConnectionHandler
>> from lnst.Controller.NetTestController import NetTestError
>>
>> +
>> class MessageDispatcherLite(ConnectionHandler):
>> +
>> def __init__(self):
>> super(MessageDispatcherLite, self).__init__()
>>
>> @@ -25,9 +27,8 @@ class MessageDispatcherLite(ConnectionHandler):
>> def send_message(self, machine_id, data):
>> soc = self.get_connection(1)
>>
>> - if send_data(soc, data) == False:
>> - msg = "Connection error from slave %s" % str(1)
>> - raise NetTestError(msg)
>> + if send_data(soc, data) is False:
>> + raise NetTestError("Connection error from slave")
>>
>> def wait_for_result(self, machine_id):
>> wait = True
>> @@ -47,7 +48,7 @@ class MessageDispatcherLite(ConnectionHandler):
>>
>> if connected_slaves != remaining_slaves:
>> disconnected_slaves = set(connected_slaves) -\
>> - set(remaining_slaves)
>> + set(remaining_slaves)
>> msg = "Slaves " + str(list(disconnected_slaves)) + \
>> " disconnected from the controller."
>> raise NetTestError(msg)
>> diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py
>> index c38aae5..7db8459 100644
>> --- a/lnst/Controller/Wizard.py
>> +++ b/lnst/Controller/Wizard.py
>> @@ -1,7 +1,7 @@
>> """
>> -Wizard class for creating machine pool .xml files
>> +Wizard class for creating slave machine config files
>>
>> -Copyright 2014 Red Hat Inc.
>> +Copyright 2015 Red Hat Inc.
>> Licensed under the GNU General Public Licence, version 2 as
>> published by the Free Software Foundation; see COPYING for details.
>> """
>> @@ -9,150 +9,149 @@ published by the Free Software Foundation; see COPYING for
details.
>> __author__ = """
>> jprochaz(a)redhat.com (Jiri Prochazka)
>> """
>> +
>> import socket
>> import sys
>> +import time
>> import os
>> -from lnst.Controller.Machine import Machine
>> -from lnst.Controller.MessageDispatcherLite import MessageDispatcherLite
>> -from lnst.Common.NetUtils import test_tcp_connection
>> from lnst.Common.Utils import mkdir_p
>> from lnst.Common.Config import DefaultRPCPort
>> +from lnst.Common.ConnectionHandler import send_data, recv_data
>> from xml.dom.minidom import getDOMImplementation
>>
>> +DefaultPoolDir = os.path.expanduser("~/.lnst/pool/")
>> +
>> +
>> class Wizard:
>> - def __init__(self):
>> - self._msg_dispatcher = MessageDispatcherLite()
>> - self._pool_dir = os.path.expanduser("~/.lnst/pool")
>>
>> - def interactive(self):
>> + def interactive(self, pool_dir=None):
>> + """ Starts Wizard in an interactive mode
>> + @param pool_dir Path to pool directory (optional)
>> """
>> - Starts Wizard in interactive mode. Wizard requests hostname and port
>> - from user, tests connectivity to entered host, then he tries to connect
>> - and configure slave on host machine, and finally he requests list of
>> - ethernet interfaces in state DOWN. Then he writes them into .xml file
>> - named by user. User can choose which interfaces should be added to the
>> - .xml file.
>> - """
>> -
>> - pool_dir = raw_input("Enter path to pool directory "\
>> - "(default: ~/.lnst/pool): ")
>> - if pool_dir != "":
>> - self._pool_dir = os.path.expanduser(pool_dir)
>> - print "Pool directory set to %s" % self._pool_dir
>> + if pool_dir is None:
>> + pool_dir = self._query_pool_dir()
>> + # Invalid pool dir entered, query for correct one
>> + elif not self._check_pool_dir(pool_dir):
>> + pool_dir = self._query_pool_dir()
> IMO, this is not the best approach. E.g. user won't specify a pool dir,
> so the user is prompted to enter directory. But there's no check of it.
>
> Also if user specifies the pool dir, it's checked and if it fails then
> the user is prompted back but there's no check after that.
>
> Please fix this.
>
>> while True:
>> - while True:
>> - hostname = raw_input("Enter hostname: ")
>> - try:
>> - # Tests if hostname is translatable into IP address
>> - socket.gethostbyname(hostname)
>> - break
>> - except:
>> - sys.stderr.write("Hostname is not translatable into
valid"\
>> - " IP address.\n")
>> + hostname = self._query_hostname()
>> + port = self._query_port()
>> +
>> + sock = self._get_connection(hostname, port)
>> + if sock is None:
>> + if self._query_continuation():
>> continue
>> - while True:
>> - port = raw_input("Enter port(default: 9999): ")
>> - if port == "":
>> - port = DefaultRPCPort
>> - try:
>> - port = int(port)
>> + else:
>> break
>> - except:
>> - sys.stderr.write("Invalid port.")
>> - continue
>> - msg =
self._get_suitable_interfaces(socket.gethostbyname(hostname),\
>> - port)
>> - if msg:
>> - self._write_to_xml(msg, hostname, port,
"interactive")
>> - next_machine = raw_input("Do you want to add another machine?
"\
>> - "[Y/n]: ")
>> - if next_machine.lower() == 'y' or next_machine ==
"":
>> +
>> + machine_interfaces = self._get_machine_interfaces(sock)
>> + sock.close()
>> +
>> + if machine_interfaces == {}:
>> + sys.stderr.write("No suitable interfaces found on the host
"
>> + "'%s:%s'\n" % (hostname,
port))
>> + elif machine_interfaces is not None:
>> + filename = self._query_filename(hostname)
>> + self._create_xml(machine_interfaces, hostname,
>> + port, pool_dir, filename,
"interactive")
>> + if self._query_continuation():
>> continue
>> else:
>> break
>> - return
>>
>> - def noninteractive(self, hostlist):
>> - """
>> - Starts Wizard in noninteractive mode. Wizard gets list of hosts and
>> - ports as arguments. He tries to connect and get info about suitable
>> - interfaces for each of the hosts. Noninteractive mode does not prompt
>> - user about anything, it automatically adds all suitable interfaces into
>> - .xml file named the same as the hostname of the selected machine.
>> + def noninteractive(self, hostlist, pool_dir):
>> + """ Starts Wizard in noninteractive mode
>> + @param hostlist List of hosts (mandatory)
>> + @param pool_dir Path to pool_directory (optional)
>> """
>> - self._mode = "noninteractive"
>> + if pool_dir is None:
>> + pool_dir = DefaultPoolDir
>> + # Invalid pool_dir entered, abort wizard
>> + if not self._check_pool_dir(pool_dir):
>> + sys.stderr.write("Pool wizard aborted\n")
>> + return
>>
>> for host in hostlist:
>> - # Checks if port was entered along with hostname
>> + print("Processing host '%s'" % host)
>> + # Check if port was entered along with hostname
>> if host.find(":") != -1:
>> - hostname = host.split(':')[0]
>> - try:
>> - port = int(host.split(':')[1])
>> - except:
>> - port = DefaultRPCPort
>> + hostname = host.split(":")[0]
>> + if hostname == "":
>> + msg = "'%s' does not contain valid
hostname\n" % host
>> + sys.stderr.write(msg)
>> + sys.stderr.write("Skipping host '%s'\n" %
host)
>> + continue
>> + try:
>> + port = int(host.split(":")[1])
>> + except:
>> + port = DefaultRPCPort
>> + msg = "Invalid port entered, "\
>> + "using '%s' instead\n" % port
>> + sys.stderr.write(msg)
>> else:
>> hostname = host
>> port = DefaultRPCPort
>> - msg = self._get_suitable_interfaces(hostname, port)
>> - if not msg:
>> +
>> + if not self._check_hostname(hostname):
>> + sys.stderr.write("Skipping host '%s'\n" %
host)
>> continue
>> - self._write_to_xml(msg, hostname, port, "noninteractive")
>>
>> - def _get_suitable_interfaces(self, hostname, port):
>> - """
>> - Calls all functions, which are used by both interactive and
>> - noninteractive mode to get list of suitable interfaces. The list is
>> - saved to variable msg.
>> - """
>> - if not test_tcp_connection(hostname, port):
>> - sys.stderr.write("Host destination '%s:%s'
unreachable.\n"
>> - % (hostname, port))
>> - return False
>> - if not self._connect_and_configure_machine(hostname, port):
>> - return False
>> - msg = self._get_device_ifcs(hostname, port)
>> - self._msg_dispatcher.disconnect_slave(1)
>> - return msg
>> + sock = self._get_connection(hostname, port)
>> + if sock is None:
>> + sys.stderr.write("Skipping host '%s'\n" %
host)
>> + continue
>>
>> - def _get_device_ifcs(self, hostname, port):
>> - """
>> - Sends RPC call request to Slave to call function get_devices, which
>> - returns list of ethernet devices which are in state DOWN.
>> - """
>> - msg = self._machine._rpc_call("get_devices")
>> - if msg == {}:
>> - sys.stderr.write("No suitable interfaces found on the slave
"\
>> - "'%s:%s'.\n" % (hostname, port))
>> - return False
>> - return msg
>> + machine_interfaces = self._get_machine_interfaces(sock)
>> + sock.close()
>>
>> - def _connect_and_configure_machine(self, hostname, port):
>> - """
>> - Connects to Slave and configures it
>> + if machine_interfaces is {}:
>> + sys.stderr.write("No suitable interfaces found on the host
"
>> + "'%s:%s'\n" % (hostname,
port))
>> + continue
>> + elif machine_interfaces is None:
>> + continue
>> + else:
>> + filename = hostname + ".xml"
>> + self._create_xml(machine_interfaces, hostname,
>> + port, pool_dir, filename,
"noninteractive")
>> +
>> + def _check_hostname(self, hostname):
>> + """ Checks hostnames translatibility
>> + @param hostname Hostname which is checked whether it's valid
>> + @return True if valid hostname was entered, False otherwise
>> """
>> try:
>> - self._machine = Machine(1, hostname, None, port)
>> - self._machine.set_rpc(self._msg_dispatcher)
>> - self._machine.configure("MachinePoolWizard")
>> + # Test if hostname is translatable into IP address
>> + socket.gethostbyname(hostname)
>> return True
>> except:
>> - sys.stderr.write("Remote machine '%s:%s' configuration
failed!\n"
>> - % (hostname, port))
>> - self._msg_dispatcher.disconnect_slave(1)
>> + sys.stderr.write("Hostname '%s' is not translatable
into a valid "
>> + "IP address\n" % hostname)
>> return False
>>
>> - def _write_to_xml(self, msg, hostname, port, mode):
>> + def _check_pool_dir(self, pool_dir):
>> + """ Checks users access to selected directory
>> + @param pool_dir Path to checked directory
>> + @return True if user can write in entered directory, False otherwise
>> """
>> - Used for writing desired output into .xml file. In interactive mode,
>> - user is prompted for every interface, in noninteractive mode all
>> - interfaces are automatically added to the .xml file.
>> + if os.access(pool_dir, os.W_OK):
>> + return True
>> + else:
>> + sys.stderr.write("Directory '%s' is not
writable\n"
>> + % pool_dir)
>> + return False
>> +
>> + def _create_xml(self, machine_interfaces, hostname,
>> + port, pool_dir, filename, mode):
>> + """ Creates slave machine XML file
>> + @param machine_interfaces Dictionary with machine's interfaces
>> + @param hostname Hostname of the machine
>> + @param port Port on which LNST listens on the machine
>> + @param pool_dir Path to directory where XML file will be created
>> + @param filename Name of the XML file
>> + @param mode Mode in which wizard was started
>> """
>> - if mode == "interactive":
>> - output_file = raw_input("Enter the name of the output .xml file
"\
>> - "(without .xml, default is
hostname.xml): ")
>> - if mode == "noninteractive" or output_file == "":
>> - output_file = hostname
>>
>> impl = getDOMImplementation()
>> doc = impl.createDocument(None, "slavemachine", None)
>> @@ -166,42 +165,138 @@ class Wizard:
>> interfaces_el = doc.createElement("interfaces")
>> top_el.appendChild(interfaces_el)
>>
>> - devices_added = 0
>> - for interface in msg.itervalues():
>> - if mode == 'interactive':
>> + interfaces_added = 0
>> + for iface in machine_interfaces.itervalues():
>> + if mode == "interactive":
>> answer = raw_input("Do you want to add interface
'%s' (%s) "
>> - "to the recipe? [Y/n]" %
(interface['name'],
>> -
interface['hwaddr']))
>> - if mode == "noninteractive" or answer.lower() ==
'y'\
>> + "to the recipe? [Y/n]: " %
(iface["name"],
>> +
iface["hwaddr"]))
>> + if mode == "noninteractive" or answer.lower() ==
"y"\
>> or answer == "":
>> - devices_added += 1
>> + interfaces_added += 1
>> eth_el = doc.createElement("eth")
>> - eth_el.setAttribute("id", interface['name'])
>> + eth_el.setAttribute("id", iface["name"])
>> eth_el.setAttribute("label",
"default_network")
>> interfaces_el.appendChild(eth_el)
>> params_el = doc.createElement("params")
>> eth_el.appendChild(params_el)
>> param_el = doc.createElement("param")
>> param_el.setAttribute("name", "hwaddr")
>> - param_el.setAttribute("value",
interface['hwaddr'])
>> + param_el.setAttribute("value",
iface["hwaddr"])
>> params_el.appendChild(param_el)
> How about adding driver information as well?
>
>> - if devices_added == 0:
>> - sys.stderr.write("You didn't add any interface, no file
'%s.xml' "\
>> - "will be created!\n" % output_file)
>> + if interfaces_added == 0:
>> + sys.stderr.write("You didn't add any interface, no file
"
>> + "'%s' will be created\n" %
filename)
>> return
>>
>> - mkdir_p(self._pool_dir)
>> + mkdir_p(pool_dir)
>>
>> try:
>> - f = open(self._pool_dir + "/" + output_file +
".xml", 'w')
>> + f = open(pool_dir + "/" + filename, "w")
>> f.write(doc.toprettyxml())
>> f.close()
>> except:
>> - sys.stderr.write("File '%s.xml' could not be opened
"\
>> - "or data written." %
output_file+"\n")
>> - raise WizardException()
>> + msg = "File '%s/%s' could not be opened or data
written\n"\
>> + % (pool_dir, filename)
>> + raise WizardException(msg)
>> +
>> + print("File '%s' successfuly created" % filename)
>> +
>> + def _get_connection(self, hostname, port):
>> + """ Connects to machine
>> + @param hostname Hostname of the machine
>> + @param port Port of the machine
>> + @return Connected socket if connection was successful, None otherwise
>> + """
>> + try:
>> + sock = socket.create_connection((hostname, port))
>> + return sock
>> + except socket.error:
>> + sys.stderr.write("Connection to remote host '%s:%s'
failed\n"
>> + % (hostname, port))
>> + return None
>> +
>> + def _get_machine_interfaces(self, sock):
>> + """ Gets machine interfaces via RPC call
>> + @param sock Socket used for connecting to machine
>> + @return Dictionary with machine interfaces or None if RPC call fails
>> + """
>> + msg = {"type": "command", "method_name":
"get_devices", "args": {}}
>> + if not send_data(sock, msg):
>> + sys.stderr.write("Could not send request to slave
machine\n")
>> + return None
>> +
>> + while True:
>> + data = recv_data(sock)
>> + if data["type"] == "result":
>> + return data["result"]
>> +
>> + def _query_continuation(self):
>> + """ Queries user for adding next machine
>> + @return True if user wants to add another machine, False otherwise
>> + """
>> + answer = raw_input("Do you want to add another machine? [Y/n]:
")
>> + if answer.lower() == "y" or answer == "":
>> + return True
>> + else:
>> + return False
>> +
>> + def _query_filename(self, hostname):
>> + """ Queries user for name of the file
>> + @hostname Hostname of the machine which is used as default filename
>> + @return Name of the file with .xml extension
>> + """
>> + output_file = raw_input("Enter the name of the output .xml file
"
>> + "(without .xml, default is
'%s.xml'): "
>> + % hostname)
>> + if output_file == "":
>> + return hostname + ".xml"
>> + else:
>> + return output_file + ".xml"
>> +
>> + def _query_hostname(self):
>> + """ Queries user for hostname
>> + @return Valid (is translatable to an IP address) hostname
>> + """
>> + while True:
>> + hostname = raw_input("Enter hostname: ")
>> + if hostname == "":
>> + sys.stderr.write("No hostname entered\n")
>> + continue
>> + if self._check_hostname(hostname):
>> + return hostname
>> +
>> + def _query_pool_dir(self):
>> + """ Queries user for pool directory
>> + @return Valid (is writable by user) path to directory
>> + """
>> + while True:
>> + pool_dir = raw_input("Enter path to a pool directory "
>> + "(default: '%s'): " %
DefaultPoolDir)
>> + if pool_dir == "":
>> + return DefaultPoolDir
>> +
>> + pool_dir = os.path.expanduser(pool_dir)
>> + # check if dir is writable
>> + if self._check_pool_dir(pool_dir):
>> + print("Pool directory set to '%s'" %
pool_dir)
>> + return pool_dir
>> +
>> + def _query_port(self):
>> + """ Queries user for port
>> + @return Integer representing port
>> + """
>> + while True:
>> + port = raw_input("Enter port (default: %d): " %
DefaultRPCPort)
>> + if port == "":
>> + return DefaultRPCPort
>> + else:
>> + try:
>> + port = int(port)
>> + return port
>> + except:
>> + sys.stderr.write("Invalid port entered\n")
>>
>> - print "File '%s.xml' successfuly created." %
output_file
>>
>> class WizardException(Exception):
>> pass
>> --
>> 2.4.3
>>
>> _______________________________________________
>> LNST-developers mailing list
>> LNST-developers(a)lists.fedorahosted.org
>>
https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
> _______________________________________________
> LNST-developers mailing list
> LNST-developers(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/lnst-developers