[PATCH conductor] BZ 743677 - stop/terminate instances when disabling inaccessible provider

jprovazn at redhat.com jprovazn at redhat.com
Wed Feb 1 10:35:38 UTC 2012


From: Jan Provaznik <jprovazn at redhat.com>

If a provider is not accessible and has running instances, allow a user to stop them
on conductor side (no matter what is their real state).

If a provider is accessible but stop request fails for any reason, the provider is not
disabled (before the provider was disabled in any case).
---
 src/app/controllers/providers_controller.rb        |   37 ++++++++++---
 src/app/models/provider.rb                         |   58 ++++++++++++++------
 .../views/providers/confirm_terminate.html.haml    |   14 +++++
 src/config/locales/en.yml                          |    8 +++
 src/features/provider.feature                      |   12 ++++
 src/features/step_definitions/provider_steps.rb    |    9 +++
 src/spec/models/provider_spec.rb                   |   30 ++++++++++-
 7 files changed, 144 insertions(+), 24 deletions(-)
 create mode 100644 src/app/views/providers/confirm_terminate.html.haml

diff --git a/src/app/controllers/providers_controller.rb b/src/app/controllers/providers_controller.rb
index 69252cc..969281c 100644
--- a/src/app/controllers/providers_controller.rb
+++ b/src/app/controllers/providers_controller.rb
@@ -119,14 +119,13 @@ class ProvidersController < ApplicationController
     require_privilege(Privilege::MODIFY, @provider)
     @provider.attributes = params[:provider]
     provider_disabled = @provider.enabled_changed? && !@provider.enabled
+
+    if provider_disabled
+      disable_provider
+      return
+    end
+
     if @provider.save
-      # stop running instances when a provider is disabled
-      # it would be better to have this in provider's observer
-      # but we need to display error message when an error occurs
-      if provider_disabled
-        errs = @provider.stop_instances(current_user)
-        flash[:error] = errs unless errs.empty?
-      end
       flash[:notice] = t"providers.flash.notice.updated"
       redirect_to edit_provider_path(@provider)
     else
@@ -177,6 +176,30 @@ class ProvidersController < ApplicationController
     @providers = Provider.list_for_user(current_user, Privilege::VIEW)
   end
 
+  def disable_provider
+    @instances_to_terminate = @provider.instances_to_terminate
+    if @instances_to_terminate.any? and not params[:terminate]
+      render :action => "confirm_terminate"
+      return
+    end
+
+    res = @provider.disable(current_user)
+    if res[:failed_to_stop].present?
+      flash[:error] = {
+        :summary => t("providers.flash.warning.not_stopped_instances"),
+        :failures => res[:failed_to_stop]
+      }
+    elsif res[:failed_to_terminate].present?
+      flash[:error] = {
+        :summary => t("providers.flash.warning.not_terminated_instances"),
+        :failures => res[:failed_to_terminate].map {|i| i.name}
+      }
+    else
+      flash[:notice] = t"providers.flash.notice.disabled"
+    end
+    redirect_to edit_provider_path(@provider)
+  end
+
   def provider_alerts(provider)
     alerts = []
 
diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb
index 01c0d3e..44f403c 100644
--- a/src/app/models/provider.rb
+++ b/src/app/models/provider.rb
@@ -108,25 +108,48 @@ class Provider < ActiveRecord::Base
   def supported_types
   end
 
+  def disable(user)
+    res = {}
+    if valid_framework?
+      # if we can connect to the provider, try to stop running instances
+      # TODO: now provider is disabled even if stop request fails, is it ok?
+      res[:failed_to_stop] = stop_instances(user)
+    else
+      # if the provider is not accessible and there are no running
+      # instances, we just change state of all instances to stopped
+      res[:failed_to_terminate] = stoppable_instances.select do |i|
+        !i.update_attributes(:state => Instance::STATE_STOPPED)
+      end
+    end
+    if res[:failed_to_stop].blank? and res[:failed_to_terminate].blank?
+      update_attribute(:enabled, false)
+    end
+    res
+  end
+
+  def instances_to_terminate
+    valid_framework? ? [] : stoppable_instances
+  end
+
+  protected
+
   def stop_instances(user)
     errs = []
-    provider_accounts.each do |pa|
-      pa.instances.stopable.each do |instance|
-        begin
-          unless instance.valid_action?('stop')
-            raise "stop is an invalid action."
-          end
+    stoppable_instances.each do |instance|
+      begin
+        unless instance.valid_action?('stop')
+          raise "stop is an invalid action."
+        end
 
-          unless @task = instance.queue_action(user, 'stop')
-            raise "stop cannot be performed on this instance."
-          end
-          Taskomatic.stop_instance(@task)
-        rescue Exception => e
-          err = "Error while stopping an instance #{instance.name}: #{e.message}"
-          errs << err
-          logger.error err
-          logger.error e.backtrace.join("\n  ")
+        unless @task = instance.queue_action(user, 'stop')
+          raise "stop cannot be performed on this instance."
         end
+        Taskomatic.stop_instance(@task)
+      rescue Exception => e
+        err = "Error while stopping an instance #{instance.name}: #{e.message}"
+        errs << err
+        logger.error err
+        logger.error e.backtrace.join("\n  ")
       end
     end
     errs
@@ -192,7 +215,10 @@ class Provider < ActiveRecord::Base
 
   end
 
-  protected
+  def stoppable_instances
+    provider_accounts.inject([]) {|all, pa| all += pa.instances.stopable}
+  end
+
   def validate_provider
     if !nil_or_empty(url)
       errors.add("url", "must be a valid provider url") unless valid_framework?
diff --git a/src/app/views/providers/confirm_terminate.html.haml b/src/app/views/providers/confirm_terminate.html.haml
new file mode 100644
index 0000000..9df53f5
--- /dev/null
+++ b/src/app/views/providers/confirm_terminate.html.haml
@@ -0,0 +1,14 @@
+= render :partial => 'layouts/admin_nav'
+= render :partial => 'section_header'
+
+%section.admin-content-section
+  %header
+    %h2= t(".disable_provider?")
+
+  .content
+    = t ".terminate_instances"
+    %ul
+      - @instances_to_terminate.each do |i|
+        %li= i.name
+    = link_to t('cancel'), edit_provider_path(@provider), :class => 'button'
+    = button_to "Disable", provider_path(@provider, :terminate => true, :provider => {:enabled => false}), :class => 'button', :method => :put, :id => 'disable_button'
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index 2d1749a..0467072 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -973,18 +973,26 @@ en:
       test_connection: Test Connection
       required_field: Required field.
       caution_image: Caution
+    confirm_terminate:
+      terminate_instances: "Provider is not accessible, status of following instances will be changed to 'stopped' but their actual state is unknown."
+      disabled_provider: "Disable Provider"
+      disable: "Disable"
     flash:
       notice:
         added: "Provider added."
         updated: "Provider updated."
+        disabled: "Provider disabled."
         connected: "Successfully Connected to Provider"
         deleted: "Provider has been successfully deleted."
       error:
         not_added: "Cannot add the provider."
         not_updated: "Cannot update the provider."
+        not_disabled: "Cannot disabled the provider."
         not_deleted: "Provider was not deleted"
       warning:
         connect_failed: "Failed to connect to Provider!"
+        not_stopped_instances: "Provider was not disabled, failed to stop following instances:"
+        not_terminated_instances: "Provider was not disabled, failed to change status to 'stopped' for following instances:"
     alerts:
       type:
         critical: Critical
diff --git a/src/features/provider.feature b/src/features/provider.feature
index bcce43c..85a4568 100644
--- a/src/features/provider.feature
+++ b/src/features/provider.feature
@@ -92,6 +92,18 @@ Feature: Manage Providers
     And I should see "Provider is disabled."
     And I should not see "Error while stopping an instance"
 
+  Scenario: Disable an inaccessible provider
+    Given there is a provider named "provider1"
+    And this provider has a provider account with 2 running instances
+    And provider "provider1" is not accessible
+    When I go to the provider1's edit provider page
+    And I press "provider_submit"
+    Then I should see "Provider is not accessible, status of following instances will be changed to 'stopped'"
+    When I press "disable_button"
+    Then I should be on the provider1's edit provider page
+    And I should see "Provider is disabled."
+    And provider "provider1" should have all instances stopped
+
   Scenario: Persist selected provider
     Given there is a provider named "provider1"
     And there is a provider named "provider2"
diff --git a/src/features/step_definitions/provider_steps.rb b/src/features/step_definitions/provider_steps.rb
index 9699211..fc25f87 100644
--- a/src/features/step_definitions/provider_steps.rb
+++ b/src/features/step_definitions/provider_steps.rb
@@ -26,6 +26,10 @@ Given /^there is a provider named "([^\"]*)"$/ do |name|
   @provider = FactoryGirl.create(:mock_provider, :name => name)
 end
 
+Given /^provider "([^"]*)" is not accessible$/ do |arg1|
+  Provider.any_instance.stub(:valid_framework?).and_return(false)
+end
+
 Then /^I should have a provider named "([^\"]*)"$/ do |name|
   Provider.find_by_name(name).should_not be_nil
 end
@@ -117,3 +121,8 @@ end
 When /^I click on the Providers icon in the menu$/ do
   find('#administer_nav a.providers').click
 end
+
+Then /^provider "([^"]*)" should have all instances stopped$/ do |arg1|
+  p = Provider.find_by_name(arg1)
+  p.instances_to_terminate.should be_empty
+end
diff --git a/src/spec/models/provider_spec.rb b/src/spec/models/provider_spec.rb
index fbb4729..ca6843e 100644
--- a/src/spec/models/provider_spec.rb
+++ b/src/spec/models/provider_spec.rb
@@ -104,10 +104,20 @@ describe Provider do
       pa = FactoryGirl.create(:mock_provider_account, :provider => provider1)
       inst1 = FactoryGirl.create(:instance, :provider_account => pa)
       inst2 = FactoryGirl.create(:instance, :provider_account => pa)
-      errs = provider1.stop_instances(nil)
+      errs = provider1.disable(nil)[:failed_to_stop]
       errs.should be_empty
     end
 
+    it "should not disable provider if instance stop action fails" do
+      provider1 = Factory.create(:mock_provider)
+      pa = FactoryGirl.create(:mock_provider_account, :provider => provider1)
+      inst1 = FactoryGirl.create(:instance, :provider_account => pa)
+      Instance.any_instance.stub(:valid_action?).and_return(false)
+      errs = provider1.disable(nil)[:failed_to_stop]
+      errs.should_not be_empty
+      provider1.enabled.should be_true
+    end
+
   end
 
   context "(using original connect method)" do
@@ -122,4 +132,22 @@ describe Provider do
     end
   end
 
+  context "(inaccessible provider with running instance)" do
+    before(:each) do
+      @provider = FactoryGirl.create(:mock_provider)
+      @provider.stub!(:valid_framework?).and_return(false)
+      acc = FactoryGirl.create(:mock_provider_account)
+      inst = FactoryGirl.create(:instance, :provider_account => acc)
+      @provider.provider_accounts << acc
+    end
+
+    it "should return list of instances to terminate" do
+      @provider.instances_to_terminate.should_not be_empty
+    end
+
+    it "should return empty list of not terminated instances when disabled" do
+      @provider.disable(nil)[:failed_to_terminate].should be_empty
+    end
+  end
+
 end
-- 
1.7.7.5




More information about the aeolus-devel mailing list