[libvirt] [test-API PATCH 5/6] Cleanup and fix of domain/define test

[Martin Kletzander]

On 03/27/2012 10:59 AM, Guannan Ren wrote:
> On 03/27/2012 03:57 PM, Martin Kletzander wrote:
>>>           It's better not to use libvirt API to check the result of one
>>> another API.
>>>           We should use native approach to do the checking. So I insist
>>> on the original checking.
>>>
>>
>> There was no lookupByName function, but I agree it's better to use the
>> same approach, so I'll add this method to the connection API.
>> One more question, whilst on this subject, though. I still didn't get
>> why we encapsulate libvirt API into one more class where we don't make
>> use of any persistence, inheritance nor any other OOP approach. It would
>> help me to understand so I don't make future mistakes.
> 
>         I think you don't need to add  lookupByName() in connectAPI.py
>     The APIs is domain related, so we suggest to make use of it in
> domainAPI.py
>     only. The ideas is that all of hypervisor related APIs go to
> connectAPI.py.
>         The relationship between classes domain, network, storage,
> nodedev is
>     loose. If we had a virtnetwork subclass, It would be better to make
> virtnetwork inherite
>     network for better OOP.
>         The encapsulated API files in lib directory is easy to manage
> and use. For example
>     If we want to write a domain testcases, we probably don't need to
> import network
>     and storage module in lib.
>

We both probably talk about something else, let me clarify.

My apologies first, because I misunderstood that you wanted to use the
native approach. I thought you meant implementing the function in
connectAPI whether you meant checking on the machine without using
libvirt. That said, there is no point in talking about lookupByName
implementation for now.

About the actual checking if the domain was created. Leaving it as it is
now, any misconfiguration will make this check fail (meaning the
configuration of sshd, libvirt, etc.). It will work for now as we are
the only ones using that right now (I guess), I'm just trying to think
ahead and I don't see that big problem with checking the domain being
created using libvirt again, but that's just my opinion.

About the re-implementation of the API, I was just looking into the
connectAPI class for now, but what I see there is only constructor that
saves the connection from libvirt and then for each method of libvirts
connection, there is connectAPI method that does the same, just using
different method names (and raising different exception). Unfortunately
these aren't even consistent. For example:

libvirts openAuth is encapsulated as openAuth
libvirts isEncrypted is encapsulated as isEncrypted

but

libvirts getCapabilities is encapsulated as get_caps
libvirts getHostname is encapsulated as get_host_name

and so on.

Don't get me wrong, I'm not trying to complain or anything, I'm just
trying to understand because for me it doesn't make any sense to use
this class.

I missed few other answers in my previous mail, so just to inform you
that I acknowledge them:

>            try ... except ...finally is new syntax since 2.5,
>            But we need to support 2.4, so should use
>            try:
>                 try:
>                 except:
>            finally
> 

I didn't know that, good point, Python 2.4 is still used somewhere probably.

>>
>> -    if target_machine:
>> -        REMOVE_SSH = "ssh %s \"rm -rf /root/.ssh/*\"" % (target_machine)
>> -        logger.info("remove ssh key on remote machine")
>> -        status, ret = util.exec_cmd(REMOVE_SSH, shell=True)
>> -        if status:
>> -            logger.error("failed to remove ssh key")
>> -
>> -        REMOVE_LOCAL_SSH = "rm -rf /root/.ssh/*"
> 
>                   Never remove or change the local ssh directory like this.
>                   The autotest have ssh configuration file stored here.
> 

This is a code that what was already in the test before, however I
probably copy-pasted it into the utils as it is. Better approach would
definitely be generating the keys somewhere, then pasting them into ssh
parameter '-i' and backing up the keys on the remote, while putting them
back after the test. However the _best_ option in this case is not using
keys (we have to use pexpect and connect there with password anyway) at all.

> 
>> -        logger.info("remove local ssh key")
>> -        status, ret = util.exec_cmd(REMOVE_LOCAL_SSH, shell=True)
>> -        if status:
>> -            logger.error("failed to remove local ssh key")
>> -
>>       if test_result:
>>           return 0
>>       else:
>> diff --git a/utils/Python/utils.py b/utils/Python/utils.py
>> index 55c1cb5..7382abb 100644
>> --- a/utils/Python/utils.py
>> +++ b/utils/Python/utils.py
>> @@ -1,6 +1,6 @@
>>   #!/usr/bin/env python
>>   #
>> -# libvirt-test-API is copyright 2010 Red Hat, Inc.
>> +# libvirt-test-API is copyright 2010, 2012 Red Hat, Inc.
>>   #
>>   # libvirt-test-API is free software: you can redistribute it and/or
>> modify it
>>   # under the terms of the GNU General Public License as published by
>> @@ -433,3 +433,46 @@ class Utils(object):
>>                   return 1
>>
>>           return 0
>> +
>> +
>> +    def ssh_keygen(logger):
> 
>           I agree with this, the migrate.py use this too.
>           could you help clean migrate.py along with this together?
>           If we put this in utils.py,  It's better not to accept
> "logger" argument just like other utilities do
> 
>           If we create a ssh-keygen and ssh_tunnel as a standalone
> testcase. we will use the connect
>           for all of following testcases rather than setup a ssh
> connection in each case. This will be good.
> 

If we agree on need for this, then we can do that, however since we got
into the deep conversation how to do what, I'd rather wait till more
things are decided.

Thanks for the patience,
Martin