[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