[Ovirt-devel] [PATCH server] Fix VM shutdown

[Hugh O. Brock]

On Wed, Mar 25, 2009 at 12:01:44PM -0700, Ian Main wrote:
> This patch fixes VM shutdown.  Previously when shutting down a vm we
> just set it to 'stopped'.  However, this is not always the case as it's
> possible for the guest OS to ignore the shutdown event.  Also it is
> possible for a VM to be stopped by the user in which case it would be
> nice to set it stopped in the DB and clear the flags.
> 
> Signed-off-by: Ian Main <imain at redhat.com>
> ---
>  src/db-omatic/db_omatic.rb    |   20 +++++++++++++++++++-
>  src/task-omatic/taskomatic.rb |   25 +++++++++++++++++++------
>  2 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/src/db-omatic/db_omatic.rb b/src/db-omatic/db_omatic.rb
> index 8409c91..7ae08da 100755
> --- a/src/db-omatic/db_omatic.rb
> +++ b/src/db-omatic/db_omatic.rb
> @@ -181,8 +181,26 @@ class DbOmatic < Qpid::Qmf::Console
>          end
>  
>          @logger.info "Updating VM #{domain['name']} to state #{state}"
> +
> +        if state == Vm::STATE_STOPPED
> +            @logger.info "VM has moved to stopped, clearing VM attributes."
> +            qmf_vm = @session.object(:class => "domain", 'uuid' => vm.uuid)
> +            if qmf_vm
> +                @logger.info "Deleting VM #{vm.description}."
> +                result = qmf_vm.undefine
> +                if result.status == 0
> +                    @logger.info "Delete of VM #{vm.description} successful, syncing DB."
> +                    vm.host_id = nil
> +                    vm.memory_used = nil
> +                    vm.num_vcpus_used = nil
> +                    vm.state = Vm::STATE_STOPPED
> +                    vm.needs_restart = nil
> +                    vm.vnc_port = nil
> +                end
> +            end
> +        end
>          vm.state = state
> -        vm.save
> +        vm.save!
>  
>          domain[:synced] = true
>      end
> diff --git a/src/task-omatic/taskomatic.rb b/src/task-omatic/taskomatic.rb
> index 7e3540c..df59e8d 100755
> --- a/src/task-omatic/taskomatic.rb
> +++ b/src/task-omatic/taskomatic.rb
> @@ -266,7 +266,12 @@ class TaskOmatic
>      raise "Unable to get node that vm is on??" unless node
>  
>      if vm.state == "shutdown" or vm.state == "shutoff"
> -      set_vm_shut_down(db_vm)
> +      result = vm.undefine
> +      if result.status == 0
> +        @logger.info "Deleted VM #{db_vm.description}."
> +        set_vm_shut_down(db_vm)
> +        teardown_storage_pools(node)
> +      end
>        return
>      elsif vm.state == "suspended"
>        raise "Cannot shutdown suspended domain"
> @@ -276,9 +281,11 @@ class TaskOmatic
>  
>      if action == :shutdown
>        result = vm.shutdown
> +      @logger.info "shutdown - result.status is #{result.status}"
>        raise "Error shutting down VM: #{result.text}" unless result.status == 0
>      elsif action == :destroy
>        result = vm.destroy
> +      @logger.info "destroy - result.status is #{result.status}"
>        raise "Error destroying VM: #{result.text}" unless result.status == 0
>      end
>  
> @@ -288,12 +295,18 @@ class TaskOmatic
>      # FIXME: we really should have a marker in the database somehow so that
>      # we can tell if this domain was migrated; that way, we can tell the
>      # difference between a real undefine failure and one because of migration
> +    #
> +    # We only set the vm to shutdown if the undefine succeeds.  Otherwise,
> +    # dbomatic will pick up any state changes.  doing a 'shutdown' rather
> +    # than destroy for instance can possibly take quite some time.
>      result = vm.undefine
> -    @logger.info "Error undefining VM: #{result.text}" unless result.status == 0
> -
> -    teardown_storage_pools(node)
> -
> -    set_vm_shut_down(db_vm)
> +    if result.status == 0
> +        @logger.info "Deleted VM #{db_vm.description}."
> +        set_vm_shut_down(db_vm)
> +        teardown_storage_pools(node)
> +    else
> +        @logger.info "Error undefining VM: #{result.text}" unless result.status == 0
> +    end
>    end
>  
>    def task_start_vm(task)
> -- 

Tested this and it appears to work, with the caveat that ACPI shutdown
does not appear to be functional on Fedora 10 (works for win2k3
however). Therefore ACK.

--Hugh