aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--debian/changelog7
-rw-r--r--lib/mauve/alert.rb4
-rw-r--r--lib/mauve/alert_changed.rb4
-rw-r--r--lib/mauve/alert_group.rb33
-rw-r--r--lib/mauve/notification.rb28
-rw-r--r--lib/mauve/notifier.rb2
-rw-r--r--lib/mauve/person.rb67
-rw-r--r--lib/mauve/version.rb2
-rw-r--r--test/tc_mauve_alert_changed.rb10
-rw-r--r--test/tc_mauve_notification.rb79
10 files changed, 134 insertions, 102 deletions
diff --git a/debian/changelog b/debian/changelog
index 3dee2a0..21f2cbc 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+mauvealert (3.4.3) stable; urgency=low
+
+ * Fixed notification race-conditions.
+ * Reminders now get sent at the start of the next during period.
+
+ -- Patrick J Cherry <patrick@bytemark.co.uk> Mon, 22 Aug 2011 11:58:28 +0100
+
mauvealert (3.4.2) stable; urgency=low
* Improved polling interval timing.
diff --git a/lib/mauve/alert.rb b/lib/mauve/alert.rb
index eb5f11b..c294e8c 100644
--- a/lib/mauve/alert.rb
+++ b/lib/mauve/alert.rb
@@ -392,7 +392,7 @@ module Mauve
#
#
def all_raised
- all(:raised_at.not => nil, :cleared_at => nil) - all_acknowledged
+ all(:raised_at.not => nil, :acknowledged_at => nil, :cleared_at => nil)
end
def all_acknowledged
@@ -400,7 +400,7 @@ module Mauve
end
def all_cleared
- all(:cleared_at.not => nil) - all_acknowledged
+ all(:cleared_at.not => nil, :acknowledged_at => nil)
end
# Returns a hash of all the :urgent, :normal and :low alerts.
diff --git a/lib/mauve/alert_changed.rb b/lib/mauve/alert_changed.rb
index 83332ff..ac864a0 100644
--- a/lib/mauve/alert_changed.rb
+++ b/lib/mauve/alert_changed.rb
@@ -19,12 +19,10 @@ module Mauve
property :remind_at, Time
# property :updated_at, Time, :required => true
- def inspect
+ def to_s
"#<AlertChanged #{id}: alert_id #{alert_id}, for #{person}, update_type #{update_type}>"
end
- alias to_s inspect
-
belongs_to :alert
def was_relevant=(value)
diff --git a/lib/mauve/alert_group.rb b/lib/mauve/alert_group.rb
index 114c39b..df76f40 100644
--- a/lib/mauve/alert_group.rb
+++ b/lib/mauve/alert_group.rb
@@ -95,16 +95,45 @@ module Mauve
#
# If there are no notifications defined.
#
- if notifications.nil?
+ if notifications.nil?
logger.warn("No notifications found for #{self.inspect}")
return
end
#
+ # This is where we set the reminder -- i.e. on a per-alert-group basis.
+ #
+ remind_at = notifications.inject(nil) do |reminder_time, notification|
+ this_time = notification.remind_at_next(alert)
+ if reminder_time.nil? or (!this_time.nil? and reminder_time > this_time)
+ this_time
+ else
+ reminder_time
+ end
+ end
+
+ #
+ # OK got the next reminder time.
+ #
+ unless remind_at.nil?
+ this_reminder = AlertChanged.new(
+ :level => level.to_s,
+ :alert_id => alert.id,
+ :person => self.name,
+ :at => Time.now,
+ :update_type => alert.update_type,
+ :remind_at => remind_at,
+ :was_relevant => true)
+
+ this_reminder.save
+ end
+
+ #
# The notifications are specified in the config file.
#
+ sent_to = []
notifications.each do |notification|
- notification.notify(alert)
+ sent_to = notification.notify(alert, sent_to)
end
end
diff --git a/lib/mauve/notification.rb b/lib/mauve/notification.rb
index c57d2a9..43de1a7 100644
--- a/lib/mauve/notification.rb
+++ b/lib/mauve/notification.rb
@@ -145,7 +145,6 @@ module Mauve
attr_reader :thread_list
def initialize(people, level)
-
self.level = level
self.every = 300
self.people = people
@@ -153,7 +152,7 @@ module Mauve
def logger ; Log4r::Logger.new self.class.to_s ; end
- def notify(alert)
+ def notify(alert, already_sent_to = [])
if people.nil? or people.empty?
logger.warn "No people found in for notification #{list}"
@@ -161,9 +160,7 @@ module Mauve
end
# Should we notify at all?
- is_relevant = DuringRunner.new(Time.now, alert, &during).now?
-
- n_sent = 0
+ return already_sent_to unless DuringRunner.new(Time.now, alert, &during).now?
people.collect do |person|
case person
@@ -176,16 +173,29 @@ module Mauve
[]
end
end.flatten.uniq.each do |person|
- n_sent += 1 if person.send_alert(self.level, alert, is_relevant, remind_at_next(alert))
+ #
+ # A bit of alert de-bouncing.
+ #
+ if already_sent_to.include?(person.username)
+ logger.info("Already sent notification of #{alert} to #{person.username}")
+ else
+ Server.notification_push([person, level, alert])
+ already_sent_to << person.username
+ end
end
- return n_sent
+ return already_sent_to
end
def remind_at_next(alert)
- return DuringRunner.new(Time.now, alert, &during).find_next(every) if alert.raised?
+ return nil unless alert.raised?
+
+ if DuringRunner.new(Time.now, alert, &during).now?
+ return DuringRunner.new(Time.now, alert, &during).find_next(every)
+ else
+ return DuringRunner.new(Time.now, alert, &during).find_next(0)
+ end
- return nil
end
end
diff --git a/lib/mauve/notifier.rb b/lib/mauve/notifier.rb
index 4737c37..2853485 100644
--- a/lib/mauve/notifier.rb
+++ b/lib/mauve/notifier.rb
@@ -25,7 +25,7 @@ module Mauve
#
next if person.nil?
- person.do_send_alert(*args)
+ person.send_alert(*args)
end
end
diff --git a/lib/mauve/person.rb b/lib/mauve/person.rb
index 26425da..60ad60a 100644
--- a/lib/mauve/person.rb
+++ b/lib/mauve/person.rb
@@ -86,72 +86,9 @@ module Mauve
end
#
- # Sends the alert, and updates when the AlertChanged database to set the next reminder.
+ # Sends the alert
#
- def send_alert(level, alert, is_relevant=true, remind_at=nil)
- #
- # First check that we've not just sent an notification to this person for
- # this alert
- #
- last_reminder = AlertChanged.first(:alert => alert, :person => username, :update_type => alert.update_type, :at.gte => (Time.now - 1.minute) )
-
- if last_reminder and last_reminder.at >= (Time.now - 1.minute)
- #
- #
- logger.info("Not sending notification to #{username} for #{alert} because one has just been sent.")
- return false
- end
-
- #
- # For the love of God, do not remind about ack'd or cleared alerts.
- #
- if alert.acknowledged? or alert.cleared?
- remind_at = nil
- end
-
- this_reminder = AlertChanged.new(
- :level => level.to_s,
- :alert_id => alert.id,
- :person => username,
- :at => Time.now,
- :update_type => alert.update_type,
- :remind_at => remind_at,
- :was_relevant => is_relevant)
-
- #
- # Check to make sure that we've not got a sooner reminder set.
- #
- unless remind_at.nil?
- next_reminder = AlertChanged.first(:alert => alert, :remind_at.gt => Time.now, :person => username, :update_type => alert.update_type)
-
- if next_reminder
- #
- # If the reminder is further in the future than the one we're about
- # to put on, then just update it.
- #
- # Otherwise if it is sooner, we don't need to create a new one.
- #
- if next_reminder.remind_at > remind_at
- next_reminder.remind_at = remind_at
- logger.info("Not inserting a new reminder, as there is already one in place sooner")
- this_reminder = next_reminder
- else
- this_reminder = nil
- end
- end
- end
-
- this_reminder.save unless this_reminder.nil?
-
- if is_relevant
- Server.notification_push([self, level, alert])
- return true
- end
-
- return false
- end
-
- def do_send_alert(level, alert)
+ def send_alert(level, alert)
now = Time.now
was_suppressed = self.suppressed?
diff --git a/lib/mauve/version.rb b/lib/mauve/version.rb
index 1c9b8d7..7e94be0 100644
--- a/lib/mauve/version.rb
+++ b/lib/mauve/version.rb
@@ -1,5 +1,5 @@
module Mauve
- VERSION="3.4.2"
+ VERSION="3.4.3"
end
diff --git a/test/tc_mauve_alert_changed.rb b/test/tc_mauve_alert_changed.rb
index 0e57120..3c43a2a 100644
--- a/test/tc_mauve_alert_changed.rb
+++ b/test/tc_mauve_alert_changed.rb
@@ -7,7 +7,7 @@ require 'mauve/configuration_builder'
require 'mauve/configuration_builders'
require 'th_mauve'
-class TcMauveAlertChanged < Mauve::UnitTest
+class TcMauveAlertChanged < Mauve::UnitTest
include Mauve
def setup
@@ -51,13 +51,13 @@ EOF
notifications = 1
mins = 0
- 121.times do
+ 11.times do
mins += 1
assert_equal(notifications, Server.instance.notification_buffer.length)
assert_equal(reminders, AlertChanged.count)
- Timecop.freeze(Time.now+1.minutes)
+ Timecop.freeze(Time.now+1.minute)
if mins % 5 == 0
notifications += 1
@@ -67,10 +67,10 @@ EOF
AlertChanged.all.each{|ac| ac.poll}
end
- # OK now clear the alert, send one notification and set an alert_changed.
+ # OK now clear the alert, send one notification and but not an alert_changed.
alert.clear!
notifications += 1
- reminders += 1
+
assert_equal(notifications, Server.instance.notification_buffer.length)
assert_equal(reminders, AlertChanged.count)
diff --git a/test/tc_mauve_notification.rb b/test/tc_mauve_notification.rb
index 167d069..018c31a 100644
--- a/test/tc_mauve_notification.rb
+++ b/test/tc_mauve_notification.rb
@@ -195,7 +195,7 @@ alert_group("default") {
every 10.minutes
}
- notify("test1") {
+ notify("testers") {
every 15.minutes
}
@@ -222,30 +222,81 @@ EOF
alert.raise!
assert_equal(1, Alert.count, "Wrong number of alerts saved")
+
+ #
+ # Also make sure that only 2 notifications has been sent..
+ #
+ assert_equal(2, Server.instance.notification_buffer.size, "Wrong number of notifications sent")
#
# Although there are four clauses above for notifications, test1 should be
# alerted in 10 minutes time, and the 15 minutes clause is ignored, since
# 10 minutes is sooner.
#
- assert_equal(3, AlertChanged.count, "Wrong number of reminders inserted")
+ assert_equal(1, AlertChanged.count, "Wrong number of reminders inserted")
+
+ a = AlertChanged.first
+ assert_equal("urgent", a.level, "Level is wrong for #{a.person}")
+ assert_equal("raised", a.update_type, "Update type is wrong for #{a.person}")
+ assert_equal(Time.now + 10.minutes, a.remind_at,"reminder time is wrong for #{a.person}")
#
- # Also make sure that only 1 notification has been sent..
+ # OK now roll the clock forward 10 minutes
+ # TODO
+
+ end
+
+
+ #
+ # Makes sure a reminder is set at the start of the notify clause.
+ #
+ def test_reminder_is_set_at_start_of_during
+
+ config=<<EOF
+person ("test1") {
+ all { true }
+}
+
+person ("test2") {
+ all { true }
+}
+
+alert_group("default") {
+ level URGENT
+ notify("test1") {
+ every 10.minutes
+ }
+
+ notify("test2") {
+ every 10.minutes
+ during { hours_in_day 8..10 }
+ }
+
+}
+EOF
+
+ #
+ # Wind forward until 7.55am
#
- assert_equal(1, Server.instance.notification_buffer.size, "Wrong number of notifications sent")
+ Timecop.freeze(Time.now + 7.hours + 55.minutes)
- reminder_times = {
- "test1" => t + 10.minutes,
- "test2" => t + 1.hour,
- "test3" => t + 2.hours
- }
+ Configuration.current = ConfigurationBuilder.parse(config)
+ Server.instance.setup
+ alert = Alert.new(
+ :alert_id => "test",
+ :source => "test",
+ :subject => "test"
+ )
+ alert.raise!
- AlertChanged.all.each do |a|
- assert_equal("urgent", a.level, "Level is wrong for #{a.person}")
- assert_equal("raised", a.update_type, "Update type is wrong for #{a.person}")
- assert_equal(reminder_times[a.person], a.remind_at,"reminder time is wrong for #{a.person}")
- end
+ assert_equal(1, Alert.count, "Wrong number of alerts saved")
+
+ assert_equal(1, AlertChanged.count, "Wrong number of reminders inserted")
+
+ a = AlertChanged.first
+ assert_equal("urgent", a.level, "Level is wrong for #{a.person}")
+ assert_equal("raised", a.update_type, "Update type is wrong for #{a.person}")
+ assert_equal(Time.now + 5.minutes, a.remind_at,"reminder time is wrong for #{a.person}")
end