[PATCH conductor 1/4] 3320 - Added deployment rollback

Matt Wagner matt.wagner at redhat.com
Mon May 21 18:39:51 UTC 2012


A few nits inline:

On Mon, May 21, 2012 at 03:49:23PM +0200, jprovazn at redhat.com wrote:
> From: Jan Provaznik <jprovazn at 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



More information about the aeolus-devel mailing list