Hi Jozef,
sending some feedback (inline).
Jan
On 08/25/2011 05:40 PM, jzigmund(a)redhat.com wrote:
From: Jozef Zigmund<jzigmund(a)redhat.com>
could you please add some short basic description to this commit
---
src/Gemfile | 3 +-
src/app/helpers/application_helper.rb | 28 +++++++++++++++-----------
src/app/models/deployment.rb | 23 ++++++++++++++++++++++
src/app/models/instance.rb | 12 ++++++++++-
src/app/models/instance_observer.rb | 12 +++++++++++
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 | 6 +++++
src/spec/models/instance_observer_spec.rb | 11 ++++++++++
src/spec/models/instance_spec.rb | 24 +++++++++++++++++++++++
11 files changed, 121 insertions(+), 16 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
diff --git a/src/app/helpers/application_helper.rb
b/src/app/helpers/application_helper.rb
index 55de326..b8ce8c8 100644
--- a/src/app/helpers/application_helper.rb
+++ b/src/app/helpers/application_helper.rb
@@ -216,20 +216,24 @@ module ApplicationHelper
end
def count_uptime(start_time)
- result_string = []
- difference = Time.now.utc - start_time
+ if start_time
+ result_string = []
+ difference = Time.now.utc - start_time
- 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 = difference % 60
+ difference = (difference - seconds) / 60
+ minutes = difference % 60
+ difference = (difference - minutes) / 60
+ hours = difference % 24
+ difference = (difference - hours) / 24
+ days = difference % 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..68f2a18 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
+
after_create "assign_owner_roles(owner)"
validates_presence_of :pool_id
@@ -243,4 +246,24 @@ class Deployment< ActiveRecord::Base
end
errs
end
+
+ def uptime_1st_instance
'1st_...' could be replaced with 'any_...' on all places (suggested in
'Define useful uptimes' thread)
+ if events.empty?
+ nil
+ else
+ events.find_by_summary("1st instance running").event_time
+ end
No need to use if-else: find_by returns nil if record is not found
Also I think it would be better not use finding by message (search for
text "1st instance running". Using "status_code" field which is
defined
on Event model seems to be better to me, so there will be status codes:
any_running, all_running
+ end
+
+ def uptime_all
+ Rails.logger.info("#{events.inspect}\nLAST => #{events.last.inspect}")
^ looks like forgotten debug line
+ if events.empty?
+ nil
+ else
+ events.last.event_time if events.last.summary == "All instances
running"
+ end
+ #unless events.last.nil?
+ # events.last.summary == "All instances running" ? events.last.event_time
: nil
+ #end
same as above - only find_by can be used, commented code can be removed
+ end
end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index b682c06..dd6af41 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -374,9 +374,19 @@ class Instance< ActiveRecord::Base
})
end
+ def first_running?
+ deployment.instances.reject {|i| i == self}.each {|instance| return false if
instance.state == Instance::STATE_RUNNING }
you could simplify this by using scope 'deployed' & find:
deployment.instances.deployed.find {|i| i != self} ? true : false
+ true
+ end
+
+ def all_running?
+ deployment.instances.each{|instance| return false if instance.state !=
Instance::STATE_RUNNING}
+ true
+ end
+
Both methods above should be part of Deployment model.
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..93b2a4c 100644
--- a/src/app/models/instance_observer.rb
+++ b/src/app/models/instance_observer.rb
@@ -72,6 +72,18 @@ 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,
+ :summary => "1st instance running") if
instance.first_running?
+ event2 = Event.new(:source => instance.deployment, :event_time =>
DateTime.now,
+ :summary => "All instances running") if
instance.all_running?
+ event2.save! if event2
if there is be only one instance in deployment, then first_running is
not logged, I think both states should be logged even for single
instance deployments.
+ elsif (instance.state == Instance::STATE_STOPPED ||
instance.state == Instance::STATE_SHUTTING_DOWN || instance.state ==
Instance::STATE_ERROR)&& instance.deployment
+ event3 = Event.new(:source => instance.deployment, :event_time =>
DateTime.now,
+ :summary => "Instance not running")
this will log "instance not running" anytime state is changed even in
inactive states (new -> create_failed, shutting_down -> error,...)
you can simplify by:
elsif (instance.state_changed? and instance.state_was == 'running')
+ event3.save! if event3
+ end
+
end
end
diff --git a/src/app/stylesheets/layout.scss b/src/app/stylesheets/layout.scss
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)
^ this uptime displays Time.now - time when first instance was started.
Another problem is that uptime is displayed only for running
deployments, so a user has no chance to check how long a stopped
deployment was running. Not sure if view was already discussed with Ken,
but for first iteration I would expect:
global uptime:
(first stopped instance || Time.now) - all_running_instance
uptime:
(all stopped instances || Time now) - first_running_instance
Uptime should be also displayed for single instances.
\ 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..a3f8be4 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-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..fb90184 100644
--- a/src/spec/models/deployment_spec.rb
+++ b/src/spec/models/deployment_spec.rb
@@ -124,4 +124,10 @@ 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
end
diff --git a/src/spec/models/instance_observer_spec.rb
b/src/spec/models/instance_observer_spec.rb
index c4d0a42..ae95440 100644
--- a/src/spec/models/instance_observer_spec.rb
+++ b/src/spec/models/instance_observer_spec.rb
@@ -199,4 +199,15 @@ 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_running_instance, :deployment => deployment
+ instance2.state = Instance::STATE_ERROR
+ instance2.save!
+ deployment.events.last.summary.should == "Instance not running"
+ end
+
end
diff --git a/src/spec/models/instance_spec.rb b/src/spec/models/instance_spec.rb
index da0f849..4e41d50 100644
--- a/src/spec/models/instance_spec.rb
+++ b/src/spec/models/instance_spec.rb
@@ -210,5 +210,29 @@ describe Instance do
export_string.include?(instance.id.to_s).should be_true
export_string.include?("Instance").should be_true
export_string.include?("created").should be_true
+
+ 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)
+ instance1.state = Instance::STATE_RUNNING
+ 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 first 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]}
+ instance1.all_running?.should be_false
+ instance3.state = Instance::STATE_RUNNING
+ instance1.all_running?.should be_true
+ end
end
end