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

jprovazn at redhat.com jprovazn at redhat.com
Thu Aug 30 13:00:14 UTC 2012


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
-- 
1.7.11.4




More information about the aeolus-devel mailing list