[PATCH conductor] BZ 812740 - check associated images before destroying account or pool family (rev. 2)

jprovazn at redhat.com jprovazn at redhat.com
Tue Sep 4 12:52:00 UTC 2012


From: Jan Provaznik <jprovazn at redhat.com>

Environment or provider account can't be deleted if there is any existing
image (or provider image) associated with them.

This required a little bit more refactoring. All destroy 'validations'
are done in before_destroy callbacks. If some callback fails, an exception
is raised so destroy is aborted and reason is in the exception message.
To avoid having rescue blocks in all controllers, new 'safe_destroy' ActiveRecord
method is defined, this method adds exception into object's errors and returns
true/false.
---
 src/app/controllers/pool_families_controller.rb    | 42 ++++++-----------
 .../controllers/provider_accounts_controller.rb    | 55 ++++++++++++----------
 src/app/controllers/providers_controller.rb        | 30 +++++++-----
 src/app/models/pool_family.rb                      | 41 ++++++++++++++--
 src/app/models/provider.rb                         | 16 -------
 src/app/models/provider_account.rb                 | 31 ++++++++++--
 src/config/initializers/destroy_extension.rb       | 14 ++++++
 src/config/locales/en.yml                          | 19 ++++----
 src/lib/exceptions.rb                              |  1 +
 .../provider_accounts_controller_spec.rb           |  4 +-
 src/spec/models/provider_account_spec.rb           | 10 ++--
 11 files changed, 159 insertions(+), 104 deletions(-)
 create mode 100644 src/config/initializers/destroy_extension.rb

diff --git a/src/app/controllers/pool_families_controller.rb b/src/app/controllers/pool_families_controller.rb
index 0ec70ed..585e92d 100644
--- a/src/app/controllers/pool_families_controller.rb
+++ b/src/app/controllers/pool_families_controller.rb
@@ -77,7 +77,7 @@ class PoolFamiliesController < ApplicationController
     @title = @pool_family.name
     save_breadcrumb(pool_family_path(@pool_family), @pool_family.name)
     require_privilege(Privilege::VIEW, @pool_family)
-    @all_images = Aeolus::Image::Warehouse::Image.by_environment(@pool_family.name)
+    @all_images = @pool_family.images
     @images = paginate_collection(@all_images, params[:page], PER_PAGE)
 
     load_pool_family_tabs
@@ -96,15 +96,20 @@ class PoolFamiliesController < ApplicationController
   def destroy
     pool_family = PoolFamily.find(params[:id])
     require_privilege(Privilege::MODIFY, pool_family)
-    if pool_family == PoolFamily.default
-      flash[:error] = t("pool_families.flash.error.default_pool_family_not_deleted")
-    elsif pool_family.destroy
-      flash[:notice] = t "pool_families.flash.notice.deleted"
-    else
-      flash[:error] = t "pool_families.flash.error.not_deleted"
-    end
 
-    redirect_to pool_families_path
+    respond_to do |format|
+      if pool_family.safe_destroy
+        format.html do
+          flash[:notice] = t("pool_families.flash.notice.deleted")
+          redirect_to pool_families_path
+        end
+      else
+        format.html do
+          flash[:error] = pool_family.errors.full_messages
+          redirect_to pool_families_path
+        end
+      end
+    end
   end
 
   def add_provider_accounts
@@ -150,25 +155,6 @@ class PoolFamiliesController < ApplicationController
     end
   end
 
-  def multi_destroy
-    deleted = []
-    not_deleted = []
-    PoolFamily.find(params[:pool_family_selected]).each do |pool_family|
-      if check_privilege(Privilege::MODIFY, pool_family) && pool_family.destroy
-        deleted << pool_family.name
-      else
-        not_deleted << pool_family.name
-      end
-    end
-    if deleted.size > 0
-      flash[:notice] = t 'pool_families.flash.notice.more_deleted', :list => deleted.join(', ')
-    end
-    if not_deleted.size > 0
-      flash[:error] = t 'pool_families.flash.error.more_not_deleted', :list => not_deleted.join(', ')
-    end
-    redirect_to pool_families_path
-  end
-
   def remove_provider_accounts
     @pool_family = PoolFamily.find(params[:id])
     require_privilege(Privilege::MODIFY, @pool_family)
diff --git a/src/app/controllers/provider_accounts_controller.rb b/src/app/controllers/provider_accounts_controller.rb
index b73e57f..1abc022 100644
--- a/src/app/controllers/provider_accounts_controller.rb
+++ b/src/app/controllers/provider_accounts_controller.rb
@@ -170,26 +170,27 @@ class ProviderAccountsController < ApplicationController
   def destroy
     @provider_account = ProviderAccount.find(params[:id])
     require_privilege(Privilege::MODIFY, @provider_account)
-    is_deleted = @provider_account && @provider_account.destroy
 
     respond_to do |format|
-      format.html {
-        if is_deleted
-          flash[:notice] = t"provider_accounts.flash.notice.deleted"
-        else
-          flash[:error] = t"provider_accounts.flash.error.not_deleted"
+      if @provider_account.safe_destroy
+        format.html do
+          flash[:notice] = t("provider_accounts.flash.notice.deleted")
+          redirect_to edit_provider_path(@provider_account.provider,
+                                         :details_tab => 'accounts')
         end
-
-        @provider = Provider.find(params[:provider_id])
-        redirect_to edit_provider_path(@provider, :details_tab => 'accounts')
-      }
-      format.xml {
-        if is_deleted
+        format.xml do
           render 'destroy', :locals => { :provider_account_id => @provider_account.id }
-        else
-          raise(Aeolus::Conductor::API::ProviderAccountNotFound.new(404, t("api.error_messages.provider_account_not_found_for_id", :id => params[:id])))
         end
-      }
+      else
+        format.html do
+          flash[:error] = @provider_account.errors.full_messages
+          redirect_to edit_provider_path(@provider_account.provider,
+                                         :details_tab => 'accounts')
+        end
+        format.xml do
+          raise Aeolus::Conductor::API::Error.new(500, @provider_account.errors.full_messages.join(', '))
+        end
+      end
     end
   end
 
@@ -202,21 +203,25 @@ class ProviderAccountsController < ApplicationController
 
     succeeded = []
     failed = []
-    @provider_accounts = ProviderAccount.find(params[:accounts_selected]).each do |account|
-      if check_privilege(Privilege::MODIFY, account) && account.destroyable?
-        account.destroy
-        succeeded << account.label
+    ProviderAccount.find(params[:accounts_selected]).each do |account|
+      if !check_privilege(Privilege::MODIFY, account)
+        failed << t('application_controller.permission_denied_with_prefix',
+                    :prefix => account.name)
+      elsif account.safe_destroy
+        succeeded << account.name
       else
-        failed << account.label
+        failed << t('provider_accounts.flash.error.not_deleted_with_err',
+                    :account => account.name,
+                    :err => account.errors.full_messages.join(', '))
       end
     end
 
-    unless succeeded.empty?
-      flash[:notice] = t 'provider_accounts.flash.notice.account_deleted', :count => succeeded.length, :list => succeeded.join(', ')
-    end
-    unless failed.empty?
-      flash[:error] = t 'provider_accounts.flash.error.account_not_deleted', :count => failed.length, :list => failed.join(', ')
+    if succeeded.present?
+      flash[:notice] = t('provider_accounts.flash.notice.account_deleted',
+                         :count => succeeded.length,
+                         :list => succeeded.join(', '))
     end
+    flash[:error] = failed if failed.present?
     redirect_to edit_provider_path(@provider, :details_tab => 'accounts')
   end
 
diff --git a/src/app/controllers/providers_controller.rb b/src/app/controllers/providers_controller.rb
index f3a9302..3b6ec23 100644
--- a/src/app/controllers/providers_controller.rb
+++ b/src/app/controllers/providers_controller.rb
@@ -184,20 +184,26 @@ class ProvidersController < ApplicationController
   def destroy
     provider = Provider.find(params[:id])
     require_privilege(Privilege::MODIFY, provider)
-    if provider.destroy
-      session[:current_provider_id] = nil
-      flash[:notice] = t("providers.flash.notice.deleted")
-    else
-      flash[:error] = {
-        :summary => t("providers.flash.error.not_deleted"),
-        :failures => provider.errors.values.flatten
-      }
-    end
 
     respond_to do |format|
-      format.html { redirect_to providers_path }
-      # FIXME: what to return in body of response, if anything?
-      format.xml { render :text => '<message>OK</message>', :status => 200 }
+      if provider.safe_destroy
+        session[:current_provider_id] = nil
+        format.html do
+          flash[:notice] = t("providers.flash.notice.deleted")
+          redirect_to providers_path
+        end
+        format.xml { render :text => '<message>OK</message>', :status => 200 }
+      else
+        format.html do
+          flash[:error] = t("providers.flash.error.not_deleted_with_err",
+                            :err => provider.errors.full_messages.join(', '))
+          redirect_to providers_path
+        end
+        # FIXME: what to return in body of response, if anything?
+        format.xml do
+          raise(Aeolus::Conductor::API::Error.new(500, error))
+        end
+      end
     end
   end
 
diff --git a/src/app/models/pool_family.rb b/src/app/models/pool_family.rb
index f464723..a644651 100644
--- a/src/app/models/pool_family.rb
+++ b/src/app/models/pool_family.rb
@@ -36,6 +36,10 @@ class PoolFamily < ActiveRecord::Base
   include ActionView::Helpers::NumberHelper
   DEFAULT_POOL_FAMILY_KEY = "default_pool_family"
 
+  before_destroy :check_name!
+  before_destroy :check_pools!
+  before_destroy :check_images!
+
   after_update :fix_iwhd_environment_tags
 
   has_many :pools,  :dependent => :destroy
@@ -59,7 +63,6 @@ class PoolFamily < ActiveRecord::Base
   validates_uniqueness_of :name
   validates_presence_of :quota
 
-  before_destroy :destroyable?
 
   def self.default
     MetadataObject.lookup(DEFAULT_POOL_FAMILY_KEY)
@@ -82,9 +85,39 @@ class PoolFamily < ActiveRecord::Base
     [Pool, Quota]
   end
 
-  def destroyable?
-    # A PoolFamily is destroyable if its pools are destroyable and it is not  the default PoolFamily
-    pools.all? {|p| p.destroyable? } && self != PoolFamily.default
+  def check_pools!
+    cant_destroy = pools.find_all {|p| !p.destroyable?}
+    if cant_destroy.empty?
+      true
+    else
+      raise Aeolus::Conductor::Base::NotDestroyable,
+        I18n.t('pool_families.errors.not_destroyable_pools',
+               :list => cant_destroy.map {|p| p.name}.join(', '))
+    end
+  end
+
+  def check_name!
+    if self == PoolFamily.default
+      raise Aeolus::Conductor::Base::NotDestroyable,
+        I18n.t('pool_families.errors.default_pool_family_not_deleted')
+    else
+      true
+    end
+  end
+
+  def check_images!
+    referenced_images = images
+    if referenced_images.empty?
+      true
+    else
+      raise Aeolus::Conductor::Base::NotDestroyable,
+        I18n.t('pool_families.errors.associated_images',
+               :list => referenced_images.map {|i| i.name}.join(', '))
+    end
+  end
+
+  def images
+    Aeolus::Image::Warehouse::Image.by_environment(self.name)
   end
 
   def all_providers_disabled?
diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb
index 64ddb33..d1ae263 100644
--- a/src/app/models/provider.rb
+++ b/src/app/models/provider.rb
@@ -62,8 +62,6 @@ class Provider < ActiveRecord::Base
   has_many :provider_priority_group_elements, :as => :value, :dependent => :destroy
   has_many :provider_priority_groups, :through => :provider_priority_group_elements
 
-  before_destroy :destroyable?
-
   def derived_subtree(role = nil)
     subtree = super(role)
     subtree += provider_accounts if (role.nil? or role.privilege_target_match(ProviderAccount))
@@ -82,20 +80,6 @@ class Provider < ActiveRecord::Base
     end
     return url + url_extras
   end
-  # there is a destroy dependency for a cloud accounts association,
-  # but a cloud account is silently not destroyed when there is
-  # an instance for the cloud account
-  def destroyable?
-    unless self.provider_accounts.empty?
-      self.provider_accounts.each do |c|
-        unless c.instances.empty?
-          inst_list = c.instances.map {|i| i.name}.join(', ')
-          self.errors.add(:base, "there are instances for cloud account '#{c.name}': #{inst_list}")
-        end
-      end
-    end
-    return self.errors.empty?
-  end
 
   def connect
     begin
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index 0da9184..f23b286 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -39,6 +39,9 @@ class ProviderAccount < ActiveRecord::Base
   end
   include PermissionedObject
 
+  before_destroy :check_destroyable_instances!
+  before_destroy :check_provider_images!
+
   # Relations
   belongs_to :provider
   belongs_to :quota, :autosave => true, :dependent => :destroy
@@ -82,7 +85,6 @@ class ProviderAccount < ActiveRecord::Base
 
   before_create :populate_profiles_and_validate
   after_create :populate_realms_and_validate
-  before_destroy :destroyable?
 
   scope :enabled, lambda { where(:provider_id => Provider.enabled) }
 
@@ -125,8 +127,31 @@ class ProviderAccount < ActiveRecord::Base
     [Quota]
   end
 
-  def destroyable?
-    instances.empty? || instances.all? { |i| i.destroyable? }
+  def provider_images
+    Aeolus::Image::Warehouse::ProviderImage.where(
+    "($provider == \"#{provider.name}\" && $provider_account_identifier == \"#{credentials_hash['username']}\")")
+  end
+
+  def check_provider_images!
+    imgs = provider_images.map {|pi| pi.target_image.build.image.name}
+    if imgs.empty?
+      true
+    else
+      raise Aeolus::Conductor::Base::NotDestroyable,
+        I18n.t('provider_accounts.errors.associated_images',
+               :images => imgs.join(', '))
+    end
+  end
+
+  def check_destroyable_instances!
+    not_destroyable_instances = instances.find_all {|i| !i.destroyable?}
+    if not_destroyable_instances.empty?
+      true
+    else
+      raise Aeolus::Conductor::Base::NotDestroyable,
+        I18n.t('provider_accounts.errors.not_destroyable_instances',
+               :instances => not_destroyable_instances.map{|i| i.name}.join(', '))
+    end
   end
 
   def connect
diff --git a/src/config/initializers/destroy_extension.rb b/src/config/initializers/destroy_extension.rb
new file mode 100644
index 0000000..e3d9f5d
--- /dev/null
+++ b/src/config/initializers/destroy_extension.rb
@@ -0,0 +1,14 @@
+class ActiveRecord::Base
+  # this method is used in in controllers where we need to get
+  # message why some before_destroy callback failed
+  #
+  # I'm not entirely sure that it's ideal to use obj.errors to pass
+  # errors from before_destroy callbacks, but it allows us to avoid
+  # begin-rescue blocks in most of controllers
+  def safe_destroy
+    destroy
+  rescue Aeolus::Conductor::Base::NotDestroyable => ex
+    errors.add(:base, ex.message)
+    false
+  end
+end
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index 063c46d..da62126 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -791,19 +791,21 @@ en:
         deleted: "Environment was deleted."
         provider_accounts_added: "These Provider Account have been added"
         provider_accounts_removed: "These Provider Accounts were removed"
-        more_deleted: "Deleted the following Environments: %{list}."
       warning:
         creation_failed: "Environment creation failed."
       error:
         not_updated: "Environment wasn't updated."
-        not_deleted: "Environment cannot be deleted. Check that all Instances are stopped."
-        default_pool_family_not_deleted: "The default Environment cannot be deleted."
-        more_not_deleted: "Could not delete the following Environments: %{list}."
+        not_deleted: "Environment cannot be deleted."
+        not_deleted_with_err: "Environment cannot be deleted: %{err}"
         provider_accounts_not_added: "Could not add these Provider Accounts"
         provider_accounts_not_removed: "Could not remove these Provider Accounts"
         select_to_add_accounts: "You must select at least one Provider Account to add."
         select_to_remove_accounts: "You must select at least one Provider Account to remove."
         no_provider_accounts: "There are no Provider Accounts available."
+    errors:
+      not_destroyable_pools: "Can not destroy following pools: %{list}."
+      default_pool_family_not_deleted: "The default Environment cannot be deleted."
+      associated_images: "There are following associated images: %{list}. Delete them first."
   catalog_entries:
     new_catalog_entry: New Deployable
     index:
@@ -1184,6 +1186,7 @@ en:
         not_updated: "Cannot update the Provider."
         not_disabled: "Cannot disable the Provider."
         not_deleted: "Provider was not deleted"
+        not_deleted_with_err: "Provider was not deleted: %{err}"
       warning:
         connect_failed: "Failed to connect to Provider"
         not_stopped_instances: "Provider was not disabled. Failed to stop following instances:"
@@ -1293,10 +1296,7 @@ en:
           one: "Account %{list} could not be added"
           other: "Accounts %{list} could not be added"
         not_updated: "Provider Account wasn't updated"
-        not_deleted: "Provider account was not deleted"
-        account_not_deleted:
-          one: "Account %{list} was not deleted. There are Instances associated with it."
-          other: "Accounts %{list} were not deleted. They have Instances associated with them."
+        not_deleted_with_err: "Account %{account} was not deleted: %{err}"
         test_connection_failed_invalid: "Test Connection Failed: Invalid Account Details"
         test_connection_failed_provider: "Test Connection Failed: Could not connect to Provider"
       warning:
@@ -1308,6 +1308,8 @@ en:
       populate_hardware_profiles_failed: "Failed to populate hardware_profiles: %{message}"
       populate_realms_failed: "Failed to populate Realms: %{message}"
       could_not_connect: "Could not connect to Provider Account.  Please contact an Administrator."
+      not_destroyable_instances: "Can not destroy following instances: %{instances}"
+      associated_images: "There are following associated provider images: %{images}. Delete them first."
   provider_priority_groups:
     index:
       add_new: Add new
@@ -1470,6 +1472,7 @@ en:
       settings: Settings
   application_controller:
     permission_denied: You have insufficient privileges to perform the selected action.
+    permission_denied_with_prefix: "%{prefix}: You have insufficient privileges to perform the selected action."
     access_denied: Access denied
     some_actions_failed: Some actions failed
     action_error: Action Error
diff --git a/src/lib/exceptions.rb b/src/lib/exceptions.rb
index 200aa3f..3076172 100644
--- a/src/lib/exceptions.rb
+++ b/src/lib/exceptions.rb
@@ -48,6 +48,7 @@ module Aeolus
       class ImageNotFound < StandardError; end
       class BlankImageId < StandardError; end
       class NotStoppableDeployment < StandardError; end
+      class NotDestroyable < StandardError; end
     end
 
     module MultiError
diff --git a/src/spec/controllers/provider_accounts_controller_spec.rb b/src/spec/controllers/provider_accounts_controller_spec.rb
index 35079b3..e693c2f 100644
--- a/src/spec/controllers/provider_accounts_controller_spec.rb
+++ b/src/spec/controllers/provider_accounts_controller_spec.rb
@@ -235,13 +235,13 @@ describe ProviderAccountsController do
           end
 
           it "when requested provider account doesn't exists" do
-            ProviderAccount.stub(:find).and_return(nil)
+            ProviderAccount.stub(:find).and_raise(ActiveRecord::RecordNotFound)
             get :destroy, :id => "id_that_does_not_exist"
             response.status.should be_eql(404)
             response.should have_content_type("application/xml")
             response.body.should be_xml
             subject = Nokogiri::XML(response.body)
-            subject.xpath('//error/code').text.strip.should == "ProviderAccountNotFound"
+            subject.xpath('//error/code').text.strip.should == "RecordNotFound"
           end
         end #destroy
 
diff --git a/src/spec/models/provider_account_spec.rb b/src/spec/models/provider_account_spec.rb
index 8820be9..5a49146 100644
--- a/src/spec/models/provider_account_spec.rb
+++ b/src/spec/models/provider_account_spec.rb
@@ -24,19 +24,17 @@ describe ProviderAccount do
 
   it "should not be destroyable if it has instance with status other than stopped" do
     @provider_account.instances << Instance.new
-    @provider_account.destroyable?.should be_false
-    @provider_account.destroy.should be_false
+    lambda {@provider_account.destroy}.should
+      raise_error(Aeolus::Conductor::Base::NotDestroyable)
     @provider_account.instances.each { |i| i.state = "stopped" }
-    @provider_account.destroyable?.should be_true
+    @provider_account.check_destroyable_instances!.should be_true
     @provider_account.instances.clear
-    @provider_account.destroyable?.should be_true
+    @provider_account.check_destroyable_instances!.should be_true
     @provider_account.destroy.equal?(@provider_account).should be_true
     @provider_account.should be_frozen
   end
 
   it "should be destroyable if it has a config server" do
-    @provider_account.config_server = ConfigServer.new
-    @provider_account.destroyable?.should be_true
     @provider_account.destroy.equal?(@provider_account).should be_true
     @provider_account.should be_frozen
   end
-- 
1.7.11.4




More information about the aeolus-devel mailing list