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

Tomas Sedovic tsedovic at redhat.com
Mon Feb 6 12:59:10 UTC 2012


On 02/01/2012 11:35 AM, jprovazn at redhat.com wrote:
> 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

ACK



More information about the aeolus-devel mailing list