Hi Tomas,
overall looks good, but sending some notes inline.
Jan
On 09/06/2011 05:42 PM, tsedovic(a)redhat.com wrote:
From: Tomas Sedovic<tsedovic(a)redhat.com>
https://www.aeolusproject.org/redmine/issues/2229
This allows providers to be disabled.
A disabled provider is prevented from launching instances.
---
src/app/models/deployment.rb | 2 +-
src/app/models/instance.rb | 16 ++++++++++++++++
src/app/models/pool.rb | 4 ++++
src/app/models/pool_family.rb | 4 ++++
src/app/models/provider_account.rb | 4 ++++
...20110905150738_add_enabled_field_to_provider.rb | 9 +++++++++
src/spec/factories/instance.rb | 8 ++++++++
src/spec/factories/provider.rb | 4 ++++
src/spec/factories/provider_account.rb | 4 ++++
src/spec/models/instance_spec.rb | 12 ++++++++++++
10 files changed, 66 insertions(+), 1 deletions(-)
create mode 100644 src/db/migrate/20110905150738_add_enabled_field_to_provider.rb
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index ad6e5e3..acd195f 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -80,7 +80,7 @@ class Deployment< ActiveRecord::Base
end
def pool_must_be_enabled
- errors.add(:pool, "must be enabled") unless pool and pool.enabled
+ errors.add(:pool, "must be enabled") unless pool and pool.enabled?
end
def object_list
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index fad5b3d..a37d26b 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -116,12 +116,15 @@ class Instance< ActiveRecord::Base
validates_inclusion_of :state,
:in => STATES
+ validate :pool_and_account_enabled_validation
+
This check should be done only before an instance is created. If it's
done as general validation, it won't be possible for example to update
state from dbomatic when an instance is stopped.
before_destroy :destroyable?
# A user should only be able to update certain attributes, but the API may permit
other attributes to be
# changed if called from another Aeolus component, so attr_protected isn't quite
what we want:
USER_MUTABLE_ATTRS = ['name']
+
def object_list
super + [pool, deployment]
end
@@ -158,6 +161,19 @@ class Instance< ActiveRecord::Base
return_val
end
+ def enabled?
+ pool and pool.enabled? and provider_account and provider_account.enabled?
+ end
+
+ def pool_and_account_enabled_validation
+ return if enabled?
+
+ errors.add(:pool, 'must be enabled') unless pool and pool.enabled?
+ unless provider_account and provider_account.enabled?
+ errors.add(:provider_account, 'must be enabled')
+ end
+ end
+
def image
Image.find(image_uuid) if image_uuid
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index d29675d..76516b7 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -76,6 +76,10 @@ class Pool< ActiveRecord::Base
instances.all? {|i| i.destroyable? }
end
+ def enabled?
+ self.enabled and pool_family and pool_family.enabled?
+ end
+
# TODO: Implement Alerts and Updates
def statistics
# TODO - Need to set up cache invalidation before this is safe
diff --git a/src/app/models/pool_family.rb b/src/app/models/pool_family.rb
index 1c82dbe..e80427b 100644
--- a/src/app/models/pool_family.rb
+++ b/src/app/models/pool_family.rb
@@ -64,4 +64,8 @@ class PoolFamily< ActiveRecord::Base
# A PoolFamily is destroyable unless it is the default PoolFamily
self != PoolFamily.default
end
+
+ def enabled?
+ provider_accounts.all? {|account| account.enabled?}
+ end
I think it should return false only if all accounts are disabled.
For example if only one account associated with a pool family is
disabled, I would expect that pool family is still usable (if it has
other associated provider accounts)
end
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index 538c4d6..6b3b6e9 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -123,6 +123,10 @@ class ProviderAccount< ActiveRecord::Base
instances.empty? || instances.all? { |i| i.destroyable? }
end
+ def enabled?
+ provider and provider.enabled?
+ end
+
def connect
begin
return DeltaCloud.new(credentials_hash['username'],
credentials_hash['password'], provider.url)
diff --git a/src/db/migrate/20110905150738_add_enabled_field_to_provider.rb
b/src/db/migrate/20110905150738_add_enabled_field_to_provider.rb
new file mode 100644
index 0000000..110ef16
--- /dev/null
+++ b/src/db/migrate/20110905150738_add_enabled_field_to_provider.rb
@@ -0,0 +1,9 @@
+class AddEnabledFieldToProvider< ActiveRecord::Migration
+ def self.up
+ add_column :providers, :enabled, :boolean, :null => false, :default => true
Could you please set value for existing providers?
something like: Provider.update_all(:enabled => true)
+ end
+
+ def self.down
+ remove_column :providers, :enabled
+ end
+end
diff --git a/src/spec/factories/instance.rb b/src/spec/factories/instance.rb
index de5768b..6fcedb7 100644
--- a/src/spec/factories/instance.rb
+++ b/src/spec/factories/instance.rb
@@ -39,4 +39,12 @@ FactoryGirl.define do
association :instance_key, :factory => :ec2_instance_key1
end
+ factory :instance_with_disabled_provider, :parent => :new_instance do
+ association :provider_account, :factory => :disabled_provider_account
+ end
+
+ factory :instance_in_disabled_pool, :parent => :new_instance do
+ association :pool, :factory => :disabled_pool
+ end
+
end
diff --git a/src/spec/factories/provider.rb b/src/spec/factories/provider.rb
index e62afcc..597d675 100644
--- a/src/spec/factories/provider.rb
+++ b/src/spec/factories/provider.rb
@@ -28,4 +28,8 @@ FactoryGirl.define do
after_create { |p| p.realms<< FactoryGirl.create(:realm4, :provider =>
p) }
end
+ factory :disabled_provider, :parent => :mock_provider do
+ enabled false
+ end
+
end
diff --git a/src/spec/factories/provider_account.rb
b/src/spec/factories/provider_account.rb
index d8a4c5e..be60c0c 100644
--- a/src/spec/factories/provider_account.rb
+++ b/src/spec/factories/provider_account.rb
@@ -26,4 +26,8 @@ FactoryGirl.define do
end
+ factory :disabled_provider_account, :parent => :mock_provider_account do
+ association :provider, :factory => :disabled_provider
+ end
+
end
diff --git a/src/spec/models/instance_spec.rb b/src/spec/models/instance_spec.rb
index 2da0841..2c43738 100644
--- a/src/spec/models/instance_spec.rb
+++ b/src/spec/models/instance_spec.rb
@@ -212,6 +212,18 @@ describe Instance do
export_string.include?("created").should be_true
end
+ it "should not be launchable if its provider is disabled" do
+ instance = FactoryGirl.build(:instance_with_disabled_provider)
+ instance.should_not be_enabled
+ instance.should_not be_valid
+ end
+
+ it "should not be launchable if its pool is disabled" do
+ instance = FactoryGirl.build(:instance_in_disabled_pool)
+ instance.should_not be_enabled
+ instance.should_not be_valid
+ end
+
context "When more instances of deployment are starting" do
it "should return true if first instance of deployment is running" do
deployment = Factory :deployment