On Fri, 2013-02-15 at 15:43 -0500, jvl...@redhat.com wrote:
> From: Joe VLcek <jvl...@redhat.com>
ACK with a couple of comments:
I would rewrite the commit message in the following way
RHEV-M: avoid failure of GET instance details
A backtrace was sporadically produced for a
GET /api/instance/<instance id>
When one instance is being destroyed during a GET for
another instance, a backtrace is produced. This is because
a query for all instances (vms) was being done. The list of
all instances is gathered, one instance is destroyed, then the
list contained an invalid instance ID which when queried
produced
"Entity not found: id "
Fixes DTACLOUD-462
The important things about the commit message:
* the bug being fixed doesn't need to go into the subject line,
it's preferrable to have it at the end of the body of the commit
message (and including a full URL there might be marginally more
convenient)
* include a full description of the problem in the commit message;
this will make it much easier for somebody coming back to this
change in a few months to remember why it was made (you might
recognize the text from your mail accompanying the patch ;)
* include the driver name in the subject line if the change
affects only one driver as that makes it easier to scan the
commit log
What happens when somebody requests /api/instances while an instance is
being destroyed ? The fix you have for the single-instance case is spot
on, but we must also guard against tripping over this when listing all
instances.
> diff --git a/server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb
> b/server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb
> index 65ba26d..9212c26 100644
> --- a/server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb
> +++ b/server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb
> @@ -286,7 +287,7 @@ class RhevmDriver < Deltacloud::BaseDriver
> # UNASSIGNED, DOWN, UP, POWERING_UP, POWERED_DOWN, PAUSED, MIGRATING_FROM,
> # MIGRATING_TO, UNKNOWN, NOT_RESPONDING, WAIT_FOR_LAUNCH,
> REBOOT_IN_PROGRESS,
> # SAVING_STATE, RESTORING_STATE, SUSPENDED, IMAGE_ILLEGAL,
> - # IMAGE_LOCKED or POWERING_DOWN
> + # IMAGE_LOCKED, MIGRATING or POWERING_DOWN
> #
> def convert_state(state)
> unless state.respond_to?(:upcase)
> @@ -295,7 +296,7 @@ class RhevmDriver < Deltacloud::BaseDriver
> state = state.gsub('\\', '').strip.upcase
> return 'PENDING' if ['WAIT_FOR_LAUNCH', 'REBOOT_IN_PROGRESS',
> 'SAVING_STATE',
> 'RESTORING_STATE', 'POWERING_UP', 'IMAGE_LOCKED',
> 'SAVING_STATE'].include? state
> - return 'STOPPING' if state == 'POWERING_DOWN'
> + return 'STOPPING' if ['POWERING_DOWN', 'MIGRATING'].include? state
> return 'STOPPED' if ['UNASSIGNED', 'DOWN', 'NOT_RESPONDING',
> 'IMAGE_ILLEGAL', 'UNKNOWN'].include? state
> return 'RUNNING' if ['UP', 'MIGRATING_TO', 'MIGRATING_FROM'].include?
> state
It might be time to switch convert_state into something a bit more data
driven by putting the state mapping into a hash:
STATE_MAPPING = {
'WAIT_FOR_LAUNCH' => 'PENDING',
'REBOOT_IN_PROGRESS' => 'PENDING',
...
}
def convert_state(state)
... massage state ...
s = STATE_MAPPING[state]
raise "Unexpected state '#{state}'" unless s
s
end
Also, there is a list of all the states we should be reporting in
base_driver.rb in STATE_MACHINE_OPTS[:all_states] - I fear our drivers
diverge from that quite a bit. Needs some cleanup across all drivers.
David
