Code review comment for lp:~rackspace-titan/nova/instance_states

Revision history for this message
Brian Lamar (blamar) wrote :

> 16: I think you might want to expand this comment to explain yourself better
> better

Updated with link to EC2 spec, does that help?

>
> 53/54: Should this be 'stopped' and 'terminated'? According to the allowed EC2
> values, these don't seem correct.

Fixed. They weren't like that before, but you're suggestion seems logical.

>
> 189: Can you log the output of the command you allude to?

Fixed to output status.

> 955/990: You should probably file a bug for this, seems like a simple cleanup
> item

Not sure it's a 'bug', but I created a BP https://blueprints.launchpad.net/nova/+spec/remove-virt-driver-callbacks

>
> 1224: Thank you for cleaning this function up.

Thanks, I really want to highlight this because I don't want anyone surprised at the difference in functionality. All we're doing in the interval tasks right now is sync'ing power states. All actual VM state transition should be done through explicit database updates in the compute API/manager and NOT through this method.

>
> 1335/1390: I would love to expand on these module-level docstrings. I think
> adding a bit more context/explanation of what the task/vm_states represent
> would be very helpful.

Added some details. Do these help?

>
> 1458/1471: Not sure what these comments mean...

Whoops. Those methods shouldn't have still been there. Removed those.

« Back to merge proposal