[1986] Adressed review comments (b10-ddns side)

- fix log messages
- added more send/recv error catches
- more tests

src/bin/ddns/ddns.py.in

@@ -228,7 +228,7 @@ class DDNSServer:
         # Outstanding TCP context: fileno=>(context_obj, dst)
         self._tcp_ctxs = {}
 
-        # Notify Auth server that DDNS update packets can now be forwarded
+        # Notify Auth server that DDNS update messages can now be forwarded
         self.__notify_start_forwarder()
 
     class InternalError(Exception):
@@ -384,7 +384,7 @@ class DDNSServer:
         Do NOT call this to initialize shutdown, use trigger_shutdown().
 
         '''
-        # tell Auth not to forward UPDATE packets anymore
+        # tell Auth not to forward UPDATE messages anymore
         self.__notify_stop_forwarder()
         # tell the ModuleCCSession to send a message that this module is
         # stopping.
@@ -542,22 +542,28 @@ class DDNSServer:
         return True
 
     def __notify_start_forwarder(self):
-        '''Notify auth that DDNS Update packets can now be forwarded'''
-        seq = self._cc._session.group_sendmsg(create_command(
-                "start_ddns_forwarder"), AUTH_MODULE_NAME)
-        answer, _ = self._cc._session.group_recvmsg(False, seq)
-        rcode, error_msg = parse_answer(answer)
-        if rcode != 0:
-            logger.error(DDNS_START_FORWARDER_ERROR, error_msg)
+        '''Notify auth that DDNS Update messages can now be forwarded'''
+        try:
+            seq = self._cc._session.group_sendmsg(create_command(
+                    "start_ddns_forwarder"), AUTH_MODULE_NAME)
+            answer, _ = self._cc._session.group_recvmsg(False, seq)
+            rcode, error_msg = parse_answer(answer)
+            if rcode != 0:
+                logger.error(DDNS_START_FORWARDER_ERROR, error_msg)
+        except (SessionTimeout, SessionError, ProtocolError) as ex:
+            logger.error(DDNS_START_FORWARDER_FAIL, ex)
 
     def __notify_stop_forwarder(self):
-        '''Notify auth that DDNS Update packets can now be forwarded'''
-        seq = self._cc._session.group_sendmsg(create_command(
-                "stop_ddns_forwarder"), AUTH_MODULE_NAME)
-        answer, _ = self._cc._session.group_recvmsg(False, seq)
-        rcode, error_msg = parse_answer(answer)
-        if rcode != 0:
-            logger.error(DDNS_STOP_FORWARDER_ERROR, error_msg)
+        '''Notify auth that DDNS Update messages should no longer be forwarded'''
+        try:
+            seq = self._cc._session.group_sendmsg(create_command(
+                    "stop_ddns_forwarder"), AUTH_MODULE_NAME)
+            answer, _ = self._cc._session.group_recvmsg(False, seq)
+            rcode, error_msg = parse_answer(answer)
+            if rcode != 0:
+                logger.error(DDNS_STOP_FORWARDER_ERROR, error_msg)
+        except (SessionTimeout, SessionError, ProtocolError) as ex:
+            logger.error(DDNS_STOP_FORWARDER_FAIL, ex)
 
     def __notify_auth(self, zname, zclass):
         '''Notify auth of the update, if necessary.'''

src/bin/ddns/ddns_messages.mes

@@ -192,11 +192,24 @@ be completed, after which the process will exit.
 The ddns process has successfully started and is now ready to receive commands
 and updates.
 
-% DDNS_START_FOWARDER_ERROR Error from b10-auth when requesting DDNS UPDATE forwarding: %1
+% DDNS_START_FORWARDER_ERROR Error from b10-auth when requesting DDNS UPDATE forwarding: %1
 There was an error response from b10-auth to the command to start
 forwarding DDNS UPDATE messages to b10-ddns.
+It is almost certain that DDNS UPDATE messages are not handled now, and that
+they will be responded to with a NOTIMP error code, even though b10-ddns is
+running.
 The error message is printed, and additional information may be found in
-the b10-auth log output.
+the b10-auth log output. Since this is an error that is sent by the
+b10-auth module, it should have more information as to what the actual
+problem was.
+
+% DDNS_START_FORWARDER_FAIL Error sending request for DDNS UPDATE forwarding to b10-auth: %1
+There was an error when attempting to send b10-auth the request to forward
+DDNS UPDATE messages to the b10-ddns module. This points to an internal
+problem using the inter-module messaging system.
+This needs to be inspected, as it is almost certain that DDNS UPDATE messages
+are not handled now.
+The specific error is printed in the log message.
 
 % DDNS_STOPPED ddns server has stopped
 The ddns process has successfully stopped and is no longer listening for or
@@ -206,12 +219,24 @@ handling commands or updates, and will now exit.
 There was a keyboard interrupt signal to stop the ddns process. The
 process will now shut down.
 
-% DDNS_STOP_FOWARDER_ERROR Error from b10-auth when requesting to stop DDNS UPDATE forwarding: %1
+% DDNS_STOP_FORWARDER_ERROR Error from b10-auth when requesting to stop DDNS UPDATE forwarding: %1
 There was an error response from b10-auth to the command to stop
 forwarding DDNS UPDATE messages to b10-ddns.
+This specific error may not be fatal; instead of returning NOTIMP for future
+DDNS UPDATE messages, it will return SERVFAIL. However, this does point to
+an underlying problem in the messaging system, and should be inspected.
 The error message is printed, and additional information may be found in
 the b10-auth log output.
 
+% DDNS_STOP_FORWARDER_FAIL Error sending request to stop DDNS UPDATE forwarding to b10-auth: %1
+There was an error when attempting to send b10-auth the request to stop
+forwarding DDNS UPDATE messages to the b10-ddns module. This points to an
+internal problem using the inter-module messaging system.
+This specific error may not be fatal; instead of returning NOTIMP for future
+DDNS UPDATE messages, it will return SERVFAIL. However, this does point to
+an underlying problem in the messaging system, and should be inspected.
+The specific error is printed in the log message.
+
 % DDNS_UNCAUGHT_EXCEPTION uncaught exception of type %1: %2
 The b10-ddns process encountered an uncaught exception and will now shut
 down. This is indicative of a programming error and should not happen under

src/bin/ddns/tests/ddns_test.py

@@ -1183,7 +1183,6 @@ class TestDDNSSession(unittest.TestCase):
         '''Check that the command 'start_ddns_forwarder' has been called
            This test removes said message from the sent message queue.
         '''
-
         sent_msg, sent_group = self.__cc_session._sent_msg.pop(0)
         sent_cmd = sent_msg['command']
         self.assertEqual('Auth', sent_group)
@@ -1194,7 +1193,7 @@ class TestDDNSSession(unittest.TestCase):
         self.__cc_session._recvmsg_called = 0
 
     def check_session_stop_forwarder_called(self):
-        '''Check that the command 'start_ddns_forwarder' has been called
+        '''Check that the command 'stop_ddns_forwarder' has been called
            This test removes said message from the sent message queue.
         '''
         # check the last message sent
@@ -1207,6 +1206,8 @@ class TestDDNSSession(unittest.TestCase):
     def test_session_msg(self):
         '''Test post update communication with other modules.'''
 
+        # Check that start_ddns_forwarder has been called upon
+        # initialization
         self.check_session_start_forwarder_called()
 
         # Normal cases, confirming communication takes place iff update
@@ -1306,6 +1307,37 @@ class TestDDNSSession(unittest.TestCase):
         # check the result
         self.check_session(UPDATE_DROP)
 
+    def test_session_start_stop_forwarder_failures(self):
+        '''Check that we don't crash if the server reports an error
+           setting up or closing down the DDNS UPDATE message forwarder,
+           or if there is an exception from the message queue.'''
+        self.__cc_session._answer_code = 1
+        self.server._DDNSServer__notify_start_forwarder()
+        self.server._DDNSServer__notify_stop_forwarder()
+
+        for exc in [ SessionError("sessionerror"),
+                     SessionTimeout("sessiontimeout"),
+                     ProtocolError("protocolerror") ]:
+            self.__cc_session._recvmsg_exception = exc
+            self.server._DDNSServer__notify_start_forwarder()
+            self.server._DDNSServer__notify_stop_forwarder()
+            self.__cc_session._recvmsg_exception = None
+
+            self.__cc_session._sendmsg_exception = exc
+            self.server._DDNSServer__notify_start_forwarder()
+            self.server._DDNSServer__notify_stop_forwarder()
+            self.__cc_session._recvmsg_exception = None
+
+    def test_session_auth_started(self):
+        '''Check that 'start_ddns_forwarder' is sent when the notification
+           'auth_started' is received'''
+        # It should have already been called during init
+        self.check_session_start_forwarder_called()
+
+        # auth_started message should trigger it again
+        answer = self.server.command_handler('auth_started', None)
+        self.check_session_start_forwarder_called()
+
 class TestMain(unittest.TestCase):
     def setUp(self):
         self._server = MyDDNSServer()