diff options
| -rw-r--r-- | debian/changelog | 7 | ||||
| -rw-r--r-- | lib/mauve/alert.rb | 4 | ||||
| -rw-r--r-- | lib/mauve/alert_changed.rb | 4 | ||||
| -rw-r--r-- | lib/mauve/alert_group.rb | 33 | ||||
| -rw-r--r-- | lib/mauve/notification.rb | 28 | ||||
| -rw-r--r-- | lib/mauve/notifier.rb | 2 | ||||
| -rw-r--r-- | lib/mauve/person.rb | 67 | ||||
| -rw-r--r-- | lib/mauve/version.rb | 2 | ||||
| -rw-r--r-- | test/tc_mauve_alert_changed.rb | 10 | ||||
| -rw-r--r-- | test/tc_mauve_notification.rb | 79 |
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 |