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

Guannan Ren gren at redhat.com
Tue Mar 27 10:35:37 UTC 2012


On 03/27/2012 05:42 PM, Martin Kletzander wrote:
> 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.

       Ok, let me keep it temporarily, because it doesn't affect testing.
     After some practice,  it proves to be useless,  we can remove them 
anytime.
     Is that okay?

>
> 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.

         Yep,  I just want to give a hint about that autotest view ssh 
as critical thing.
         It's my hint. no comment on you.
         I agree with your ideas.

>
> 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




More information about the libvir-list mailing list