<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; color: rgb(0, 0, 0); font-size: 14px; font-family: Calibri, sans-serif;">
<div style="color: rgb(0, 0, 0);">
<div style="font-family: Calibri, sans-serif; font-size: 14px;">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:</div>
<div style="font-family: Calibri, sans-serif; font-size: 14px;"><br>
</div>
<div style="font-family: Calibri, sans-serif; font-size: 14px;">Nova is already being called here as part of the polling done by FreshInstanceTasks#create_instance (<a href="https://github.com/openstack/trove/blob/06196fcf67b27f0308381da192da5cc8ae65b157/trove/taskmanager/models.py#L413">https://github.com/openstack/trove/blob/06196fcf67b27f0308381da192da5cc8ae65b157/trove/taskmanager/models.py#L413</a>)
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?</div>
</div>
<div style="color: rgb(0, 0, 0);"><br>
</div>
<div style="color: rgb(0, 0, 0);">"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."</div>
<div style="color: rgb(0, 0, 0);"><br>
</div>
<div>I<span style="color: rgb(0, 0, 0);"> don't understand the above. What does the new status give you?</span></div>
<div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px;">
<br>
</div>
<span id="OLK_SRC_BODY_SECTION" style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px;">
<div style="font-family:Calibri; font-size:11pt; text-align:left; color:black; BORDER-BOTTOM: medium none; BORDER-LEFT: medium none; PADDING-BOTTOM: 0in; PADDING-LEFT: 0in; PADDING-RIGHT: 0in; BORDER-TOP: #b5c4df 1pt solid; BORDER-RIGHT: medium none; PADDING-TOP: 3pt">
<span style="font-weight:bold">From: </span>Tim Simpson <<a href="mailto:tim.simpson@rackspace.com">tim.simpson@rackspace.com</a>><br>
<span style="font-weight:bold">Reply-To: </span>"OpenStack Development Mailing List (not for usage questions)" <<a href="mailto:openstack-dev@lists.openstack.org">openstack-dev@lists.openstack.org</a>><br>
<span style="font-weight:bold">Date: </span>Thursday, September 11, 2014 at 12:52 PM<br>
<span style="font-weight:bold">To: </span>"OpenStack Development Mailing List (not for usage questions) ?[<a href="mailto:openstack-dev@lists.openstack.org">openstack-dev@lists.openstack.org</a>]?" <<a href="mailto:openstack-dev@lists.openstack.org">openstack-dev@lists.openstack.org</a>><br>
<span style="font-weight:bold">Cc: </span>"eBay SF, mlowery" <<a href="mailto:mlowery@ebaysf.com">mlowery@ebaysf.com</a>>, "McReynolds, Auston" <<a href="mailto:amcreynolds@ebay.com">amcreynolds@ebay.com</a>><br>
<span style="font-weight:bold">Subject: </span>[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:/<br>
</div>
<div><br>
</div>
<div dir="ltr"><style type="text/css" id="owaParaStyle"></style>
<div fpstyle="1" ocsi="0">
<div style="direction: ltr;font-family: Tahoma;color: #000000;font-size: 10pt;">Hi everyone,
<div><br>
</div>
<div>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. </div>
<div><br>
</div>
<div>The best example is here: <a href="https://github.com/openstack/trove/commit/06196fcf67b27f0308381da192da5cc8ae65b157#diff-a4d09d28bd2b650c2327f5d8d81be3a9R89">https://github.com/openstack/trove/commit/06196fcf67b27f0308381da192da5cc8ae65b157#diff-a4d09d28bd2b650c2327f5d8d81be3a9R89</a><a href="https://github.com/openstack/trove/commit/06196fcf67b27f0308381da192da5cc8ae65b157#diff-a4d09d28bd2b650c2327f5d8d81be3a9R89" target="_blank" style="font-size: 10pt;">https://github.com/openstack/trove/commit/06196fcf67b27f0308381da192da5cc8ae65b157#diff-a4d09d28bd2b650c2327f5d8d81be3a9R89</a></div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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. </div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>This is a minor nitpick but I think we should fix it before too much time passes.</div>
<div><br>
</div>
<div>Thanks,</div>
<div><br>
</div>
<div>Tim</div>
</div>
</div>
</div>
</span>
</body>
</html>