[640] initial part of review comments

src/bin/bind10/bind10_src.py.in

@@ -683,8 +683,7 @@ class BoB:
         # is stopping. Since everything will be stopped shortly, this is not
         # really necessary, but this is done to reflect that boss is also
         # 'just' a module.
-        if self.ccs is not None:
+        self.ccs.send_stopping()
-            self.ccs.send_stopping()
 
         # try using the BIND 10 request to stop
         try:

src/bin/cmdctl/cmdctl.py.in

@@ -315,8 +315,13 @@ class CommandControl():
             # set it. If it is None, delete the module if it is
             # known (and ignore otherwise).
             with self._lock:
-                if args[1] is None and args[0] in self.modules_spec:
+                if args[1] is None:
-                    del self.modules_spec[args[0]]
+                    if args[0] in self.modules_spec:
+                        del self.modules_spec[args[0]]
+                    else:
+                        answer = ccsession.create_answer(1,
+                                                         'No such module: ' +
+                                                         args[0])
                 else:
                     self.modules_spec[args[0]] = args[1]
 

src/bin/cmdctl/tests/cmdctl_test.py

@@ -368,6 +368,17 @@ class TestCommandControl(unittest.TestCase):
         # Should no longer be present
         self.assertFalse("foo" in self.cmdctl.modules_spec)
 
+        # Don't store 'None' if it wasn't there in the first place!
+        answer = self.cmdctl.command_handler(
+            ccsession.COMMAND_MODULE_SPECIFICATION_UPDATE, [ "foo", None ])
+        rcode, msg = ccsession.parse_answer(answer)
+        self.assertEqual(rcode, 1)
+        self.assertEqual(msg, "No such module: foo")
+
+        # Should still not present
+        self.assertFalse("foo" in self.cmdctl.modules_spec)
+
+
     def test_check_config_handler(self):
         answer = self.cmdctl.config_handler({'non-exist': 123})
         self._check_answer(answer, 1, 'unknown config item: non-exist')

src/bin/stats/tests/b10-stats_test.py

@@ -31,6 +31,7 @@ import imp
 import stats
 import isc.cc.session
 from test_utils import BaseModules, ThreadingServerManager, MyStats, SignalHandler, send_command, send_shutdown
+from isc.testutils.ccsession_mock import MockModuleCCSession
 
 class TestUtilties(unittest.TestCase):
     items = [
@@ -201,7 +202,10 @@ class TestStats(unittest.TestCase):
         self.assertEqual(send_command("status", "Stats"),
                 (0, "Stats is up. (PID " + str(os.getpid()) + ")"))
         self.assertTrue(self.stats.running)
+        # Override moduleCCSession so we can check if send_stopping is called
+        self.stats.mccs = MockModuleCCSession()
         self.assertEqual(send_shutdown("Stats"), (0, None))
+        self.assertTrue(self.stats.mccs.stopped)
         self.assertFalse(self.stats.running)
         self.stats_server.shutdown()
 

src/lib/python/isc/config/ccsession.py

@@ -429,11 +429,15 @@ class UIModuleCCSession(MultiConfigData):
            passed must have send_GET and send_POST functions"""
         MultiConfigData.__init__(self)
         self._conn = conn
-        self.request_specifications()
+        self.update_specs_and_config()
-        self.request_current_config()
 
     def request_specifications(self):
-        """Request the module specifications from b10-cmdctl"""
+        """Clears the current list of specifications, and requests a new
+            list from b10-cmdctl. As other actions may have caused modules
+            to be stopped, or new modules to be added, this is expected to
+            be run after each interaction (at this moment). It is usually
+            also combined with request_current_config(). For that reason,
+            we provide update_specs_and_config() which calls both."""
         # this step should be unnecessary but is the current way cmdctl returns stuff
         # so changes are needed there to make this clean (we need a command to simply get the
         # full specs for everything, including commands etc, not separate gets for that)
@@ -442,18 +446,24 @@ class UIModuleCCSession(MultiConfigData):
         for module in specs.keys():
             self.set_specification(isc.config.ModuleSpec(specs[module]))
 
-    def update_specs_and_config(self):
-        self.request_specifications()
-        self.request_current_config()
-
     def request_current_config(self):
         """Requests the current configuration from the configuration
-           manager through b10-cmdctl, and stores those as CURRENT"""
+           manager through b10-cmdctl, and stores those as CURRENT. This
+           does not modify any local changes, it just updates to the current
+           state of the server itself."""
         config = self._conn.send_GET('/config_data')
         if 'version' not in config or config['version'] != BIND10_CONFIG_DATA_VERSION:
             raise ModuleCCSessionError("Bad config version")
         self._set_current_config(config)
 
+    def update_specs_and_config(self):
+        """Convenience function to both clear and update the known list of
+           module specifications, and update the current configuration on
+           the server side. There are a few cases where the caller might only
+           want to run one of these tasks, but often they are both needed."""
+        self.request_specifications()
+        self.request_current_config()
+
     def _add_value_to_list(self, identifier, value, module_spec):
         cur_list, status = self.get_value(identifier)
         if not cur_list:
@@ -602,7 +612,7 @@ class UIModuleCCSession(MultiConfigData):
             # answer is either an empty dict (on success), or one
             # containing errors
             if answer == {}:
-                self.request_current_config()
+                self.update_specs_and_config()
                 self.clear_local_changes()
             elif "error" in answer:
                 raise ModuleCCSessionError("Error: " + str(answer["error"]) + "\n" + "Configuration not committed")

src/lib/python/isc/config/tests/cfgmgr_test.py

@@ -240,9 +240,6 @@ class TestConfigManager(unittest.TestCase):
 
     def test_read_config(self):
         self.assertEqual(self.cm.config.data, {'version': config_data.BIND10_CONFIG_DATA_VERSION})
-        self.cm.read_config()
-        # due to what get written, the value here is what the last set_config command in test_handle_msg does
-        self.assertEqual(self.cm.config.data, {'TestModule': {'test': 125}, 'version': config_data.BIND10_CONFIG_DATA_VERSION})
         self.cm.data_path = "/no_such_path"
         self.cm.read_config()
         self.assertEqual(self.cm.config.data, {'version': config_data.BIND10_CONFIG_DATA_VERSION})
@@ -255,115 +252,174 @@ class TestConfigManager(unittest.TestCase):
         answer = self.cm.handle_msg(msg)
         self.assertEqual(expected_answer, answer)
 
-    def test_handle_msg(self):
+    def test_handle_msg_basic_commands(self):
-        self._handle_msg_helper({}, { 'result': [ 1, 'Unknown message format: {}']})
+        # Some basic commands, where not much interaction happens, just
-        self._handle_msg_helper("", { 'result': [ 1, 'Unknown message format: ']})
+        # check the result
-        self._handle_msg_helper({ "command": [ "badcommand" ] }, { 'result': [ 1, "Unknown command: badcommand"]})
+        self._handle_msg_helper({},
-        self._handle_msg_helper({ "command": [ "get_commands_spec" ] }, { 'result': [ 0, {} ]})
+            { 'result': [ 1, 'Unknown message format: {}']})
-        self._handle_msg_helper({ "command": [ "get_statistics_spec" ] }, { 'result': [ 0, {} ]})
+        self._handle_msg_helper("",
-        self._handle_msg_helper({ "command": [ "get_module_spec" ] }, { 'result': [ 0, {} ]})
+            { 'result': [ 1, 'Unknown message format: ']})
-        self._handle_msg_helper({ "command": [ "get_module_spec", { "module_name": "Spec2" } ] }, { 'result': [ 0, {} ]})
+        self._handle_msg_helper({ "command": [ "badcommand" ] },
-        #self._handle_msg_helper({ "command": [ "get_module_spec", { "module_name": "nosuchmodule" } ] },
+            { 'result': [ 1, "Unknown command: badcommand"]})
-        #                        {'result': [1, 'No specification for module nosuchmodule']})
+        self._handle_msg_helper({ "command": [ "get_commands_spec" ] },
+                                { 'result': [ 0, {} ]})
+        self._handle_msg_helper({ "command": [ "get_statistics_spec" ] },
+                                { 'result': [ 0, {} ]})
+        self._handle_msg_helper({ "command": [ "get_module_spec" ] },
+                                { 'result': [ 0, {} ]})
+        self._handle_msg_helper({ "command": [ "get_module_spec",
+                                               { "module_name": "Spec2" } ] },
+                                { 'result': [ 0, {} ]})
         self._handle_msg_helper({ "command": [ "get_module_spec", 1 ] },
-                                {'result': [1, 'Bad get_module_spec command, argument not a dict']})
+                                {'result': [1, 'Bad get_module_spec command, '+
+                                               'argument not a dict']})
         self._handle_msg_helper({ "command": [ "get_module_spec", { } ] },
-                                {'result': [1, 'Bad module_name in get_module_spec command']})
+                                {'result': [1, 'Bad module_name in '+
-        self._handle_msg_helper({ "command": [ "get_config" ] }, { 'result': [ 0, { 'version': config_data.BIND10_CONFIG_DATA_VERSION } ]})
+                                               'get_module_spec command']})
-        self._handle_msg_helper({ "command": [ "get_config", { "module_name": "nosuchmodule" } ] },
+        self._handle_msg_helper({ "command": [ "get_config" ] },
-                                {'result': [0, { 'version': config_data.BIND10_CONFIG_DATA_VERSION }]})
+                                { 'result': [ 0, { 'version':
+                                    config_data.BIND10_CONFIG_DATA_VERSION }]})
+        self._handle_msg_helper({ "command": [ "get_config",
+                                    { "module_name": "nosuchmodule" } ] },
+                                {'result': [0, { 'version':
+                                    config_data.BIND10_CONFIG_DATA_VERSION }]})
         self._handle_msg_helper({ "command": [ "get_config", 1 ] },
-                                {'result': [1, 'Bad get_config command, argument not a dict']})
+                                {'result': [1, 'Bad get_config command, '+
+                                               'argument not a dict']})
         self._handle_msg_helper({ "command": [ "get_config", { } ] },
-                                {'result': [1, 'Bad module_name in get_config command']})
+                                {'result': [1, 'Bad module_name in '+
+                                               'get_config command']})
         self._handle_msg_helper({ "command": [ "set_config" ] },
                                 {'result': [1, 'Wrong number of arguments']})
         self._handle_msg_helper({ "command": [ "set_config", [{}]] },
                                 {'result': [0]})
+
         self.assertEqual(len(self.fake_session.message_queue), 0)
 
-        # the targets of some of these tests expect specific answers, put
+    def test_handle_msg_module_and_stats_commands(self):
-        # those in our fake msgq first.
+        self._handle_msg_helper({ "command":
-        my_ok_answer = { 'result': [ 0 ] }
+                                  ["module_spec", self.spec.get_full_spec()]
+                                },
+                                {'result': [0]})
+        # There should be a message on the queue about the 'new' Spec2 module
+        # from ConfigManager to Cmdctl, containing its name and full
+        # specification
+        self.assertEqual(ccsession.create_command(
+                            ccsession.COMMAND_MODULE_SPECIFICATION_UPDATE,
+                            [ self.spec.get_module_name(),
+                              self.spec.get_full_spec()]),
+                         self.fake_session.get_message("Cmdctl", None))
+
+        self._handle_msg_helper({ "command": [ "module_spec", { 'foo': 1 } ] },
+                                {'result': [1, 'Error in data definition: no '+
+                                               'module_name in module_spec']})
+
+        self._handle_msg_helper({ "command": [ "get_module_spec" ] },
+                                { 'result': [ 0, { self.spec.get_module_name():
+                                                 self.spec.get_full_spec() } ]})
+        self._handle_msg_helper({ "command": [ "get_module_spec",
+                                               { "module_name" : "Spec2" } ] },
+                                { 'result': [ 0, self.spec.get_full_spec() ] })
+        self._handle_msg_helper({ "command": [ "get_commands_spec" ] },
+                                { 'result': [ 0, { self.spec.get_module_name():
+                                              self.spec.get_commands_spec()}]})
+        self._handle_msg_helper({ "command": [ "get_statistics_spec" ] },
+                                { 'result': [ 0, { self.spec.get_module_name():
+                                             self.spec.get_statistics_spec()}]})
+
 
+    def __test_handle_msg_update_config_helper(self, new_config):
+        # Helper function for the common pattern in
+        # test_handle_msg_update_config; send 'set config', check for
+        # update message, check if config has indeed been updated
 
+        my_ok_answer = { 'result': [ 0 ] }
         # Send the 'ok' that cfgmgr expects back to the fake queue first
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        # then send the command
+
-        self._handle_msg_helper({ "command": [ "set_config", [self.name, { "test": 123 }] ] },
+        self._handle_msg_helper({ "command": [ "set_config",
+                                               [self.name, new_config] ] },
                                 my_ok_answer)
-        # The cfgmgr should have eaten the ok message, and sent out an update again
+
+        # The cfgmgr should have eaten the ok message, and sent out an update
+        # message
         self.assertEqual(len(self.fake_session.message_queue), 1)
-        self.assertEqual({'command': [ 'config_update', {'test': 123}]},
+        self.assertEqual({'command': [ 'config_update', new_config]},
                          self.fake_session.get_message(self.name, None))
+
+        # Config should have been updated
+        self.assertEqual(self.cm.config.data, {self.name: new_config,
+                            'version': config_data.BIND10_CONFIG_DATA_VERSION})
+
         # and the queue should now be empty again
         self.assertEqual(len(self.fake_session.message_queue), 0)
 
-        # below are variations of the theme above
+    def test_handle_msg_update_config(self):
-        self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
+        # Update the configuration and check results a few times
-        self._handle_msg_helper({ "command": [ "set_config", [self.name, { "test": 124 }] ] },
+        # only work the first time
-                                my_ok_answer)
+        self.__test_handle_msg_update_config_helper({ "test": 123 })
-        self.assertEqual(len(self.fake_session.message_queue), 1)
-        self.assertEqual({'command': [ 'config_update', {'test': 124}]},
-                         self.fake_session.get_message(self.name, None))
-        self.assertEqual(len(self.fake_session.message_queue), 0)
 
+        self.__test_handle_msg_update_config_helper({ "test": 124 })
 
-        # This is the last 'succes' one, the value set here is what test_read_config expects
+        self.__test_handle_msg_update_config_helper({ "test": 125 })
-        self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self._handle_msg_helper({ "command": [ "set_config", [ { self.name: { "test": 125 } }] ] },
-                                my_ok_answer )
-        self.assertEqual(len(self.fake_session.message_queue), 1)
-        self.assertEqual({'command': [ 'config_update', {'test': 125}]},
-                         self.fake_session.get_message(self.name, None))
-        self.assertEqual(len(self.fake_session.message_queue), 0)
 
-        my_bad_answer = { 'result': [1, "bad_answer"] }
+        self.__test_handle_msg_update_config_helper({ "test": 126 })
+
+        # Now send an error result (i.e. config not accepted)
+        my_bad_answer = { 'result': [1, "bad config"] }
         self.fake_session.group_sendmsg(my_bad_answer, "ConfigManager")
-        self._handle_msg_helper({ "command": [ "set_config", [ self.name, { "test": 125 }] ] },
+        self._handle_msg_helper({ "command": [ "set_config",
+                                               [self.name, { "test": 127 }] ] },
                                 my_bad_answer )
         self.assertEqual(len(self.fake_session.message_queue), 1)
-        self.assertEqual({'command': [ 'config_update', {'test': 125}]},
+        self.assertEqual({'command': [ 'config_update', {'test': 127}]},
                          self.fake_session.get_message(self.name, None))
+        # Config should not be updated due to the error
+        self.cm.read_config()
+        self.assertEqual(self.cm.config.data, { self.name: {'test': 126},
+                            'version': config_data.BIND10_CONFIG_DATA_VERSION})
+
         self.assertEqual(len(self.fake_session.message_queue), 0)
 
         self.fake_session.group_sendmsg(None, 'ConfigManager')
         self._handle_msg_helper({ "command": [ "set_config", [ ] ] },
                                 {'result': [1, 'Wrong number of arguments']} )
-        self._handle_msg_helper({ "command": [ "set_config", [ self.name, { "test": 125 }] ] },
+        self._handle_msg_helper({ "command": [ "set_config",
-                                { 'result': [1, 'No answer message from TestModule']} )
+                                               [ self.name, { "test": 128 }]]},
-
+                                { 'result': [1, 'No answer message '+
-        #self.assertEqual(len(self.fake_session.message_queue), 1)
+                                                'from TestModule']} )
-        #self.assertEqual({'config_update': {'test': 124}},
+
-        #                 self.fake_session.get_message(self.name, None))
+        # This command should leave a message to the TestModule to update its
-        #self.assertEqual({'version': 1, 'TestModule': {'test': 124}}, self.cm.config.data)
+        # configuration (since the TestModule did not eat it)
-        #
+        self.assertEqual(len(self.fake_session.message_queue), 1)
+        self.assertEqual(
+            ccsession.create_command(ccsession.COMMAND_CONFIG_UPDATE,
+                                     { "test": 128 }),
+            self.fake_session.get_message("TestModule", None))
+
+        # Make sure queue is empty now
+        self.assertEqual(len(self.fake_session.message_queue), 0)
+
+        # Shutdown should result in 'ok' answer
+        self._handle_msg_helper({ "command":
+                                  ["shutdown"]
+                                },
+                                {'result': [0]})
+
+    def test_stopping_message(self):
+        # Update the system by announcing this module
         self._handle_msg_helper({ "command":
                                   ["module_spec", self.spec.get_full_spec()]
                                 },
                                 {'result': [0]})
-        self._handle_msg_helper({ "command": [ "module_spec", { 'foo': 1 } ] },
-                                {'result': [1, 'Error in data definition: no module_name in module_spec']})
-        self._handle_msg_helper({ "command": [ "get_module_spec" ] }, { 'result': [ 0, { self.spec.get_module_name(): self.spec.get_full_spec() } ]})
-        self._handle_msg_helper({ "command": [ "get_module_spec",
-                                               { "module_name" : "Spec2" } ] },
-                                { 'result': [ 0, self.spec.get_full_spec() ] })
-        self._handle_msg_helper({ "command": [ "get_commands_spec" ] }, { 'result': [ 0, { self.spec.get_module_name(): self.spec.get_commands_spec() } ]})
-        self._handle_msg_helper({ "command": [ "get_statistics_spec" ] }, { 'result': [ 0, { self.spec.get_module_name(): self.spec.get_statistics_spec() } ]})
-        # re-add this once we have new way to propagate spec changes (1 instead of the current 2 messages)
-        #self.assertEqual(len(self.fake_session.message_queue), 2)
-        # the name here is actually wrong (and hardcoded), but needed in the current version
-        # TODO: fix that
-        #self.assertEqual({'specification_update': [ self.name, self.spec ] },
-        #                 self.fake_session.get_message("Cmdctl", None))
-        #self.assertEqual({'commands_update': [ self.name, self.commands ] },
-        #                 self.fake_session.get_message("Cmdctl", None))
-        # drop the two messages for now
-        self.assertEqual(len(self.fake_session.message_queue), 2)
-        self.fake_session.get_message("Cmdctl", None)
-        self.fake_session.get_message("TestModule", None)
 
-        self.assertEqual(len(self.fake_session.message_queue), 0)
+        # This causes a update to be sent from the ConfigManager to the CmdCtl
+        # channel, containing the new module's name and full specification
+        self.assertEqual(ccsession.create_command(
+                            ccsession.COMMAND_MODULE_SPECIFICATION_UPDATE,
+                            [ self.spec.get_module_name(),
+                              self.spec.get_full_spec()]),
+                         self.fake_session.get_message("Cmdctl", None))
 
         # A stopping message should get no response, but should cause another
         # message to be sent, if it is a known module
@@ -375,17 +431,20 @@ class TestConfigManager(unittest.TestCase):
                                        ['Spec2', None] ] },
                          self.fake_session.get_message("Cmdctl", None))
 
-        # but not if it is either unknown or not running
+        # but if the 'stopping' module is either unknown or not running,
+        # no followup message should be sent
         self._handle_msg_helper({ "command":
                                   [ "stopping",
                                     { "module_name": "NoSuchModule" } ] },
                                 None)
         self.assertEqual(len(self.fake_session.message_queue), 0)
 
-        self._handle_msg_helper({ "command":
+        # If the module does not exist, or is not seen as 'running', the
-                                  ["shutdown"]
+        # same message should not cause new messages to be sent
-                                },
+        self._handle_msg_helper({ "command": [ "stopping",
-                                {'result': [0]})
+                                               { "module_name": "Foo"}] },
+                                None)
+        self.assertEqual(len(self.fake_session.message_queue), 0)
 
     def test_set_config_virtual(self):
         """Test that if the module is virtual, we don't send it over the

src/lib/python/isc/testutils/ccsession_mock.py

@@ -14,13 +14,21 @@
 # WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 class MockModuleCCSession():
+    """Fake ModuleCCSession with a minimal implementation as needed by the
+       tests. Currently this module only stores whether some methods have
+       been called on it (send_stopping(), and close())"""
     def __init__(self):
-        self.started = False
+        """Will be set to True when send_stopping() is called"""
         self.stopped = False
+        """Will be set to True when close() is called"""
         self.closed = False
 
     def send_stopping(self):
+        """Fake send_stopping() call. No message is sent, but only stores
+           that this method has been called."""
         self.stopped = True
 
     def close(self):
+        """Fake close() call. Nothing is closed, but only stores
+           that this method has been called."""
         self.closed = True