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

[Scott Seago]

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(-)

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)
-- 
1.5.5.1