[Ovirt-devel] [PATCH] fix validation issues for VM destroy

[Perry Myers]

Scott Seago wrote:
> Signed-off-by: Scott Seago <sseago at redhat.com>
> ---
>  src/app/controllers/vm_controller.rb |   25 +++++++++++++++++--------
>  src/app/models/vm.rb                 |   23 +++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 8 deletions(-)

This seems to work ok, so ACK and I'm going to push this.

Perry

> diff --git a/src/app/controllers/vm_controller.rb b/src/app/controllers/vm_controller.rb
> index bc88760..a9ac9a5 100644
> --- a/src/app/controllers/vm_controller.rb
> +++ b/src/app/controllers/vm_controller.rb
> @@ -137,26 +137,35 @@ class VmController < ApplicationController
>    def delete
>      vm_ids_str = params[:vm_ids]
>      vm_ids = vm_ids_str.split(",").collect {|x| x.to_i}
> -    
> +    failure_list = []
> +    success = false
>      begin
>        Vm.transaction do
>          vms = Vm.find(:all, :conditions => "id in (#{vm_ids.join(', ')})")
>          vms.each do |vm|
> -          vm.destroy
> +          if vm.is_destroyable?
> +            vm.destroy
> +          else
> +            failure_list << vm.description
> +          end
>          end
>        end
> -      render :json => { :object => "vm", :success => true, 
> -        :alert => "Virtual Machines were successfully deleted." }
> +      if failure_list.empty?
> +        success = true
> +        alert = "Virtual Machines were successfully deleted."
> +      else
> +        alert = "The following Virtual Machines were not deleted (a VM must be stopped to delete it): "
> +        alert+= failure_list.join(', ')
> +      end
>      rescue
> -      render :json => { :object => "vm", :success => false, 
> -        :alert => "Error deleting virtual machines." }
> +      alert = "Error deleting virtual machines."
>      end
> +    render :json => { :object => "vm", :success => success, :alert => alert }
>    end
>  
>    def destroy
>      vm_resource_pool = @vm.vm_resource_pool_id
> -    if ((@vm.state == Vm::STATE_STOPPED and @vm.get_pending_state == Vm::STATE_STOPPED) or
> -        (@vm.state == Vm::STATE_PENDING and @vm.get_pending_state == Vm::STATE_PENDING))
> +    if (@vm.is_destroyable?)
>        @vm.destroy
>        render :json => { :object => "vm", :success => true, 
>          :alert => "Virtual Machine was successfully deleted." }
> diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
> index d7beacf..6853157 100644
> --- a/src/app/models/vm.rb
> +++ b/src/app/models/vm.rb
> @@ -84,6 +84,11 @@ class Vm < ActiveRecord::Base
>    STATE_CREATE_FAILED  = "create_failed"
>    STATE_INVALID        = "invalid"
>  
> +  DESTROYABLE_STATES   = [STATE_PENDING,
> +                          STATE_STOPPED,
> +                          STATE_CREATE_FAILED,
> +                          STATE_INVALID]
> +
>    RUNNING_STATES       = [STATE_RUNNING,
>                            STATE_SUSPENDED,
>                            STATE_STOPPING,
> @@ -272,6 +277,24 @@ class Vm < ActiveRecord::Base
>      end
>    end
>  
> +  # whether this VM may be validly deleted. running VMs should not be
> +  # allowed to be deleted. Currently we restrict deletion to VMs that
> +  # are currently stopped, pending (new without any create_vm tasks having
> +  # been run), or create_failed. Also, get_pending_state must equal the
> +  # current state -- so that we won't delete a VM with a current pending task
> +  def is_destroyable?
> +    current_state = state
> +    pending_state = get_pending_state
> +    DESTROYABLE_STATES.include?(current_state) and (current_state == pending_state)
> +  end
> +
> +  def destroy
> +    if !is_destroyable?
> +      raise "VM must be stopped to delete it"
> +    end
> +    super
> +  end
> +
>    protected
>    def validate
>      resources = vm_resource_pool.max_resources_for_vm(self)


-- 
|=-        Red Hat, Engineering, Emerging Technologies, Boston        -=|
|=-                     Email: pmyers at redhat.com                      -=|
|=-         Office: +1 412 474 3552   Mobile: +1 703 362 9622         -=|
|=- GnuPG: E65E4F3D 88F9 F1C9 C2F3 1303 01FE 817C C5D2 8B91 E65E 4F3D -=|