[master] Merge branch 'trac2712'

src/bin/cmdctl/cmdctl.py.in

@@ -247,6 +247,7 @@ class CommandControl():
         CommandControl to communicate with other modules. '''
         self._verbose = verbose
         self._httpserver = httpserver
+        self.__msg_handler_thread = None # set in _start_msg_handle_thread
         self._lock = threading.Lock()
         self._setup_session()
         self.modules_spec = self._get_modules_specification()
@@ -326,7 +327,26 @@ class CommandControl():
                     self._cmdctl_config_data[key] = new_config[key]
         return answer
 
+    def _get_current_thread(self):
+        """A simple wrapper of returning the 'current' thread object.
+
+        This is extracted as a 'protected' method so tests can override for
+        their convenience.
+
+        """
+        return threading.currentThread()
+
     def command_handler(self, command, args):
+        """Handle commands from other modules.
+
+        This method must not be called by any other threads than
+        __msg_handler_thread invoked at the intialization of the class;
+        otherwise it would cause critical race or dead locks.
+
+        """
+        # Check the restriction described above.
+        assert self._get_current_thread() == self.__msg_handler_thread
+
         answer = ccsession.create_answer(0)
         if command == ccsession.COMMAND_MODULE_SPECIFICATION_UPDATE:
             # The 'value' of a specification update can be either
@@ -362,6 +382,7 @@ class CommandControl():
         ''' Start one thread to handle received message from msgq.'''
         td = threading.Thread(target=self._handle_msg_from_msgq)
         td.daemon = True
+        self.__msg_handler_thread = td
         td.start()
 
     def _handle_msg_from_msgq(self):
@@ -402,7 +423,7 @@ class CommandControl():
         rcode, reply = self.send_command('ConfigManager', ccsession.COMMAND_GET_MODULE_SPEC)
         return self._parse_command_result(rcode, reply)
 
-    def send_command_with_check(self, module_name, command_name, params = None):
+    def send_command_with_check(self, module_name, command_name, params=None):
         '''Before send the command to modules, check if module_name, command_name
         parameters are legal according the spec file of the module.
         Return rcode, dict. TODO, the rcode should be defined properly.
@@ -424,31 +445,34 @@ class CommandControl():
 
         return self.send_command(module_name, command_name, params)
 
-    def send_command(self, module_name, command_name, params = None):
-        '''Send the command from bindctl to proper module. '''
+    def send_command(self, module_name, command_name, params=None):
+        """Send the command from bindctl to proper module.
+
+        Note that commands sent to Cmdctl itself are also delivered via the
+        CC session.  Since this method is called from a thread handling a
+        particular HTTP session, it cannot directly call command_handler().
+
+        """
         errstr = 'unknown error'
         answer = None
         logger.debug(DBG_CMDCTL_MESSAGING, CMDCTL_SEND_COMMAND,
                      command_name, module_name)
 
-        if module_name == self._module_name:
-            # Process the command sent to cmdctl directly.
-            answer = self.command_handler(command_name, params)
-        else:
-            # FIXME: Due to the fact that we use a separate session
-            # from the module one (due to threads and blocking), and
-            # because the plain cc session does not have the high-level
-            # rpc-call method, we use the low-level way and create the
-            # command ourselves.
-            msg = ccsession.create_command(command_name, params)
-            seq = self._cc.group_sendmsg(msg, module_name, want_answer=True)
-            logger.debug(DBG_CMDCTL_MESSAGING, CMDCTL_COMMAND_SENT,
-                         command_name, module_name)
-            #TODO, it may be blocked, msqg need to add a new interface waiting in timeout.
-            try:
-                answer, env = self._cc.group_recvmsg(False, seq)
-            except isc.cc.session.SessionTimeout:
-                errstr = "Module '%s' not responding" % module_name
+        # FIXME: Due to the fact that we use a separate session
+        # from the module one (due to threads and blocking), and
+        # because the plain cc session does not have the high-level
+        # rpc-call method, we use the low-level way and create the
+        # command ourselves.
+        msg = ccsession.create_command(command_name, params)
+        seq = self._cc.group_sendmsg(msg, module_name, want_answer=True)
+        logger.debug(DBG_CMDCTL_MESSAGING, CMDCTL_COMMAND_SENT, command_name,
+                     module_name)
+        # TODO, it may be blocked, msqg need to add a new interface waiting
+        # in timeout.
+        try:
+            answer, env = self._cc.group_recvmsg(False, seq)
+        except isc.cc.session.SessionTimeout:
+            errstr = "Module '%s' not responding" % module_name
 
         if answer:
             try:

src/bin/cmdctl/tests/cmdctl_test.py

@@ -373,7 +373,30 @@ class TestSecureHTTPRequestHandler(unittest.TestCase):
             self.handler._is_user_logged_in = orig_is_user_logged_in
             self.handler._check_user_name_and_pwd = orig_check_user_name_and_pwd
 
+class MockSession:
+    """Act like isc.cc.Session, stealing group_sendmsg/recvmsg().
+
+    The initial simple version only records given parameters in
+    group_sendmsg() for later inspection and raise a timeout exception
+    from recvmsg().  As we see the need for more test cases these methods
+    should be extended.
+
+    """
+    def __init__(self, sent_messages):
+        self.__sent_messages = sent_messages
+
+    def group_sendmsg(self, msg, module_name, want_answer):
+        self.__sent_messages.append((msg, module_name))
+
+    def group_recvmsg(self, nonblock, seq):
+        raise isc.cc.session.SessionTimeout('dummy timeout')
+
 class MyCommandControl(CommandControl):
+    def __init__(self, httpserver, verbose):
+        super().__init__(httpserver, verbose)
+        self.sent_messages = [] # for inspection; allow tests to see it
+        self._cc = MockSession(self.sent_messages)
+
     def _get_modules_specification(self):
         return {}
 
@@ -390,6 +413,12 @@ class MyCommandControl(CommandControl):
     def _handle_msg_from_msgq(self):
         pass
 
+    def _start_msg_handle_thread(self): # just not bother to be threads
+        pass
+
+    def _get_current_thread(self):
+        return None
+
 class TestCommandControl(unittest.TestCase):
 
     def setUp(self):
@@ -502,8 +531,24 @@ class TestCommandControl(unittest.TestCase):
         os.remove(file_name)
 
     def test_send_command(self):
-        rcode, value = self.cmdctl.send_command('Cmdctl', 'print_settings', None)
-        self.assertEqual(rcode, 0)
+        # Send a command to other module.  We check an expected message
+        # is sent via the session (cmdct._cc).  Due to the behavior of
+        # our mock session object the anser will be "fail", but it's not
+        # the subject of this test, and so it's okay.
+        # TODO: more detailed cases should be tested.
+        rcode, value = self.cmdctl.send_command('Init', 'shutdown', None)
+        self.assertEqual(1, len(self.cmdctl.sent_messages))
+        self.assertEqual(({'command': ['shutdown']}, 'Init'),
+                         self.cmdctl.sent_messages[-1])
+        self.assertEqual(1, rcode)
+
+        # Send a command to cmdctl itself.  Should be the same effect.
+        rcode, value = self.cmdctl.send_command('Cmdctl', 'print_settings',
+                                                None)
+        self.assertEqual(2, len(self.cmdctl.sent_messages))
+        self.assertEqual(({'command': ['print_settings']}, 'Cmdctl'),
+                         self.cmdctl.sent_messages[-1])
+        self.assertEqual(1, rcode)
 
 class MySecureHTTPServer(SecureHTTPServer):
     def server_bind(self):

tests/lettuce/features/bindctl_commands.feature

@@ -154,3 +154,18 @@ Feature: control with bindctl
         bind10 module Xfrout should be running
         bind10 module Xfrin should be running
         bind10 module Zonemgr should be running
+
+    Scenario: Shutting down a certain module
+        # We could test with several modules, but for now we are particularly
+        # interested in shutting down cmdctl.  It previously caused hangup,
+        # so this scenario confirms it's certainly fixed.  Note: since cmdctl
+        # is a "needed" component, shutting it down will result in system
+        # shutdown.  So "send bind10 command" will fail (it cannot complete
+        # "quit").
+        Given I have bind10 running with configuration bindctl/bindctl.config
+        And wait for bind10 stderr message BIND10_STARTED_CC
+        And wait for bind10 stderr message CMDCTL_STARTED
+
+        When I send bind10 ignoring failure the command Cmdctl shutdown
+        And wait for bind10 stderr message CMDCTL_EXITING
+        And wait for bind10 stderr message BIND10_SHUTDOWN_COMPLETE

tests/lettuce/features/terrain/bind10_control.py

@@ -120,13 +120,15 @@ def have_bind10_running(step, config_file, cmdctl_port, process_name):
     step.given(start_step)
 
 # function to send lines to bindctl, and store the result
-def run_bindctl(commands, cmdctl_port=None):
+def run_bindctl(commands, cmdctl_port=None, ignore_failure=False):
     """Run bindctl.
        Parameters:
        commands: a sequence of strings which will be sent.
        cmdctl_port: a port number on which cmdctl is listening, is converted
                     to string if necessary. If not provided, or None, defaults
                     to 47805
+       ignore_failure(bool): if set to True, don't examin the result code
+                    of bindctl and assert it succeeds.
 
        bindctl's stdout and stderr streams are stored (as one multiline string
        in world.last_bindctl_stdout/stderr.
@@ -140,6 +142,8 @@ def run_bindctl(commands, cmdctl_port=None):
     for line in commands:
         bindctl.stdin.write(line + "\n")
     (stdout, stderr) = bindctl.communicate()
+    if ignore_failure:
+        return
     result = bindctl.returncode
     world.last_bindctl_stdout = stdout
     world.last_bindctl_stderr = stderr
@@ -306,19 +310,25 @@ def config_remove_command(step, name, value, cmdctl_port):
                 "quit"]
     run_bindctl(commands, cmdctl_port)
 
-@step('send bind10(?: with cmdctl port (\d+))? the command (.+)')
-def send_command(step, cmdctl_port, command):
+@step('send bind10(?: with cmdctl port (\d+))?( ignoring failure)? the command (.+)')
+def send_command(step, cmdctl_port, ignore_failure, command):
     """
     Run bindctl, send the given command, and exit bindctl.
     Parameters:
     command ('the command <command>'): The command to send.
     cmdctl_port ('with cmdctl port <portnr>', optional): cmdctl port to send
                 the command to. Defaults to 47805.
-    Fails if cmdctl does not exit with status code 0.
+    ignore_failure ('ignoring failure', optional): set to None if bindctl
+    is expected to succeed (normal case, which is the default); if it is
+    not None, it means bindctl is expected to fail (and it's acceptable).
+
+    Fails if bindctl does not exit with status code 0 and ignore_failure
+    is not None.
+
     """
     commands = [command,
                 "quit"]
-    run_bindctl(commands, cmdctl_port)
+    run_bindctl(commands, cmdctl_port, ignore_failure is not None)
 
 @step('bind10 module (\S+) should( not)? be running')
 def module_is_running(step, name, not_str):