Merge lp:~javier.collado/utah/sshmixin_destroy_fixes into lp:utah

[Javier Collado]

Fixes to make SSHMixin.destroy work.

When running some tests I got this error message:
Failed to destroy machine: Method destroy not defined for machine type CustomVM

This is a misleading error because CustomVM has a destroy method already. However, since it calls `super` there's a whole chain to take into account. According to the MRO this would be as follows:
(utah.provisioning.vm.libvirtvm.CustomVM,
 utah.provisioning.provisioning.CustomInstallMixin,
 utah.provisioning.provisioning.SSHMixin,
 utah.provisioning.vm.libvirtvm.LibvirtVM,
 utah.provisioning.vm.vm.VM,
 utah.provisioning.provisioning.Machine,
 object)

The problem happened in SSHMixin when calling `super` and that when through `LibvirtVM`, `VM` and, finally, `Machine` which is the method that raised the error. I already fixed it with an empty implementation in `VM`, but I still don't like this solution, so I'd like to further discuss this to know what do you think about this.

In my opinion, here there are two problems:
- The error is misleading because it doesn't really points out what's the class that is causing the problem
- Mixins shouldn't be involved in all this chained `super` calls.

My proposal would be to change the inheritance order. For example from this:
class CustomVM(CustomInstallMixin, SSHMixin, LibvirtVM)

to this:
class CustomVM(LibvirtVM, CustomInstallMixin, SSHMixin)

to keep MRO use classes in the hierarchy tree (hopefully single inheritance is still used and there's always a final class that doesn't call `super` to avoid calling the mixin methods).

Also if some mixin code has to be called, then it should be explicitly called like:
super(LibvirtVM, self).destroy(self) # Destroy calling parent class methods
CustomInstallMixin.destroy(self) # Destroy explicitly calling mixin methods
SSHMixin.destroy(self)

[Max Brustkern (nuclearbob)]

When I originally implemented the exception-raising destroy in Machine, I wasn't thinking that destroy would be something we'd need much inheritance on. I think moving that to an empty implementation in Machine (or one that just returns False) might be a good way to go.

As far as the exceptions go, I didn't really take inheritance into account well when setting up all those methods. Perhaps a clearer exception would be something like "{class} has called the {method} method of the base Machine class, which is not implemented."

I think changing the inheritance order is going to cause problems with all those exception-creating base methods. We can remove those, but if we do that, I'd like to see some way of reporting that we've called a method that a Machine does not implement that's more readable than the default python exception. Maybe we just need to be catching that and raising something else.

Finally, a note on the config file. Anything that uses a directory or information on the system on which the package is installed currently needs to go through a special routine to prevent the config info from being written when the package is built. It's this function that does that:
def dumpdefaults(build=False):
    if build:
        del DEFAULTS['bridge']
        del DEFAULTS['logfile']
        del DEFAULTS['sshprivatekey']
        del DEFAULTS['sshpublickey']
        del DEFAULTS['user']
        del DEFAULTS['vmpath']
    return json.dumps(DEFAULTS, sort_keys=True, indent=4)
The reason I do that is so that we can ship a config file that's based on config.py, but still goes in /etc/utah and takes advantage of the debian packaging facilities for tracking user changes to config files. This may not be the best approach in the long run. If you've got ideas on that, we can discuss them. For now I'm going to make the system-specific options a separate dict which gets added the defaults, and then we can remove it if we need to.

review: Approve

[Javier Collado (javier.collado)]

Thanks for the explanation. I see that the output of running utah config is precisely what is stored in /etc/utah/config. The separation of data that can be hardcoded and data that is programmatically generated (mainly through os.path.expanduser) looks fine to me. Anyway, I still don't see what kind of user changes to config files are being tracked.

Regarding inheritance and the implementation of methods in different classes, maybe it should be clarified in some way what are the expectations for each class (being a mixin or not), since it's difficult to know when a class should really implement a method or not.

[Max Brustkern (nuclearbob)]

The change tracking for the config file is all handled by the package system. At some point we may need to move away from that for flexibility, but for now I think it's useful.

I think clarifying the expectations for class implementation of methods is a good idea. The current model was developed before I started making mixin classes, and I think that's confused things somewhat. If you have any proposals or you want to just submit some changes to the documentation, go for it.

=== modified file 'utah/config.py'
--- utah/config.py	2012-07-19 20:00:02 +0000
+++ utah/config.py	2012-07-20 11:37:44 +0000
@@ -39,6 +39,7 @@
     series='precise',
     sshprivatekey=os.path.join(os.path.expanduser('~'), '.ssh', 'utah'),
     sshpublickey=os.path.join(os.path.expanduser('~'), '.ssh', 'utah.pub'),
+    sshknownhosts=os.path.join(os.path.expanduser('~'), '.ssh', 'known_hosts'),
     template=None,
     user=getpass.getuser(),
     vmpath=os.path.join(os.path.expanduser('~'), 'vm'),
@@ -64,6 +65,7 @@
         del DEFAULTS['logfile']
         del DEFAULTS['sshprivatekey']
         del DEFAULTS['sshpublickey']
+        del DEFAULTS['sshknownhosts']
         del DEFAULTS['user']
         del DEFAULTS['vmpath']
     return json.dumps(DEFAULTS, sort_keys=True, indent=4)

=== modified file 'utah/provisioning/provisioning.py'
--- utah/provisioning/provisioning.py	2012-07-19 22:04:49 +0000
+++ utah/provisioning/provisioning.py	2012-07-20 11:37:44 +0000
@@ -426,13 +426,16 @@
             else:
                 raise err
 
-        # host_keys doesn't support __delitem__, so pop doesn't work either.
-        # TODO: fix this?
-        #host_keys = self.ssh_client.get_host_keys()
-        #for address in addresses:
-        #    self.logger.debug('Removing ' + address + ' from known_hosts')
-        #    del host_keys[address]
-        #self.ssh_client.save_host_keys()
+        old_host_keys = self.ssh_client.get_host_keys()
+        new_host_keys = paramiko.HostKeys()
+        addresses = set(addresses)
+        for address, key in old_host_keys.iteritems():
+            # Skip keys so that they don't get added
+            # into the new keys (i.e. they're removed)
+            if address in addresses:
+                continue
+            new_host_keys[address] = key
+        new_host_keys.save(config.sshknownhosts)
         self.ssh_client.close()
 
         super(SSHMixin, self).destroy(*args, **kw)

=== modified file 'utah/provisioning/vm/vm.py'
--- utah/provisioning/vm/vm.py	2012-07-18 11:07:33 +0000
+++ utah/provisioning/vm/vm.py	2012-07-20 11:37:44 +0000
@@ -12,3 +12,6 @@
     def __init__(self, *args, **kw):
         super(VM, self).__init__(*args, **kw)
         self.logger.debug('VM init finished')
+
+    def destroy(self, *args, **kwargs):
+        pass