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

Hugh Brock hbrock at redhat.com
Sat Jul 14 13:17:20 UTC 2012


Jan, let's stop letting the packaging tail wag the software dog. If we need to use a gem, let's use it and figure out how to deal with the packaging later. 

-hugh

-------- Original Message --------
 From: Jan Provazník <jprovazn at redhat.com>
 Sent: Fri, 13/07/2012 11:00 AM
 To: Imre Farkas <ifarkas at redhat.com>
 CC: aeolus-devel at lists.fedorahosted.org
 Subject: Re: [PATCH conductor 2/2] 838726 (2) - restrict what deployments	can be stopped/removed

On 07/13/2012 12:01 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
>

Hi, thanks for the review, patchset is pushed.

> 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?


IIRC there was some discussion about using http://ruote.rubyforge.org/ 
(which is not exactly state machine). This tool seemed to be too big hammer.

My initial thought was that our transition model is simple enough to be 
handled by a few methods in model. I thought that current code is still 
quite clear, but this my impression might be very subjective since I'm 
familiar with it. Transition model will probably become more complicated 
with upcoming approval/delayed workflow, so it might be good idea to 
switch to state machine.

The aasm lib looks interesting (it would have to be packaged for Fedora 
of course), there is also already packaged state_machine. I'll try to 
look at it closer, but if anyone has free cycles, help would be welcome.

Jan



More information about the aeolus-devel mailing list