[master] changelog for #1570 (forgot to push, and it caused a conflict)

ChangeLog

@@ -1,4 +1,4 @@
-375.	[bug]		jinmei, vorner
+376.	[bug]		jinmei, vorner
 	The new query handling module of b10-auth did not handle type DS
 	query correctly: It didn't look for it in the parent zone, and
 	it incorrectly returned a DS from the child zone if it
@@ -7,6 +7,14 @@
 	ancestor.
 	(Trac #1570, git 2858b2098a10a8cc2d34bf87463ace0629d3670e)
 
+375.	[func]      jelte
+	Modules now inform the system when they are stopping. As a result, they
+	are removed from the 'active modules' list in bindctl, which can then
+	inform the user directly when it tries to send them a command or
+	configuration update. Previously this would result in a 'not responding'
+	error instead of 'not running'.
+	(Trac #640, git 17e78fa1bb1227340aa9815e91ed5c50d174425d)
+
 374.	[func]*     stephen
 	Alter RRsetPtr and ConstRRsetPtr to point to AbstractRRset (instead
 	of RRset) to allow for specialised implementations of RRsets in

src/bin/bind10/bind10_src.py.in

@@ -679,7 +679,13 @@ class BoB:
     def shutdown(self):
         """Stop the BoB instance."""
         logger.info(BIND10_SHUTDOWN)
-        # first try using the BIND 10 request to stop
+        # If ccsession is still there, inform rest of the system this module
+        # 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.
+        self.ccs.send_stopping()
+
+        # try using the BIND 10 request to stop
         try:
             self._component_configurator.shutdown()
         except:

src/bin/bind10/tests/bind10_test.py.in

@@ -36,6 +36,7 @@ import isc.bind10.socket_cache
 import errno
 
 from isc.testutils.parse_args import TestOptParser, OptsError
+from isc.testutils.ccsession_mock import MockModuleCCSession
 
 class TestProcessInfo(unittest.TestCase):
     def setUp(self):
@@ -1171,8 +1172,12 @@ class TestBossComponents(unittest.TestCase):
         bob._component_configurator.shutdown = self.__nullary_hook
         self.__called = False
 
+        bob.ccs = MockModuleCCSession()
+        self.assertFalse(bob.ccs.stopped)
+
         bob.shutdown()
 
+        self.assertTrue(bob.ccs.stopped)
         self.assertEqual([False, True], killed)
         self.assertTrue(self.__called)
 

src/bin/cmdctl/cmdctl.py.in

@@ -310,12 +310,25 @@ class CommandControl():
     def command_handler(self, command, args):
         answer = ccsession.create_answer(0)
         if command == ccsession.COMMAND_MODULE_SPECIFICATION_UPDATE:
+            # The 'value' of a specification update can be either
+            # a specification, or None. In the first case, simply
+            # set it. If it is None, delete the module if it is
+            # known.
             with self._lock:
-                self.modules_spec[args[0]] = args[1]
+                if args[1] is None:
+                    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]
 
         elif command == ccsession.COMMAND_SHUTDOWN:
             #When cmdctl get 'shutdown' command from boss,
             #shutdown the outer httpserver.
+            self._module_cc.send_stopping()
             self._httpserver.shutdown()
             self._serving = False
 

src/bin/cmdctl/tests/cmdctl_test.py

@@ -345,6 +345,40 @@ class TestCommandControl(unittest.TestCase):
         self.assertEqual(rcode, 0)
         self.assertTrue(msg != None)
 
+    def test_command_handler_spec_update(self):
+        # Should not be present
+        self.assertFalse("foo" in self.cmdctl.modules_spec)
+
+        answer = self.cmdctl.command_handler(
+            ccsession.COMMAND_MODULE_SPECIFICATION_UPDATE, [ "foo", {} ])
+        rcode, msg = ccsession.parse_answer(answer)
+        self.assertEqual(rcode, 0)
+        self.assertEqual(msg, None)
+
+        # Should now be present
+        self.assertTrue("foo" in self.cmdctl.modules_spec)
+
+        # When sending specification 'None', it should be removed
+        answer = self.cmdctl.command_handler(
+            ccsession.COMMAND_MODULE_SPECIFICATION_UPDATE, [ "foo", None ])
+        rcode, msg = ccsession.parse_answer(answer)
+        self.assertEqual(rcode, 0)
+        self.assertEqual(msg, None)
+
+        # 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/ddns/ddns.py.in

@@ -150,9 +150,10 @@ class DDNSServer:
         Perform any cleanup that is necessary when shutting down the server.
         Do NOT call this to initialize shutdown, use trigger_shutdown().
 
-        Currently, it does nothing, but cleanup routines are expected.
+        Currently, it only causes the ModuleCCSession to send a message that
+        this module is stopping.
         '''
-        pass
+        self._cc.send_stopping()
 
     def accept(self):
         """

src/bin/ddns/tests/ddns_test.py

@@ -58,11 +58,16 @@ class MyCCSession(isc.config.ConfigData):
             ddns.SPECFILE_LOCATION)
         isc.config.ConfigData.__init__(self, module_spec)
         self._started = False
+        self._stopped = False
 
     def start(self):
         '''Called by DDNSServer initialization, but not used in tests'''
         self._started = True
 
+    def send_stopping(self):
+        '''Called by shutdown code'''
+        self._stopped = True
+
     def get_socket(self):
         """
         Used to get the file number for select.
@@ -289,6 +294,7 @@ class TestDDNSServer(unittest.TestCase):
         self.__select_answer = ([3], [], [])
         self.ddns_server.run()
         self.assertTrue(self.ddns_server._shutdown)
+        self.assertTrue(self.__cc_session._stopped)
         self.assertIsNone(self.__select_answer)
         self.assertEqual(3, self.__hook_called)
 

src/bin/stats/stats.py.in

@@ -184,8 +184,11 @@ class Stats:
             raise StatsError("stats spec file is incorrect: "
                              + ", ".join(errors))
 
-        while self.running:
-            self.mccs.check_command(False)
+        try:
+            while self.running:
+                self.mccs.check_command(False)
+        finally:
+            self.mccs.send_stopping()
 
     def config_handler(self, new_config):
         """

src/bin/stats/stats_httpd.py.in

@@ -203,6 +203,7 @@ class StatsHttpd:
         """Closes a ModuleCCSession object"""
         if self.mccs is None:
             return
+        self.mccs.send_stopping()
 
         logger.debug(DBG_STATHTTPD_INIT, STATHTTPD_CLOSING_CC_SESSION)
         self.mccs.close()

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

@@ -37,7 +37,10 @@ import random
 import isc
 import stats_httpd
 import stats
-from test_utils import BaseModules, ThreadingServerManager, MyStats, MyStatsHttpd, SignalHandler, send_command, send_shutdown
+from test_utils import BaseModules, ThreadingServerManager, MyStats,\
+                       MyStatsHttpd, SignalHandler,\
+                       send_command, send_shutdown
+from isc.testutils.ccsession_mock import MockModuleCCSession
 
 DUMMY_DATA = {
     'Boss' : {
@@ -676,7 +679,13 @@ class TestStatsHttpd(unittest.TestCase):
 
     def test_openclose_mccs(self):
         self.stats_httpd = MyStatsHttpd(get_availaddr())
+        mccs = MockModuleCCSession()
+        self.stats_httpd.mccs = mccs
+        self.assertFalse(self.stats_httpd.mccs.stopped)
+        self.assertFalse(self.stats_httpd.mccs.closed)
         self.stats_httpd.close_mccs()
+        self.assertTrue(mccs.stopped)
+        self.assertTrue(mccs.closed)
         self.assertEqual(self.stats_httpd.mccs, None)
         self.stats_httpd.open_mccs()
         self.assertIsNotNone(self.stats_httpd.mccs)

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,9 +202,15 @@ 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.assertFalse(self.stats.running)
-        self.stats_server.shutdown()
+        # Call server.shutdown with argument True so the thread.join() call
+        # blocks and we are sure the main loop has finished (and set
+        # mccs.stopped)
+        self.stats_server.shutdown(True)
+        self.assertTrue(self.stats.mccs.stopped)
 
         # start with err
         self.stats = stats.Stats()

src/bin/stats/tests/test_utils.py

@@ -72,9 +72,19 @@ class ThreadingServerManager:
         self.server._started.wait()
         self.server._started.clear()
 
-    def shutdown(self):
+    def shutdown(self, blocking=False):
+        """Shut down the server by calling its own shutdown() method.
+           Then wait for its thread to finish. If blocking is True,
+           the thread.join() blocks until the thread finishes. If not,
+           it uses a zero timeout. The latter is necessary in a number
+           of existing tests. We should redo this part (we should not
+           even need threads in most, if not all, of these threads, see
+           ticket #1668)"""
         self.server.shutdown()
-        self.server._thread.join(0) # timeout is 0
+        if blocking:
+            self.server._thread.join()
+        else:
+            self.server._thread.join(0) # timeout is 0
 
 def do_nothing(*args, **kwargs): pass
 

src/bin/xfrin/tests/xfrin_test.py

@@ -20,6 +20,7 @@ import socket
 import sys
 import io
 from isc.testutils.tsigctx_mock import MockTSIGContext
+from isc.testutils.ccsession_mock import MockModuleCCSession
 from isc.testutils.rrset_utils import *
 from xfrin import *
 import xfrin
@@ -105,7 +106,7 @@ class XfrinTestException(Exception):
 class XfrinTestTimeoutException(Exception):
     pass
 
-class MockCC():
+class MockCC(MockModuleCCSession):
     def get_default_value(self, identifier):
         # The returned values should be identical to the spec file
         # XXX: these should be retrieved from the spec file
@@ -2052,7 +2053,9 @@ class TestXfrin(unittest.TestCase):
         self.args['tsig_key'] = ''
 
     def tearDown(self):
+        self.assertFalse(self.xfr._module_cc.stopped);
         self.xfr.shutdown()
+        self.assertTrue(self.xfr._module_cc.stopped);
         sys.stderr= self.stderr_backup
 
     def _do_parse_zone_name_class(self):

src/bin/xfrin/xfrin.py.in

@@ -1224,6 +1224,7 @@ class Xfrin:
         ''' shutdown the xfrin process. the thread which is doing xfrin should be
         terminated.
         '''
+        self._module_cc.send_stopping()
         self._shutdown_event.set()
         main_thread = threading.currentThread()
         for th in threading.enumerate():

src/bin/xfrout/tests/xfrout_test.py.in

@@ -19,6 +19,7 @@
 import unittest
 import os
 from isc.testutils.tsigctx_mock import MockTSIGContext
+from isc.testutils.ccsession_mock import MockModuleCCSession
 from isc.cc.session import *
 import isc.config
 from isc.dns import *
@@ -1423,6 +1424,32 @@ class TestInitialization(unittest.TestCase):
         xfrout.init_paths()
         self.assertEqual(xfrout.UNIX_SOCKET_FILE, "The/Socket/File")
 
+class MyNotifier():
+    def __init__(self):
+        self.shutdown_called = False
+
+    def shutdown(self):
+        self.shutdown_called = True
+
+class MyXfroutServer(XfroutServer):
+    def __init__(self):
+        self._cc = MockModuleCCSession()
+        self._shutdown_event = threading.Event()
+        self._notifier = MyNotifier()
+        self._unix_socket_server = None
+        # Disable the wait for threads
+        self._wait_for_threads = lambda : None
+
+class TestXfroutServer(unittest.TestCase):
+    def setUp(self):
+        self.xfrout_server = MyXfroutServer()
+
+    def test_shutdown(self):
+        self.xfrout_server.shutdown()
+        self.assertTrue(self.xfrout_server._notifier.shutdown_called)
+        self.assertTrue(self.xfrout_server._cc.stopped)
+
+
 if __name__== "__main__":
     isc.log.resetUnitTestRootLogger()
     unittest.main()

src/bin/xfrout/xfrout.py.in

@@ -969,12 +969,18 @@ class XfroutServer:
 
         global xfrout_server
         xfrout_server = None #Avoid shutdown is called twice
+        self._cc.send_stopping()
         self._shutdown_event.set()
         self._notifier.shutdown()
         if self._unix_socket_server:
             self._unix_socket_server.shutdown()
+        self._wait_for_threads()
 
-        # Wait for all threads to terminate
+    def _wait_for_threads(self):
+        # Wait for all threads to terminate. this is a call that is only used
+        # in shutdown(), but it has its own method, so we can test shutdown
+        # without involving thread operations (the test would override this
+        # method)
         main_thread = threading.currentThread()
         for th in threading.enumerate():
             if th is main_thread:

src/bin/zonemgr/tests/zonemgr_test.py

@@ -20,6 +20,7 @@ import unittest
 import os
 import tempfile
 from zonemgr import *
+from isc.testutils.ccsession_mock import MockModuleCCSession
 
 ZONE_NAME_CLASS1_IN = ("example.net.", "IN")
 ZONE_NAME_CLASS1_CH = ("example.net.", "CH")
@@ -48,10 +49,11 @@ class MySession():
     def group_recvmsg(self, nonblock, seq):
         return None, None
 
-class FakeCCSession(isc.config.ConfigData):
+class FakeCCSession(isc.config.ConfigData, MockModuleCCSession):
     def __init__(self):
         module_spec = isc.config.module_spec_from_file(SPECFILE_LOCATION)
         ConfigData.__init__(self, module_spec)
+        MockModuleCCSession.__init__(self)
 
     def get_remote_config_value(self, module_name, identifier):
         if module_name == "Auth" and identifier == "database_file":
@@ -683,6 +685,12 @@ class TestZonemgr(unittest.TestCase):
         self.zonemgr._config_data_check(config_data3)
         self.assertEqual(0.5, config_data3.get("refresh_jitter"))
 
+    def test_shutdown(self):
+        self.assertFalse(self.zonemgr._module_cc.stopped)
+        self.zonemgr._shutdown_event.set()
+        self.zonemgr.run()
+        self.assertTrue(self.zonemgr._module_cc.stopped)
+
     def tearDown(self):
         pass
 

src/bin/zonemgr/zonemgr.py.in

@@ -658,8 +658,11 @@ class Zonemgr:
 
     def run(self):
         self.running = True
-        while not self._shutdown_event.is_set():
-            self._module_cc.check_command(False)
+        try:
+            while not self._shutdown_event.is_set():
+                self._module_cc.check_command(False)
+        finally:
+            self._module_cc.send_stopping()
 
 zonemgrd = None
 

src/lib/config/ccsession.cc

@@ -489,6 +489,18 @@ ModuleCCSession::ModuleCCSession(
 
 }
 
+ModuleCCSession::~ModuleCCSession() {
+    try {
+        sendStopping();
+    } catch (const std::exception& exc) {
+        LOG_ERROR(config_logger,
+                  CONFIG_CCSESSION_STOPPING).arg(exc.what());
+    } catch (...) {
+        LOG_ERROR(config_logger,
+                  CONFIG_CCSESSION_STOPPING_UNKNOWN);
+    }
+};
+
 void
 ModuleCCSession::start() {
     if (started_) {
@@ -741,5 +753,16 @@ ModuleCCSession::updateRemoteConfig(const std::string& module_name,
     }
 }
 
+void
+ModuleCCSession::sendStopping() {
+    // Inform the configuration manager that this module is stopping
+    ConstElementPtr cmd(createCommand("stopping",
+                                      Element::fromJSON(
+                                          "{\"module_name\": \"" +
+                                          module_name_ + "\"}")));
+    // It's just an FYI, configmanager is not expected to respond.
+    session_.group_sendmsg(cmd, "ConfigManager");
+}
+
 }
 }

src/lib/config/ccsession.h

@@ -192,6 +192,14 @@ public:
                     bool handle_logging = true
                     );
 
+    ///
+    /// Destructor
+    ///
+    /// The destructor automatically calls sendStopping(), which sends
+    /// a message to the ConfigManager that this module is stopping
+    ///
+    virtual ~ModuleCCSession();
+
     /// Start receiving new commands and configuration changes asynchronously.
     ///
     /// This method must be called only once, and only when the ModuleCCSession
@@ -353,6 +361,7 @@ public:
 private:
     ModuleSpec readModuleSpecification(const std::string& filename);
     void startCheck();
+    void sendStopping();
 
     bool started_;
     std::string module_name_;

src/lib/config/config_messages.mes

@@ -30,6 +30,18 @@ but will not send back an answer.
 The most likely cause of this error is a programming error.  Please raise
 a bug report.
 
+% CONFIG_CCSESSION_STOPPING error sending stopping message: %1
+There was a problem when sending a message signaling that the module using
+this CCSession is stopping. This message is sent so that the rest of the
+system is aware that the module is no longer running. Apart from logging
+this message, the error itself is ignored, and the ModuleCCSession is
+still stopped. The specific exception message is printed.
+
+% CONFIG_CCSESSION_STOPPING_UNKNOWN unknown error sending stopping message
+Similar to CONFIG_CCSESSION_STOPPING, but in this case the exception that
+is seen is not a standard exception, and further information is unknown.
+This is a bug.
+
 % CONFIG_GET_FAIL error getting configuration from cfgmgr: %1
 The configuration manager returned an error when this module requested
 the configuration. The full error message answer from the configuration
@@ -37,6 +49,11 @@ manager is appended to the log error. The most likely cause is that
 the module is of a different (command specification) version than the
 running configuration manager.
 
+% CONFIG_JSON_PARSE JSON parse error in %1: %2
+There was an error parsing the JSON file. The given file does not appear
+to be in valid JSON format. Please verify that the filename is correct
+and that the contents are valid JSON.
+
 % CONFIG_LOG_EXPLICIT will use logging configuration for explicitly-named logger %1
 This is a debug message.  When processing the "loggers" part of the
 configuration file, the configuration library found an entry for the named
@@ -62,11 +79,6 @@ wildcard entry (one containing the "*" character) that matches a logger
 specification in the program. The logging configuration for the program
 will be updated with the information.
 
-% CONFIG_JSON_PARSE JSON parse error in %1: %2
-There was an error parsing the JSON file. The given file does not appear
-to be in valid JSON format. Please verify that the filename is correct
-and that the contents are valid JSON.
-
 % CONFIG_MOD_SPEC_FORMAT module specification error in %1: %2
 The given file does not appear to be a valid specification file: details
 are included in the message. Please verify that the filename is correct

src/lib/config/tests/ccsession_unittests.cc

@@ -26,6 +26,8 @@
 
 #include <log/logger_name.h>
 
+#include <boost/scoped_ptr.hpp>
+
 using namespace isc::data;
 using namespace isc::config;
 using namespace isc::cc;
@@ -190,6 +192,67 @@ TEST_F(CCSessionTest, session2) {
     EXPECT_EQ(0, session.getMsgQueue()->size());
 }
 
+TEST_F(CCSessionTest, session_close) {
+    // Test whether ModuleCCSession automatically sends a 'stopping'
+    // message when it is destroyed
+    ConstElementPtr msg;
+    std::string group, to;
+
+    EXPECT_FALSE(session.haveSubscription("Spec2", "*"));
+
+    boost::scoped_ptr<ModuleCCSession> mccs(new ModuleCCSession(
+                                         ccspecfile("spec2.spec"),
+                                         session, NULL, NULL,
+                                         true, false));
+    EXPECT_TRUE(session.haveSubscription("Spec2", "*"));
+    // The initial message is irrelevant for this test
+    // (see session2 test), drop it
+    session.getFirstMessage(group, to);
+    // Queue should now be empty
+    ASSERT_EQ(0, session.getMsgQueue()->size());
+    // Invoke the destructor
+    mccs.reset();
+    // Destructor should have caused a new message
+    ASSERT_EQ(1, session.getMsgQueue()->size());
+    msg = session.getFirstMessage(group, to);
+    EXPECT_EQ("{ \"command\": [ \"stopping\", "
+              "{ \"module_name\": \"Spec2\" } ] }", msg->str());
+    EXPECT_EQ("ConfigManager", group);
+    EXPECT_EQ("*", to);
+    EXPECT_EQ(0, session.getMsgQueue()->size());
+}
+
+TEST_F(CCSessionTest, session_close_exception) {
+    // Test whether an exception encountered during the destructor is
+    // handled correctly
+    ConstElementPtr msg;
+    std::string group, to;
+
+    EXPECT_FALSE(session.haveSubscription("Spec2", "*"));
+
+    boost::scoped_ptr<ModuleCCSession> mccs(new ModuleCCSession(
+                                         ccspecfile("spec2.spec"),
+                                         session, NULL, NULL,
+                                         true, false));
+    EXPECT_TRUE(session.haveSubscription("Spec2", "*"));
+    // The initial message is irrelevant for this test
+    // (see session2 test), drop it
+    session.getFirstMessage(group, to);
+    // Queue should now be empty
+    ASSERT_EQ(0, session.getMsgQueue()->size());
+
+    // Set fake session to throw an exception
+    session.setThrowOnSend(true);
+
+    // Invoke the destructor
+    mccs.reset();
+    // Destructor should not have caused a new message (since fakesession
+    // should have thrown an exception)
+    ASSERT_EQ(0, session.getMsgQueue()->size());
+    //EXPECT_EQ(0, session.getMsgQueue()->size());
+}
+
+
 ConstElementPtr my_config_handler(ConstElementPtr new_config) {
     if (new_config && new_config->contains("item1") &&
         new_config->get("item1")->intValue() == 5) {

src/lib/config/tests/fake_session.cc

@@ -72,7 +72,8 @@ FakeSession::FakeSession(isc::data::ElementPtr initial_messages,
     messages_(initial_messages),
     subscriptions_(subscriptions),
     msg_queue_(msg_queue),
-    started_(false)
+    started_(false),
+    throw_on_send_(false)
 {
 }
 
@@ -181,8 +182,9 @@ int
 FakeSession::group_sendmsg(ConstElementPtr msg, std::string group,
                            std::string to, std::string)
 {
-    //cout << "[XX] client sends message: " << msg << endl;
-    //cout << "[XX] to: " << group << " . " << instance << "." << to << endl;
+    if (throw_on_send_) {
+        isc_throw(Exception, "Throw on send is set in FakeSession");
+    }
     addMessage(msg, group, to);
     return (1);
 }
@@ -261,6 +263,5 @@ FakeSession::haveSubscription(ConstElementPtr group, ConstElementPtr instance)
 {
     return (haveSubscription(group->stringValue(), instance->stringValue()));
 }
-
 }
 }

src/lib/config/tests/fake_session.h

@@ -87,6 +87,14 @@ public:
     isc::data::ElementPtr getMessages() { return (messages_); }
     isc::data::ElementPtr getMsgQueue() { return (msg_queue_); }
 
+    /// Throw exception on sendmsg()
+    ///
+    /// When set to true, and sendmsg() is later called, this
+    /// will throw isc::Exception
+    ///
+    /// \param value If true, enable throw. If false, disable it
+    void setThrowOnSend(bool value) { throw_on_send_ = value; }
+
 private:
     bool recvmsg(isc::data::ConstElementPtr& msg,
                  bool nonblock = true, int seq = -1);
@@ -98,6 +106,7 @@ private:
     isc::data::ElementPtr subscriptions_;
     isc::data::ElementPtr msg_queue_;
     bool started_;
+    bool throw_on_send_;
 };
 } // namespace cc
 } // namespace isc

src/lib/datasrc/static_datasrc.cc

@@ -76,6 +76,7 @@ StaticDataSrcImpl::StaticDataSrcImpl() :
     authors->addRdata(generic::TXT("Han Feng"));
     authors->addRdata(generic::TXT("Jelte Jansen"));
     authors->addRdata(generic::TXT("Jeremy C. Reed")); 
+    authors->addRdata(generic::TXT("Xie Jiagui")); // Kevin Tes
     authors->addRdata(generic::TXT("Jin Jian"));
     authors->addRdata(generic::TXT("JINMEI Tatuya"));
     authors->addRdata(generic::TXT("Kazunori Fujiwara"));

src/lib/datasrc/tests/static_unittest.cc

@@ -58,7 +58,8 @@ protected:
         authors_data.push_back("Haidong Wang");
         authors_data.push_back("Han Feng");
         authors_data.push_back("Jelte Jansen");
-        authors_data.push_back("Jeremy C. Reed"); 
+        authors_data.push_back("Jeremy C. Reed");
+        authors_data.push_back("Xie Jiagui");
         authors_data.push_back("Jin Jian");
         authors_data.push_back("JINMEI Tatuya");
         authors_data.push_back("Kazunori Fujiwara");

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

@@ -97,6 +97,7 @@ COMMAND_SET_CONFIG = "set_config"
 COMMAND_GET_MODULE_SPEC = "get_module_spec"
 COMMAND_MODULE_SPEC = "module_spec"
 COMMAND_SHUTDOWN = "shutdown"
+COMMAND_MODULE_STOPPING = "stopping"
 
 def parse_command(msg):
     """Parses what may be a command message. If it looks like one,
@@ -210,6 +211,24 @@ class ModuleCCSession(ConfigData):
         self.__send_spec()
         self.__request_config()
 
+    def send_stopping(self):
+        """Sends a 'stopping' message to the configuration manager. This
+           message is just an FYI, and no response is expected. Any errors
+           when sending this message (for instance if the msgq session has
+           previously been closed) are logged, but ignored."""
+        # create_command could raise an exception as well, but except for
+        # out of memory related errors, these should all be programming
+        # failures and are not caught
+        msg = create_command(COMMAND_MODULE_STOPPING,
+                             self.get_module_spec().get_full_spec())
+        try:
+            self._session.group_sendmsg(msg, "ConfigManager")
+        except Exception as se:
+            # If the session was previously closed, obvously trying to send
+            # a message fails. (TODO: check if session is open so we can
+            # error on real problems?)
+            logger.error(CONFIG_SESSION_STOPPING_FAILED, se)
+
     def get_socket(self):
         """Returns the socket from the command channel session. This
            should *only* be used for select() loops to see if there
@@ -371,7 +390,7 @@ class ModuleCCSession(ConfigData):
         except isc.cc.SessionTimeout:
             # TODO: log an error?
             pass
-        
+
     def __request_config(self):
         """Asks the configuration manager for the current configuration, and call the config handler if set.
            Raises a ModuleCCSessionError if there is no answer from the configuration manager"""
@@ -410,30 +429,38 @@ class UIModuleCCSession(MultiConfigData):
            passed must have send_GET and send_POST functions"""
         MultiConfigData.__init__(self)
         self._conn = conn
-        self.request_specifications()
-        self.request_current_config()
+        self.update_specs_and_config()
 
     def request_specifications(self):
-        """Request the module specifications from b10-cmdctl"""
-        # 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)
+        """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."""
         specs = self._conn.send_GET('/module_spec')
+        self.clear_specifications()
         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:
@@ -582,7 +609,6 @@ class UIModuleCCSession(MultiConfigData):
             # answer is either an empty dict (on success), or one
             # containing errors
             if answer == {}:
-                self.request_current_config()
                 self.clear_local_changes()
             elif "error" in answer:
                 raise ModuleCCSessionError("Error: " + str(answer["error"]) + "\n" + "Configuration not committed")

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

@@ -297,7 +297,7 @@ class ConfigManager:
         """Write the current configuration to the file specificied at init()"""
         self.config.write_to_file()
 
-    def _handle_get_module_spec(self, cmd):
+    def __handle_get_module_spec(self, cmd):
         """Private function that handles the 'get_module_spec' command"""
         answer = {}
         if cmd != None:
@@ -318,7 +318,7 @@ class ConfigManager:
             answer = ccsession.create_answer(0, self.get_module_spec())
         return answer
 
-    def _handle_get_config_dict(self, cmd):
+    def __handle_get_config_dict(self, cmd):
         """Private function that handles the 'get_config' command
            where the command has been checked to be a dict"""
         if 'module_name' in cmd and cmd['module_name'] != '':
@@ -332,17 +332,17 @@ class ConfigManager:
         else:
             return ccsession.create_answer(1, "Bad module_name in get_config command")
 
-    def _handle_get_config(self, cmd):
+    def __handle_get_config(self, cmd):
         """Private function that handles the 'get_config' command"""
         if cmd != None:
             if type(cmd) == dict:
-                return self._handle_get_config_dict(cmd)
+                return self.__handle_get_config_dict(cmd)
             else:
                 return ccsession.create_answer(1, "Bad get_config command, argument not a dict")
         else:
             return ccsession.create_answer(0, self.config.data)
 
-    def _handle_set_config_module(self, module_name, cmd):
+    def __handle_set_config_module(self, module_name, cmd):
         # the answer comes (or does not come) from the relevant module
         # so we need a variable to see if we got it
         answer = None
@@ -405,7 +405,7 @@ class ConfigManager:
                 self.config.data = old_data
         return answer
 
-    def _handle_set_config_all(self, cmd):
+    def __handle_set_config_all(self, cmd):
         old_data = copy.deepcopy(self.config.data)
         got_error = False
         err_list = []
@@ -413,7 +413,7 @@ class ConfigManager:
         # sets, so we simply call set_config_module for each of those
         for module in cmd:
             if module != "version":
-                answer = self._handle_set_config_module(module, cmd[module])
+                answer = self.__handle_set_config_module(module, cmd[module])
                 if answer == None:
                     got_error = True
                     err_list.append("No answer message from " + module)
@@ -432,16 +432,16 @@ class ConfigManager:
             self.config.data = old_data
             return ccsession.create_answer(1, " ".join(err_list))
 
-    def _handle_set_config(self, cmd):
+    def __handle_set_config(self, cmd):
         """Private function that handles the 'set_config' command"""
         answer = None
 
         if cmd == None:
             return ccsession.create_answer(1, "Wrong number of arguments")
         if len(cmd) == 2:
-            answer = self._handle_set_config_module(cmd[0], cmd[1])
+            answer = self.__handle_set_config_module(cmd[0], cmd[1])
         elif len(cmd) == 1:
-            answer = self._handle_set_config_all(cmd[0])
+            answer = self.__handle_set_config_all(cmd[0])
         else:
             answer = ccsession.create_answer(1, "Wrong number of arguments")
         if not answer:
@@ -449,20 +449,41 @@ class ConfigManager:
 
         return answer
 
-    def _handle_module_spec(self, spec):
+    def __handle_module_spec(self, spec):
         """Private function that handles the 'module_spec' command"""
         # todo: validate? (no direct access to spec as
         # todo: use ModuleSpec class
         # todo: error checking (like keyerrors)
         answer = {}
         self.set_module_spec(spec)
+        self._send_module_spec_to_cmdctl(spec.get_module_name(),
+                                         spec.get_full_spec())
+        return ccsession.create_answer(0)
 
-        # We should make one general 'spec update for module' that
-        # passes both specification and commands at once
+    def __handle_module_stopping(self, arg):
+        """Private function that handles a 'stopping' command;
+           The argument is of the form { 'module_name': <name> }.
+           If the module is known, it is removed from the known list,
+           and a message is sent to the Cmdctl channel to remove it as well.
+           If it is unknown, the message is ignored."""
+        if arg['module_name'] in self.module_specs:
+            del self.module_specs[arg['module_name']]
+            self._send_module_spec_to_cmdctl(arg['module_name'], None)
+        # This command is not expected to be answered
+        return None
+
+    def _send_module_spec_to_cmdctl(self, module_name, spec):
+        """Sends the given module spec for the given module name to Cmdctl.
+           Parameters:
+           module_name: A string with the name of the module
+           spec: dict containing full module specification, as returned by
+                 ModuleSpec.get_full_spec(). This argument may also be None,
+                 in which case it signals Cmdctl to remove said module from
+                 its list.
+           No response from Cmdctl is expected."""
         spec_update = ccsession.create_command(ccsession.COMMAND_MODULE_SPECIFICATION_UPDATE,
-                                               [ spec.get_module_name(), spec.get_full_spec() ])
+                                               [ module_name, spec ])
         self.cc.group_sendmsg(spec_update, "Cmdctl")
-        return ccsession.create_answer(0)
 
     def handle_msg(self, msg):
         """Handle a command from the cc channel to the configuration manager"""
@@ -474,17 +495,19 @@ class ConfigManager:
             elif cmd == ccsession.COMMAND_GET_STATISTICS_SPEC:
                 answer = ccsession.create_answer(0, self.get_statistics_spec())
             elif cmd == ccsession.COMMAND_GET_MODULE_SPEC:
-                answer = self._handle_get_module_spec(arg)
+                answer = self.__handle_get_module_spec(arg)
             elif cmd == ccsession.COMMAND_GET_CONFIG:
-                answer = self._handle_get_config(arg)
+                answer = self.__handle_get_config(arg)
             elif cmd == ccsession.COMMAND_SET_CONFIG:
-                answer = self._handle_set_config(arg)
+                answer = self.__handle_set_config(arg)
+            elif cmd == ccsession.COMMAND_MODULE_STOPPING:
+                answer = self.__handle_module_stopping(arg)
             elif cmd == ccsession.COMMAND_SHUTDOWN:
                 self.running = False
                 answer = ccsession.create_answer(0)
             elif cmd == ccsession.COMMAND_MODULE_SPEC:
                 try:
-                    answer = self._handle_module_spec(isc.config.ModuleSpec(arg))
+                    answer = self.__handle_module_spec(isc.config.ModuleSpec(arg))
                 except isc.config.ModuleSpecError as dde:
                     answer = ccsession.create_answer(1, "Error in data definition: " + str(dde))
             else:
@@ -508,4 +531,6 @@ class ConfigManager:
             # not ask
             if msg is not None and not 'result' in msg:
                 answer = self.handle_msg(msg);
-                self.cc.group_reply(env, answer)
+                # Only respond if there actually is something to respond with
+                if answer is not None:
+                    self.cc.group_reply(env, answer)

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

@@ -313,6 +313,10 @@ class MultiConfigData:
         self._current_config = {}
         self._local_changes = {}
 
+    def clear_specifications(self):
+        """Remove all known module specifications"""
+        self._specifications = {}
+
     def set_specification(self, spec):
         """Add or update a ModuleSpec. Raises a ConfigDataError is spec is not a ModuleSpec"""
         if type(spec) != isc.config.ModuleSpec:

src/lib/python/isc/config/config_messages.mes

@@ -31,3 +31,9 @@ The configuration manager returned an error response when the module
 requested its configuration. The full error message answer from the
 configuration manager is appended to the log error.
 
+% CONFIG_SESSION_STOPPING_FAILED error sending stopping message: %1
+There was a problem when sending a message signaling that the module using
+this CCSession is stopping. This message is sent so that the rest of the
+system is aware that the module is no longer running. Apart from logging
+this message, the error itself is ignored, and the ModuleCCSession is
+still stopped. The specific exception message is printed.

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

@@ -250,6 +250,18 @@ class TestModuleCCSession(unittest.TestCase):
         self.assertEqual({'command': ['get_config', {'module_name': 'Spec2'}]},
                          fake_session.get_message('ConfigManager', None))
 
+    def test_stop(self):
+        fake_session = FakeModuleCCSession()
+        self.assertFalse("Spec1" in fake_session.subscriptions)
+        mccs = self.create_session("spec1.spec", None, None, fake_session)
+        self.assertTrue("Spec1" in fake_session.subscriptions)
+
+        self.assertEqual(len(fake_session.message_queue), 0)
+        mccs.send_stopping()
+        self.assertEqual(len(fake_session.message_queue), 1)
+        self.assertEqual({'command': ['stopping', {'module_name': 'Spec1'}]},
+                         fake_session.get_message('ConfigManager', None))
+
     def test_get_socket(self):
         fake_session = FakeModuleCCSession()
         mccs = self.create_session("spec1.spec", None, None, fake_session)
@@ -724,6 +736,38 @@ class TestUIModuleCCSession(unittest.TestCase):
         fake_conn.set_get_answer('/config_data', { 'version': 123123 })
         self.assertRaises(ModuleCCSessionError, UIModuleCCSession, fake_conn)
 
+    def test_request_specifications(self):
+        module_spec1 = isc.config.module_spec_from_file(
+                          self.spec_file("spec1.spec"))
+        module_spec_dict1 = { "module_spec": module_spec1.get_full_spec() }
+        module_spec2 = isc.config.module_spec_from_file(
+                          self.spec_file("spec2.spec"))
+        module_spec_dict2 = { "module_spec": module_spec2.get_full_spec() }
+
+        fake_conn = fakeUIConn()
+        # Set the first one in the answer
+        fake_conn.set_get_answer('/module_spec', module_spec_dict1)
+        fake_conn.set_get_answer('/config_data',
+                                 { 'version': BIND10_CONFIG_DATA_VERSION })
+        uccs = UIModuleCCSession(fake_conn)
+
+        # We should now have the first one, but not the second.
+        self.assertTrue("Spec1" in uccs._specifications)
+        self.assertEqual(module_spec1.get_full_spec(),
+                         uccs._specifications["Spec1"].get_full_spec())
+        self.assertFalse("Spec2" in uccs._specifications)
+
+        # Now set an answer where only the second one is present
+        fake_conn.set_get_answer('/module_spec', module_spec_dict2)
+
+        uccs.request_specifications()
+
+        # Now Spec1 should have been removed, and spec2 should be there
+        self.assertFalse("Spec1" in uccs._specifications)
+        self.assertTrue("Spec2" in uccs._specifications)
+        self.assertEqual(module_spec2.get_full_spec(),
+                         uccs._specifications["Spec2"].get_full_spec())
+
     def test_add_remove_value(self):
         fake_conn = fakeUIConn()
         uccs = self.create_uccs2(fake_conn)

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,195 @@ class TestConfigManager(unittest.TestCase):
         answer = self.cm.handle_msg(msg)
         self.assertEqual(expected_answer, answer)
 
-    def test_handle_msg(self):
-        self._handle_msg_helper({}, { 'result': [ 1, 'Unknown message format: {}']})
-        self._handle_msg_helper("", { 'result': [ 1, 'Unknown message format: ']})
-        self._handle_msg_helper({ "command": [ "badcommand" ] }, { 'result': [ 1, "Unknown command: badcommand"]})
-        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", { "module_name": "nosuchmodule" } ] },
-        #                        {'result': [1, 'No specification for module nosuchmodule']})
+    def test_handle_msg_basic_commands(self):
+        # Some basic commands, where not much interaction happens, just
+        # check the result
+        self._handle_msg_helper({},
+            { 'result': [ 1, 'Unknown message format: {}']})
+        self._handle_msg_helper("",
+            { 'result': [ 1, 'Unknown message format: ']})
+        self._handle_msg_helper({ "command": [ "badcommand" ] },
+            { 'result': [ 1, "Unknown command: badcommand"]})
+        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']})
-        self._handle_msg_helper({ "command": [ "get_config" ] }, { '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 }]})
+                                {'result': [1, 'Bad module_name in '+
+                                               'get_module_spec command']})
+        self._handle_msg_helper({ "command": [ "get_config" ] },
+                                { '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
-        # those in our fake msgq first.
-        my_ok_answer = { 'result': [ 0 ] }
+    def test_handle_msg_module_and_stats_commands(self):
+        self._handle_msg_helper({ "command":
+                                  ["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 }] ] },
+
+        config_version = config_data.BIND10_CONFIG_DATA_VERSION
+        self._handle_msg_helper({ "command": [ "set_config",
+                                               [ { "version": config_version,
+                                                 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_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
-        self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self._handle_msg_helper({ "command": [ "set_config", [self.name, { "test": 124 }] ] },
-                                my_ok_answer)
-        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)
+    def test_handle_msg_update_config(self):
+        # Update the configuration and check results a few times
+        # only work the first time
+        self.__test_handle_msg_update_config_helper({ "test": 123 })
 
+        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.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)
+        self.__test_handle_msg_update_config_helper({ "test": 125 })
+
+        self.__test_handle_msg_update_config_helper({ "test": 126 })
 
-        my_bad_answer = { 'result': [1, "bad_answer"] }
+        # 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 }] ] },
-                                { 'result': [1, 'No answer message from TestModule']} )
-
-        #self.assertEqual(len(self.fake_session.message_queue), 1)
-        #self.assertEqual({'config_update': {'test': 124}},
-        #                 self.fake_session.get_message(self.name, None))
-        #self.assertEqual({'version': 1, 'TestModule': {'test': 124}}, self.cm.config.data)
-        #
+        self._handle_msg_helper({ "command": [ "set_config",
+                                               [ self.name, { "test": 128 }]]},
+                                { 'result': [1, 'No answer message '+
+                                                'from TestModule']} )
+
+        # This command should leave a message to the TestModule to update its
+        # 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":
-                                  ["module_spec", self.spec.get_full_spec()]
+                                  ["shutdown"]
                                 },
                                 {'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))
 
+    def test_stopping_message(self):
+        # Update the system by announcing this module
         self._handle_msg_helper({ "command":
-                                  ["shutdown"]
+                                  ["module_spec", self.spec.get_full_spec()]
                                 },
                                 {'result': [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
+        self._handle_msg_helper({ "command": [ "stopping",
+                                               { "module_name": "Spec2"}] },
+                                None)
+        self.assertEqual(len(self.fake_session.message_queue), 1)
+        self.assertEqual({'command': [ 'module_specification_update',
+                                       ['Spec2', None] ] },
+                         self.fake_session.get_message("Cmdctl", None))
+
+        # 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)
+
     def test_set_config_virtual(self):
         """Test that if the module is virtual, we don't send it over the
            message bus, but call the checking function.
@@ -381,9 +458,9 @@ class TestConfigManager(unittest.TestCase):
             self.cm.set_virtual_module(self.spec, check_test)
             # The fake session will throw now if it tries to read a response.
             # Handy, we don't need to find a complicated way to check for it.
-            result = self.cm._handle_set_config_module(self.spec.
-                                                       get_module_name(),
-                                                       {'item1': value})
+            result = self.cm.handle_msg(ccsession.create_command(
+                        ccsession.COMMAND_SET_CONFIG,
+                        [self.spec.get_module_name(), { "item1": value }]))
             # Check the correct result is passed and our function was called
             # With correct data
             self.assertEqual(self.called_with['item1'], value)
@@ -415,19 +492,22 @@ class TestConfigManager(unittest.TestCase):
         self.assertEqual({"version": 2}, self.cm.config.data)
 
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self.cm._handle_set_config_all({"test": { "value1": 123 }})
+        self.cm.handle_msg(ccsession.create_command(
+            ccsession.COMMAND_SET_CONFIG, ["test", { "value1": 123 }]))
         self.assertEqual({"version": config_data.BIND10_CONFIG_DATA_VERSION,
                           "test": { "value1": 123 }
                          }, self.cm.config.data)
 
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self.cm._handle_set_config_all({"test": { "value1": 124 }})
+        self.cm.handle_msg(ccsession.create_command(
+            ccsession.COMMAND_SET_CONFIG, ["test", { "value1": 124 }]))
         self.assertEqual({"version": config_data.BIND10_CONFIG_DATA_VERSION,
                           "test": { "value1": 124 }
                          }, self.cm.config.data)
 
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self.cm._handle_set_config_all({"test": { "value2": True }})
+        self.cm.handle_msg(ccsession.create_command(
+            ccsession.COMMAND_SET_CONFIG, ["test", { "value2": True }]))
         self.assertEqual({"version": config_data.BIND10_CONFIG_DATA_VERSION,
                           "test": { "value1": 124,
                                     "value2": True
@@ -435,7 +515,8 @@ class TestConfigManager(unittest.TestCase):
                          }, self.cm.config.data)
 
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self.cm._handle_set_config_all({"test": { "value3": [ 1, 2, 3 ] }})
+        self.cm.handle_msg(ccsession.create_command(
+            ccsession.COMMAND_SET_CONFIG, ["test", { "value3": [ 1, 2, 3 ] }]))
         self.assertEqual({"version": config_data.BIND10_CONFIG_DATA_VERSION,
                           "test": { "value1": 124,
                                     "value2": True,
@@ -444,7 +525,8 @@ class TestConfigManager(unittest.TestCase):
                          }, self.cm.config.data)
 
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self.cm._handle_set_config_all({"test": { "value2": False }})
+        self.cm.handle_msg(ccsession.create_command(
+            ccsession.COMMAND_SET_CONFIG, ["test", { "value2": False }]))
         self.assertEqual({"version": config_data.BIND10_CONFIG_DATA_VERSION,
                           "test": { "value1": 124,
                                     "value2": False,
@@ -453,7 +535,8 @@ class TestConfigManager(unittest.TestCase):
                          }, self.cm.config.data)
 
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self.cm._handle_set_config_all({"test": { "value1": None }})
+        self.cm.handle_msg(ccsession.create_command(
+            ccsession.COMMAND_SET_CONFIG, ["test", { "value1": None }]))
         self.assertEqual({"version": config_data.BIND10_CONFIG_DATA_VERSION,
                           "test": { "value2": False,
                                     "value3": [ 1, 2, 3 ]
@@ -461,7 +544,8 @@ class TestConfigManager(unittest.TestCase):
                          }, self.cm.config.data)
 
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
-        self.cm._handle_set_config_all({"test": { "value3": [ 1 ] }})
+        self.cm.handle_msg(ccsession.create_command(
+            ccsession.COMMAND_SET_CONFIG, ["test", { "value3": [ 1 ] }]))
         self.assertEqual({"version": config_data.BIND10_CONFIG_DATA_VERSION,
                           "test": { "value2": False,
                                     "value3": [ 1 ]
@@ -472,9 +556,14 @@ class TestConfigManager(unittest.TestCase):
     def test_run(self):
         self.fake_session.group_sendmsg({ "command": [ "get_commands_spec" ] }, "ConfigManager")
         self.fake_session.group_sendmsg({ "command": [ "get_statistics_spec" ] }, "ConfigManager")
+        self.fake_session.group_sendmsg({ "command": [ "stopping", { "module_name": "FooModule" } ] }, "ConfigManager")
         self.fake_session.group_sendmsg({ "command": [ "shutdown" ] }, "ConfigManager")
+        self.assertEqual(len(self.fake_session.message_queue), 4)
         self.cm.run()
-        pass
+        # All commands should have been read out by run()
+        # Three of the commands should have been responded to, so the queue
+        # should now contain three answers
+        self.assertEqual(len(self.fake_session.message_queue), 3)
 
 
 if __name__ == '__main__':

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

@@ -312,6 +312,16 @@ class TestMultiConfigData(unittest.TestCase):
         self.mcd.remove_specification(module_spec.get_module_name())
         self.assertFalse(self.mcd.have_specification(module_spec.get_module_name()))
 
+    def test_clear_specifications(self):
+        self.assertEqual(0, len(self.mcd._specifications))
+        module_spec = isc.config.module_spec_from_file(self.data_path +
+                                                       os.sep +
+                                                       "spec1.spec")
+        self.mcd.set_specification(module_spec)
+        self.assertEqual(1, len(self.mcd._specifications))
+        self.mcd.clear_specifications()
+        self.assertEqual(0, len(self.mcd._specifications))
+
     def test_get_module_spec(self):
         module_spec = isc.config.module_spec_from_file(self.data_path + os.sep + "spec1.spec")
         self.mcd.set_specification(module_spec)

src/lib/python/isc/testutils/Makefile.am

@@ -1,4 +1,5 @@
-EXTRA_DIST = __init__.py parse_args.py tsigctx_mock.py rrset_utils.py
+EXTRA_DIST = __init__.py ccsession_mock.py parse_args.py tsigctx_mock.py \
+             rrset_utils.py
 
 CLEANDIRS = __pycache__
 

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

@@ -0,0 +1,34 @@
+# Copyright (C) 2012  Internet Systems Consortium.
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND INTERNET SYSTEMS CONSORTIUM
+# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
+# INTERNET SYSTEMS CONSORTIUM BE LIABLE FOR ANY SPECIAL, DIRECT,
+# INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
+# FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+# 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):
+        """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

tests/lettuce/features/bindctl_commands.feature

@@ -0,0 +1,38 @@
+Feature: control with bindctl
+    Assorted tests using bindctl for the administration of BIND 10.
+
+    Scenario: Removing modules
+    # This test runs the original example configuration, which has
+    # a number of modules. It then removes all non-essential modules,
+    # and checks whether they do disappear from the list of running
+    # modules (note that it 'misuses' the help command for this,
+    # there is a Boss command 'show_processes' but it's output is
+    # currently less standardized than 'help')
+    Given I have bind10 running with configuration example.org.config
+
+    Then remove bind10 configuration Boss/components/NOSUCHMODULE
+    last bindctl output should contain Error
+
+    bind10 module Xfrout should be running
+    bind10 module Stats should be running
+    bind10 module Zonemgr should be running
+    bind10 module Xfrin should be running
+    bind10 module Auth should be running
+    bind10 module StatsHttpd should be running
+
+    Then remove bind10 configuration Boss/components value b10-xfrout
+    last bindctl output should not contain Error
+    # assuming it won't error for further modules (if it does, the final
+    # 'should not be running' tests would fail anyway)
+    Then remove bind10 configuration Boss/components value b10-stats
+    Then remove bind10 configuration Boss/components value b10-zonemgr
+    Then remove bind10 configuration Boss/components value b10-xfrin
+    Then remove bind10 configuration Boss/components value b10-auth
+    Then remove bind10 configuration Boss/components value b10-stats-httpd
+
+    bind10 module Xfrout should not be running
+    bind10 module Stats should not be running
+    bind10 module Zonemgr should not be running
+    bind10 module Xfrin should not be running
+    bind10 module Auth should not be running
+    bind10 module StatsHttpd should not be running

tests/lettuce/features/terrain/bind10_control.py

@@ -112,8 +112,63 @@ def have_bind10_running(step, config_file, cmdctl_port, process_name):
     step.given(start_step)
     step.given(wait_step)
 
+# function to send lines to bindctl, and store the result
+def run_bindctl(commands, cmdctl_port=None):
+    """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
+
+       bindctl's stdout and stderr streams are stored (as one multiline string
+       in world.last_bindctl_stdout/stderr.
+       Fails if the return code is not 0
+    """
+    if cmdctl_port is None:
+        cmdctl_port = 47805
+    args = ['bindctl', '-p', str(cmdctl_port)]
+    bindctl = subprocess.Popen(args, 1, None, subprocess.PIPE,
+                               subprocess.PIPE, None)
+    for line in commands:
+        bindctl.stdin.write(line + "\n")
+    (stdout, stderr) = bindctl.communicate()
+    result = bindctl.returncode
+    world.last_bindctl_stdout = stdout
+    world.last_bindctl_stderr = stderr
+    assert result == 0, "bindctl exit code: " + str(result) +\
+                        "\nstdout:\n" + str(stdout) +\
+                        "stderr:\n" + str(stderr)
+
+
+@step('last bindctl( stderr)? output should( not)? contain (\S+)')
+def check_bindctl_output(step, stderr, notv, string):
+    """Checks the stdout (or stderr) stream of the last run of bindctl,
+       fails if the given string is not found in it (or fails if 'not' was
+       set and it is found
+       Parameters:
+       stderr ('stderr'): Check stderr instead of stdout output
+       notv ('not'): reverse the check (fail if string is found)
+       string ('contain <string>') string to look for
+    """
+    if stderr is None:
+        output = world.last_bindctl_stdout
+    else:
+        output = world.last_bindctl_stderr
+    found = False
+    if string in output:
+        found = True
+    if notv is None:
+        assert found == True, "'" + string +\
+                              "' was not found in bindctl output:\n" +\
+                              output
+    else:
+        assert not found, "'" + string +\
+                          "' was found in bindctl output:\n" +\
+                          output
+
 @step('set bind10 configuration (\S+) to (.*)(?: with cmdctl port (\d+))?')
-def set_config_command(step, name, value, cmdctl_port):
+def config_set_command(step, name, value, cmdctl_port):
     """
     Run bindctl, set the given configuration to the given value, and commit it.
     Parameters:
@@ -123,16 +178,30 @@ def set_config_command(step, name, value, cmdctl_port):
                 the command to. Defaults to 47805.
     Fails if cmdctl does not exit with status code 0.
     """
-    if cmdctl_port is None:
-        cmdctl_port = '47805'
-    args = ['bindctl', '-p', cmdctl_port]
-    bindctl = subprocess.Popen(args, 1, None, subprocess.PIPE,
-                               subprocess.PIPE, None)
-    bindctl.stdin.write("config set " + name + " " + value + "\n")
-    bindctl.stdin.write("config commit\n")
-    bindctl.stdin.write("quit\n")
-    result = bindctl.wait()
-    assert result == 0, "bindctl exit code: " + str(result)
+    commands = ["config set " + name + " " + value,
+                "config commit",
+                "quit"]
+    run_bindctl(commands, cmdctl_port)
+
+@step('remove bind10 configuration (\S+)(?: value (\S+))?(?: with cmdctl port (\d+))?')
+def config_remove_command(step, name, value, cmdctl_port):
+    """
+    Run bindctl, remove the given configuration item, and commit it.
+    Parameters:
+    name ('configuration <name>'): Identifier of the configuration to remove
+    value ('value <value>'): if name is a named set, use value to identify
+                             item to remove
+    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.
+    """
+    cmd = "config remove " + name
+    if value is not None:
+        cmd = cmd + " " + value
+    commands = [cmd,
+                "config commit",
+                "quit"]
+    run_bindctl(commands, cmdctl_port)
 
 @step('send bind10 the command (.+)(?: with cmdctl port (\d+))?')
 def send_command(step, command, cmdctl_port):
@@ -144,15 +213,21 @@ def send_command(step, command, cmdctl_port):
                 the command to. Defaults to 47805.
     Fails if cmdctl does not exit with status code 0.
     """
-    if cmdctl_port is None:
-        cmdctl_port = '47805'
-    args = ['bindctl', '-p', cmdctl_port]
-    bindctl = subprocess.Popen(args, 1, None, subprocess.PIPE,
-                               subprocess.PIPE, None)
-    bindctl.stdin.write(command + "\n")
-    bindctl.stdin.write("quit\n")
-    (stdout, stderr) = bindctl.communicate()
-    result = bindctl.returncode
-    assert result == 0, "bindctl exit code: " + str(result) +\
-                        "\nstdout:\n" + str(stdout) +\
-                        "stderr:\n" + str(stderr)
+    commands = [command,
+                "quit"]
+    run_bindctl(commands, cmdctl_port)
+
+@step('bind10 module (\S+) should( not)? be running')
+def module_is_running(step, name, not_str):
+    """
+    Convenience step to check if a module is running; can only work with
+    default cmdctl port; sends a 'help' command with bindctl, then
+    checks if the output contains the given name.
+    Parameters:
+    name ('module <name>'): The name of the module (case sensitive!)
+    not ('not'): Reverse the check (fail if it is running)
+    """
+    if not_str is None:
+        not_str = ""
+    step.given('send bind10 the command help')
+    step.given('last bindctl output should' + not_str + ' contain ' + name)