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(a)redhat.com>
Sent: Fri, 13/07/2012 11:00 AM
To: Imre Farkas <ifarkas(a)redhat.com>
CC: aeolus-devel(a)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(a)redhat.com wrote:
> From: Jan Provaznik <jprovazn(a)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