[PATCH conductor 0/3] Two related BZs -- 795794, 795891

Tomáš Hrčka thrcka at redhat.com
Wed Feb 22 17:07:55 UTC 2012


On 02/21/2012 10:08 PM, Matt Wagner wrote:
> Hi all,
>
> This is a trio of inter-related patches that do a handful of different things. They all started from https://bugzilla.redhat.com/show_bug.cgi?id=795794 which led me to conclude that there were two separate bugs.
>
> tl;dr -- The patches for different BZs are technically separate, but are related. The "BZ #795794 - Don't delete :vanished instances" patch could be omitted if required, though obviously I suggest using it.
>
> == BZ #795794 - Display a saner error message if launch fails ==
>
> This fixes the simplest possible form of the bug -- the error message generated when you attempt to launch a deployment for which the image has been deleted is a huge blob of nonsense. You get one line which tells you the actual error -- the assembly could not be launched because the image is missing. Then there is a newline and the massive blob of what almost appears to be the request object itself. It includes things like your AWS account ID. So I simply chop off anything after the first line.
>
> == BZ #795891 - Deployment should not show "running" with no instances ==
>
> This fixes another bug that relates to the mess that is #795794. If an instances goes into "vanished" state and then gets destroyed, we would show the deployment as running, even though it had no instances. This was a terrible mess, but turned out to have an easy fix -- "instances.all?" with a block will always return true if instances is an empty array. So I just checked that the instance existed first. (The "instances.empty?" conditional I added to the next block doesn't actually change any behavior -- the existing code would have implicitly matched that case anyway. But it makes it more obvious that we have considered that case.)
>
> I added the Instance::FAILED_STATES array after realizing that I was going to introduce a little bit of duplication.
>
> I considered that we might want to delete the whole deployment if we deleted the last instance because it had gone messing, but didn't like this idea, which leads to my third and final patch:
>
> == BZ #795794 - Don't delete :vanished instances ==
>
> This fixes something I argued for, but now think was a bad idea. I had proposed that we should delete instances when they disappeared on the backend provider as an interim solution until next sprint, when we could identify how to elegantly style them to indicate an error. However, this turned into a huge mess. For one, it's not safe -- if an instances temporarily stops being advertised by a provider for some reason, we will trash it and never add it back. The user would have no idea that the instance was still running and costing them money. Second, when we delete the instance, the associated event that we created gets destroyed. Ultimately, what pushed me over the edge was that I wanted to also delete the deployment to avoid leaving dangling garbage in the app, but this just felt like a bad idea, and it made me realize that the whole thing was a terrible idea on my part. With this patch, when an instance vanishes from the cloud provider, we set its state to "Vanished" and f!
>   ire an event. If it comes back, dbomatic will recover gracefully. If not, it will just stay in vanished state. This is stretching the BZ a little bit, but I think it's ultimately the best solution.
>
> Best,
> Matt
ACK. To the patch set just fix dbomatic because there is one end more 
that should be.




More information about the aeolus-devel mailing list