Re: BZ 788558 - allow terminate inaccessible instances on 'stop' action

From: Jan Provaznik <jprovazn(a)redhat.com&gt; Theis patch allows locally stopping of instances or deployments if the instance's/deployment's provider is not accessible. It fixes a bug in passing user param to queue_action method, also simplifies used error messages - only (stop|reboot)_invalid_action can be raised. This patch doesn't cover deletion of deployments which are not accessible - this is difficult because there is a before_destroy hook on deployment which tries to destroy instances on provider side. --- src/app/controllers/deployments_controller.rb | 43 ++++++++---- src/app/controllers/instances_controller.rb | 52 +++++++------- src/app/models/deployment.rb | 17 +++++ src/app/models/instance.rb | 35 ++++++++-- src/app/models/provider.rb | 15 +++- src/app/stylesheets/custom.scss | 2 +- .../views/deployments/confirm_terminate.html.haml | 16 +++++ .../views/instances/confirm_terminate.html.haml | 16 +++++ .../views/providers/confirm_terminate.html.haml | 5 +- src/config/database.yml.0 | 72 ++++++++++++++++++++ src/config/locales/cs.yml | 1 - src/config/locales/en.yml | 12 ++-- src/features/deployment.feature | 12 +++ src/features/instance.feature | 11 +++ src/features/step_definitions/deployment_steps.rb | 7 ++ src/features/step_definitions/instance_steps.rb | 8 ++ 16 files changed, 265 insertions(+), 59 deletions(-) create mode 100644 src/app/views/deployments/confirm_terminate.html.haml create mode 100644 src/app/views/instances/confirm_terminate.html.haml create mode 100644 src/config/database.yml.0 diff --git a/src/app/controllers/deployments_controller.rb b/src/app/controllers/deployments_controller.rb index 588e88e..15050fc 100644 --- a/src/app/controllers/deployments_controller.rb +++ b/src/app/controllers/deployments_controller.rb @@ -18,6 +18,7 @@ class DeploymentsController < ApplicationController before_filter :require_user before_filter :load_deployments, :only => [:index, :show] before_filter :load_deployment, :only => [:edit, :update] + before_filter :check_inaccessible_instances, :only => :multi_stop viewstate :show do |default| @@ -267,6 +268,7 @@ class DeploymentsController < ApplicationController def multi_destroy destroyed = [] failed = [] + Deployment.find(params[:deployments_selected] || []).each do |deployment| if check_privilege(Privilege::MODIFY, deployment) begin @@ -296,28 +298,28 @@ class DeploymentsController < ApplicationController def multi_stop notices = [] errors = [] - Deployment.find(params[:deployments_selected] || []).each do |deployment| + + @deployments_to_stop.each do |deployment| deployment.instances.each do |instance| + log_prefix = "#{t('deployments.deployment')}: #{instance.deployment.name}, #{t('instances.instance')}: #{instance.name}" begin - require_privilege(Privilege::USE,instance) - unless instance.valid_action?('stop') - raise ActionError.new(t('deployments.errors.stop_invalid_action')) - end - - #permissons check here - @task = instance.queue_action(current_user, 'stop') - unless @task - raise ActionError.new(t('deployments.errors.cannot_stop')) + require_privilege(Privilege::USE, instance) + if @inaccessible_instances.include?(instance) + instance.force_stop(current_user) + notices << "#{log_prefix}: #{t('instances.flash.notice.forced_stop')}" + else + instance.stop(current_user) + notices << "#{log_prefix}: #{t('instances.flash.notice.stop')}" end - Taskomatic.stop_instance(@task) - notices << "#{t('deployments.deployment')}: #{instance.deployment.name}, #{t('instances.instance')}: #{instance.name}: #{t('deployments.flash.notice.stop')}" rescue Exception => err - errors << "#{t('deployments.deployment')}: #{instance.deployment.name}, #{t('instances.instance')}: #{instance.name}: " + err + errors << "#{log_prefix}: #{err}" + logger.error err.message + logger.error err.backtrace.join("\n ") end end end # If nothing is selected, display an error message: - errors = t('deployments.flash.error.none_selected') if errors.blank? && notices.blank? + errors = t('deployments.flash.error.none_selected') if notices.blank? and errors.blank? flash[:notice] = notices unless notices.blank? flash[:error] = errors unless errors.blank? respond_to do |format| @@ -350,6 +352,19 @@ class DeploymentsController < ApplicationController private + def check_inaccessible_instances + @deployments_to_stop = Deployment.find(params[:deployments_selected] || []) + @inaccessible_instances = Deployment.stoppable_inaccessible_instances(@deployments_to_stop) + if params[:terminate].blank? and @inaccessible_instances.any? + respond_to do |format| + format.html { render :action => :confirm_terminate } + format.json { render :json => {:inaccessbile_instances => @inaccessible_instances}, :status => :unprocessable_entity } + end + return false + end + return true + end + def load_deployments @deployments_header = [ { :name => 'checkbox', :class => 'checkbox', :sortable => false }, diff --git a/src/app/controllers/instances_controller.rb b/src/app/controllers/instances_controller.rb index a44e7be..987ff9f 100644 --- a/src/app/controllers/instances_controller.rb +++ b/src/app/controllers/instances_controller.rb @@ -18,6 +18,7 @@ class InstancesController < ApplicationController before_filter :require_user before_filter :load_instance, :only => [:show, :key, :edit, :update] before_filter :set_view_vars, :only => [:show, :index, :export_events] + before_filter :check_inaccessible_instances, :only => :multi_stop def index @params = params @@ -139,27 +140,26 @@ class InstancesController < ApplicationController end def multi_stop - notices = "" - errors = "" - Instance.find(params[:instance_selected] || []).each do |instance| + notices = [] + errors = [] + + @instances_to_stop.each do |instance| begin require_privilege(Privilege::USE,instance) - unless instance.valid_action?('stop') - raise ActionError.new(t('instances.errors.stop_invalid')) - end - #permissons check here - @task = instance.queue_action(current_user, 'stop') - unless @task - raise ActionError.new(t('instances.errors.stop_not_be_performed')) + if @inaccessible_instances.include?(instance) + instance.force_stop(current_user) + notices << "#{instance.name}: #{t('instances.flash.notice.forced_stop')}" + else + instance.stop(current_user) + notices << "#{instance.name}: #{t('instances.flash.notice.stop')}" end - Taskomatic.stop_instance(@task) - notices << "#{instance.name}: #{t('instances.flash.notice.stop')}" rescue Exception => err errors << "#{instance.name}: " + err + logger.error err.message + logger.error err.backtrace.join("\n ") end end - # If nothing is selected, display an error message: errors = t('instances.none_selected') if errors.blank? && notices.blank? flash[:notice] = notices unless notices.blank? flash[:error] = errors unless errors.blank? @@ -176,11 +176,11 @@ class InstancesController < ApplicationController end def stop - do_operation(:stop) + do_operation(current_user, :stop) end def reboot - do_operation(:reboot) + do_operation(current_user, :reboot) end def multi_reboot @@ -189,7 +189,7 @@ class InstancesController < ApplicationController Instance.find(params[:instance_selected] || []).each do |instance| begin require_privilege(Privilege::USE,instance) - instance.reboot + instance.reboot(current_user) notices << "#{instance.name}: #{t('instances.flash.notice.reboot', :name => instance.name)}" rescue Exception => err errors << "#{instance.name}: " + err @@ -250,16 +250,16 @@ class InstancesController < ApplicationController end end - def do_operation(operation) - instance = Instance.find(params[:id]) - begin - instance.send(operation) - flash[:notice] = t("instances.flash.notice.#{operation}", :name => instance.name) - rescue Exception => err - flash[:error] = t('instance.error', :name => instance.name, :err => err) - end - respond_to do |format| - format.html { redirect_to deployment_path(instance.deployment, :details_tab => 'instances')} + def check_inaccessible_instances + @instances_to_stop = Instance.find(params[:instance_selected] || []) + @inaccessible_instances = Instance.stoppable_inaccessible_instances(@instances_to_stop) + if params[:terminate].blank? and @inaccessible_instances.any? + respond_to do |format| + format.html { render :action => :confirm_terminate } + format.json { render :json => {:inaccessbile_instances => @inaccessible_instances}, :status => :unprocessable_entity } + end + return false end + return true end end diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb index 9593645..880afdd 100644 --- a/src/app/models/deployment.rb +++ b/src/app/models/deployment.rb @@ -171,6 +171,18 @@ class Deployment < ActiveRecord::Base end end + def self.stoppable_inaccessible_instances(deployments) + failed_accounts = {} + res = [] + deployments.each do |d| + next unless acc = d.provider_account + failed_accounts[acc.id] = acc.connect.nil? unless failed_accounts.has_key?(acc.id) + next unless failed_accounts[acc.id] + res += d.instances.stoppable_inaccessible + end + res + end + def launch_parameters @launch_parameters ||= {} end @@ -329,6 +341,11 @@ class Deployment < ActiveRecord::Base inst && inst.provider_account && inst.provider_account.provider end + def provider_account + inst = instances.joins(:provider_account).first + inst && inst.provider_account + end + # we try to create an instance for each assembly and check # if a match is found def check_assemblies_matches(user) diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb index 0042074..e6ce567 100644 --- a/src/app/models/instance.rb +++ b/src/app/models/instance.rb @@ -114,6 +114,8 @@ class Instance < ActiveRecord::Base STATE_SHUTTING_DOWN, STATE_STOPPED, STATE_CREATE_FAILED, STATE_ERROR, STATE_VANISHED] + STOPPABLE_INACCESSIBLE_STATES = [STATE_NEW, STATE_PENDING, STATE_RUNNING, STATE_SHUTTING_DOWN] + scope :deployed, :conditions => { :state => [STATE_RUNNING, STATE_SHUTTING_DOWN] } # FIXME: "pending" is misleading as it doesn't just cover STATE_PENDING scope :pending, :conditions => { :state => [STATE_NEW, STATE_PENDING] } @@ -122,6 +124,7 @@ class Instance < ActiveRecord::Base scope :stopped, :conditions => {:state => STATE_STOPPED} scope :not_stopped, :conditions => "state <> 'stopped'" scope :stopable, :conditions => { :state => [STATE_NEW, STATE_PENDING, STATE_RUNNING] } + scope :stoppable_inaccessible, :conditions => { :state => STOPPABLE_INACCESSIBLE_STATES } scope :ascending_by_name, :order => 'instances.name ASC' @@ -434,12 +437,20 @@ class Instance < ActiveRecord::Base not deployment.instances.deployed.any? {|i| i != self} end - def stop - do_operation('stop') + def stop(user) + do_operation(user, 'stop') + end + + def reboot(user) + do_operation(user, 'reboot') end - def reboot - do_operation('reboot') + def force_stop(user) + self.state = STATE_STOPPED + save! + event = Event.create!(:source => self, :event_time => Time.now, + :summary => "Instance is not accessible, state changed to stopped", + :status_code => "force_stop") end def deployed? @@ -466,6 +477,16 @@ class Instance < ActiveRecord::Base end end + def self.stoppable_inaccessible_instances(instances) + failed_accounts = {} + instances.select do |i| + next unless STOPPABLE_INACCESSIBLE_STATES.include?(i.state) + next unless i.provider_account + failed_accounts[i.provider_account.id] = i.provider_account.connect.nil? unless failed_accounts.has_key?(i.provider_account.id) + failed_accounts[i.provider_account.id] + end + end + private def self.apply_search_filter(search) @@ -488,10 +509,10 @@ class Instance < ActiveRecord::Base self[:uuid] = UUIDTools::UUID.timestamp_create.to_s end - def do_operation(operation) - @task = self.queue_action(@current_user, operation) + def do_operation(user, operation) + @task = self.queue_action(user, operation) unless @task - raise I18n.t("instances.errors.#{operation}_not_be_performed") + raise I18n.t("instances.errors.#{operation}_invalid_action") end Taskomatic.send("#{operation}_instance", @task) end diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb index 22e4e95..9d5877c 100644 --- a/src/app/models/provider.rb +++ b/src/app/models/provider.rb @@ -117,8 +117,16 @@ class Provider < ActiveRecord::Base 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) + res[:failed_to_terminate] = instances_to_terminate.select do |i| + begin + i.update_attributes(:state => Instance::STATE_STOPPED) + false + rescue + true + # this should never happen, so display an error only in log file + logger.warn "failed to stop instance #{i.name}: #{$!.message}" + logger.warn $!.backtrace.join("\n ") + end end end if res[:failed_to_stop].blank? and res[:failed_to_terminate].blank? @@ -128,7 +136,8 @@ class Provider < ActiveRecord::Base end def instances_to_terminate - valid_framework? ? [] : stoppable_instances + return [] if valid_framework? + provider_accounts.inject([]) {|all, pa| all += pa.instances.stoppable_inaccessible} end def populate_realms diff --git a/src/app/stylesheets/custom.scss b/src/app/stylesheets/custom.scss index ee1ce34..be3e1ec 100644 --- a/src/app/stylesheets/custom.scss +++ b/src/app/stylesheets/custom.scss @@ -245,7 +245,7 @@ form.generic section.services { margin-bottom: 30px; } -.instance_details { +.instance_details, .confirm_instances_termination { h3{ font-size: 22px; diff --git a/src/app/views/deployments/confirm_terminate.html.haml b/src/app/views/deployments/confirm_terminate.html.haml new file mode 100644 index 0000000..2ac762f --- /dev/null +++ b/src/app/views/deployments/confirm_terminate.html.haml @@ -0,0 +1,16 @@ +%header.page-header + %h1{:class => controller.controller_name}= t "instances.confirm_terminate.terminate_instances" + .corner &nbsp; + +%section.content-section + .content.confirm_instances_termination + .align-center + %strong= t('instances.confirm_terminate.terminate_instances_description') + = form_tag multi_stop_deployments_path, :method => :post do + - @deployments_to_stop.each do |i| + = hidden_field_tag 'deployments_selected[]', i.id + %ul + - @inaccessible_instances.each do |i| + %li= i.name + = link_to t('cancel'), pools_path(:view => 'filter', :details_tab => 'deployments'), :class => 'button' + = submit_tag t('instances.confirm_terminate.terminate'), :name => 'terminate', :class => 'button', :id => 'terminate_button' diff --git a/src/app/views/instances/confirm_terminate.html.haml b/src/app/views/instances/confirm_terminate.html.haml new file mode 100644 index 0000000..ea680f2 --- /dev/null +++ b/src/app/views/instances/confirm_terminate.html.haml @@ -0,0 +1,16 @@ +%header.page-header + %h1{:class => controller.controller_name}= t ".terminate_instances" + .corner &nbsp; + +%section.content-section + .content.confirm_instances_termination + .align-center + %strong= t('.terminate_instances_description') + = form_tag multi_stop_instances_path, :method => :post do + - @instances_to_stop.each do |i| + = hidden_field_tag 'instance_selected[]', i.id + %ul + - @inaccessible_instances.each do |i| + %li= i.name + = link_to t('cancel'), pools_path(:view => 'filter', :details_tab => 'instances'), :class => 'button' + = submit_tag t('.terminate'), :name => 'terminate', :class => 'button', :id => 'terminate_button' diff --git a/src/app/views/providers/confirm_terminate.html.haml b/src/app/views/providers/confirm_terminate.html.haml index 9df53f5..ef60061 100644 --- a/src/app/views/providers/confirm_terminate.html.haml +++ b/src/app/views/providers/confirm_terminate.html.haml @@ -5,8 +5,9 @@ %header %h2= t(".disable_provider?") - .content - = t ".terminate_instances" + .content.confirm_instances_termination + .align-center + %strong= t('instances.confirm_terminate.terminate_instances_description') %ul - @instances_to_terminate.each do |i| %li= i.name diff --git a/src/config/database.yml.0 b/src/config/database.yml.0 new file mode 100644 index 0000000..2deb8ca --- /dev/null +++ b/src/config/database.yml.0 @@ -0,0 +1,72 @@ +# PostgresSQL. Versions 8.2 recommended. +# +# Fedora Install Instructions: +# sudo yum install ruby-postgres postgresql-server +# sudo /sbin/service postgresql initdb +# +# On a local system you may need to edit this file to have only the following +# /var/lib/pgsql/data/pg_hba.conf +# local all all trust +# host all all 127.0.0.1/32 trust +# +# sudo /sbin/service postgresql start +# sudo su - postgres +# $ psql +# $-# CREATE USER aeolus WITH PASSWORD 'v23zj59an' CREATEDB; +# $-# \q +# $ exit +# +# To start off a new environment, use the following commands. +# If running from /usr/share/aeolus-conductor, use 'sudo' or run as root +# as the migrate will write to a file. +# +# rake db:drop:all +# rake db:create:all +# +# If you are running this in /usr/share/aeolus-conductor, use: +# sudo RAILS_ENV=production rake db:migrate +# +# or else just: +# rake db:migrate +# +# rake db:test:prepare +# +# And when there are changes to the database, simply run: +# +# rake db:migrate +# rake db:test:prepare +# for the environment you wish to update. +# + + +development: + adapter: postgresql + database: conductor_development + username: aeolus + password: v23zj59an + host: localhost + allow_concurrency: true + min_messages: warning + +# Warning: The database defined as 'test' will be erased and +# re-generated from your development database when you run 'rake test'. +# Do not set this db to the same as development or production. +test: &TEST + adapter: postgresql + database: conductor_test + username: aeolus + password: v23zj59an + host: localhost + allow_concurrency: true + min_messages: warning + +production: + adapter: postgresql + database: conductor + username: aeolus + password: v23zj59an + host: localhost + allow_concurrency: true + +cucumber: + <<: *TEST diff --git a/src/config/locales/cs.yml b/src/config/locales/cs.yml index 7ada0cc..6b0f7b4 100644 --- a/src/config/locales/cs.yml +++ b/src/config/locales/cs.yml @@ -476,7 +476,6 @@ cs: invalid_state: okhbeh kigib bog mhknokdmj klonm kcolb ndjhmhk neafi no_config_server_available: "%{account_name}: ae hmenoo lidmgk mboinakkm boc cnfcjbli abicnag" user_quota_reached: kone bdgjo kbggdbl - stop_not_be_performed: cdim lfnggh nk ogldenhne gk mhde makmngjfl provider_account_quota_too_low: "%{match_provider_account} ifcjl omcdh lkm dke gi gkcomo efehjbfgnl" provider_account_quota_reached: "%{account_name}: boefladm ebbjhmd caidj klmghik" image_not_pushed_to_provider: "%{account_name}: ofeif km fef minkbc gc bcei gncmokeo hgeickk" diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml index addb68e..2e4f8c9 100644 --- a/src/config/locales/en.yml +++ b/src/config/locales/en.yml @@ -279,7 +279,6 @@ en: other: "The deployments %{list} were successfully updated." errors: config_server_connection: 'Cannot connect to the config server' - stop_invalid_action: "stop is an invalid action." cannot_stop: "stop cannot be performed on this instance." all_stopped: "All instances must be stopped or running" not_valid_deployable_xml: "seems to be not valid deployable XML: %{msg}" @@ -343,6 +342,10 @@ en: shared_to: "Shared to: " error: "Error: " n_a: "N/A" + confirm_terminate: + terminate: "Terminate" + terminate_instances: "Terminate Instances" + terminate_instances_description: "Provider is not accessible, status of following instances will be changed to 'stopped' but their actual state is unknown." flash: warning: ssh_key_not_found: "SSH Key not found for this Instance." @@ -363,6 +366,7 @@ en: other: "The instances %{list} were successfully deleted." notice: stop: "stop action was successfully queued." + forced_stop: "state changed to stopped." reboot: "%{name}: reboot action was successfully queued." errors: image_not_found: "No image build was found with uuid %{b_uuid} and no image was found with uuid %{i_uuid}" @@ -378,9 +382,8 @@ en: no_config_server_available: "%{account_name}: no config server available for provider account" realm_not_mapped: "%{account_name}: realm %{frontend_realm_name} is not mapped to any applicable provider or provider realm" provider_account_quota_too_low: "%{match_provider_account} quota limit too low to launch deployable" - stop_invalid: "stop is an invalid action." - stop_not_be_performed: "stop cannot be performed on this instance." - reboot_not_be_performed: "cannot be rebooted because it's not currently running." + stop_invalid_action: "stop is an invalid action." + reboot_invalid_action: "reboot is an invalid action." must_be_enabled: "%{account_name}: provider must be enabled" provider_not_available: "%{account_name}: provider is not available" cannot_destroy: "destroy cannot be performed on this instance." @@ -1007,7 +1010,6 @@ en: 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: diff --git a/src/features/deployment.feature b/src/features/deployment.feature index 7b6a663..b9e195e 100644 --- a/src/features/deployment.feature +++ b/src/features/deployment.feature @@ -82,6 +82,18 @@ Feature: Manage Deployments And I press "stop_button" Then I should see "testdeployment" + Scenario: Stop inaccessible deployments + Given there is a deployment named "testdeployment" belonging to "testdeployable" owned by "testuser" + And "testdeployment" deployment's provider is not accessible + When I go to the deployments page + Then I should see "testdeployment" + When I check "testdeployment" deployment + And I press "stop_button" + Then I should see "Terminate Instances" + When I press "terminate_button" + Then I should see "state changed to stopped" + + Scenario: Stop a deployment over XHR Given there is a deployment named "testdeployment" belonging to "testdeployable" owned by "testuser" And I request XHR diff --git a/src/features/instance.feature b/src/features/instance.feature index 1d91333..446ee53 100644 --- a/src/features/instance.feature +++ b/src/features/instance.feature @@ -71,6 +71,17 @@ Feature: Manage Instances Then I should be on the instances page And I should see "mock1: stop action was successfully queued" + Scenario: Stop inaccessible instance + Given there is a "mock1" running instance + And I am on the instances page + And "mock1" instance's provider is not accessible + When I check "mock1" instance + And I press "stop_selected_instances" + Then I should see "Terminate Instances" + And I should see "mock1" + When I press "terminate_button" + Then I should see "mock1: state changed to stopped" + Scenario: Stop multiple instances Given there is a "mock1" running instance And there is a "mock2" running instance diff --git a/src/features/step_definitions/deployment_steps.rb b/src/features/step_definitions/deployment_steps.rb index 5eedc1d..0640d9d 100644 --- a/src/features/step_definitions/deployment_steps.rb +++ b/src/features/step_definitions/deployment_steps.rb @@ -38,6 +38,13 @@ Given /^the deployment "([^"]*)" has an instance named "([^"]*)"$/ do |d_name, i deployment.instances << FactoryGirl.create(:instance, :name => i_name, :pool => deployment.pool) end +Given /^"([^"]*)" deployment's provider is not accessible$/ do |arg1| + # FIXME: didn't find a way how to create an inaccessible provider + # cleanly + provider = @deployment.provider + provider.update_attribute(:url, 'http://localhost:3002/invalid_api') +end + When /^I am viewing the deployment "([^"]*)"$/ do |arg1| visit deployment_url(Deployment.find_by_name(arg1)) end diff --git a/src/features/step_definitions/instance_steps.rb b/src/features/step_definitions/instance_steps.rb index 268fcdb..5f4d3bb 100644 --- a/src/features/step_definitions/instance_steps.rb +++ b/src/features/step_definitions/instance_steps.rb @@ -39,6 +39,14 @@ Given /^the instance "([^"]*)" is in the (\w*) state$/ do |instance, state| instance.save! end +Given /^"([^"]*)" instance's provider is not accessible$/ do |arg1| + instance = Instance.find_by_name(arg1) + # FIXME: didn't find a way how to create an inaccessible provider + # cleanly + provider = instance.provider_account.provider + provider.update_attribute(:url, 'http://localhost:3002/invalid_api') +end + When /^I am viewing the pending instance detail$/ do visit instance_url(pending_instance) end -- 1.7.7.6