[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