From: Jan Provaznik <jprovazn(a)redhat.com&gt; Most important change is that quota.running_instances now includes 'pending' and 'shutting_down' instances too. Till now only 'running' instances were counted. Also an instance is set as 'pending' once it's passed to condor in condormatic_instance_create. Above changes are important because of (mostly) removing atomic problem with quota check: running instances quota is checked before passing job to condor, but instances running state is set in dbomatic (after instance is really running) -> in this time quota check didn't work properly. --- src/app/models/instance_observer.rb | 21 ++++++++++----------- src/app/util/condormatic.rb | 3 ++- src/spec/models/instance_observer_spec.rb | 20 +++++++++++++++++++- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/app/models/instance_observer.rb b/src/app/models/instance_observer.rb index eedf813..800da10 100644 --- a/src/app/models/instance_observer.rb +++ b/src/app/models/instance_observer.rb @@ -1,6 +1,7 @@ class InstanceObserver < ActiveRecord::Observer ACTIVE_STATES = [ Instance::STATE_PENDING, Instance::STATE_RUNNING, Instance::STATE_SHUTTING_DOWN, Instance::STATE_STOPPED ] + RUNNING_STATES = [ Instance::STATE_PENDING, Instance::STATE_RUNNING, Instance::STATE_SHUTTING_DOWN ] def before_save(an_instance) if an_instance.changed? @@ -42,18 +43,16 @@ class InstanceObserver < ActiveRecord::Observer if parent quota = parent.quota if quota - if state_to == Instance::STATE_RUNNING - quota.running_instances += 1 - elsif state_from == Instance::STATE_RUNNING - quota.running_instances -= 1 - end + if !RUNNING_STATES.include?(state_from) && RUNNING_STATES.include?(state_to) + quota.running_instances += 1 + elsif RUNNING_STATES.include?(state_from) && !RUNNING_STATES.include?(state_to) + quota.running_instances -= 1 + end - if state_from != nil - if !ACTIVE_STATES.include?(state_from) && ACTIVE_STATES.include?(state_to) - quota.total_instances += 1 - elsif ACTIVE_STATES.include?(state_from) && !ACTIVE_STATES.include?(state_to) - quota.total_instances -= 1 - end + if !ACTIVE_STATES.include?(state_from) && ACTIVE_STATES.include?(state_to) + quota.total_instances += 1 + elsif ACTIVE_STATES.include?(state_from) && !ACTIVE_STATES.include?(state_to) + quota.total_instances -= 1 end quota.save! end diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb index 107ad6e..4b42dee 100644 --- a/src/app/util/condormatic.rb +++ b/src/app/util/condormatic.rb @@ -67,7 +67,7 @@ def match(instance) # FIXME: we should have something smarter here that prioritizes # and/or chooses the "cheapest" possibility. For now, just return the # first that fits under quota - if Quota.can_create_instance?(instance, match.account) + if Quota.can_start_instance?(instance, match.account) return match end end @@ -167,6 +167,7 @@ def condormatic_instance_create(task) instance.state = Instance::STATE_CREATE_FAILED else task.state = Task::STATE_PENDING + instance.state = Instance::STATE_PENDING end instance.save! task.save! diff --git a/src/spec/models/instance_observer_spec.rb b/src/spec/models/instance_observer_spec.rb index 767712b..69d702c 100644 --- a/src/spec/models/instance_observer_spec.rb +++ b/src/spec/models/instance_observer_spec.rb @@ -139,7 +139,10 @@ describe InstanceObserver do end end - it "should update a pool, cloud account and user quota when an instance state goes from running to another active state" do + it "should not update pool, cloud account and user quota when an instance state goes from pending to running to shutting down" do + @instance.state = Instance::STATE_PENDING + @instance.save! + @instance.state = Instance::STATE_RUNNING @instance.save! @@ -148,6 +151,21 @@ describe InstanceObserver do [@provider_account_quota, @pool_quota, @user_quota].each do |quota| quota = Quota.find(quota.id) + quota.running_instances.should == 1 + quota.total_instances.should == 1 + end + end + + + it "should update a pool, cloud account and user quota when an instance state goes from running to stopped state" do + @instance.state = Instance::STATE_RUNNING + @instance.save! + + @instance.state = Instance::STATE_STOPPED + @instance.save! + + [@provider_account_quota, @pool_quota, @user_quota].each do |quota| + quota = Quota.find(quota.id) quota.running_instances.should == 0 quota.total_instances.should == 1