[Ovirt-devel] [PATCH server] BUG#460147 - Deleting a VM based on a Cobbler system does not delete

Darryl L. Pierce dpierce at redhat.com
Fri Oct 24 13:48:44 UTC 2008


This patch addresses this issue, first calling Cobbler and deleting the
system prior to deleting the VM. It also changes ovirt-server's dependence
to since this is new functionality in the rubygem-cobbler package.

Also, since there were several lines with trailing white spaces, this
patch strips those out of the vm_controller.rb file.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 ovirt-server.spec.in                 |    2 +-
 src/app/controllers/vm_controller.rb |   31 ++++++++++++++++++-------------
 src/app/models/vm.rb                 |    6 ++++++
 src/test/unit/vm_test.rb             |   14 +++++++++++++-
 4 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/ovirt-server.spec.in b/ovirt-server.spec.in
index 123e280..e2620d1 100644
--- a/ovirt-server.spec.in
+++ b/ovirt-server.spec.in
@@ -17,7 +17,7 @@ Requires: rubygem(activeldap) >= 0.10.0
 Requires: rubygem(rails) >= 2.1.1
 Requires: rubygem(mongrel) >= 1.0.1
 Requires: rubygem(krb5-auth) >= 0.6
-Requires: rubygem(cobbler) >= 0.0.2
+Requires: rubygem(cobbler) >= 0.1.2
 Requires: rubygem(gettext)
 Requires: ruby-flexmock
 Requires: postgresql-server
diff --git a/src/app/controllers/vm_controller.rb b/src/app/controllers/vm_controller.rb
index 585e524..5ab80f5 100644
--- a/src/app/controllers/vm_controller.rb
+++ b/src/app/controllers/vm_controller.rb
@@ -1,4 +1,4 @@
-# 
+#
 # Copyright (C) 2008 Red Hat, Inc.
 # Written by Scott Seago <sseago at redhat.com>
 #
@@ -32,7 +32,7 @@ class VmController < ApplicationController
       flash[:notice] = 'You do not have permission to view this vm: redirecting to top level'
       redirect_to :controller => 'resources', :controller => 'dashboard'
     end
-    render :layout => 'selection'    
+    render :layout => 'selection'
   end
 
   def add_to_smart_pool
@@ -41,7 +41,7 @@ class VmController < ApplicationController
   end
 
   def new
-    render :layout => 'popup'    
+    render :layout => 'popup'
   end
 
   def create
@@ -73,14 +73,14 @@ class VmController < ApplicationController
       render :json => { :object => "vm", :success => true, :alert => alert  }
     rescue Exception => error
       # FIXME: need to distinguish vm vs. task save errors (but should mostly be vm)
-      render :json => { :object => "vm", :success => false, 
+      render :json => { :object => "vm", :success => false,
                         :errors => @vm.errors.localize_error_messages.to_a }
     end
 
   end
 
   def edit
-    render :layout => 'popup'    
+    render :layout => 'popup'
   end
 
   def update
@@ -124,16 +124,16 @@ class VmController < ApplicationController
       end
 
 
-      render :json => { :object => "vm", :success => true, 
+      render :json => { :object => "vm", :success => true,
                         :alert => 'Vm was successfully updated.'  }
     rescue
       # FIXME: need to distinguish vm vs. task save errors (but should mostly be vm)
-      render :json => { :object => "vm", :success => false, 
+      render :json => { :object => "vm", :success => false,
                         :errors => @vm.errors.localize_error_messages.to_a }
     end
   end
 
-  #FIXME: we need permissions checks. user must have permission. Also state checks 
+  #FIXME: we need permissions checks. user must have permission. Also state checks
   def delete
     vm_ids_str = params[:vm_ids]
     vm_ids = vm_ids_str.split(",").collect {|x| x.to_i}
@@ -144,6 +144,11 @@ class VmController < ApplicationController
         vms = Vm.find(:all, :conditions => "id in (#{vm_ids.join(', ')})")
         vms.each do |vm|
           if vm.is_destroyable?
+            # Destroy the Cobbler system first if it's defined
+            if vm.uses_cobbler?
+              system = Cobbler::System.find_one(vm.cobbler_system_name)
+              system.remove if system
+            end
             vm.destroy
           else
             failure_list << vm.description
@@ -167,10 +172,10 @@ class VmController < ApplicationController
     vm_resource_pool = @vm.vm_resource_pool_id
     if (@vm.is_destroyable?)
       @vm.destroy
-      render :json => { :object => "vm", :success => true, 
+      render :json => { :object => "vm", :success => true,
         :alert => "Virtual Machine was successfully deleted." }
     else
-      render :json => { :object => "vm", :success => false, 
+      render :json => { :object => "vm", :success => false,
         :alert => "Vm must be stopped to delete it." }
     end
   end
@@ -180,7 +185,7 @@ class VmController < ApplicationController
     vm_pool_id = params[:vm_pool_id]
     @vm = id ? Vm.find(id) : nil
     @vm_pool = vm_pool_id ? VmResourcePool.find(vm_pool_id) : nil
-      
+
     json_list(StorageVolume.find_for_vm(@vm, @vm_pool),
               [:id, :display_name, :size_in_gb, :get_type_label])
   end
@@ -302,9 +307,9 @@ class VmController < ApplicationController
     # random MAC
     mac = [ 0x00, 0x16, 0x3e, rand(0x7f), rand(0xff), rand(0xff) ]
     # random uuid
-    uuid = ["%02x" * 4, "%02x" * 2, "%02x" * 2, "%02x" * 2, "%02x" * 6].join("-") % 
+    uuid = ["%02x" * 4, "%02x" * 2, "%02x" * 2, "%02x" * 2, "%02x" * 6].join("-") %
       Array.new(16) {|x| rand(0xff) }
-    newargs = { 
+    newargs = {
       :vm_resource_pool_id => params[:vm_resource_pool_id],
       :vnic_mac_addr => mac.collect {|x| "%02x" % x}.join(":"),
       :uuid => uuid
diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
index 2eff87a..19c2912 100644
--- a/src/app/models/vm.rb
+++ b/src/app/models/vm.rb
@@ -276,6 +276,12 @@ class Vm < ActiveRecord::Base
       self.provisioning[/^.*@.*:(.*)/,1]
     end
   end
+  
+  # Returns the system name in Cobbler for this VM.
+  #
+  def cobbler_system_name
+    self.uuid
+  end
 
   # whether this VM may be validly deleted. running VMs should not be
   # allowed to be deleted. Currently we restrict deletion to VMs that
diff --git a/src/test/unit/vm_test.rb b/src/test/unit/vm_test.rb
index 22164e8..a9ed9dc 100644
--- a/src/test/unit/vm_test.rb
+++ b/src/test/unit/vm_test.rb
@@ -1,4 +1,4 @@
-# 
+#
 # Copyright (C) 2008 Red Hat, Inc.
 # Written by Scott Seago <sseago at redhat.com>
 #
@@ -76,4 +76,16 @@ class VmTest < Test::Unit::TestCase
       vm.cobbler_name,
       "Wrong name reported."
   end
+
+  # Ensures that the right value is used when requesting the cobbler system
+  # name for a VM backed by Cobbler.
+  #
+  def test_cobbler_system_name
+    @vm = Vm.new
+    @vm.provisioning_and_boot_settings = @cobbler_profile_provisioning
+    @vm.uuid = "oicu812"
+
+    assert_equal @vm.cobbler_system_name, @vm.uuid,
+      "VMs should be using the UUID as their Cobbler system name."
+  end
 end
-- 
1.5.5.1




More information about the ovirt-devel mailing list