[PATCH conductor] RM 3635 - added better error message if deployment deletion failed (rebased)

Jirka Tomasek jtomasek at redhat.com
Thu Aug 30 15:40:48 UTC 2012


On 08/30/2012 03:00 PM, jprovazn at redhat.com wrote:
> From: Jan Provaznik <jprovazn at redhat.com>
>
> A reason why deletion failed is now displayed. This required more changes
> (cleanups):
> - fixes formatting of flash error
> - moves destroy_deployment_config call into before_destroy callback
> - fixes setting of flash message on multi_destroy action
> - adds new exception to catch (most common) stop&destroy error - unstoppable
> instance
> ---
>   src/app/controllers/deployments_controller.rb     | 87 +++++++++++++++--------
>   src/app/models/deployment.rb                      | 25 +++----
>   src/app/views/layouts/_new_notification.html.haml |  7 +-
>   src/config/locales/en.yml                         |  7 +-
>   src/lib/exceptions.rb                             |  1 +
>   src/spec/models/deployment_spec.rb                | 10 +++
>   6 files changed, 88 insertions(+), 49 deletions(-)
>
> diff --git a/src/app/controllers/deployments_controller.rb b/src/app/controllers/deployments_controller.rb
> index d9ec800..7df640b 100644
> --- a/src/app/controllers/deployments_controller.rb
> +++ b/src/app/controllers/deployments_controller.rb
> @@ -256,53 +256,84 @@ class DeploymentsController < ApplicationController
>   
>     def destroy
>       deployment = Deployment.find(params[:id])
> -    if check_privilege(Privilege::MODIFY, deployment)
> -      begin
> -        deployment.stop_instances_and_destroy!
> -        flash[:success] = t('deployments.flash.success.deleted', :list => deployment.name, :count => 1)
> -      rescue
> -        flash[:error] = t('deployments.flash.error.not_deleted', :list => deployment.name, :count => 1)
> -      end
> -    else
> -      flash[:error] = t('deployments.flash.error.not_deleted', :list => deployment.name, :count => 1)
> +    cant_stop = false
> +    errors = []
> +    begin
> +      require_privilege(Privilege::MODIFY, deployment)
> +      deployment.stop_instances_and_destroy!
> +    rescue Aeolus::Conductor::Base::NotStoppableDeployment
> +      cant_stop = true
> +      errors = deployment.not_stoppable_or_destroyable_instances.map {|i|
> +                 t('deployments.errors.instance_state',
> +                   :name => i.name,
> +                   :state => i.state)}
> +    rescue
> +      errors = t('deployments.flash.error.not_deleted',
> +                 :name => deployment.name,
> +                 :error => $!.message)
>       end
> +
>       respond_to do |format|
>         format.js do
>           load_deployments
>           render :partial => 'list'
>         end
> -      format.html { redirect_to pools_url(:view => 'filter', :details_tab => 'deployments') }
> -      format.json { render :json => {:success => destroyed, :errors => failed} }
> +
> +      format.html do
> +        if errors.empty?
> +          flash[:success] = t('deployments.flash.success.deleted',
> +                              :list => deployment.name, :count => 1)
> +        elsif cant_stop
> +          flash[:error] = {:summary => t('deployments.errors.cannot_stop',
> +                                         :name => deployment.name),
> +                           :failures => errors}
> +        else
> +          flash[:error] = errors
> +        end
> +        redirect_to pools_url(:view => 'filter', :details_tab => 'deployments')
> +      end
> +
> +      format.json { render :json => {:success => errors.empty?,
> +                                     :errors => errors} }
>       end
>     end
>   
>     def multi_destroy
>       destroyed = []
> -    failed = []
> +    errors = []
>   
> -    Deployment.find(params[:deployments_selected] || []).each do |deployment|
> -      if check_privilege(Privilege::MODIFY, deployment)
> -        begin
> -          deployment.stop_instances_and_destroy!
> -          destroyed << deployment.name
> -        rescue
> -          failed << deployment.name
> -        end
> -      else
> -        failed << deployment.name
> +    ids = Array(params[:deployments_selected])
> +    Deployment.find(ids).each do |deployment|
> +      begin
> +        require_privilege(Privilege::MODIFY, deployment)
> +        deployment.stop_instances_and_destroy!
> +        destroyed << deployment.name
> +      rescue
> +        errors << t('deployments.flash.error.not_deleted',
> +                    :name => deployment.name,
> +                    :error => $!.message)
>         end
>       end
> -    # If nothing is selected, display an error message:
> -    flash[:error] = t('deployments.flash.error.none_selected') if failed.blank? && destroyed.blank?
> -    flash[:success] = t('deployments.flash.success.deleted', :list => destroyed.to_sentence, :count => destroyed.size) if destroyed.present?
> -    flash[:error] = t('deployments.flash.error.not_deleted', :list => failed.to_sentence, :count => failed.size) if failed.present?
>       respond_to do |format|
> -      format.html { redirect_to params[:backlink] || pools_url(:view => 'filter', :details_tab => 'deployments') }
> +      format.html do
> +        if ids.empty?
> +          flash[:error] = t('deployments.flash.error.none_selected')
> +        elsif errors.present?
> +          flash[:error] = errors
> +        end
> +        flash[:success] = t('deployments.flash.success.deleted',
> +                            :list => destroyed.to_sentence,
> +                            :count => destroyed.size) if destroyed.present?
> +
> +        redirect_to params[:backlink] ||
> +          pools_url(:view => 'filter', :details_tab => 'deployments')
> +      end
> +
>         format.js do
>           load_deployments
>           render :partial => 'list'
>         end
> -      format.json { render :json => {:success => destroyed, :errors => failed} }
> +      format.json { render :json => {:success => destroyed, :errors => errors} }
>       end
>     end
>   
> diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
> index 6e5e5d7..5fe72f6 100644
> --- a/src/app/models/deployment.rb
> +++ b/src/app/models/deployment.rb
> @@ -77,6 +77,7 @@ class Deployment < ActiveRecord::Base
>     validates_presence_of :owner_id
>     validate :pool_must_be_enabled
>     before_destroy :destroyable?
> +  before_destroy :destroy_deployment_config
>     before_create :inject_launch_parameters
>     before_create :generate_uuid
>     before_create :set_pool_family
> @@ -164,28 +165,28 @@ class Deployment < ActiveRecord::Base
>       [STATE_RUNNING, STATE_INCOMPLETE].include?(self.state)
>     end
>   
> +  def not_stoppable_or_destroyable_instances
> +    instances.find_all {|i| !(i.destroyable? or
> +                              i.state == Instance::STATE_RUNNING)}
> +  end
> +
>     def stop_instances_and_destroy!
> -    if destroyable?
> -      destroy_deployment_config
> -      destroy
> -      return
> +    unless not_stoppable_or_destroyable_instances.empty?
> +      raise Aeolus::Conductor::Base::NotStoppableDeployment,
> +            I18n.t("deployments.errors.all_stopped")
>       end
>   
> -    if instances.all? {|i| i.destroyable? or i.state == Instance::STATE_RUNNING}
> +    if destroyable?
> +      destroy
> +    else
>         self.state = Deployment::STATE_SHUTTING_DOWN
> -      destroy_deployment_config
>         # The deployment will be destroyed from an InstanceObserver callback once
>         # all instances are stopped.
>         self.scheduled_for_deletion = true
>         self.save!
>   
>         # stop all deployment's instances
> -      instances.each do |instance|
> -        next unless instance.state == Instance::STATE_RUNNING
> -        instance.stop(instance.owner)
> -      end
> -    else
> -      raise I18n.t("deployments.errors.all_stopped")
> +      instances.running.each {|instance| instance.stop(instance.owner)}
>       end
>     end
>   
> diff --git a/src/app/views/layouts/_new_notification.html.haml b/src/app/views/layouts/_new_notification.html.haml
> index d807020..3150ef2 100644
> --- a/src/app/views/layouts/_new_notification.html.haml
> +++ b/src/app/views/layouts/_new_notification.html.haml
> @@ -61,11 +61,8 @@
>                 =image_tag 'flash_error_icon.png', :alt => 'Errors'
>               %ul.flashes
>                 -if !flash[:error][:failures].blank?
> -                -if flash[:error][:failures].length > 1
> -                  -flash[:error][:failures].each do |k, v|
> -                    %li= [k, v.present? ? ": #{v}" : '']
> -                -else
> -                  %li= flash[:error][:failures]
> +                -flash[:error][:failures].each do |k, v|
> +                  %li= v.present? ? "#{k}: #{v}" : k.to_s
>                 -else
>                   -flash[:error].each do |e|
>                     %li= e
> diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
> index 0faa9db..e8cda15 100644
> --- a/src/config/locales/en.yml
> +++ b/src/config/locales/en.yml
> @@ -393,9 +393,7 @@ en:
>         error:
>           not_launched: "Some Assemblies will not be launched:"
>           failed_to_launch_assemblies: "Failed to launch following Assemblies:"
> -        not_deleted:
> -          one: "The Deployment %{list} could not be deleted."
> -          other: "The Deployments %{list} could not be deleted."
> +        not_deleted: "The deployment %{name} could not be deleted: %{error}"
>           none_selected: "You must select one or more Deployments."
>           not_updated:
>             one: "The Deployment %{list} could not be updated."
> @@ -411,7 +409,8 @@ en:
>             other: "The Deployments %{list} were successfully updated."
>       errors:
>         config_server_connection: 'Cannot connect to the Config Server'
> -      cannot_stop: "Stop cannot be performed on this instance."
> +      cannot_stop: "The deployment %{name} can not be deleted because following instances can not be stopped:"
> +      instance_state: "The instance %{name} is in state %{state}."
>         all_stopped: "All Instances must be stopped or running"
>         not_valid_deployable_xml: "seems to be not valid Deployable XML: %{msg}"
>         match_not_found: "Match not found: %{errors}"
> diff --git a/src/lib/exceptions.rb b/src/lib/exceptions.rb
> index d56ddb3..200aa3f 100644
> --- a/src/lib/exceptions.rb
> +++ b/src/lib/exceptions.rb
> @@ -47,6 +47,7 @@ module Aeolus
>       module Base
>         class ImageNotFound < StandardError; end
>         class BlankImageId < StandardError; end
> +      class NotStoppableDeployment < StandardError; end
>       end
>   
>       module MultiError
> diff --git a/src/spec/models/deployment_spec.rb b/src/spec/models/deployment_spec.rb
> index aa78f50..17b0892 100644
> --- a/src/spec/models/deployment_spec.rb
> +++ b/src/spec/models/deployment_spec.rb
> @@ -335,6 +335,16 @@ describe Deployment do
>         lambda { Instance.find(inst1.id) }.should raise_error(ActiveRecord::RecordNotFound)
>         lambda { Instance.find(inst2.id) }.should raise_error(ActiveRecord::RecordNotFound)
>       end
> +
> +    it "should raise an exception if some instances can not be stopped" do
> +      @deployment.save!
> +      inst1 = Factory.create(:mock_running_instance,
> +                             :state => Instance::STATE_PENDING,
> +                             :deployment_id => @deployment.id)
> +      @deployment.reload
> +      lambda { @deployment.stop_instances_and_destroy!}.should
> +        raise_error(Aeolus::Conductor::Base::NotStoppableDeployment)
> +    end
>     end
>   
>     describe ".any_instance_running?" do

ACK, nice!

Jirka



More information about the aeolus-devel mailing list