A few nits inline:
On Mon, May 21, 2012 at 03:49:23PM +0200, jprovazn(a)redhat.com wrote:
From: Jan Provaznik <jprovazn(a)redhat.com>
https://www.aeolusproject.org/redmine/issues/3320
---
src/app/models/deployment.rb | 45 ++++++++++++++++++++++++++++++++++-
src/app/models/instance.rb | 3 +-
src/spec/models/deployment_spec.rb | 31 ++++++++++++++++++++++++-
3 files changed, 75 insertions(+), 4 deletions(-)
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index 8618489..c410957 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -251,6 +251,7 @@ class Deployment < ActiveRecord::Base
instance_configs = {}
end
+
# TODO: in follow up patch there should be only delayed job call
# on this place
# For now, we just call DC create intance method on foreground, if an error
@@ -607,8 +608,7 @@ class Deployment < ActiveRecord::Base
if instances.all? {|i| i.state == Instance::STATE_RUNNING}
self.state = STATE_RUNNING
elsif Instance::FAILED_STATES.include?(instance.state)
- # TODO: initiate rollback. For now if an error occurs, deployment will
- # stay in pending state
+ deployment_rollback
end
end
@@ -629,4 +629,45 @@ class Deployment < ActiveRecord::Base
self.state = STATE_STOPPED
end
end
+
+ def state_transition_from_rollback_in_progress(instance)
+ # TODO: distinguish if an instance was created on provider side
+ # or error occurred on create_instance request - in such case
+ # the instance has not to be rollbacked
+ if Instance::ACTIVE_FAILED_STATES.include?(instance.state)
+ # if this instance stop failed, whole deployment rollback failed
+ self.state = STATE_ROLLBACK_FAILED
+ elsif instances.all? {|i| i.inactive?}
+ # some other instances might be failed (because their
+ # launch failed), but it shouldn't be a problem if all
+ # running instances stopped correctly
+ self.state = STATE_ROLLBACK_COMPLETE
+ end
+ end
I suspect I missed something, but where is the above actually called
from?
+
+ def deployment_rollback
+ stoppable_instances = instances.stopable
+ if stoppable_instances.empty?
+ self.state = STATE_ROLLBACK_COMPLETE
+ save!
+ return
+ end
+
+ self.state = STATE_ROLLBACK_IN_PROGRESS
+ save!
+ error_occured = false
+ stoppable_instances.each do |i|
+ begin
+ i.stop(nil)
A real nit, for sure, and not something my ACK is conditional on, but I
would personally prefer to call this 'instance' instead of 'i'. (Though
it's not like it's particularly ambiguous what it is here.)
+ rescue
+ error_occured = true
+ # TODO: add instance event for this exception
+ logger.error $!.message
+ logger.error $!.backtrace.join("\n ")
+ end
+ end
+ if error_occured
+ self.state = STATE_ROLLBACK_FAILED
+ end
+ end
end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index e599e75..ea901fa 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -123,6 +123,7 @@ class Instance < ActiveRecord::Base
STOPPABLE_INACCESSIBLE_STATES = [STATE_NEW, STATE_PENDING, STATE_RUNNING,
STATE_SHUTTING_DOWN]
# States that indicate some sort of failure/problem with an instance:
FAILED_STATES = [STATE_CREATE_FAILED, STATE_ERROR, STATE_VANISHED]
+ ACTIVE_FAILED_STATES = [STATE_ERROR, STATE_VANISHED]
scope :deployed, :conditions => { :state => [STATE_RUNNING,
STATE_SHUTTING_DOWN] }
# FIXME: "pending" is misleading as it doesn't just cover STATE_PENDING
@@ -132,7 +133,7 @@ class Instance < ActiveRecord::Base
scope :failed, :conditions => { :state => FAILED_STATES }
scope :stopped, :conditions => {:state => STATE_STOPPED}
scope :not_stopped, :conditions => "state <> 'stopped'"
- scope :stopable, :conditions => { :state => [STATE_NEW, STATE_PENDING,
STATE_RUNNING] }
+ scope :stopable, :conditions => { :state => [STATE_PENDING, STATE_RUNNING] }
While it's not introduced by your patch (and should thus probably be
tackled in a separate patch), "stopable" is misspelled here -- it should
read "stoppable".
scope :stoppable_inaccessible, :conditions => { :state =>
STOPPABLE_INACCESSIBLE_STATES }
SEARCHABLE_COLUMNS = %w(name state)
diff --git a/src/spec/models/deployment_spec.rb b/src/spec/models/deployment_spec.rb
index 9a48ea3..eee587e 100644
--- a/src/spec/models/deployment_spec.rb
+++ b/src/spec/models/deployment_spec.rb
@@ -357,7 +357,7 @@ describe Deployment do
end
end
- describe "deployment state" do
+ describe "state" do
context "in running state" do
before :each do
@deployment.state = Deployment::STATE_RUNNING
@@ -401,6 +401,35 @@ describe Deployment do
@deployment.reload
@deployment.state.should == Deployment::STATE_RUNNING
end
+
+ it "should be rollback_in_progress if an instance fails" do
+ @inst2.state = Instance::STATE_ERROR
+ @inst2.save!
+ @deployment.reload
+ @deployment.state.should == Deployment::STATE_ROLLBACK_IN_PROGRESS
+ end
+
+ it "should stop all running instances" do
+ Instance.any_instance.stub(:stop).and_return(true)
+ Instance.any_instance.should_receive(:stop)
+ @inst2.state = Instance::STATE_ERROR
+ @inst2.save!
+ end
+ end
+
+ context "in rollback_in_progress state" do
+ before :each do
+ @deployment.state = Deployment::STATE_ROLLBACK_IN_PROGRESS
+ @deployment.save!
+ @inst1 = Factory.create(:instance,
+ :deployment => @deployment,
+ :state => Instance::STATE_ERROR)
+ @inst2 = Factory.create(:instance,
+ :deployment => @deployment,
+ :state => Instance::STATE_PENDING)
+ @deployment.instances << @inst1
+ @deployment.instances << @inst2
+ end
end
context "in incomplete state" do
--
1.7.7.6