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

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

Darryl L. Pierce wrote:
> 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.
Can you separate this into a whitespace patch and a functional patch?  The 
whitespace patch you can just go ahead and push without any acks (since 
it'll be nonfunctional changes).  Then post the functional portion of the 
patch for review.

Thanks!

Perry

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.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 ovirt-server.spec.in                 |    2 +-
 src/app/controllers/vm_controller.rb |    7 ++++++-
 src/app/models/vm.rb                 |    6 ++++++
 src/test/unit/vm_test.rb             |   12 ++++++++++++
 4 files changed, 25 insertions(+), 2 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..c7772b5 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>
 #
@@ -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
diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
index 2eff87a..8f335c9 100644
--- a/src/app/models/vm.rb
+++ b/src/app/models/vm.rb
@@ -277,6 +277,12 @@ class Vm < ActiveRecord::Base
     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
   # are currently stopped, pending (new without any create_vm tasks having
diff --git a/src/test/unit/vm_test.rb b/src/test/unit/vm_test.rb
index 22164e8..d583286 100644
--- a/src/test/unit/vm_test.rb
+++ b/src/test/unit/vm_test.rb
@@ -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.6.5

Darryl L. Pierce wrote:
> 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.
>   
NACK. This works for the multi-VM deletion code path, but for the single 
VM deletion action (delete link on the details pane) the cobbler profile 
remains.
> 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
> @@ -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
>   
This code also needs to be added into the vm_controller.destroy, since 
that's the action used for deleting a single VM. Once that's done it 
should all work properly.

Scott

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.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 ovirt-server.spec.in                 |    2 +-
 src/app/controllers/vm_controller.rb |   12 ++++++++++++
 src/app/models/vm.rb                 |    6 ++++++
 src/test/unit/vm_test.rb             |   12 ++++++++++++
 4 files changed, 31 insertions(+), 1 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 a0fcfbc..fc992b3 100644
--- a/src/app/controllers/vm_controller.rb
+++ b/src/app/controllers/vm_controller.rb
@@ -144,6 +144,7 @@ class VmController < ApplicationController
         vms = Vm.find(:all, :conditions => "id in (#{vm_ids.join(',
')})")
         vms.each do |vm|
           if vm.is_destroyable?
+            destroy_cobbler_system(vm)
             vm.destroy
           else
             failure_list << vm.description
@@ -166,6 +167,7 @@ class VmController < ApplicationController
   def destroy
     vm_resource_pool = @vm.vm_resource_pool_id
     if (@vm.is_destroyable?)
+      destroy_cobbler_system(@vm)
       @vm.destroy
       render :json => { :object => "vm", :success => true,
         :alert => "Virtual Machine was successfully deleted." }
@@ -349,4 +351,14 @@ class VmController < ApplicationController
     pre_edit
     authorize_user
   end
+
+  private
+
+  def destroy_cobbler_system(vm)
+    # 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
+  end
 end
diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
index 05073f0..ae157f7 100644
--- a/src/app/models/vm.rb
+++ b/src/app/models/vm.rb
@@ -277,6 +277,12 @@ class Vm < ActiveRecord::Base
     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
   # are currently stopped, pending (new without any create_vm tasks having
diff --git a/src/test/unit/vm_test.rb b/src/test/unit/vm_test.rb
index 2291e9d..a9ed9dc 100644
--- a/src/test/unit/vm_test.rb
+++ b/src/test/unit/vm_test.rb
@@ -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.6.5