[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