I agree with your suggestion to stop hitting the service_statuses table directly and instead hit the instance model. But now I have an observation:
Nova is already being called here as part of the polling done by FreshInstanceTasks#create_instance (https://github.com/openstack/trove/blob/06196fcf67b27f0308381da192da5cc8ae65b157/trove/taskmanager/models.py#L413) and I think that a call to the instance model (which would hit Nova also) will double up on those Nova calls. So is the implication that we cannot re-use create_instance verbatim and rather we should (1) use instance.status in ClusterTasks#create_cluster and (2) modify FreshInstanceTasks#create_instance it doesn't poll the instance when cluster_config is not None? "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." I don't understand the above. What does the new status give you? From: Tim Simpson <tim.simp...@rackspace.com<mailto:tim.simp...@rackspace.com>> Reply-To: "OpenStack Development Mailing List (not for usage questions)" <openstack-dev@lists.openstack.org<mailto:openstack-dev@lists.openstack.org>> Date: Thursday, September 11, 2014 at 12:52 PM To: "OpenStack Development Mailing List (not for usage questions) ?[openstack-dev@lists.openstack.org<mailto:openstack-dev@lists.openstack.org>]?" <openstack-dev@lists.openstack.org<mailto:openstack-dev@lists.openstack.org>> Cc: "eBay SF, mlowery" <mlow...@ebaysf.com<mailto:mlow...@ebaysf.com>>, "McReynolds, Auston" <amcreyno...@ebay.com<mailto:amcreyno...@ebay.com>> Subject: [openstack-dev] [Trove] Cluster implementation is grabbing instance's gutsHi guys, 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:/ 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
