[PATCH conductor] Fixed deployment rollback

Jozef Zigmund jzigmund at redhat.com
Thu Jun 7 15:26:04 UTC 2012


On Thursday 07 June 2012 15:38:00 jprovazn at redhat.com wrote:
> From: Jan Provaznik <jprovazn at redhat.com>
> 
> When rollback was initiated, 'stop' request was sent to instances
> in 'pending' state too. But it seems that 'stop' is not valid action
> for 'pending' state. Now we wait until pending instances are running
> and send 'stop' request then.
> 
> This is fix for already pushed task:
> https://www.aeolusproject.org/redmine/issues/3320
> ---
>  src/app/models/deployment.rb        |   14 +++++++++-----
>  src/app/models/instance.rb          |    1 +
>  src/app/models/instance_observer.rb |    2 +-
>  src/spec/models/deployment_spec.rb  |    2 ++
>  4 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
> index 0cf1137..7b66e56 100644
> --- a/src/app/models/deployment.rb
> +++ b/src/app/models/deployment.rb
> @@ -654,6 +654,8 @@ class Deployment < ActiveRecord::Base
>      if Instance::ACTIVE_FAILED_STATES.include?(instance.state)
>        # if this instance stop failed, whole deployment rollback failed
>        self.state = STATE_ROLLBACK_FAILED
> +    elsif instance.state == Instance::STATE_RUNNING
> +      deployment_rollback
>      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
> @@ -663,17 +665,19 @@ class Deployment < ActiveRecord::Base
>    end
> 
>    def deployment_rollback
> -    stoppable_instances = instances.stoppable
> -    if stoppable_instances.empty?
> +    unless self.state == STATE_ROLLBACK_IN_PROGRESS
> +      self.state = STATE_ROLLBACK_IN_PROGRESS
> +      save!
> +    end
> +
> +    if instances.all? {|i| i.inactive?}

needs to add 'or i == Instance::STATE_NEW' then it works properly as I tested
>        self.state = STATE_ROLLBACK_COMPLETE
>        save!
>        return
>      end
> 
> -    self.state = STATE_ROLLBACK_IN_PROGRESS
> -    save!
>      error_occured = false
> -    stoppable_instances.each do |instance|
> +    instances.running.each do |instance|
>        begin
>          instance.stop(nil)
>        rescue
> diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
> index aee01a1..3806d16 100644
> --- a/src/app/models/instance.rb
> +++ b/src/app/models/instance.rb
> @@ -128,6 +128,7 @@ class Instance < ActiveRecord::Base
>    scope :deployed,  :conditions => { :state => [STATE_RUNNING,
> STATE_SHUTTING_DOWN] } # FIXME: "pending" is misleading as it doesn't just
> cover STATE_PENDING scope :pending,   :conditions => { :state =>
> [STATE_NEW, STATE_PENDING] } +  scope :running,   :conditions => { :state
> => [STATE_RUNNING] }
>    scope :pending_or_deployed,   :conditions => { :state => [STATE_NEW,
> STATE_PENDING, STATE_RUNNING, STATE_SHUTTING_DOWN] } # FIXME: "failed" is
> misleading too...
>    scope :failed,    :conditions => { :state => FAILED_STATES }
> diff --git a/src/app/models/instance_observer.rb
> b/src/app/models/instance_observer.rb index 77b2e12..d42fbe6 100644
> --- a/src/app/models/instance_observer.rb
> +++ b/src/app/models/instance_observer.rb
> @@ -95,7 +95,6 @@ class InstanceObserver < ActiveRecord::Observer
>      # This can get stale, so reload it -- if it exists
>      instance.deployment.reload if instance.deployment
>      if instance.state_changed?
> -      instance.deployment.update_state(instance) if instance.deployment
>        event = Event.new(:source => instance, :event_time => DateTime.now,
> 
>                          :summary => "state changed to #{instance.state}",
>                          :change_hash => instance.changes, :status_code =>
>                          :instance.state)
> 
> @@ -126,6 +125,7 @@ class InstanceObserver < ActiveRecord::Observer
> 
>            :status_code => 'vanished', :summary => "Instance disappeared
>            :from cloud provider"
>          )
>        end
> +      instance.deployment.update_state(instance) if instance.deployment
> 
>      end
>    end
> diff --git a/src/spec/models/deployment_spec.rb
> b/src/spec/models/deployment_spec.rb index 3ffbae9..48df0ad 100644
> --- a/src/spec/models/deployment_spec.rb
> +++ b/src/spec/models/deployment_spec.rb
> @@ -428,6 +428,8 @@ describe Deployment do
>          end
> 
>          it "should stop all running instances if an instance fails" do
> +          @inst1.state = Instance::STATE_RUNNING
> +          @inst1.save!
>            Instance.any_instance.stub(:stop).and_return(true)
>            Instance.any_instance.should_receive(:stop)
>            @inst2.state = Instance::STATE_ERROR


ACK works for me, pls check inline note.



More information about the aeolus-devel mailing list