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: 

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 


OpenStack-dev mailing list

Reply via email to