Following Scott's suggestion, this adds the `destroyable?` method to the Instance model, which makes it easier to do consistent validation across multiple objects (pools, deployments, provider accounts, etc.).
From: Tomas Sedovic tsedovic@redhat.com
This fixes Redmine #1098
Pools can be deleted only if they either don't have any instances associated with them or if all those instances are stopped and stateless.
This also provides a dummy check to see whether an instance is stateful or stateless. Currently, we only support stateless instances, but when we modify the instance.restartable? method later on, nothing should break. --- src/app/controllers/resources/pools_controller.rb | 20 ++++++++++++++++++-- src/app/models/instance.rb | 13 +++++++++++++ src/app/models/pool.rb | 6 ++++++ src/config/locales/en.yml | 10 +++++++++- src/config/navigation.rb | 2 +- 5 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/src/app/controllers/resources/pools_controller.rb b/src/app/controllers/resources/pools_controller.rb index 337a1b8..04fdf41 100644 --- a/src/app/controllers/resources/pools_controller.rb +++ b/src/app/controllers/resources/pools_controller.rb @@ -73,16 +73,32 @@ class Resources::PoolsController < ApplicationController end
def multi_destroy + destroyed = [] + failed = [] + error_messages = [] Pool.find(params[:pools_selected]).each do |pool| # FIXME: remove this check when pools can be assigned to new users # default_pool cannot be deleted because metadata object has it tied # to id of 1 and deleting it prevents new users from being created if pool.id == MetadataObject.lookup("self_service_default_pool").id - flash[:notice] = "The default pool cannot be deleted" + error_messages << "The default pool cannot be deleted" + elsif check_privilege(Privilege::MODIFY, pool) && pool.destroyable? + pool.destroy + destroyed << pool.name else - pool.destroy if check_privilege(Privilege::MODIFY, pool) + failed << pool.name end end + + unless destroyed.empty? + flash[:notice] = t('pools.index.pool_deleted', :count => destroyed.length, :list => destroyed.join(', ')) + end + unless failed.empty? + error_messages << t('pools.index.pool_not_deleted', :count => failed.length, :list => failed.join(', ')) + end + unless error_messages.empty? + flash[:error] = error_messages.join('<br />') + end redirect_to resources_pools_url end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb index 1841ecc..2f5776d 100644 --- a/src/app/models/instance.rb +++ b/src/app/models/instance.rb @@ -121,6 +121,8 @@ class Instance < ActiveRecord::Base validates_inclusion_of :state, :in => STATES
+ before_destroy :destroyable? + def validate if assembly and template errors.add(:assembly, "Please specify either template or assembly, but not both") @@ -267,6 +269,17 @@ class Instance < ActiveRecord::Base return stats end
+ def restartable? + # TODO: we don't support stateful instances yet, so it's `false` for the time being. + # In the meantime, we can use this method to write validation code for cases + # where does matter whether an instance is stateful or stateless. + false + end + + def destroyable? + (state == STATE_CREATE_FAILED) or (state == STATE_STOPPED and not restartable?) + end + named_scope :with_hardware_profile, lambda { {:include => :hardware_profile} } diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb index ebee420..2e00655 100644 --- a/src/app/models/pool.rb +++ b/src/app/models/pool.rb @@ -60,6 +60,8 @@ class Pool < ActiveRecord::Base :include => [:role], :order => "permissions.id ASC"
+ before_destroy :destroyable? + def cloud_accounts accounts = [] instances.each do |instance| @@ -73,4 +75,8 @@ class Pool < ActiveRecord::Base HardwareProfile.find(:all, :conditions => {:provider_id => nil}) end
+ def destroyable? + instances.all? {|i| i.destroyable? } + end + end diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml index 7510a05..677af3b 100644 --- a/src/config/locales/en.yml +++ b/src/config/locales/en.yml @@ -69,7 +69,6 @@ en: choose_treatment: Choose Treatment apply: Apply resource_management: Resource Management - pools: Pools deployments: Deployments instances: Instances searches: Searches @@ -83,6 +82,15 @@ en: provider_accounts_item: Provider Account cloud_engine_hardware_profiles: Hardware Profiles cloud_engine_realms: Realms + pools: + index: + pools: Pools + pool_deleted: + one: "Pool %{list} was deleted." + other: "Pools %{list} were deleted." + pool_not_deleted: + one: "Pool %{list} was not deleted. There are instances associated with it." + other: "Pools %{list} were not deleted. They have instances associated with them." pool_families: pool_families: Pool Families index: diff --git a/src/config/navigation.rb b/src/config/navigation.rb index b6944a5..78a7862 100644 --- a/src/config/navigation.rb +++ b/src/config/navigation.rb @@ -2,7 +2,7 @@ SimpleNavigation::Configuration.run do |navigation| navigation.autogenerate_item_ids = false navigation.items do |first_level| first_level.item :resource_management, t(:resource_management), resources_pools_path, :highlights_on => /^/$/ do |second_level| - second_level.item :pools, t(:pools), resources_pools_path + second_level.item :pools, t('pools.index.pools'), resources_pools_path second_level.item :deployments, t(:deployments),resources_deployments_path, :highlights_on => /^/$|/deployments/ second_level.item :instances, t(:instances), resources_instances_path end
From: Tomas Sedovic tsedovic@redhat.com
--- src/features/pool.feature | 3 +- .../step_definitions/provider_account_steps.rb | 5 +++- src/spec/models/pool_spec.rb | 26 ++++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/src/features/pool.feature b/src/features/pool.feature index 7f1a00b..6a12f19 100644 --- a/src/features/pool.feature +++ b/src/features/pool.feature @@ -51,7 +51,8 @@ Feature: Manage Pools And I press "Destroy" Then there should only be 1 pools And I should be on the resources pools page - And I should not see "Redhat Voyager Pool" + When I go to the resources pools page + Then I should not see "Redhat Voyager Pool" And I should not see "Amazon Startrek Pool"
Scenario: Create multiple pools diff --git a/src/features/step_definitions/provider_account_steps.rb b/src/features/step_definitions/provider_account_steps.rb index 2dcd427..2b6c49c 100644 --- a/src/features/step_definitions/provider_account_steps.rb +++ b/src/features/step_definitions/provider_account_steps.rb @@ -3,7 +3,10 @@ Given /^the account has an instance associated with it$/ do end
When /^I delete all instances from the account$/ do - @provider_account.instances.each { |i| i.destroy } + @provider_account.instances.each do |i| + i.state = Instance::STATE_STOPPED + i.destroy + end end
Then /^there should be no provider accounts$/ do diff --git a/src/spec/models/pool_spec.rb b/src/spec/models/pool_spec.rb index ca754fa..58ddec4 100644 --- a/src/spec/models/pool_spec.rb +++ b/src/spec/models/pool_spec.rb @@ -22,4 +22,30 @@ describe Pool do u.errors[:name].should =~ /^is too long.*/ end
+ it "should not be destroyable when it has running instances" do + pool = Factory.create(:pool) + Pool.find(pool.id).should be_destroyable + + instance = Factory.create(:instance, :pool_id => pool.id) + Pool.find(pool.id).should_not be_destroyable + + instance.state = Instance::STATE_STOPPED + instance.save! + Pool.find(pool.id).should be_destroyable + end + + it "should not be destroyable when it has stopped stateful instances" do + pool = Factory.build(:pool) + pool.should be_destroyable + + instance = Factory.build(:instance, :pool_id => pool.id) + instance.stub!(:restartable?).and_return(true) + pool.instances << instance + pool.should_not be_destroyable + + instance.state = Instance::STATE_STOPPED + instance.stub!(:restartable?).and_return(true) + pool.should_not be_destroyable + end + end
On Thu, Apr 14, 2011 at 02:43:46PM +0200, tsedovic@redhat.com wrote:
Following Scott's suggestion, this adds the `destroyable?` method to the Instance model, which makes it easier to do consistent validation across multiple objects (pools, deployments, provider accounts, etc.).
ACK to this set.
-- Matt
aeolus-devel@lists.fedorahosted.org