Hey Jozef,
Overall this looks good and does what it's supposed to, but there is a
few issues.
Cucumber reports 3 failing scenarios:
cucumber features/pool.feature:140 # Scenario: View a pool over XHR
cucumber features/pool.feature:176 # Scenario: Pools#show pretty view
cucumber features/pool.feature:182 # Scenario: Pools#show filter view
Clicking on a pool results in an error. It seems that's what causes
cucumbers to fail.
Next, please rename the event "Any instances running" to "Not all
instances running" or maybe "Some instances stopped". The word
"any" is
used mainly in questions and negative statements.
Also, please put the English messages for events to event.summary.
`event.status_code` should have short simple strings that could be
easily converted to symbols. Like "instance_stopped", "state_changed",
"all_running", etc.
You could have both. For example:
Event.new(:source => instance.deployment, :event_time => DateTime.now,
:status_code => :all_started,
:summary => "All instances were launched")
You could then do all the event filtering and comparison on the status
code regardless of any changes or translation of the summary text.
More notes on the code inline.
Thomas
On 08/31/2011 01:15 AM, jzigmund(a)redhat.com wrote:
From: Jozef Zigmund<jzigmund(a)redhat.com>
Added new events for counting uptimes of deployment with these status codes:
- 1st instance running - when 1st instance of deployment is running(start point of
counting uptime)
- all instances runinng - when all instances of deployment are running(start point of
counting global uptime)
- any instances running - when one of running instances is stopped/failed(end point of
counting global uptime)
- all instances stopped - when all instances of deployment are stopped/failed(end point
of counting uptime)
---
src/Gemfile | 3 +-
src/app/helpers/application_helper.rb | 29 ++++++++++--------
src/app/models/deployment.rb | 42 +++++++++++++++++++++++++++
src/app/models/instance.rb | 6 +++-
src/app/models/instance_observer.rb | 17 +++++++++++
src/app/stylesheets/layout.scss | 2 +-
src/app/views/pools/_deployments.haml | 8 ++++-
src/spec/helpers/application_helper_spec.rb | 8 +++++
src/spec/models/deployment_spec.rb | 16 ++++++++++
src/spec/models/instance_observer_spec.rb | 15 +++++++++-
src/spec/models/instance_spec.rb | 24 +++++++++++++++
11 files changed, 152 insertions(+), 18 deletions(-)
diff --git a/src/Gemfile b/src/Gemfile
index 05d3b7c..a4cdab7 100644
--- a/src/Gemfile
+++ b/src/Gemfile
@@ -31,6 +31,7 @@ group :development, :test do
#gem 'cucumber-rails'
#gem 'database_cleaner'
#gem 'vcr'
- #gem 'webmock'
#gem 'launchy'
+ #gem 'ruby-debug'
+ #gem 'webmock'
end
This ^ looks like forgotten debugging code. Please remove it.
diff --git a/src/app/helpers/application_helper.rb
b/src/app/helpers/application_helper.rb
index 930ad49..ff1fca7 100644
--- a/src/app/helpers/application_helper.rb
+++ b/src/app/helpers/application_helper.rb
@@ -215,21 +215,24 @@ module ApplicationHelper
title.split(' ').join('_').downcase
end
- def count_uptime(start_time)
- result_string = []
- difference = Time.now.utc - start_time
+ def count_uptime(time)
+ if time
+ result_string = []
- seconds = difference % 60
- difference = (difference - seconds) / 60
- minutes = difference % 60
- difference = (difference - minutes) / 60
- hours = difference % 24
- difference = (difference - hours) / 24
- days = difference % 7
+ seconds = time % 60
+ time = (time - seconds) / 60
+ minutes = time % 60
+ time = (time - minutes) / 60
+ hours = time % 24
+ time = (time - hours) / 24
+ days = time % 7
- result_string<< pluralize(days.to_i, 'day') if days != 0
-
result_string<<"#{"%02d"%hours.to_i}:#{"%02d"%minutes.to_i}:#{"%02d"%seconds.to_i}"
- result_string.join(", ")
+ result_string<< pluralize(days.to_i, 'day') if days != 0
+
result_string<<"#{"%02d"%hours.to_i}:#{"%02d"%minutes.to_i}:#{"%02d"%seconds.to_i}"
+ result_string.join(", ")
+ else
+ "N/A"
+ end
end
def owner_name(obj)
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index d0ee262..256d804 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -50,6 +50,9 @@ class Deployment< ActiveRecord::Base
:include => [:role],
:order => "permissions.id ASC"
belongs_to :owner, :class_name => "User", :foreign_key =>
"owner_id"
+
+ has_many :events, :as => :source, :dependent => :destroy
+
Events should not be `:dependent => :destroy`. We want to keep them
around even after the source object that created is gone.
after_create "assign_owner_roles(owner)"
validates_presence_of :pool_id
@@ -243,4 +246,43 @@ class Deployment< ActiveRecord::Base
end
errs
end
+
+ def all_instances_running?
+ instances.each{|instance| return false if instance.state != (Instance::STATE_RUNNING
|| Instance::STATE_SHUTTING_DOWN)}
This expression:
(Instance::STATE_RUNNING || Instance::STATE_SHUTTING_DOWN)
will always return `Instance::STATE_RUNNING`. So the `instance.state`
will never be checked with STATE_SHUTTING_DOWN.
+ true
+ end
You can replace the entire body of that method ^ with this line:
instances.all? {|instance| [Instance::STATE_RUNNING,
Instance::STATE_SHUTTING_DOWN].include? instance.state }
+
+ def any_instance_running?
+ instances.detect {|i| i.state == Instance::STATE_RUNNING } ? true : false
You can simplify this as:
instances.any? {|i| i.state == Instance::STATE_RUNNING }
+ end
+
+ def uptime_1st_instance
+ unless events.empty?
You can replace this ^ with:
return unless event.empty?
This will decrease the indentation level of the rest of the body.
+ unless instances.deployed.empty?
I think it's more readable to use if/else than unless/else. Unless
should be used only when there's no else block, imho.
+ if instances.count> 1&&
events.find_by_status_code("1st instance running")
+ Time.now.utc - events.find_by_status_code("1st instance
running").event_time
+ elsif events.find_by_status_code("All instances running")
+ Time.now.utc - events.find_by_status_code("All instances
running").event_time
+ end
+ else
+ if instances.count> 1&& events.find_by_status_code("All
instances stopped")&& events.find_by_status_code("1st instance
running")
+ events.find_by_status_code("All instances stopped").event_time -
events.find_by_status_code("1st instance running").event_time
+ elsif events.find_by_status_code("All instances stopped")&&
events.find_by_status_code("All instances running")
+ events.find_by_status_code("All instances stopped").event_time -
events.find_by_status_code("All instances running").event_time
+ end
+ end
+ end
+ end
+
+ def uptime_all
+ unless events.empty?
+ if instances.deployed.count == instances.count&& events.last.status_code
== "All instances running"
As soon as we add other events to the deployments, the code above will
break, because `events.last` can be some other unrelated event.
Something like
Event.find :last, :conditions => { :status_code => "All instances
running" }
would be more reliable. But then you'd have to check that no
instance_stopped event follows this one.
It may be a good idea to create a scope or a filter function that would
return you only the events you care about: 'all running','1st running',
'all stopped', etc.
+ Time.now.utc - events.last.event_time
+ elsif instances.count> 1&& events.find_by_status_code("All
instances running")&& events.find_by_status_code("Any instances
running")
+ events.find_by_status_code("Any instances running").event_time -
events.find_by_status_code("All instances running").event_time
+ elsif events.find_by_status_code("All instances stopped")&&
events.find_by_status_code("All instances running")
+ events.find_by_status_code("All instances stopped").event_time -
events.find_by_status_code("All instances running").event_time
+ end
+ end
+ end
end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index b682c06..52317f4 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -374,9 +374,13 @@ class Instance< ActiveRecord::Base
})
end
+ def first_running?
+ (deployment.instances.detect {|i| i.state == Instance::STATE_RUNNING&& i !=
self} ? false : true) if deployment
Again, you can replace `detect` with `any?` and get rid of the `?` operator:
(not deployment.instances.any? {|i| i.state == Instance::STATE_RUNNING
&& i != self}) if deployment
+ end
+
private
def key_name
"#{self.name}_#{Time.now.to_i}_key_#{self.object_id}".gsub(/[^a-zA-Z0-9\.\-]/,
'_')
- end
+ end
end
diff --git a/src/app/models/instance_observer.rb b/src/app/models/instance_observer.rb
index a7b53ed..a805743 100644
--- a/src/app/models/instance_observer.rb
+++ b/src/app/models/instance_observer.rb
@@ -72,6 +72,23 @@ class InstanceObserver< ActiveRecord::Observer
event = Event.new(:source => instance, :event_time => DateTime.now,
:summary => "state changed to
#{instance.state}")
event.save!
+ if instance.state == Instance::STATE_RUNNING&& instance.deployment
+ event2 = Event.new(:source => instance.deployment, :event_time =>
DateTime.now,
+ :status_code => "1st instance running") if
instance.first_running?
+ event2 = Event.new(:source => instance.deployment, :event_time =>
DateTime.now,
+ :status_code => "All instances running") if
instance.deployment.all_instances_running?
+ event2.save! if event2
Jan's comment on the previous patch still stand here, I think.
If the deployment has only 1 instance, the "1st instance running" event
will not be logged. Please fix it so that it does.
Doing this will simplify the `uptime_1st_instance` and `uptime_all`
methods, too.
+ elsif (instance.state == Instance::STATE_STOPPED ||
instance.state == Instance::STATE_ERROR)&& instance.deployment
+ if instance.deployment.any_instance_running?&&
!(instance.deployment.events.empty?)
Again, relying on deployment.events to be empty will break as soon as we
add unrelated events for deployments.
It would be good if you could scope/filter it out by the supported
status_codes.
+ event3 = Event.new(:source => instance.deployment,
:event_time => DateTime.now,
+ :status_code => "Any instances running")
unless instance.deployment.events.last.status_code == "Any instances running"
The same point as before ^, as soon as we add more deployment events,
this will break.
+ else
+ event3 = Event.new(:source => instance.deployment, :event_time =>
DateTime.now,
+ :status_code => "All instances stopped")
+ end
+ event3.save! if event3
+ end
+
end
end
diff --git a/src/app/stylesheets/layout.scss b/src/app/stylesheets/layout.scss
Layout.scss is autogenerated (it's written in its header). Ken Keiter is
in charge of that.
Please send the changes to Ken so he can incorporate them into the
layout.scss or put them into custom.scss (which exists for this purpose).
index af41d65..7fdab64 100644
--- a/src/app/stylesheets/layout.scss
+++ b/src/app/stylesheets/layout.scss
@@ -2191,7 +2191,7 @@ ul.deployment-array.large + ul.deployment-array.large{
/********************************************* Small Deployable Card Arrays */
/************************************************************************** */
-$deployment-small-height: 84px;
+$deployment-small-height: 110px; /*change it from 84px, because of uptimes displaying*/
$deployment-small-width: 174px;
$deployment-small-padding: 8px;
diff --git a/src/app/views/pools/_deployments.haml
b/src/app/views/pools/_deployments.haml
index cdd99ad..6a4833a 100644
--- a/src/app/views/pools/_deployments.haml
+++ b/src/app/views/pools/_deployments.haml
@@ -4,8 +4,14 @@
%li.deployment
%h3.name
= link_to deployment.name, deployment_path(deployment)
+ Global uptime
+ = count_uptime(deployment.uptime_all)
%dl.statistics
%ul
%li.right
%dt.instances.count Instances
- %dd= deployment.instances.count || 0
\ No newline at end of file
+ %dd= deployment.instances.count || 0
+
+ %li.left
+ %dt Uptime
+ %dd= count_uptime(deployment.uptime_1st_instance)
\ No newline at end of file
diff --git a/src/spec/helpers/application_helper_spec.rb
b/src/spec/helpers/application_helper_spec.rb
index fe5c956..e417044 100644
--- a/src/spec/helpers/application_helper_spec.rb
+++ b/src/spec/helpers/application_helper_spec.rb
@@ -32,6 +32,14 @@ describe ApplicationHelper do
node.first['id'].should == 'delete'
end
+ it "count_uptime should return right values" do
+ count_uptime(Time.now-(Time.now-10)).should be_true
+ end
+
+ it "count_uptime should return N/A if nil is passed as parameter" do
+ count_uptime(nil).should == "N/A"
+ end
+
end
end
diff --git a/src/spec/models/deployment_spec.rb b/src/spec/models/deployment_spec.rb
index 7359910..182b48d 100644
--- a/src/spec/models/deployment_spec.rb
+++ b/src/spec/models/deployment_spec.rb
@@ -124,4 +124,20 @@ describe Deployment do
lambda {Instance.find(inst1.id)}.should raise_error(ActiveRecord::RecordNotFound)
lambda {Instance.find(inst2.id)}.should raise_error(ActiveRecord::RecordNotFound)
end
+
+ it "should be return nil if deployment has no events " do
+ deployment = Factory :deployment
+ deployment.uptime_1st_instance.should be_nil
+ deployment.uptime_all.should be_nil
+ end
+
+ it "should return false if no deployed instances" do
+ deployment = Factory.build :deployment
+ instance = Factory.build(:mock_running_instance, :deployment => deployment)
+ instance2 = Factory.build(:mock_pending_instance, :deployment => deployment)
+ deployment.stub(:instances){[instance, instance2]}
+ deployment.any_instance_running?.should be_true
+ instance.state = Instance::STATE_PENDING
+ deployment.any_instance_running?.should be_false
+ end
end
diff --git a/src/spec/models/instance_observer_spec.rb
b/src/spec/models/instance_observer_spec.rb
index c4d0a42..4f419f2 100644
--- a/src/spec/models/instance_observer_spec.rb
+++ b/src/spec/models/instance_observer_spec.rb
@@ -77,7 +77,7 @@ describe InstanceObserver do
it "should set accumlated shutting down time when instance changes state from
state shutting down" do
@instance.state = Instance::STATE_SHUTTING_DOWN
- @instance.save!;
+ @instance.save!
Timecop.freeze(Time.now + 1.second)
@@ -199,4 +199,17 @@ describe InstanceObserver do
@instance.instance_key.reload
@instance.instance_key.should be_nil
end
+
+ it "should create event when one of deployment's instance
stop/fail'" do
+ deployment = Factory :deployment
+ instance1 = Factory :mock_running_instance, :deployment => deployment
+ instance2 = Factory :mock_running_instance, :deployment => deployment
+ instance3 = Factory :mock_pending_instance, :deployment => deployment
+ instance3.state = Instance::STATE_RUNNING
+ instance3.save!
+ instance2.state = Instance::STATE_ERROR
+ instance2.save!
+ deployment.events.last.status_code.should == "Any instances running"
+ end
+
end
diff --git a/src/spec/models/instance_spec.rb b/src/spec/models/instance_spec.rb
index da0f849..78d0c0f 100644
--- a/src/spec/models/instance_spec.rb
+++ b/src/spec/models/instance_spec.rb
@@ -211,4 +211,28 @@ describe Instance do
export_string.include?("Instance").should be_true
export_string.include?("created").should be_true
end
+
+ context "When more instances of deployment are starting" do
+ it "should return true if first instance of deployment is running" do
+ deployment = Factory.build :deployment
+ instance1 = Factory.build(:mock_running_instance, :deployment => deployment)
+ instance2 = Factory.build(:mock_pending_instance, :deployment => deployment)
+ instance3 = Factory.build(:mock_pending_instance, :deployment => deployment)
+ deployment.stub(:instances){[instance1, instance2, instance3]}
+ instance1.first_running?.should be_true
+ instance2.state = Instance::STATE_RUNNING
+ instance1.first_running?.should be_false
+ end
+
+ it "should return true if all instance of deployment is running" do
+ deployment = Factory.build :deployment
+ instance1 = Factory.build(:mock_running_instance, :deployment => deployment)
+ instance2 = Factory.build(:mock_running_instance, :deployment => deployment)
+ instance3 = Factory.build(:mock_pending_instance, :deployment => deployment)
+ deployment.stub(:instances){[instance1, instance3, instance2]}
+ deployment.all_instances_running?.should be_false
+ instance3.state = Instance::STATE_RUNNING
+ deployment.all_instances_running?.should be_true
+ end
+ end
end