Yelp/Tron — pull request #263 by Codeacious (2013-08-02 17:42:24+00:00)
Title: Made service reconfiguration slightly better

This is a first pass at fixing #228.

The idea here is that we don't want to cause a service to be completely disabled or recreated under certain conditions. However, because of how Twisted SSH classes work, and how the current Node class is factored, reconfiguration of a node will create an entirely new NodePool and Node objects. This, of course, often means the old Node instance is defunct, and with it any ActionCommands that were executing on it.

Unfortunately, this means that ServiceInstances running on these old instances of Node have to be restarted, even if the Node changed something that the Service may not really care about (like its public key, which is only used by the Node for the Twisted SSH connection). Right now, though, everything about a service is completely disabled when this happens, as the entire Service is recreated.

This code aims to make that process at least slightly better; now, `ServiceCollection.load_from_config` will call a more complex `_build` method that attempts to compare the service configurations prior to actually making a new Service object (configs are compared based on service name). This includes a new step of attempting to update the Service's instance count (`config.count`) and THEN comparing the two configurations to see if they're equal. If the configurations are still not equal, we create a new Service as normal.

If the two configurations are found to be the same, we call a new `update_node_pool` method on the Service. This, in turn, calls `update_node_pool` and `clear_extra` on the Service's ServiceInstanceCollection, and `self.repair()` if the Service is enabled.

The `update_node_pool` function in ServceInstanceCollection attempts to load a new NodePool object from the NodePoolRepository using the same name as it did when it was instantiated (which should be the same after reconfiguration, as this is only called if the configs are the same). From there, if the NodePools are equal, then we return and do nothing.

If the retrieved NodePool is different, then the function goes through each of its ServiceInstances and attempts to load the Node that it's operating on from the NodePool via `node_pool.get_by_name(instance.node.name)`. If the returned Node is None or not equal to the ServiceInstance's Node, then we `stop` the instance and remove it from the instances list stored by the ServiceInstanceCollection. If the Service is enabled, the call to `repair` in the Service will recreate these instances with the new Node objects.

The `clear_extra` function is simply compensation for when the number of instances is reduced; it pops instances off of the collection until `len(instances)` reaches equality with `config.count`.

This will fix most of #228, but it's still not a great or complete solution. The Node and NodePool classes need to be refactored in such a way that updating their configuration doesn't force them to create entirely new object instances every time, as this results in every Job and Service runs having to be restarted in order to use and resubmit their ActionRuns/Commands to the new Node object.