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

Imre Farkas ifarkas at redhat.com
Tue Sep 4 15:17:10 UTC 2012


On 09/04/2012 02:52 PM, jprovazn at redhat.com wrote:
> 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
>

Nice work! ACK

As we discussed on IRC, it's worth noting that this patch exposes 
another bug: not all images are listed on images#index, only those which 
has a valid environment. In case the user has an image associated with a 
provider account but deleted the environment, cannot access the image. 
With this patch any attempt to delete that provider account will be 
rejected because of the associated but inaccessible images.




More information about the aeolus-devel mailing list