New line was missing in print of an error message of _create_dir method
Signed-off-by: Jiri Prochazka jprochaz@redhat.com --- lnst/Controller/Wizard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index f2c645c..51856e8 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -186,7 +186,7 @@ class Wizard: print("Dir '%s' has been created" % pool_dir) return pool_dir except: - sys.stderr.write("Failed creating dir") + sys.stderr.write("Failed creating dir\n") return None
def _create_xml(self, machine_interfaces, hostname,
This patch introduces several changes to the _query_pool_dir method in lnst.Controller.Wizard: - Method was renamed to _check_and_query_pool_dir to better express nature of the method - Method now uses 1 optional argument - pool_dir - if set to something else than None, it checks if it's valid path to existing or creatable dir, if it is, uses it, else queries user for a new path (which is also checked) - Method noninteractive has added default value (None) for argument pool_dir
This method was reworked to prevent code duplication which would occur when mode for virtual machines would be introduced
Signed-off-by: Jiri Prochazka jprochaz@redhat.com --- lnst/Controller/Wizard.py | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 51856e8..b35b216 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -32,22 +32,7 @@ class Wizard: """ Starts Wizard in an interactive mode @param pool_dir Path to pool directory (optional) """ - if pool_dir is None: - pool_dir = self._query_pool_dir() - else: - rv = self._check_path(pool_dir) - if rv == PATH_IS_DIR_ACCESSIBLE: - print("Pool directory set to '%s'" % pool_dir) - elif rv == PATH_DOES_NOT_EXIST: - sys.stderr.write("Path '%s' does not exist\n" % pool_dir) - pool_dir = self._query_pool_dir() - elif rv == PATH_NOT_DIR: - sys.stderr.write("Path '%s' exists but is not a directory\n" - % pool_dir) - pool_dir = self._query_pool_dir() - elif rv == PATH_IS_DIR_NOT_ACCESSIBLE: - sys.stderr.write("Directory '%s' is not writable\n" % pool_dir) - pool_dir = self._query_pool_dir() + pool_dir = self._check_and_query_pool_dir(pool_dir)
while True: hostname = self._query_hostname() @@ -75,7 +60,7 @@ class Wizard: else: break
- def noninteractive(self, hostlist, pool_dir): + def noninteractive(self, hostlist, pool_dir=None): """ Starts Wizard in noninteractive mode @param hostlist List of hosts (mandatory) @param pool_dir Path to pool_directory (optional) @@ -330,13 +315,15 @@ class Wizard: sys.stderr.write("Hostname '%s' is not translatable into a " "valid IP address\n" % hostname)
- def _query_pool_dir(self): + def _check_and_query_pool_dir(self, pool_dir): """ Queries user for pool directory + @param pool_dir Optional pool_dir which will be checked and used if OK @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 is None: + pool_dir = raw_input("Enter path to a pool directory " + "(default: '%s'): " % DefaultPoolDir) if pool_dir == "": pool_dir = DefaultPoolDir
@@ -359,6 +346,7 @@ class Wizard: elif rv == PATH_IS_DIR_NOT_ACCESSIBLE: sys.stderr.write("Directory '%s' is not writable\n" % pool_dir) + pool_dir = None
def _query_port(self): """ Queries user for port
The method tries to create a file and write in it contents of result XML, if it success, returns True, else it returns False
This method was created to improve readability and modularity of the pool wizard code.
Signed-off-by: Jiri Prochazka jprochaz@redhat.com --- lnst/Controller/Wizard.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index b35b216..e91413d 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -226,17 +226,20 @@ class Wizard: return
+ def _write_to_file(self, pool_dir, filename, doc): + """ Writes contents of XML to a file + @param pool_dir Path to directory where the file will be created + @param filename Name of the created file + @param doc Contents of XML file + @return True if file was successfuly written, False otherwise + """ try: f = open(pool_dir + "/" + filename, "w") f.write(doc.toprettyxml()) f.close() + return True except: - msg = "File '%s/%s' could not be opened or data written\n"\ - % (pool_dir, filename) - sys.stderr.write(msg) - return - - print("File '%s' successfuly created" % filename) + return False
def _get_connection(self, hostname, port): """ Connects to machine
This patch introduces several minor changes to _create_xml method: - Removes redundant argument "port" - Adds new argument "libvirt_domain" which will be used in virtual mode - Updates documentation for the method - Adds default values (None) to all arguments - Calls of the functions in interactive and noninteractive methods were adjusted to be up-to-date with the changes
Changes were made as preparation for implementing virtual mode, because new virtual method will use _create_xml method differently than existing ones
Signed-off-by: Jiri Prochazka jprochaz@redhat.com --- lnst/Controller/Wizard.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index e91413d..1a3ecc0 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -53,8 +53,9 @@ class Wizard: "'%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") + self._create_xml(machine_interfaces=machine_interfaces, + hostname=hostname, pool_dir=pool_dir, + filename=filename, mode="interactive") if self._query_continuation(): continue else: @@ -131,8 +132,9 @@ class Wizard: continue else: filename = hostname + ".xml" - self._create_xml(machine_interfaces, hostname, - port, pool_dir, filename, "noninteractive") + self._create_xml(machine_interfaces=machine_interfaces, + hostname=hostname, pool_dir=pool_dir, + filename=filename, mode="noninteractive")
def _check_hostname(self, hostname): """ Checks hostnames translatibility @@ -174,15 +176,16 @@ class Wizard: sys.stderr.write("Failed creating dir\n") return None
- def _create_xml(self, machine_interfaces, hostname, - port, pool_dir, filename, mode): + def _create_xml(self, machine_interfaces=None, hostname=None, + pool_dir=None, filename=None, mode=None, + libvirt_domain=None): """ 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 + @param libvirt_domain Libvirt domain of virtual host """
impl = getDOMImplementation()
This patch prepares _create_xml method for support of virtual machines: - Adds support for virtual mode, which will write only hostname and libvirt domain to the output XML file - Changes the method so it uses newly created _write_to_xml method
Signed-off-by: Jiri Prochazka jprochaz@redhat.com --- lnst/Controller/Wizard.py | 67 ++++++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 30 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 1a3ecc0..570f0d1 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -197,37 +197,44 @@ class Wizard: param_el.setAttribute("name", "hostname") param_el.setAttribute("value", hostname) params_el.appendChild(param_el) - interfaces_el = doc.createElement("interfaces") - top_el.appendChild(interfaces_el) - - 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]: " % (iface["name"], - iface["hwaddr"])) - if mode == "noninteractive" or answer.lower() == "y"\ - or answer == "": - interfaces_added += 1 - eth_el = doc.createElement("eth") - 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", iface["hwaddr"]) - params_el.appendChild(param_el) - param_el = doc.createElement("param") - param_el.setAttribute("name", "driver") - param_el.setAttribute("value", iface["driver"]) - params_el.appendChild(param_el) - if interfaces_added == 0: - sys.stderr.write("You didn't add any interface, no file " - "'%s' will be created\n" % filename) - return + if mode == "virtual": + param_el = doc.createElement("param") + param_el.setAttribute("name", "libvirt_domain") + param_el.setAttribute("value", libvirt_domain) + params_el.appendChild(param_el) + else: + interfaces_el = doc.createElement("interfaces") + top_el.appendChild(interfaces_el) + + interfaces_added = 0 + for iface in machine_interfaces.itervalues(): + if mode == "interactive": + msg = "Do you want to add interface '%s' (%s) to the "\ + "recipe? [Y/n]: " % (iface["name"], iface["hwaddr"]) + answer = raw_input(msg) + if mode == "noninteractive" or answer.lower() == "y"\ + or answer == "": + interfaces_added += 1 + eth_el = doc.createElement("eth") + 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", iface["hwaddr"]) + params_el.appendChild(param_el) + if interfaces_added == 0: + sys.stderr.write("You didn't add any interface, no file " + "'%s' will be created\n" % filename) + return
+ if self._write_to_file(pool_dir, filename, doc): + print("File '%s/%s' successfuly created." % (pool_dir, filename)) + else: + sys.stderr.write("File '%s/%s' could not be opened " + "or data written.\n" % (pool_dir, filename))
def _write_to_file(self, pool_dir, filename, doc): """ Writes contents of XML to a file
This patch adds import of libvirt library This patch adds new method _query_libvirt_domain: - This method needs imported libvirt library to work - It tries to connect to hypervisor, if it succeeds, it checks if guest with entered name exists and tries to recover it's IP from DHCP lease in "default" network. If it fails, it queries user for new libvirt domain, otherwise it returns the string with the domain
Signed-off-by: Jiri Prochazka jprochaz@redhat.com --- lnst/Controller/Wizard.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 570f0d1..137e758 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -10,6 +10,7 @@ __author__ = """ jprochaz@redhat.com (Jiri Prochazka) """
+import libvirt import socket import sys import time @@ -27,6 +28,9 @@ PATH_NOT_DIR = 3
class Wizard: + def __init__(self): + # used for storing libvirt connection + self._conn = None
def interactive(self, pool_dir=None): """ Starts Wizard in an interactive mode @@ -375,3 +379,39 @@ class Wizard: return port except: sys.stderr.write("Invalid port entered\n") + + def _query_libvirt_domain(self): + """ Queries user for libvirt_domain + @note Virtual host must be running under libvirt + and has to have an IP from "default" network + DHCP server + @return String representing libvirt_domain of host + """ + if self._conn is None: + self._conn = libvirt.openReadOnly("qemu:///system") + + if self._conn is None: + sys.stderr.write("Failed to open connection to hypervisor\n") + return (None, None) + + while True: + libvirt_domain = raw_input("Enter libvirt domain of virtual host: ") + if libvirt_domain == "": + sys.stderr.write("No domain entered\n") + continue + try: + self._conn.lookupByName(libvirt_domain) + except: + continue + + for lease in self._conn.networkLookupByName("default").DHCPLeases(): + if lease["hostname"] == libvirt_domain: + return (lease["ipaddr"], libvirt_domain) + + sys.stderr.write("Couldn't find any IP associated with selected libvirt_domain\n") + answer = raw_input("Do you want to add hostname manually? [Y/n]: ") + if answer.lower() == "y" or answer == "": + hostname = self._query_hostname() + return libvirt_domain, hostname + else: + continue
This patch adds method for adding virtual machines via pool wizard: - Method queries user for pool_dir andlibvirt_domain, if valid ones are entered and successfuly checked, it creates XML file containing hostname and libvirt_domain needed for valid slave virtual machine config
Signed-off-by: Jiri Prochazka jprochaz@redhat.com --- lnst/Controller/Wizard.py | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 137e758..a4e5772 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -15,7 +15,7 @@ import socket import sys import time import os -from lnst.Common.Utils import mkdir_p +from lnst.Common.Utils import mkdir_p, check_process_running from lnst.Common.Config import DefaultRPCPort from lnst.Common.ConnectionHandler import send_data, recv_data from xml.dom.minidom import getDOMImplementation @@ -140,6 +140,38 @@ class Wizard: hostname=hostname, pool_dir=pool_dir, filename=filename, mode="noninteractive")
+ def virtual(self, pool_dir=None): + """ Starts Wizard in a virtual mode + @param pool_dir Path to pool directory (optional) + """ + + print("WARNING: For LNST Pool Wizard to work with virtual guests, "\ + "several conditions have to be met: \n"\ + "\t1) Guests must be running under libvirt and QEMU\n"\ + "\t2) Guests must be in "default" network and have an IP "\ + "address from DHCP in that network") + + if not check_process_running("libvirtd"): + sys.stderr.write("libvirtd is not running, aborting wizard\n") + return + + pool_dir = self._check_and_query_pool_dir(pool_dir) + + while True: + hostname, libvirt_domain= self._query_libvirt_domain() + if hostname is None: + return + filename = self._query_filename(libvirt_domain) + + self._create_xml(hostname=hostname, pool_dir=pool_dir, + filename=filename, mode="virtual", + libvirt_domain=libvirt_domain) + + if self._query_continuation(): + continue + else: + break + def _check_hostname(self, hostname): """ Checks hostnames translatibility @param hostname Hostname which is checked whether it's valid
Users now can use the pool wizard for adding virtual machines running under libvirt and QEMU.
To use wizard in the VM mode, use option -v or --virtual. The option is compatible with -p and --pool-dir option
Signed-off-by: Jiri Prochazka jprochaz@redhat.com --- lnst-pool-wizard | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/lnst-pool-wizard b/lnst-pool-wizard index 9da8911..a1ed03f 100755 --- a/lnst-pool-wizard +++ b/lnst-pool-wizard @@ -29,6 +29,7 @@ def help(retval=0): " -i, --interactive start wizard in interactive mode (this "\ "is default mode)\n"\ " -n, --noninteractive start wizard in noninteractive mode\n"\ + " -v, --virtual start wizard in mode for VMs\n"\ "Examples:\n"\ " lnst-pool-wizard --interactive\n"\ " lnst-pool-wizard --noninteractive 192.168.122.2\n"\ @@ -41,8 +42,8 @@ def main(): try: opts, args = getopt.getopt( sys.argv[1:], - "hinp:", - ["help", "interactive", "noninteractive", "pool_dir="] + "hinvp:", + ["help", "interactive", "noninteractive", "virtual", "pool_dir="] ) except getopt.GetoptError as err: sys.stderr.write(str(err)) @@ -63,6 +64,8 @@ def main(): else: hostlist = args mode = "noninteractive" + elif opt in ("-v", "--virtual"): + mode = "virtual" elif opt in ("-p", "--pool_dir"): if not arg: sys.stderr.write("No pool directory specified\n") @@ -75,6 +78,8 @@ def main():
if mode == "noninteractive": w.noninteractive(hostlist, pool_dir) + elif mode == "virtual": + w.virtual(pool_dir) else: w.interactive(pool_dir)
Fri, Aug 28, 2015 at 11:17:22AM CEST, jprochaz@redhat.com wrote:
This patch adds import of libvirt library This patch adds new method _query_libvirt_domain:
- This method needs imported libvirt library to work
- It tries to connect to hypervisor, if it succeeds, it checks if guest with entered name exists and tries to recover it's IP from DHCP lease in "default" network. If it fails, it queries user for new libvirt domain, otherwise it returns the string with the domain
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 570f0d1..137e758 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -10,6 +10,7 @@ __author__ = """ jprochaz@redhat.com (Jiri Prochazka) """
+import libvirt
Make this import conditional please, there might be users that do not have the module available.
import socket import sys import time @@ -27,6 +28,9 @@ PATH_NOT_DIR = 3
class Wizard:
def __init__(self):
# used for storing libvirt connection
self._conn = None
def interactive(self, pool_dir=None): """ Starts Wizard in an interactive mode
@@ -375,3 +379,39 @@ class Wizard: return port except: sys.stderr.write("Invalid port entered\n")
- def _query_libvirt_domain(self):
""" Queries user for libvirt_domain
@note Virtual host must be running under libvirt
and has to have an IP from "default" network
DHCP server
@return String representing libvirt_domain of host
"""
if self._conn is None:
self._conn = libvirt.openReadOnly("qemu:///system")
if self._conn is None:
sys.stderr.write("Failed to open connection to hypervisor\n")
return (None, None)
Can you move this code one level up to virtual()? It looks a bit weird here.
while True:
libvirt_domain = raw_input("Enter libvirt domain of virtual host: ")
if libvirt_domain == "":
sys.stderr.write("No domain entered\n")
continue
try:
self._conn.lookupByName(libvirt_domain)
except:
continue
^^^ Please be friendly to user, print at least something, ideally exception name but anything simple would work, too.
for lease in self._conn.networkLookupByName("default").DHCPLeases():
if lease["hostname"] == libvirt_domain:
return (lease["ipaddr"], libvirt_domain)
sys.stderr.write("Couldn't find any IP associated with selected libvirt_domain\n")
Which libvirt_domain? Please write the value of libvirt_domain.
answer = raw_input("Do you want to add hostname manually? [Y/n]: ")
if answer.lower() == "y" or answer == "":
hostname = self._query_hostname()
return libvirt_domain, hostname
else:
continue
I don't get it. So if I do not answer Y to the question I will be silently ignored? I'd rather expect to be asked to enter the hostname directly and not whether I want to add it manually. Is there option to have it automagically? I think the question is unnecessary here.
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
Fri, Aug 28, 2015 at 11:17:22AM CEST, jprochaz@redhat.com wrote:
This patch adds import of libvirt library This patch adds new method _query_libvirt_domain:
- This method needs imported libvirt library to work
- It tries to connect to hypervisor, if it succeeds, it checks if guest with entered name exists and tries to recover it's IP from DHCP lease in "default" network. If it fails, it queries user for new libvirt domain, otherwise it returns the string with the domain
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 570f0d1..137e758 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -10,6 +10,7 @@ __author__ = """ jprochaz@redhat.com (Jiri Prochazka) """
+import libvirt import socket import sys import time @@ -27,6 +28,9 @@ PATH_NOT_DIR = 3
class Wizard:
def __init__(self):
# used for storing libvirt connection
self._conn = None
def interactive(self, pool_dir=None): """ Starts Wizard in an interactive mode
@@ -375,3 +379,39 @@ class Wizard: return port except: sys.stderr.write("Invalid port entered\n")
- def _query_libvirt_domain(self):
""" Queries user for libvirt_domain
@note Virtual host must be running under libvirt
and has to have an IP from "default" network
DHCP server
@return String representing libvirt_domain of host
"""
if self._conn is None:
self._conn = libvirt.openReadOnly("qemu:///system")
if self._conn is None:
sys.stderr.write("Failed to open connection to hypervisor\n")
return (None, None)
while True:
libvirt_domain = raw_input("Enter libvirt domain of virtual host: ")
if libvirt_domain == "":
sys.stderr.write("No domain entered\n")
continue
try:
self._conn.lookupByName(libvirt_domain)
except:
continue
for lease in self._conn.networkLookupByName("default").DHCPLeases():
if lease["hostname"] == libvirt_domain:
return (lease["ipaddr"], libvirt_domain)
Here you return 1. hostname/ip_address 2. domain
sys.stderr.write("Couldn't find any IP associated with selected libvirt_domain\n")
answer = raw_input("Do you want to add hostname manually? [Y/n]: ")
if answer.lower() == "y" or answer == "":
hostname = self._query_hostname()
return libvirt_domain, hostname
Here you return 1. domain 2. hostname/ip_address. I think this is a bug.
else:
continue
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
On Fri, Aug 28, 2015 at 11:17:18AM +0200, Jiri Prochazka wrote:
This patch introduces several changes to the _query_pool_dir method in lnst.Controller.Wizard:
- Method was renamed to _check_and_query_pool_dir to better express nature of the method
- Method now uses 1 optional argument - pool_dir - if set to something else than None, it checks if it's valid path to existing or creatable dir, if it is, uses it, else queries user for a new path (which is also checked)
- Method noninteractive has added default value (None) for argument pool_dir
This method was reworked to prevent code duplication which would occur when mode for virtual machines would be introduced
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 51856e8..b35b216 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -32,22 +32,7 @@ class Wizard: """ Starts Wizard in an interactive mode @param pool_dir Path to pool directory (optional) """
if pool_dir is None:
pool_dir = self._query_pool_dir()
else:
rv = self._check_path(pool_dir)
if rv == PATH_IS_DIR_ACCESSIBLE:
print("Pool directory set to '%s'" % pool_dir)
elif rv == PATH_DOES_NOT_EXIST:
sys.stderr.write("Path '%s' does not exist\n" % pool_dir)
pool_dir = self._query_pool_dir()
elif rv == PATH_NOT_DIR:
sys.stderr.write("Path '%s' exists but is not a directory\n"
% pool_dir)
pool_dir = self._query_pool_dir()
elif rv == PATH_IS_DIR_NOT_ACCESSIBLE:
sys.stderr.write("Directory '%s' is not writable\n" % pool_dir)
pool_dir = self._query_pool_dir()
pool_dir = self._check_and_query_pool_dir(pool_dir) while True: hostname = self._query_hostname()
@@ -75,7 +60,7 @@ class Wizard: else: break
- def noninteractive(self, hostlist, pool_dir):
- def noninteractive(self, hostlist, pool_dir=None):
1. Why "None" and not "DefaultPoolDir" ? 2. Doesn't it make more sense to split this into it's own patch? It doesn't look like it's logically connected to anything else changed in this patch.
""" Starts Wizard in noninteractive mode @param hostlist List of hosts (mandatory) @param pool_dir Path to pool_directory (optional)
@@ -330,13 +315,15 @@ class Wizard: sys.stderr.write("Hostname '%s' is not translatable into a " "valid IP address\n" % hostname)
- def _query_pool_dir(self):
- def _check_and_query_pool_dir(self, pool_dir): """ Queries user for pool directory
@param pool_dir Optional pool_dir which will be checked and used if OK @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 is None:
pool_dir = raw_input("Enter path to a pool directory "
"(default: '%s'): " % DefaultPoolDir) if pool_dir == "": pool_dir = DefaultPoolDir
@@ -359,6 +346,7 @@ class Wizard: elif rv == PATH_IS_DIR_NOT_ACCESSIBLE: sys.stderr.write("Directory '%s' is not writable\n" % pool_dir)
pool_dir = None
def _query_port(self): """ Queries user for port
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
On Fri, Aug 28, 2015 at 11:17:20AM +0200, Jiri Prochazka wrote:
This patch introduces several minor changes to _create_xml method:
- Removes redundant argument "port"
I'm guessing this was removed because it wasn't used? But then I see it as a bug since we do have a slave machine param for the RPC port -> "rpc_port", I see that in the wiki it's documented incorrectly as "rpcport" so I'll fix that.
- Adds new argument "libvirt_domain" which will be used in virtual mode
- Updates documentation for the method
- Adds default values (None) to all arguments
- Calls of the functions in interactive and noninteractive methods were adjusted to be up-to-date with the changes
Changes were made as preparation for implementing virtual mode, because new virtual method will use _create_xml method differently than existing ones
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index e91413d..1a3ecc0 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -53,8 +53,9 @@ class Wizard: "'%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")
self._create_xml(machine_interfaces=machine_interfaces,
hostname=hostname, pool_dir=pool_dir,
filename=filename, mode="interactive") if self._query_continuation(): continue else:
@@ -131,8 +132,9 @@ class Wizard: continue else: filename = hostname + ".xml"
self._create_xml(machine_interfaces, hostname,
port, pool_dir, filename, "noninteractive")
self._create_xml(machine_interfaces=machine_interfaces,
hostname=hostname, pool_dir=pool_dir,
filename=filename, mode="noninteractive")
def _check_hostname(self, hostname): """ Checks hostnames translatibility
@@ -174,15 +176,16 @@ class Wizard: sys.stderr.write("Failed creating dir\n") return None
- def _create_xml(self, machine_interfaces, hostname,
port, pool_dir, filename, mode):
- def _create_xml(self, machine_interfaces=None, hostname=None,
pool_dir=None, filename=None, mode=None,
libvirt_domain=None): """ 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
@param libvirt_domain Libvirt domain of virtual host """ impl = getDOMImplementation()
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
On Fri, Aug 28, 2015 at 11:17:19AM +0200, Jiri Prochazka wrote:
The method tries to create a file and write in it contents of result XML, if it success, returns True, else it returns False
This method was created to improve readability and modularity of the pool wizard code.
I'd say this patch should also update the create_xml method that this was split from, instead of doing the update in a different patch.
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index b35b216..e91413d 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -226,17 +226,20 @@ class Wizard: return
- def _write_to_file(self, pool_dir, filename, doc):
""" Writes contents of XML to a file
@param pool_dir Path to directory where the file will be created
@param filename Name of the created file
@param doc Contents of XML file
@return True if file was successfuly written, False otherwise
""" try: f = open(pool_dir + "/" + filename, "w") f.write(doc.toprettyxml()) f.close()
return True except:
msg = "File '%s/%s' could not be opened or data written\n"\
% (pool_dir, filename)
sys.stderr.write(msg)
return
print("File '%s' successfuly created" % filename)
return False
def _get_connection(self, hostname, port): """ Connects to machine
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
2015-09-02 15:18 GMT+02:00 Ondrej Lichtner olichtne@redhat.com:
On Fri, Aug 28, 2015 at 11:17:18AM +0200, Jiri Prochazka wrote:
This patch introduces several changes to the _query_pool_dir method in
lnst.Controller.Wizard:
- Method was renamed to _check_and_query_pool_dir to better express
nature of the method
- Method now uses 1 optional argument - pool_dir - if set to something
else than None,
it checks if it's valid path to existing or creatable dir, if it is,
uses it,
else queries user for a new path (which is also checked)
- Method noninteractive has added default value (None) for argument
pool_dir
This method was reworked to prevent code duplication which would occur
when mode for virtual
machines would be introduced
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 51856e8..b35b216 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -32,22 +32,7 @@ class Wizard: """ Starts Wizard in an interactive mode @param pool_dir Path to pool directory (optional) """
if pool_dir is None:
pool_dir = self._query_pool_dir()
else:
rv = self._check_path(pool_dir)
if rv == PATH_IS_DIR_ACCESSIBLE:
print("Pool directory set to '%s'" % pool_dir)
elif rv == PATH_DOES_NOT_EXIST:
sys.stderr.write("Path '%s' does not exist\n" %
pool_dir)
pool_dir = self._query_pool_dir()
elif rv == PATH_NOT_DIR:
sys.stderr.write("Path '%s' exists but is not a
directory\n"
% pool_dir)
pool_dir = self._query_pool_dir()
elif rv == PATH_IS_DIR_NOT_ACCESSIBLE:
sys.stderr.write("Directory '%s' is not writable\n" %
pool_dir)
pool_dir = self._query_pool_dir()
pool_dir = self._check_and_query_pool_dir(pool_dir) while True: hostname = self._query_hostname()
@@ -75,7 +60,7 @@ class Wizard: else: break
- def noninteractive(self, hostlist, pool_dir):
- def noninteractive(self, hostlist, pool_dir=None):
- Why "None" and not "DefaultPoolDir" ?
Because when pool_dir is set to None, the loop will continue and user is queried again.
- Doesn't it make more sense to split this into it's own patch? It
doesn't look like it's logically connected to anything else changed in this patch.
To split what in it's own patch?
""" Starts Wizard in noninteractive mode @param hostlist List of hosts (mandatory) @param pool_dir Path to pool_directory (optional)
@@ -330,13 +315,15 @@ class Wizard: sys.stderr.write("Hostname '%s' is not translatable
into a "
"valid IP address\n" % hostname)
- def _query_pool_dir(self):
- def _check_and_query_pool_dir(self, pool_dir): """ Queries user for pool directory
@param pool_dir Optional pool_dir which will be checked and
used if OK
@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 is None:
pool_dir = raw_input("Enter path to a pool directory "
"(default: '%s'): " %
DefaultPoolDir)
if pool_dir == "": pool_dir = DefaultPoolDir
@@ -359,6 +346,7 @@ class Wizard: elif rv == PATH_IS_DIR_NOT_ACCESSIBLE: sys.stderr.write("Directory '%s' is not writable\n" % pool_dir)
pool_dir = None
def _query_port(self): """ Queries user for port
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
2015-09-02 15:32 GMT+02:00 Ondrej Lichtner olichtne@redhat.com:
On Fri, Aug 28, 2015 at 11:17:20AM +0200, Jiri Prochazka wrote:
This patch introduces several minor changes to _create_xml method:
- Removes redundant argument "port"
I'm guessing this was removed because it wasn't used? But then I see it as a bug since we do have a slave machine param for the RPC port -> "rpc_port", I see that in the wiki it's documented incorrectly as "rpcport" so I'll fix that.
Yes, it wasn't used, but it would be handy to have it there as well, so I
will rework this again so the config contains the port as well
- Adds new argument "libvirt_domain" which will be used in virtual mode
- Updates documentation for the method
- Adds default values (None) to all arguments
- Calls of the functions in interactive and noninteractive methods were adjusted to be up-to-date with the changes
Changes were made as preparation for implementing virtual mode, because new virtual method will use _create_xml method differently than existing
ones
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index e91413d..1a3ecc0 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -53,8 +53,9 @@ class Wizard: "'%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")
self._create_xml(machine_interfaces=machine_interfaces,
hostname=hostname, pool_dir=pool_dir,
filename=filename, mode="interactive") if self._query_continuation(): continue else:
@@ -131,8 +132,9 @@ class Wizard: continue else: filename = hostname + ".xml"
self._create_xml(machine_interfaces, hostname,
port, pool_dir, filename,
"noninteractive")
self._create_xml(machine_interfaces=machine_interfaces,
hostname=hostname, pool_dir=pool_dir,
filename=filename,
mode="noninteractive")
def _check_hostname(self, hostname): """ Checks hostnames translatibility
@@ -174,15 +176,16 @@ class Wizard: sys.stderr.write("Failed creating dir\n") return None
- def _create_xml(self, machine_interfaces, hostname,
port, pool_dir, filename, mode):
- def _create_xml(self, machine_interfaces=None, hostname=None,
pool_dir=None, filename=None, mode=None,
libvirt_domain=None): """ 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
@param libvirt_domain Libvirt domain of virtual host """ impl = getDOMImplementation()
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
On Thu, Sep 03, 2015 at 10:36:49AM +0200, Jiri Prochazka wrote:
2015-09-02 15:18 GMT+02:00 Ondrej Lichtner olichtne@redhat.com:
On Fri, Aug 28, 2015 at 11:17:18AM +0200, Jiri Prochazka wrote:
This patch introduces several changes to the _query_pool_dir method in
lnst.Controller.Wizard:
- Method was renamed to _check_and_query_pool_dir to better express
nature of the method
- Method now uses 1 optional argument - pool_dir - if set to something
else than None,
it checks if it's valid path to existing or creatable dir, if it is,
uses it,
else queries user for a new path (which is also checked)
- Method noninteractive has added default value (None) for argument
pool_dir
This method was reworked to prevent code duplication which would occur
when mode for virtual
machines would be introduced
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 51856e8..b35b216 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -32,22 +32,7 @@ class Wizard: """ Starts Wizard in an interactive mode @param pool_dir Path to pool directory (optional) """
if pool_dir is None:
pool_dir = self._query_pool_dir()
else:
rv = self._check_path(pool_dir)
if rv == PATH_IS_DIR_ACCESSIBLE:
print("Pool directory set to '%s'" % pool_dir)
elif rv == PATH_DOES_NOT_EXIST:
sys.stderr.write("Path '%s' does not exist\n" %
pool_dir)
pool_dir = self._query_pool_dir()
elif rv == PATH_NOT_DIR:
sys.stderr.write("Path '%s' exists but is not a
directory\n"
% pool_dir)
pool_dir = self._query_pool_dir()
elif rv == PATH_IS_DIR_NOT_ACCESSIBLE:
sys.stderr.write("Directory '%s' is not writable\n" %
pool_dir)
pool_dir = self._query_pool_dir()
pool_dir = self._check_and_query_pool_dir(pool_dir) while True: hostname = self._query_hostname()
@@ -75,7 +60,7 @@ class Wizard: else: break
- def noninteractive(self, hostlist, pool_dir):
- def noninteractive(self, hostlist, pool_dir=None):
- Why "None" and not "DefaultPoolDir" ?
Because when pool_dir is set to None, the loop will continue and user is queried again.
This is noninteractive mode... no user queries.
- Doesn't it make more sense to split this into it's own patch? It
doesn't look like it's logically connected to anything else changed in this patch.
To split what in it's own patch?
This change of the default parameter, it doesn't look like it's connected to any other changes this patch makes so shouldn't it be in it's own patch?
""" Starts Wizard in noninteractive mode @param hostlist List of hosts (mandatory) @param pool_dir Path to pool_directory (optional)
@@ -330,13 +315,15 @@ class Wizard: sys.stderr.write("Hostname '%s' is not translatable
into a "
"valid IP address\n" % hostname)
- def _query_pool_dir(self):
- def _check_and_query_pool_dir(self, pool_dir): """ Queries user for pool directory
@param pool_dir Optional pool_dir which will be checked and
used if OK
@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 is None:
pool_dir = raw_input("Enter path to a pool directory "
"(default: '%s'): " %
DefaultPoolDir)
if pool_dir == "": pool_dir = DefaultPoolDir
@@ -359,6 +346,7 @@ class Wizard: elif rv == PATH_IS_DIR_NOT_ACCESSIBLE: sys.stderr.write("Directory '%s' is not writable\n" % pool_dir)
pool_dir = None
def _query_port(self): """ Queries user for port
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
2015-09-01 17:30 GMT+02:00 Jan Tluka jtluka@redhat.com:
Fri, Aug 28, 2015 at 11:17:22AM CEST, jprochaz@redhat.com wrote:
This patch adds import of libvirt library This patch adds new method _query_libvirt_domain:
- This method needs imported libvirt library to work
- It tries to connect to hypervisor, if it succeeds, it checks if guest with entered name exists and tries to recover it's IP from DHCP lease in "default" network. If it fails, it queries user for new libvirt domain, otherwise it returns the string with the domain
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 570f0d1..137e758 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -10,6 +10,7 @@ __author__ = """ jprochaz@redhat.com (Jiri Prochazka) """
+import libvirt
Make this import conditional please, there might be users that do not have the module available.
import socket import sys import time @@ -27,6 +28,9 @@ PATH_NOT_DIR = 3
class Wizard:
def __init__(self):
# used for storing libvirt connection
self._conn = None
def interactive(self, pool_dir=None): """ Starts Wizard in an interactive mode
@@ -375,3 +379,39 @@ class Wizard: return port except: sys.stderr.write("Invalid port entered\n")
- def _query_libvirt_domain(self):
""" Queries user for libvirt_domain
@note Virtual host must be running under libvirt
and has to have an IP from "default" network
DHCP server
@return String representing libvirt_domain of host
"""
if self._conn is None:
self._conn = libvirt.openReadOnly("qemu:///system")
if self._conn is None:
sys.stderr.write("Failed to open connection to hypervisor\n")
return (None, None)
Can you move this code one level up to virtual()? It looks a bit weird here.
while True:
libvirt_domain = raw_input("Enter libvirt domain of virtual
host: ")
if libvirt_domain == "":
sys.stderr.write("No domain entered\n")
continue
try:
self._conn.lookupByName(libvirt_domain)
except:
continue
^^^ Please be friendly to user, print at least something,
ideally exception name but anything simple would work, too.
The libvirt itself prints error message for user -
"libvirt: QEMU Driver error : Domain not found: no domain with matching name 'xyz'"
for lease in
self._conn.networkLookupByName("default").DHCPLeases():
if lease["hostname"] == libvirt_domain:
return (lease["ipaddr"], libvirt_domain)
sys.stderr.write("Couldn't find any IP associated with
selected libvirt_domain\n")
Which libvirt_domain? Please write the value of libvirt_domain.
answer = raw_input("Do you want to add hostname manually?
if answer.lower() == "y" or answer == "":
hostname = self._query_hostname()
return libvirt_domain, hostname
else:
continue
I don't get it. So if I do not answer Y to the question I will be silently ignored? I'd rather expect to be asked to enter the hostname directly and not whether I want to add it manually. Is there option to have it automagically? I think the question is unnecessary here.
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
Thu, Sep 03, 2015 at 11:35:34AM CEST, jprochaz@redhat.com wrote:
2015-09-01 17:30 GMT+02:00 Jan Tluka jtluka@redhat.com:
Fri, Aug 28, 2015 at 11:17:22AM CEST, jprochaz@redhat.com wrote:
This patch adds import of libvirt library This patch adds new method _query_libvirt_domain:
- This method needs imported libvirt library to work
- It tries to connect to hypervisor, if it succeeds, it checks if guest with entered name exists and tries to recover it's IP from DHCP lease in "default" network. If it fails, it queries user for new libvirt domain, otherwise it returns the string with the domain
Signed-off-by: Jiri Prochazka jprochaz@redhat.com
lnst/Controller/Wizard.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py index 570f0d1..137e758 100644 --- a/lnst/Controller/Wizard.py +++ b/lnst/Controller/Wizard.py @@ -10,6 +10,7 @@ __author__ = """ jprochaz@redhat.com (Jiri Prochazka) """
+import libvirt
Make this import conditional please, there might be users that do not have the module available.
import socket import sys import time @@ -27,6 +28,9 @@ PATH_NOT_DIR = 3
class Wizard:
def __init__(self):
# used for storing libvirt connection
self._conn = None
def interactive(self, pool_dir=None): """ Starts Wizard in an interactive mode
@@ -375,3 +379,39 @@ class Wizard: return port except: sys.stderr.write("Invalid port entered\n")
- def _query_libvirt_domain(self):
""" Queries user for libvirt_domain
@note Virtual host must be running under libvirt
and has to have an IP from "default" network
DHCP server
@return String representing libvirt_domain of host
"""
if self._conn is None:
self._conn = libvirt.openReadOnly("qemu:///system")
if self._conn is None:
sys.stderr.write("Failed to open connection to hypervisor\n")
return (None, None)
Can you move this code one level up to virtual()? It looks a bit weird here.
while True:
libvirt_domain = raw_input("Enter libvirt domain of virtual
host: ")
if libvirt_domain == "":
sys.stderr.write("No domain entered\n")
continue
try:
self._conn.lookupByName(libvirt_domain)
except:
continue
^^^ Please be friendly to user, print at least something,
ideally exception name but anything simple would work, too.
The libvirt itself prints error message for user -
"libvirt: QEMU Driver error : Domain not found: no domain with matching name 'xyz'"
Ok, no big deal then.
for lease in
self._conn.networkLookupByName("default").DHCPLeases():
if lease["hostname"] == libvirt_domain:
return (lease["ipaddr"], libvirt_domain)
sys.stderr.write("Couldn't find any IP associated with
selected libvirt_domain\n")
Which libvirt_domain? Please write the value of libvirt_domain.
answer = raw_input("Do you want to add hostname manually?
if answer.lower() == "y" or answer == "":
hostname = self._query_hostname()
return libvirt_domain, hostname
else:
continue
I don't get it. So if I do not answer Y to the question I will be silently ignored? I'd rather expect to be asked to enter the hostname directly and not whether I want to add it manually. Is there option to have it automagically? I think the question is unnecessary here.
-- 2.4.3
LNST-developers mailing list LNST-developers@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
lnst-developers@lists.fedorahosted.org