deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH] DTACLOUD-462 - a GET for an instance can fail while a different instance is being destroyed.
Date Fri, 15 Feb 2013 22:51:04 GMT
On Fri, 2013-02-15 at 15:43 -0500, jvlcek@redhat.com wrote:
> From: Joe VLcek <jvlcek@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



Mime
View raw message