Merge branch 'devel-2.1'

* devel-2.1:
  Test for errors during inotify callback
  SingleFileEventHandler: Remove try/except blocks
  ErrorLoggingAsyncNotifier
  daemon.GanetiBaseAsyncoreDispatcher

Signed-off-by: Guido Trotter <ultrotter@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

daemons/ganeti-confd

@@ -117,7 +117,8 @@ class ConfdConfigurationReloader(object):
     self.inotify_handler = asyncnotifier.SingleFileEventHandler(self.wm,
                                                                 self.OnInotify,
                                                                 cfg_file)
-    self.notifier = asyncnotifier.AsyncNotifier(self.wm, self.inotify_handler)
+    notifier_class = asyncnotifier.ErrorLoggingAsyncNotifier
+    self.notifier = notifier_class(self.wm, self.inotify_handler)
 
     self.timer_handle = None
     self._EnableTimer()

lib/asyncnotifier.py

@@ -31,6 +31,7 @@ try:
 except ImportError:
   import pyinotify
 
+from ganeti import daemon
 from ganeti import errors
 
 # We contributed the AsyncNotifier class back to python-pyinotify, and it's
@@ -65,6 +66,16 @@ class AsyncNotifier(asyncore.file_dispatcher):
     self.notifier.process_events()
 
 
+class ErrorLoggingAsyncNotifier(AsyncNotifier,
+                                daemon.GanetiBaseAsyncoreDispatcher):
+  """An asyncnotifier that can survive errors in the callbacks.
+
+  We define this as a separate class, since we don't want to make AsyncNotifier
+  diverge from what we contributed upstream.
+
+  """
+
+
 class SingleFileEventHandler(pyinotify.ProcessEvent):
   """Handle modify events for a single file.
 
@@ -122,14 +133,7 @@ class SingleFileEventHandler(pyinotify.ProcessEvent):
     # by the callback by calling "enable" again on us.
     logging.debug("Received 'ignored' inotify event for %s", event.path)
     self.watch_handle = None
-
-    try:
-      self.callback(False)
-    except: # pylint: disable-msg=W0702
-      # we need to catch any exception here, log it, but proceed, because even
-      # if we failed handling a single request, we still want our daemon to
-      # proceed.
-      logging.error("Unexpected exception", exc_info=True)
+    self.callback(False)
 
   # pylint: disable-msg=C0103
   # this overrides a method in pyinotify.ProcessEvent
@@ -139,14 +143,7 @@ class SingleFileEventHandler(pyinotify.ProcessEvent):
     # replacing any file with a new one, at filesystem level, rather than
     # actually changing it. (see utils.WriteFile)
     logging.debug("Received 'modify' inotify event for %s", event.path)
-
-    try:
-      self.callback(True)
-    except: # pylint: disable-msg=W0702
-      # we need to catch any exception here, log it, but proceed, because even
-      # if we failed handling a single request, we still want our daemon to
-      # proceed.
-      logging.error("Unexpected exception", exc_info=True)
+    self.callback(True)
 
   def process_default(self, event):
     logging.error("Received unhandled inotify event: %s", event)

lib/daemon.py

@@ -72,7 +72,26 @@ class AsyncoreScheduler(sched.scheduler):
     sched.scheduler.__init__(self, timefunc, AsyncoreDelayFunction)
 
 
-class AsyncUDPSocket(asyncore.dispatcher):
+class GanetiBaseAsyncoreDispatcher(asyncore.dispatcher):
+  """Base Ganeti Asyncore Dispacher
+
+  """
+  # this method is overriding an asyncore.dispatcher method
+  def handle_error(self):
+    """Log an error in handling any request, and proceed.
+
+    """
+    logging.exception("Error while handling asyncore request")
+
+  # this method is overriding an asyncore.dispatcher method
+  def writable(self):
+    """Most of the time we don't want to check for writability.
+
+    """
+    return False
+
+
+class AsyncUDPSocket(GanetiBaseAsyncoreDispatcher):
   """An improved asyncore udp socket.
 
   """
@@ -80,7 +99,7 @@ class AsyncUDPSocket(asyncore.dispatcher):
     """Constructor for AsyncUDPSocket
 
     """
-    asyncore.dispatcher.__init__(self)
+    GanetiBaseAsyncoreDispatcher.__init__(self)
     self._out_queue = []
     self.create_socket(socket.AF_INET, socket.SOCK_DGRAM)
 
@@ -119,13 +138,6 @@ class AsyncUDPSocket(asyncore.dispatcher):
     utils.IgnoreSignals(self.sendto, payload, 0, (ip, port))
     self._out_queue.pop(0)
 
-  # this method is overriding an asyncore.dispatcher method
-  def handle_error(self):
-    """Log an error in handling any request, and proceed.
-
-    """
-    logging.exception("Error while handling asyncore request")
-
   def enqueue_send(self, ip, port, payload):
     """Enqueue a datagram to be sent when possible
 

test/ganeti.asyncnotifier_unittest.py

@@ -34,14 +34,27 @@ except ImportError:
 from ganeti import asyncnotifier
 from ganeti import daemon
 from ganeti import utils
+from ganeti import errors
 
 import testutils
 
 
+class _MyErrorLoggingAsyncNotifier(asyncnotifier.ErrorLoggingAsyncNotifier):
+  def __init__(self, *args, **kwargs):
+    asyncnotifier.ErrorLoggingAsyncNotifier.__init__(self, *args, **kwargs)
+    self.error_count = 0
+
+  def handle_error(self):
+    self.error_count += 1
+    # We should also terminate while handling an error, so that any unexpected
+    # error is registered and can be checked.
+    os.kill(os.getpid(), signal.SIGTERM)
+
+
 class TestSingleFileEventHandler(testutils.GanetiTestCase):
   """Test daemon.Mainloop"""
 
-  NOTIFIERS = [NOTIFIER_TERM, NOTIFIER_NORM] = range(2)
+  NOTIFIERS = [NOTIFIER_TERM, NOTIFIER_NORM, NOTIFIER_ERR] = range(3)
 
   def setUp(self):
     testutils.GanetiTestCase.setUp(self)
@@ -57,8 +70,8 @@ class TestSingleFileEventHandler(testutils.GanetiTestCase):
                                                           self.cbk[i],
                                                           self.chk_files[i])
                       for i in range(len(self.NOTIFIERS))]
-    self.notifiers = [asyncnotifier.AsyncNotifier(self.wms[i],
-                                                  self.ihandler[i])
+    self.notifiers = [_MyErrorLoggingAsyncNotifier(self.wms[i],
+                                                   self.ihandler[i])
                        for i in range(len(self.NOTIFIERS))]
     # TERM notifier is enabled by default, as we use it to get out of the loop
     self.ihandler[self.NOTIFIER_TERM].enable()
@@ -73,12 +86,16 @@ class TestSingleFileEventHandler(testutils.GanetiTestCase):
       self.notified[self.i] = True
       if self.i == self.testobj.NOTIFIER_TERM:
         os.kill(os.getpid(), signal.SIGTERM)
+      elif self.i == self.testobj.NOTIFIER_ERR:
+        raise errors.GenericError("an error")
 
   def testReplace(self):
     utils.WriteFile(self.chk_files[self.NOTIFIER_TERM], data="dummy")
     self.mainloop.Run()
     self.assert_(self.notified[self.NOTIFIER_TERM])
     self.assert_(not self.notified[self.NOTIFIER_NORM])
+    self.assertEquals(self.notifiers[self.NOTIFIER_TERM].error_count, 0)
+    self.assertEquals(self.notifiers[self.NOTIFIER_NORM].error_count, 0)
 
   def testEnableDisable(self):
     self.ihandler[self.NOTIFIER_TERM].enable()
@@ -91,6 +108,8 @@ class TestSingleFileEventHandler(testutils.GanetiTestCase):
     self.mainloop.Run()
     self.assert_(self.notified[self.NOTIFIER_TERM])
     self.assert_(not self.notified[self.NOTIFIER_NORM])
+    self.assertEquals(self.notifiers[self.NOTIFIER_TERM].error_count, 0)
+    self.assertEquals(self.notifiers[self.NOTIFIER_NORM].error_count, 0)
 
   def testDoubleEnable(self):
     self.ihandler[self.NOTIFIER_TERM].enable()
@@ -99,6 +118,8 @@ class TestSingleFileEventHandler(testutils.GanetiTestCase):
     self.mainloop.Run()
     self.assert_(self.notified[self.NOTIFIER_TERM])
     self.assert_(not self.notified[self.NOTIFIER_NORM])
+    self.assertEquals(self.notifiers[self.NOTIFIER_TERM].error_count, 0)
+    self.assertEquals(self.notifiers[self.NOTIFIER_NORM].error_count, 0)
 
   def testDefaultDisabled(self):
     utils.WriteFile(self.chk_files[self.NOTIFIER_NORM], data="dummy")
@@ -107,6 +128,8 @@ class TestSingleFileEventHandler(testutils.GanetiTestCase):
     self.assert_(self.notified[self.NOTIFIER_TERM])
     # NORM notifier is disabled by default
     self.assert_(not self.notified[self.NOTIFIER_NORM])
+    self.assertEquals(self.notifiers[self.NOTIFIER_TERM].error_count, 0)
+    self.assertEquals(self.notifiers[self.NOTIFIER_NORM].error_count, 0)
 
   def testBothEnabled(self):
     self.ihandler[self.NOTIFIER_NORM].enable()
@@ -115,6 +138,17 @@ class TestSingleFileEventHandler(testutils.GanetiTestCase):
     self.mainloop.Run()
     self.assert_(self.notified[self.NOTIFIER_TERM])
     self.assert_(self.notified[self.NOTIFIER_NORM])
+    self.assertEquals(self.notifiers[self.NOTIFIER_TERM].error_count, 0)
+    self.assertEquals(self.notifiers[self.NOTIFIER_NORM].error_count, 0)
+
+  def testError(self):
+    self.ihandler[self.NOTIFIER_ERR].enable()
+    utils.WriteFile(self.chk_files[self.NOTIFIER_ERR], data="dummy")
+    self.mainloop.Run()
+    self.assert_(self.notified[self.NOTIFIER_ERR])
+    self.assertEquals(self.notifiers[self.NOTIFIER_ERR].error_count, 1)
+    self.assertEquals(self.notifiers[self.NOTIFIER_NORM].error_count, 0)
+    self.assertEquals(self.notifiers[self.NOTIFIER_TERM].error_count, 0)
 
 
 if __name__ == "__main__":