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

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.

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