Hi everyone, I was looking through the clustering code today and noticed a lot of it is grabbing what I'd call the guts of the instance models code.
The best example is here: https://github.com/openstack/trove/commit/06196fcf67b27f0308381da192da5cc8ae65b157#diff-a4d09d28bd2b650c2327f5d8d81be3a9R89https://github.com/openstack/trove/commit/06196fcf67b27f0308381da192da5cc8ae65b157#diff-a4d09d28bd2b650c2327f5d8d81be3a9R89 In the "_all_instances_ready" function, I would have expected trove.instance.models.load_any_instance to be called for each instance ID and it's status to be checked. Instead, the service_status is being called directly. That is a big mistake. For now it works, but in general it mixes the concern of "what is an instance stauts?" to code outside of the instance class itself. For an example of why this is bad, look at the method "_instance_ids_with_failures." The code is checking for failures by seeing if the service status is failed. What if the Nova server or Cinder volume have tanked instead? The code won't work as expected. It could be we need to introduce another status besides BUILD to instance statuses, or we need to introduce a new internal property to the SimpleInstance base class we can check. But whatever we do we should add this extra logic to the instance class itself rather than put it in the clustering models code. This is a minor nitpick but I think we should fix it before too much time passes. Thanks, Tim
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
