On 05/21/2012 08:39 PM, Matt Wagner wrote:
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?
it's called from deployment's "update_state" method:
send(transition_method, changed_instance)...
> +
> + 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
Hi Matt,
I've pushed the patchset after updating it according to your feedback.
Thanks for the review.
Jan