[PATCH conductor 2/2] 838726 (2) - restrict what deployments can be stopped/removed

Petr Blaho pblaho at redhat.com
Fri Jul 13 11:27:41 UTC 2012


On Friday, July 13, 2012 12:01:09 PM Imre Farkas wrote:
> On 07/12/2012 08:45 PM, jprovazn at redhat.com wrote:
> > From: Jan Provaznik <jprovazn at redhat.com>
> >
> > Only running deployemnts can be stopped and only deployments
> > with destroyable or running instances can be destroyed. This
> > patch makes sure that deployemnt doesn't transit from new/pending state
> > directly to shutting down state.
> > ---
> >   src/app/controllers/deployments_controller.rb     |    5 +++++
> >   src/app/controllers/instances_controller.rb       |    2 +-
> >   src/app/models/deployment.rb                      |   22 ++++++---------------
> >   src/app/models/instance.rb                        |   16 +++++++++++++++
> >   src/config/locales/en.yml                         |    1 +
> >   src/features/deployment.feature                   |   10 ++++++++++
> >   src/features/step_definitions/deployment_steps.rb |    4 ++++
> >   7 files changed, 43 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/app/controllers/deployments_controller.rb b/src/app/controllers/deployments_controller.rb
> > index 6dc2c70..99cee49 100644
> > --- a/src/app/controllers/deployments_controller.rb
> > +++ b/src/app/controllers/deployments_controller.rb
> > @@ -311,6 +311,11 @@ class DeploymentsController < ApplicationController
> >
> >       @deployments_to_stop.each do |deployment|
> >         # TODO: move all this logic to model
> > +      unless deployment.can_stop?
> > +        errors << t('deployments.flash.error.cant_stop',
> > +                    :name => deployment.name)
> > +        next
> > +      end
> >         deployment.state = Deployment::STATE_SHUTTING_DOWN
> >         deployment.save!
> >         deployment.instances.each do |instance|
> > diff --git a/src/app/controllers/instances_controller.rb b/src/app/controllers/instances_controller.rb
> > index 121e7d2..f34a58e 100644
> > --- a/src/app/controllers/instances_controller.rb
> > +++ b/src/app/controllers/instances_controller.rb
> > @@ -152,7 +152,7 @@ class InstancesController < ApplicationController
> >             notices << "#{instance.name}: #{t('instances.flash.notice.stop')}"
> >           end
> >         rescue Exception => err
> > -        errors << "#{instance.name}: " + err
> > +        errors << "#{instance.name}: " + err.message
> >           logger.error err.message
> >           logger.error err.backtrace.join("\n ")
> >         end
> > diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
> > index 3b4e9c1..be60802 100644
> > --- a/src/app/models/deployment.rb
> > +++ b/src/app/models/deployment.rb
> > @@ -160,6 +160,10 @@ class Deployment < ActiveRecord::Base
> >       instances.all? {|i| i.destroyable? }
> >     end
> >
> > +  def can_stop?
> > +    [STATE_RUNNING, STATE_INCOMPLETE].include?(self.state)
> > +  end
> > +
> >     def stop_instances_and_destroy!
> >       if destroyable?
> >         destroy_deployment_config
> > @@ -167,9 +171,8 @@ class Deployment < ActiveRecord::Base
> >         return
> >       end
> >
> > -    self.state = Deployment::STATE_SHUTTING_DOWN
> > -    save!
> >       if instances.all? {|i| i.destroyable? or i.state == Instance::STATE_RUNNING}
> > +      self.state = Deployment::STATE_SHUTTING_DOWN
> >         destroy_deployment_config
> >         # The deployment will be destroyed from an InstanceObserver callback once
> >         # all instances are stopped.
> > @@ -700,20 +703,7 @@ class Deployment < ActiveRecord::Base
> >     def send_rollback_requests
> >       error_occured = false
> >       instances.running.each do |instance|
> > -      begin
> > -        instance.stop(nil)
> > -      rescue
> > -        error_occured = true
> > -        self.events << Event.create(
> > -          :source => instance,
> > -          :event_time => DateTime.now,
> > -          :status_code => 'instance_stop_failed',
> > -          :summary => "Failed to stop instance #{instance.name}",
> > -          :description => $!.message
> > -        )
> > -        logger.error $!.message
> > -        logger.error $!.backtrace.join("\n  ")
> > -      end
> > +      error_occured = true unless instance.stop_with_event(nil)
> >       end
> >       if error_occured
> >         cleanup_failed_launch
> > diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
> > index 8b8021e..fad0d03 100644
> > --- a/src/app/models/instance.rb
> > +++ b/src/app/models/instance.rb
> > @@ -447,6 +447,22 @@ class Instance < ActiveRecord::Base
> >       do_operation(user, 'stop')
> >     end
> >
> > +  def stop_with_event(user)
> > +    stop(user)
> > +    true
> > +  rescue
> > +    self.events << Event.create(
> > +      :source => self,
> > +      :event_time => DateTime.now,
> > +      :status_code => 'instance_stop_failed',
> > +      :summary => "Failed to stop instance #{self.name}",
> > +      :description => $!.message
> > +    )
> > +    logger.error $!.message
> > +    logger.error $!.backtrace.join("\n  ")
> > +    false
> > +  end
> > +
> >     def reboot(user)
> >       do_operation(user, 'reboot')
> >     end
> > diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
> > index 9295373..519d4c6 100644
> > --- a/src/config/locales/en.yml
> > +++ b/src/config/locales/en.yml
> > @@ -390,6 +390,7 @@ en:
> >             one: "The deployment %{list} could not be updated."
> >             other: "The deployments %{list} could not be updated."
> >           no_hwp_permission: "You do not have sufficient permission to access the %{hwp} hardware profile."
> > +        cant_stop: "The deployment %{name} could not be stopped - only running deployments can be stopped."
> >         success:
> >           deleted:
> >             one: "The deployment %{list} was scheduled for deletion."
> > diff --git a/src/features/deployment.feature b/src/features/deployment.feature
> > index a5ebf73..15e54c5 100644
> > --- a/src/features/deployment.feature
> > +++ b/src/features/deployment.feature
> > @@ -82,8 +82,18 @@ Feature: Manage Deployments
> >       And I press "stop_button"
> >       Then I should see "testdeployment"
> >
> > +  Scenario: Stop pending deployment
> > +    Given there is a deployment named "testdeployment" belonging to "testdeployable" owned by "testuser"
> > +    And deployment "testdeployment" is "pending"
> > +    When I go to the deployments page
> > +    Then I should see "testdeployment"
> > +    When I check "testdeployment" deployment
> > +    And I press "stop_button"
> > +    Then I should see "deployment testdeployment could not be stopped"
> > +
> >     Scenario: Stop inaccessible deployments
> >       Given there is a deployment named "testdeployment" belonging to "testdeployable" owned by "testuser"
> > +    And deployment "testdeployment" is "incomplete"
> >       And "testdeployment" deployment's provider is not accessible
> >       When I go to the deployments page
> >       Then I should see "testdeployment"
> > diff --git a/src/features/step_definitions/deployment_steps.rb b/src/features/step_definitions/deployment_steps.rb
> > index 4ebab17..d9e80bc 100644
> > --- a/src/features/step_definitions/deployment_steps.rb
> > +++ b/src/features/step_definitions/deployment_steps.rb
> > @@ -38,6 +38,10 @@ Given /^the deployment "([^"]*)" has an instance named "([^"]*)"$/ do |d_name, i
> >     deployment.instances << FactoryGirl.create(:instance, :name => i_name, :pool => deployment.pool)
> >   end
> >
> > +Given /^deployment "([^"]*)" is "([^"]*)"$/ do |arg1, arg2|
> > +  Deployment.find_by_name(arg1).update_attribute(:state, arg2)
> > +end
> > +
> >   Given /^"([^"]*)" deployment's provider is not accessible$/ do |arg1|
> >     # FIXME: didn't find a way how to create an inaccessible provider
> >     # cleanly
> >
> 
> ACK to the patchset
> 
> Just one question not related to this issue: Managing states, 
> transitions and related things seem to be quite messy by now, have we 
> ever considered using a state machine library like e.g. aasm?

+1 on state machines

-- 

With regards
Petr Blaho



More information about the aeolus-devel mailing list