[libvirt] [test-API PATCH 0/7] Multiple fixes and improvements series
This is a set of more or less independent fixes and improvements to the test API. I ran across these while trying to write a basic test case as a "Hello world!" to the test-API. Improvements are in fields of cross-distro compatibility, broken API's and typos and usability. Peter Krempa (7): utils: Make ipget.sh more portable lib: fix streamAPI class domainAPI: Add wrapper method to work with domain's console connections repos/domain/create.py: Fix typo in path to python executable utils: Allow suffixes in path to the main checkout folder parser: Be more specific on mistakes in case files domain/[start|destroy]: Add a optional noping flag to skip the ping test exception.py | 10 +++++++++ lib/domainAPI.py | 9 ++++++++ lib/streamAPI.py | 52 +++++++++++++++++++++++----------------------- proxy.py | 4 +++ repos/domain/create.py | 2 +- repos/domain/destroy.py | 50 +++++++++++++++++++++++++------------------- repos/domain/start.py | 50 +++++++++++++++++++++----------------------- utils/Python/utils.py | 2 +- utils/ipget.sh | 4 +- 9 files changed, 105 insertions(+), 78 deletions(-) -- 1.7.3.4
Change the test for existence of nmap to be a little more cross-distro portable. --- utils/ipget.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/ipget.sh b/utils/ipget.sh index cc1949b..4e6b3dd 100755 --- a/utils/ipget.sh +++ b/utils/ipget.sh @@ -6,8 +6,8 @@ if [[ -z $mac ]]; then exit 1 fi -if ! rpm -qa |grep -q nmap ;then - echo "need nmap rpmball installed." +if ! which nmap > /dev/null; then + echo "nmap package needs to be installed." exit 1 fi -- 1.7.3.4
On 2012年03月21日 20:46, Peter Krempa wrote:
Change the test for existence of nmap to be a little more cross-distro portable. --- utils/ipget.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/utils/ipget.sh b/utils/ipget.sh index cc1949b..4e6b3dd 100755 --- a/utils/ipget.sh +++ b/utils/ipget.sh @@ -6,8 +6,8 @@ if [[ -z $mac ]]; then exit 1 fi
-if ! rpm -qa |grep -q nmap ;then - echo "need nmap rpmball installed." +if ! which nmap> /dev/null; then + echo "nmap package needs to be installed." exit 1 fi
Make sense, and ACK.
On 03/21/2012 08:26 AM, Osier Yang wrote:
On 2012年03月21日 20:46, Peter Krempa wrote:
Change the test for existence of nmap to be a little more cross-distro portable. --- utils/ipget.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/utils/ipget.sh b/utils/ipget.sh index cc1949b..4e6b3dd 100755 --- a/utils/ipget.sh +++ b/utils/ipget.sh @@ -6,8 +6,8 @@ if [[ -z $mac ]]; then exit 1 fi
-if ! rpm -qa |grep -q nmap ;then - echo "need nmap rpmball installed." +if ! which nmap> /dev/null; then
'which' is not as portable as 'type'; POSIX recommends: type nmap >/dev/null 2>&1 for checking whether nmap is an installed command. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Change the test for existence of nmap to be a little more cross-distro portable. --- utils/ipget.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/ipget.sh b/utils/ipget.sh index cc1949b..8d44b14 100755 --- a/utils/ipget.sh +++ b/utils/ipget.sh @@ -6,8 +6,8 @@ if [[ -z $mac ]]; then exit 1 fi -if ! rpm -qa |grep -q nmap ;then - echo "need nmap rpmball installed." +if ! type nmap >/dev/null 2>&1; then + echo "nmap package needs to be installed." exit 1 fi -- 1.7.3.4
On 03/21/2012 10:53 PM, Peter Krempa wrote:
Change the test for existence of nmap to be a little more cross-distro portable. --- utils/ipget.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/utils/ipget.sh b/utils/ipget.sh index cc1949b..8d44b14 100755 --- a/utils/ipget.sh +++ b/utils/ipget.sh @@ -6,8 +6,8 @@ if [[ -z $mac ]]; then exit 1 fi
-if ! rpm -qa |grep -q nmap ;then - echo "need nmap rpmball installed." +if ! type nmap>/dev/null 2>&1; then + echo "nmap package needs to be installed." exit 1 fi
ACK ,thanks Guannan Ren
The streamAPI class that encapsulates work with libvirt's streams was fundamentaly broken: - each call to one of the methods created a new stream and performed the call - some methods called virStream methods with different numbers of arguments - there was no way to extract the actual libvirt stream object --- lib/streamAPI.py | 52 ++++++++++++++++++++++++++-------------------------- 1 files changed, 26 insertions(+), 26 deletions(-) diff --git a/lib/streamAPI.py b/lib/streamAPI.py index bc7d217..69e183e 100644 --- a/lib/streamAPI.py +++ b/lib/streamAPI.py @@ -38,76 +38,76 @@ append_path(result.group(0)) import exception class StreamAPI(object): - def __init__(self, connection): - self.conn = connection + def __init__(self, conn, flags = 0): + try: + self.stream = conn.newStream(flags) + except libvirt.libvirtError, e: + message = e.get_error_message() + code = e.get_error_code() + raise exception.LibvirtAPI(message, code) + + def getStream(self): + return self.stream - def abort(self, flag = 0): + def abort(self): try: - stream_obj = newStream(flag) - return stream_obj.abort() + return self.stream.abort() except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code) - def connect(self, flag = 0): + def connect(self): try: - stream_obj = newStream(flag) - return stream_obj.connect() + return self.stream.connect() except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code) - def finish(self, flag = 0): + def finish(self): try: - stream_obj = newStream(flag) - return stream_obj.finish() + return self.stream.finish() except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code) - def recv(self, flag = 0, data, nbytes): + def recv(self, nbytes): try: - stream_obj = newStream(flag) - return stream_obj.recv(data, nbytes) + return self.stream.recv(nbytes) except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code) - def send(self, flag = 0, data, nbytes): + def send(self, data): try: - stream_obj = newStream(flag) - return stream_obj.send(data, nbytes) + return self.stream.send(data) except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code) - def eventAddCallback(self, flag = 0, cb, opaque): + def eventAddCallback(self, cb, opaque): try: - stream_obj = newStream(flag) - return stream_obj.eventAddCallback(cb, opaque) + return self.stream.eventAddCallback(cb, opaque) except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code) - def eventRemoveCallback(self, flag = 0): + def eventRemoveCallback(self): try: - stream_obj = newStream(flag) - return stream_obj.eventRemoveCallback() + return self.stream.eventRemoveCallback() except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code) - def eventUpdateCallback(self, flag = 0, events) + def eventUpdateCallback(self, events): try: - stream_obj = newStream(flag) - return stream_obj.eventUpdateCallback(events) + return self.stream.eventUpdateCallback(events) except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() -- 1.7.3.4
On 03/21/2012 08:46 PM, Peter Krempa wrote:
The streamAPI class that encapsulates work with libvirt's streams was fundamentaly broken: - each call to one of the methods created a new stream and performed the call - some methods called virStream methods with different numbers of arguments - there was no way to extract the actual libvirt stream object thanks for the patch.
--- lib/streamAPI.py | 52 ++++++++++++++++++++++++++-------------------------- 1 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/lib/streamAPI.py b/lib/streamAPI.py index bc7d217..69e183e 100644 --- a/lib/streamAPI.py +++ b/lib/streamAPI.py @@ -38,76 +38,76 @@ append_path(result.group(0)) import exception
class StreamAPI(object): - def __init__(self, connection): - self.conn = connection + def __init__(self, conn, flags = 0): + try: + self.stream = conn.newStream(flags) + except libvirt.libvirtError, e: + message = e.get_error_message() + code = e.get_error_code() + raise exception.LibvirtAPI(message, code) + + def getStream(self): + return self.stream
- def abort(self, flag = 0): + def abort(self): try: - stream_obj = newStream(flag) - return stream_obj.abort() + return self.stream.abort() except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code)
- def connect(self, flag = 0): + def connect(self): try: - stream_obj = newStream(flag) - return stream_obj.connect() + return self.stream.connect() except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code)
- def finish(self, flag = 0): + def finish(self): try: - stream_obj = newStream(flag) - return stream_obj.finish() + return self.stream.finish() except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code)
- def recv(self, flag = 0, data, nbytes): + def recv(self, nbytes): try: - stream_obj = newStream(flag) - return stream_obj.recv(data, nbytes) + return self.stream.recv(nbytes) except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code)
- def send(self, flag = 0, data, nbytes): + def send(self, data): try: - stream_obj = newStream(flag) - return stream_obj.send(data, nbytes) + return self.stream.send(data) except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code)
- def eventAddCallback(self, flag = 0, cb, opaque): + def eventAddCallback(self, cb, opaque):
The API need an argument event to pass in which is virEventHandleType VIR_EVENT_HANDLE_READABLE = 1 VIR_EVENT_HANDLE_WRITABLE = 2 VIR_EVENT_HANDLE_ERROR = 4 VIR_EVENT_HANDLE_HANGUP = 8 We could just overdefine it in streamAPI.py which make use of them easier for writing testcase. It's not a good idea to import libvirt.py directly in testcases.
try: - stream_obj = newStream(flag) - return stream_obj.eventAddCallback(cb, opaque) + return self.stream.eventAddCallback(cb, opaque) except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code)
- def eventRemoveCallback(self, flag = 0): + def eventRemoveCallback(self): try: - stream_obj = newStream(flag) - return stream_obj.eventRemoveCallback() + return self.stream.eventRemoveCallback() except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code() raise exception.LibvirtAPI(message, code)
- def eventUpdateCallback(self, flag = 0, events) + def eventUpdateCallback(self, events): try: - stream_obj = newStream(flag) - return stream_obj.eventUpdateCallback(events) + return self.stream.eventUpdateCallback(events) except libvirt.libvirtError, e: message = e.get_error_message() code = e.get_error_code()
Miss a "raise exception.LibvirtAPI(message, code)" in the eventUpdateCallback The framework has a its own exception set, it includes the error exception from libvirtError So, in each testcases, we just catch the LibvirtAPI to raise API error in try..catch clause rather than catch all of other errors from python basic modules or somewhere. Guannan Ren
On 03/21/2012 04:45 PM, Guannan Ren wrote:
The streamAPI class that encapsulates work with libvirt's streams was fundamentaly broken: - each call to one of the methods created a new stream and performed the call - some methods called virStream methods with different numbers of arguments - there was no way to extract the actual libvirt stream object
On 03/21/2012 08:46 PM, Peter Krempa wrote: thanks for the patch.
--- lib/streamAPI.py | 52 ++++++++++++++++++++++++++-------------------------- 1 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/lib/streamAPI.py b/lib/streamAPI.py index bc7d217..69e183e 100644 --- a/lib/streamAPI.py +++ b/lib/streamAPI.py @@ -38,76 +38,76 @@ append_path(result.group(0)) import exception
- def eventAddCallback(self, flag = 0, cb, opaque): + def eventAddCallback(self, cb, opaque):
The API need an argument event to pass in which is virEventHandleType VIR_EVENT_HANDLE_READABLE = 1 VIR_EVENT_HANDLE_WRITABLE = 2 VIR_EVENT_HANDLE_ERROR = 4 VIR_EVENT_HANDLE_HANGUP = 8
We could just overdefine it in streamAPI.py which make use of them easier for writing testcase. It's not a good idea to import libvirt.py directly in testcases.
Oh, in this case I'll need to redefine the virConsoleFlags enum in the domainAPI too as I'm importing libvirt.py in my (not yet published ) test that deals with consoles. Peter
This patch adds a wrapper that enables work with consoles in the test-API. --- lib/domainAPI.py | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/lib/domainAPI.py b/lib/domainAPI.py index 91f2ba3..bc0069b 100644 --- a/lib/domainAPI.py +++ b/lib/domainAPI.py @@ -878,6 +878,15 @@ class DomainAPI(object): code = e.get_error_code() raise exception.LibvirtAPI(message, code) + def openConsole(self, domname, device, stream, flags = 0): + try: + dom_obj = self.get_domain_by_name(domname) + st_obj = stream.getStream() + return dom_obj.openConsole(device, st_obj, flags) + except libvirt.libvirtError, e: + message = e.get_error_message() + code = e.get_error_code() + raise exception.LibvirtAPI(message, code) # DomainState VIR_DOMAIN_NOSTATE = 0 -- 1.7.3.4
On 03/21/2012 08:46 PM, Peter Krempa wrote:
This patch adds a wrapper that enables work with consoles in the test-API. --- lib/domainAPI.py | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/lib/domainAPI.py b/lib/domainAPI.py index 91f2ba3..bc0069b 100644 --- a/lib/domainAPI.py +++ b/lib/domainAPI.py @@ -878,6 +878,15 @@ class DomainAPI(object): code = e.get_error_code() raise exception.LibvirtAPI(message, code)
+ def openConsole(self, domname, device, stream, flags = 0): + try: + dom_obj = self.get_domain_by_name(domname) + st_obj = stream.getStream()
we could get the st_obj in testcase then pass it into openConsole If so, st_obj could be reused in the following statement in testcase like: st_obj = stream.getStream() dom.openConsole(domname, None, st_obj) receivedData = st_obj.recv(1024) (the last statement probably resides in a registered stream callback function via eventAddCallback API) Thanks for the patch. Guannan Ren
On 03/21/2012 05:02 PM, Guannan Ren wrote:
On 03/21/2012 08:46 PM, Peter Krempa wrote:
This patch adds a wrapper that enables work with consoles in the test-API. --- lib/domainAPI.py | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/lib/domainAPI.py b/lib/domainAPI.py index 91f2ba3..bc0069b 100644 --- a/lib/domainAPI.py +++ b/lib/domainAPI.py @@ -878,6 +878,15 @@ class DomainAPI(object): code = e.get_error_code() raise exception.LibvirtAPI(message, code)
+ def openConsole(self, domname, device, stream, flags = 0): + try: + dom_obj = self.get_domain_by_name(domname) + st_obj = stream.getStream()
we could get the st_obj in testcase then pass it into openConsole If so, st_obj could be reused in the following statement in testcase like:
st_obj = stream.getStream() dom.openConsole(domname, None, st_obj) receivedData = st_obj.recv(1024) (the last statement probably resides in a registered stream callback function via eventAddCallback API)
Wouldn't this actualy defeat the encapsulation that is provided in the StreamAPI class? With this change you can reuse the StreamAPI object for other actions later on too, without loosing the abstraction. With the chagne you pushed: diff --git a/lib/domainAPI.py b/lib/domainAPI.py index 91f2ba3..ddcdc91 100644 --- a/lib/domainAPI.py +++ b/lib/domainAPI.py @@ -878,6 +878,14 @@ class DomainAPI(object): code = e.get_error_code() raise exception.LibvirtAPI(message, code) + def openConsole(self, domname, device, st_obj, flags = 0): + try: + dom_obj = self.get_domain_by_name(domname) + return dom_obj.openConsole(device, st_obj, flags) + except libvirt.libvirtError, e: + message = e.get_error_message() + code = e.get_error_code() + raise exception.LibvirtAPI(message, code) You have to create a new StreamAPI object with DomainAPI.newStream() and then you have to extract the virStream object from that to pass it manualy to DomainAPI.openConsole(). IMO this will encourage to use the virStream object directly without the encapsulation that is provided by the StreamAPI class. Peter
Thanks for the patch.
Guannan Ren
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Wouldn't this actualy defeat the encapsulation that is provided in the StreamAPI class? With this change you can reuse the StreamAPI object for other actions later on too, without loosing the abstraction.
With the chagne you pushed: diff --git a/lib/domainAPI.py b/lib/domainAPI.py index 91f2ba3..ddcdc91 100644 --- a/lib/domainAPI.py +++ b/lib/domainAPI.py @@ -878,6 +878,14 @@ class DomainAPI(object): code = e.get_error_code() raise exception.LibvirtAPI(message, code)
+ def openConsole(self, domname, device, st_obj, flags = 0): + try: + dom_obj = self.get_domain_by_name(domname) + return dom_obj.openConsole(device, st_obj, flags) + except libvirt.libvirtError, e: + message = e.get_error_message() + code = e.get_error_code() + raise exception.LibvirtAPI(message, code)
You have to create a new StreamAPI object with DomainAPI.newStream() and then you have to extract the virStream object from that to pass it manualy to DomainAPI.openConsole(). IMO this will encourage to use the virStream object directly without the encapsulation that is provided by the StreamAPI class.
Peter
Yeath, you are right, we can not cross the encapsulation line. We have to operate above the abstraction layer. Your patch is correct definitely. Guannan Ren
--- repos/domain/create.py | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/repos/domain/create.py b/repos/domain/create.py index b6e1e66..4e3e1c4 100644 --- a/repos/domain/create.py +++ b/repos/domain/create.py @@ -1,4 +1,4 @@ -#!/usr/bin/evn python +#!/usr/bin/env python """this test case is used for testing create domain from xml mandatory arguments:guesttype -- 1.7.3.4
On 03/21/2012 08:46 PM, Peter Krempa wrote:
--- repos/domain/create.py | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/repos/domain/create.py b/repos/domain/create.py index b6e1e66..4e3e1c4 100644 --- a/repos/domain/create.py +++ b/repos/domain/create.py @@ -1,4 +1,4 @@ -#!/usr/bin/evn python +#!/usr/bin/env python
ACK, thanks Guannan Ren
This patch enables the util scripts to be found in correct path when using a not-so-standard checkout path. (.../libvirt-test-API.git/...). --- utils/Python/utils.py | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/utils/Python/utils.py b/utils/Python/utils.py index 6e23138..e0e2dba 100644 --- a/utils/Python/utils.py +++ b/utils/Python/utils.py @@ -317,7 +317,7 @@ class Utils(object): def locate_utils(self): """Get the directory path of 'utils'""" pwd = os.getcwd() - result = re.search('(.*)libvirt-test-API', pwd) + result = re.search('(.*)libvirt-test-API(.*)', pwd) return result.group(0) + "/utils" def mac_to_ip(self, mac, timeout): -- 1.7.3.4
On 03/21/2012 08:46 PM, Peter Krempa wrote:
This patch enables the util scripts to be found in correct path when using a not-so-standard checkout path. (.../libvirt-test-API.git/...). --- utils/Python/utils.py | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/utils/Python/utils.py b/utils/Python/utils.py index 6e23138..e0e2dba 100644 --- a/utils/Python/utils.py +++ b/utils/Python/utils.py @@ -317,7 +317,7 @@ class Utils(object): def locate_utils(self): """Get the directory path of 'utils'""" pwd = os.getcwd() - result = re.search('(.*)libvirt-test-API', pwd) + result = re.search('(.*)libvirt-test-API(.*)', pwd) return result.group(0) + "/utils"
def mac_to_ip(self, mac, timeout):
ACK, thanks Guannan Ren
This patch adds an error exception if the specification of the test module from "repos/" ends with a colon (does not specify the module name) instead of a meaningless backtrace. --- exception.py | 10 ++++++++++ proxy.py | 4 ++++ 2 files changed, 14 insertions(+), 0 deletions(-) diff --git a/exception.py b/exception.py index 1a6d6f1..eeb899a 100644 --- a/exception.py +++ b/exception.py @@ -75,6 +75,16 @@ class FileExist(LibvirtException): class CaseConfigfileError(LibvirtException): code = 210 message = "Case config file Error" + def __init__(self, errorstr=None): + self.errorstr = errorstr + + def __str__(self): + return repr(self.errorstr) + + def response(self): + self.status = {'code':self.code, 'message':"%s:%s" % + (self.message, str(self))} + return self.status class MissingVariable(LibvirtException): code = 210 diff --git a/proxy.py b/proxy.py index aa34d9a..16d498b 100644 --- a/proxy.py +++ b/proxy.py @@ -18,6 +18,8 @@ # The proxy examines the list of unique test cases, received from the # generator and import each test case from appropriate module directory. +import exception + class Proxy(object): """ The Proxy class is used for getting real function call reference """ @@ -86,6 +88,8 @@ class Proxy(object): # Import recursively module for component in components[1:]: + if component=="": + raise exception.CaseConfigfileError("Missing module name after \":\"") case_mod = getattr(case_mod, component) main_function_ref = getattr(case_mod, func) return main_function_ref -- 1.7.3.4
On 03/21/2012 08:46 PM, Peter Krempa wrote:
This patch adds an error exception if the specification of the test module from "repos/" ends with a colon (does not specify the module name) instead of a meaningless backtrace. --- exception.py | 10 ++++++++++ proxy.py | 4 ++++ 2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/exception.py b/exception.py index 1a6d6f1..eeb899a 100644 --- a/exception.py +++ b/exception.py @@ -75,6 +75,16 @@ class FileExist(LibvirtException): class CaseConfigfileError(LibvirtException): code = 210 message = "Case config file Error" + def __init__(self, errorstr=None): + self.errorstr = errorstr + + def __str__(self): + return repr(self.errorstr) + + def response(self): + self.status = {'code':self.code, 'message':"%s:%s" % + (self.message, str(self))} + return self.status
The CaseConfigfileError is the subclass of LibvirtException, so we just raise it with specific error string, we don't need to overload its method unless the subclass want different action in the method.
class MissingVariable(LibvirtException): code = 210 diff --git a/proxy.py b/proxy.py index aa34d9a..16d498b 100644 --- a/proxy.py +++ b/proxy.py @@ -18,6 +18,8 @@ # The proxy examines the list of unique test cases, received from the # generator and import each test case from appropriate module directory.
+import exception +
class Proxy(object): """ The Proxy class is used for getting real function call reference """ @@ -86,6 +88,8 @@ class Proxy(object):
# Import recursively module for component in components[1:]: + if component=="": + raise exception.CaseConfigfileError("Missing module name after \":\"")
Raising an exception here make parsing stronger, thanks. BTW, It's better to use if component == "": Guannan Ren
For some tests it's not needed to ping the guest in the startup process. This patch adds a flag to the start and destroy test to skip such attempts (that consume a lot of time) --- repos/domain/destroy.py | 54 ++++++++++++++++++++++++++-------------------- repos/domain/start.py | 50 ++++++++++++++++++++---------------------- 2 files changed, 54 insertions(+), 50 deletions(-) diff --git a/repos/domain/destroy.py b/repos/domain/destroy.py index f98b602..12399d6 100644 --- a/repos/domain/destroy.py +++ b/repos/domain/destroy.py @@ -50,7 +50,10 @@ def destroy(params): {'guestname': guestname} logger -- an object of utils/Python/log.py - guestname -- same as the domain name + guestname -- the domain name + flags -- optional arguments: + noping: Don't do the ping test + Return 0 on SUCCESS or 1 on FAILURE """ @@ -62,6 +65,7 @@ def destroy(params): if params_check_result: return 1 guestname = params['guestname'] + flags = params['flags'] # Connect to local hypervisor connection URI util = utils.Utils() @@ -73,18 +77,19 @@ def destroy(params): dom_obj = domainAPI.DomainAPI(virconn) dom_name_list = dom_obj.get_list() if guestname not in dom_name_list: - logger.error("guest %s doesn't exist or not be running." % guestname) + logger.error("guest %s doesn't exist or isn't running." % guestname) conn.close() logger.info("closed hypervisor connection") return 1 timeout = 60 logger.info('destroy domain') - # Get domain ip - mac = util.get_dom_mac_addr(guestname) - logger.info("get ip by mac address") - ip = util.mac_to_ip(mac, 180) - logger.info("the ip address of guest is %s" % ip) + if not("noping" in flags): + # Get domain ip + mac = util.get_dom_mac_addr(guestname) + logger.info("get ip by mac address") + ip = util.mac_to_ip(mac, 180) + logger.info("the ip address of guest is %s" % ip) # Destroy domain try: @@ -93,30 +98,31 @@ def destroy(params): except LibvirtAPI, e: logger.error("API error message: %s, error code is %s" % \ (e.response()['message'], e.response()['code'])) - logger.error("fail to destroy domain") + logger.error("failed to destroy domain") return 1 finally: conn.close() logger.info("closed hypervisor connection") # Check domain status by ping ip - while timeout: - time.sleep(10) - timeout -= 10 - logger.info(str(timeout) + "s left") - - logger.info('ping guest') - - if util.do_ping(ip, 30): - logger.error('The guest is still active, IP: ' + str(ip)) + if not "noping" in flags: + while timeout: + time.sleep(10) + timeout -= 10 + logger.info(str(timeout) + "s left") + + logger.info('ping guest') + + if util.do_ping(ip, 30): + logger.error('The guest is still active, IP: ' + str(ip)) + return 1 + else: + logger.info("domain %s was destroyed successfully" % guestname) + break + + if timeout <= 0: + logger.error("the domain couldn't be destroyed within 60 seconds.") return 1 - else: - logger.info("domain %s is destroied successfully" % guestname) - break - - if timeout <= 0: - logger.error("the domain couldn't be destroied within 60 secs.") - return 1 return 0 diff --git a/repos/domain/start.py b/repos/domain/start.py index 39ac47f..483ea7a 100644 --- a/repos/domain/start.py +++ b/repos/domain/start.py @@ -66,7 +66,7 @@ def start(params): logger -- an object of utils/Python/log.py mandatory arguments : guestname -- same as the domain name - optional arguments : flags -- domain create flags <none|start_paused> + optional arguments : flags -- domain create flags <none|start_paused|noping> Return 0 on SUCCESS or 1 on FAILURE """ @@ -75,13 +75,11 @@ def start(params): check_params(params) domname = params['guestname'] logger = params['logger'] + flags = params['flags'] - flags = None - if params.has_key('flags'): - flags = params['flags'] - if flags != 'none' and flags != 'start_paused': - logger.error("flags value either \"none\" or \"start_paused\""); - return 1 + if "none" in flags and "start_paused" in flags: + logger.error("Flags error: Can't specify none and start_paused simultaneously") + return (conn, logger, 1) # Connect to local hypervisor connection URI util = utils.Utils() @@ -95,21 +93,18 @@ def start(params): logger.info('start domain') try: - if flags == "none": + if "none" in flags: dom_obj.start_with_flags(domname, NONE) - elif flags == "start_paused": + elif "start_paused" in flags: dom_obj.start_with_flags(domname, START_PAUSED) - elif not flags: - dom_obj.start(domname) else: - logger.error("flags error") - return (conn, logger, 1) + dom_obj.start(domname) except LibvirtAPI, e: logger.error(str(e)) logger.error("start failed") return return_close(conn, logger, 1) - if flags == "start_paused": + if "start_paused" in flags: state = dom_obj.get_state(domname) if state == "paused": logger.info("guest start with state paused successfully") @@ -119,29 +114,32 @@ def start(params): return return_close(conn, logger, 1) while timeout: - time.sleep(10) - timeout -= 10 - logger.info(str(timeout) + "s left") - state = dom_obj.get_state(domname) expect_states = ['running', 'no state', 'blocked'] if state in expect_states: break + time.sleep(10) + timeout -= 10 + logger.info(str(timeout) + "s left") + if timeout <= 0: logger.error('The domain state is not as expected, state: ' + state) return return_close(conn, logger, 1) - # Get domain ip and ping ip to check domain's status - mac = util.get_dom_mac_addr(domname) - logger.info("get ip by mac address") - ip = util.mac_to_ip(mac, 180) + logger.info("Guest started") - logger.info('ping guest') - if not util.do_ping(ip, 300): - logger.error('Failed on ping guest, IP: ' + str(ip)) - return return_close(conn, logger, 1) + # Get domain ip and ping ip to check domain's status + if not "noping" in flags: + mac = util.get_dom_mac_addr(domname) + logger.info("get ip by mac address") + ip = util.mac_to_ip(mac, 180) + + logger.info('ping guest') + if not util.do_ping(ip, 300): + logger.error('Failed on ping guest, IP: ' + str(ip)) + return return_close(conn, logger, 1) logger.info("PASS") return return_close(conn, logger, 0) -- 1.7.3.4
On 03/21/2012 08:46 PM, Peter Krempa wrote:
For some tests it's not needed to ping the guest in the startup process. This patch adds a flag to the start and destroy test to skip such attempts (that consume a lot of time) --- repos/domain/destroy.py | 54 ++++++++++++++++++++++++++-------------------- repos/domain/start.py | 50 ++++++++++++++++++++---------------------- 2 files changed, 54 insertions(+), 50 deletions(-)
diff --git a/repos/domain/destroy.py b/repos/domain/destroy.py index f98b602..12399d6 100644 --- a/repos/domain/destroy.py +++ b/repos/domain/destroy.py @@ -50,7 +50,10 @@ def destroy(params): {'guestname': guestname}
logger -- an object of utils/Python/log.py - guestname -- same as the domain name + guestname -- the domain name + flags -- optional arguments: + noping: Don't do the ping test +
Return 0 on SUCCESS or 1 on FAILURE """ @@ -62,6 +65,7 @@ def destroy(params): if params_check_result: return 1 guestname = params['guestname'] + flags = params['flags']
The 'flags' is optional, then we have to check if the dictionary of params has key or not if params.has_key('flags'): ... otherwise, it will report "KeyError: " If 'flags' is mandatory, it'd better to to check it in check_params function.
# Connect to local hypervisor connection URI util = utils.Utils() @@ -73,18 +77,19 @@ def destroy(params): dom_obj = domainAPI.DomainAPI(virconn) dom_name_list = dom_obj.get_list() if guestname not in dom_name_list: - logger.error("guest %s doesn't exist or not be running." % guestname) + logger.error("guest %s doesn't exist or isn't running." % guestname)
Thanks
conn.close() logger.info("closed hypervisor connection") return 1 timeout = 60 logger.info('destroy domain')
- # Get domain ip - mac = util.get_dom_mac_addr(guestname) - logger.info("get ip by mac address") - ip = util.mac_to_ip(mac, 180) - logger.info("the ip address of guest is %s" % ip) + if not("noping" in flags): + # Get domain ip + mac = util.get_dom_mac_addr(guestname) + logger.info("get ip by mac address") + ip = util.mac_to_ip(mac, 180) + logger.info("the ip address of guest is %s" % ip)
# Destroy domain try: @@ -93,30 +98,31 @@ def destroy(params): except LibvirtAPI, e: logger.error("API error message: %s, error code is %s" % \ (e.response()['message'], e.response()['code'])) - logger.error("fail to destroy domain") + logger.error("failed to destroy domain")
thanks to correct it.
return 1 finally: conn.close() logger.info("closed hypervisor connection")
# Check domain status by ping ip - while timeout: - time.sleep(10) - timeout -= 10 - logger.info(str(timeout) + "s left") - - logger.info('ping guest') - - if util.do_ping(ip, 30): - logger.error('The guest is still active, IP: ' + str(ip)) + if not "noping" in flags: + while timeout: + time.sleep(10) + timeout -= 10 + logger.info(str(timeout) + "s left") + + logger.info('ping guest') + + if util.do_ping(ip, 30): + logger.error('The guest is still active, IP: ' + str(ip)) + return 1 + else: + logger.info("domain %s was destroyed successfully" % guestname) + break + + if timeout<= 0: + logger.error("the domain couldn't be destroyed within 60 seconds.") return 1 - else: - logger.info("domain %s is destroied successfully" % guestname) - break - - if timeout<= 0: - logger.error("the domain couldn't be destroied within 60 secs.") - return 1
return 0
diff --git a/repos/domain/start.py b/repos/domain/start.py index 39ac47f..483ea7a 100644 --- a/repos/domain/start.py +++ b/repos/domain/start.py @@ -66,7 +66,7 @@ def start(params):
logger -- an object of utils/Python/log.py mandatory arguments : guestname -- same as the domain name - optional arguments : flags -- domain create flags<none|start_paused> + optional arguments : flags -- domain create flags<none|start_paused|noping>
Return 0 on SUCCESS or 1 on FAILURE """ @@ -75,13 +75,11 @@ def start(params): check_params(params) domname = params['guestname'] logger = params['logger'] + flags = params['flags']
Yes, please notice this like above. It seems that you want flags to be mandatory in the following code. Add checking for it in check_params() Guannan Ren
On 03/21/2012 06:13 PM, Guannan Ren wrote:
On 03/21/2012 08:46 PM, Peter Krempa wrote:
For some tests it's not needed to ping the guest in the startup process. This patch adds a flag to the start and destroy test to skip such attempts (that consume a lot of time) --- repos/domain/destroy.py | 54 ++++++++++++++++++++++++++-------------------- repos/domain/start.py | 50 ++++++++++++++++++++---------------------- 2 files changed, 54 insertions(+), 50 deletions(-)
diff --git a/repos/domain/destroy.py b/repos/domain/destroy.py index f98b602..12399d6 100644 --- a/repos/domain/destroy.py +++ b/repos/domain/destroy.py @@ -50,7 +50,10 @@ def destroy(params): {'guestname': guestname}
logger -- an object of utils/Python/log.py - guestname -- same as the domain name + guestname -- the domain name + flags -- optional arguments: + noping: Don't do the ping test +
Return 0 on SUCCESS or 1 on FAILURE """ @@ -62,6 +65,7 @@ def destroy(params): if params_check_result: return 1 guestname = params['guestname'] + flags = params['flags']
The 'flags' is optional, then we have to check if the dictionary of params has key or not if params.has_key('flags'): ... otherwise, it will report "KeyError: "
If 'flags' is mandatory, it'd better to to check it in check_params function.
I'd rather do it using the get() method for dictionaries with some default, i.e. params.get('flags', None). Just my $0.02 Martin
On 03/22/2012 03:02 PM, Martin Kletzander wrote:
I'd rather do it using the get() method for dictionaries with some default, i.e. params.get('flags', None).
Just my $0.02 Martin
Thanks, This belongs to enhancement work, let us do it a little later. Probably, we need a cleanup patch for these.
On 03/21/2012 01:46 PM, Peter Krempa wrote:
This is a set of more or less independent fixes and improvements to the test API. I ran across these while trying to write a basic test case as a "Hello world!" to the test-API.
Improvements are in fields of cross-distro compatibility, broken API's and typos and usability.
Peter Krempa (7): utils: Make ipget.sh more portable lib: fix streamAPI class domainAPI: Add wrapper method to work with domain's console connections repos/domain/create.py: Fix typo in path to python executable utils: Allow suffixes in path to the main checkout folder parser: Be more specific on mistakes in case files domain/[start|destroy]: Add a optional noping flag to skip the ping test
exception.py | 10 +++++++++ lib/domainAPI.py | 9 ++++++++ lib/streamAPI.py | 52 +++++++++++++++++++++++----------------------- proxy.py | 4 +++ repos/domain/create.py | 2 +- repos/domain/destroy.py | 50 +++++++++++++++++++++++++------------------- repos/domain/start.py | 50 +++++++++++++++++++++----------------------- utils/Python/utils.py | 2 +- utils/ipget.sh | 4 +- 9 files changed, 105 insertions(+), 78 deletions(-)
I don't know if my ACK would mean something, it should be probably checked by someone else, too. There's no need for parentheses around the second '.*' block in 5/7 and you have different types of quotes and string formatting in 6/7 and 7/7, but I feel there is bigger need for this to work then spending huge amount of time creating some python coding style, so (probably meaningless) ACK from me for the whole series.
On 03/21/2012 08:46 PM, Peter Krempa wrote:
This is a set of more or less independent fixes and improvements to the test API. I ran across these while trying to write a basic test case as a "Hello world!" to the test-API.
Improvements are in fields of cross-distro compatibility, broken API's and typos and usability.
Peter Krempa (7): utils: Make ipget.sh more portable lib: fix streamAPI class domainAPI: Add wrapper method to work with domain's console connections repos/domain/create.py: Fix typo in path to python executable utils: Allow suffixes in path to the main checkout folder parser: Be more specific on mistakes in case files domain/[start|destroy]: Add a optional noping flag to skip the ping test
exception.py | 10 +++++++++ lib/domainAPI.py | 9 ++++++++ lib/streamAPI.py | 52 +++++++++++++++++++++++----------------------- proxy.py | 4 +++ repos/domain/create.py | 2 +- repos/domain/destroy.py | 50 +++++++++++++++++++++++++------------------- repos/domain/start.py | 50 +++++++++++++++++++++----------------------- utils/Python/utils.py | 2 +- utils/ipget.sh | 4 +- 9 files changed, 105 insertions(+), 78 deletions(-)
Hi Peter I have pushed the set of patches after some small modifications based on my comments. If you have any comment, please be free to tell me. Thanks your patches. Guannan Ren
On 03/22/2012 08:54 AM, Guannan Ren wrote:
On 03/21/2012 08:46 PM, Peter Krempa wrote:
This is a set of more or less independent fixes and improvements to the test API. I ran across these while trying to write a basic test case as a "Hello world!" to the test-API.
Improvements are in fields of cross-distro compatibility, broken API's and typos and usability.
Peter Krempa (7): utils: Make ipget.sh more portable lib: fix streamAPI class domainAPI: Add wrapper method to work with domain's console connections repos/domain/create.py: Fix typo in path to python executable utils: Allow suffixes in path to the main checkout folder parser: Be more specific on mistakes in case files domain/[start|destroy]: Add a optional noping flag to skip the ping test
exception.py | 10 +++++++++ lib/domainAPI.py | 9 ++++++++ lib/streamAPI.py | 52 +++++++++++++++++++++++----------------------- proxy.py | 4 +++ repos/domain/create.py | 2 +- repos/domain/destroy.py | 50 +++++++++++++++++++++++++------------------- repos/domain/start.py | 50 +++++++++++++++++++++----------------------- utils/Python/utils.py | 2 +- utils/ipget.sh | 4 +- 9 files changed, 105 insertions(+), 78 deletions(-)
Hi Peter
I have pushed the set of patches after some small modifications based on my comments. If you have any comment, please be free to tell me. Thanks your patches.
Thanks for the review,pointing out some mistakes (I'm a python beginner), and pushing the series. Peter
Guannan Ren
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (5)
-
Eric Blake -
Guannan Ren -
Martin Kletzander -
Osier Yang -
Peter Krempa