[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