better handling of bad/old config data file

b10-cfgmgr now prints the specific error on startup (and not a stack trace), which includes the full path to the bad file
moved ConfigManagerData.CONFIG_VERSION to config_data.BIND10_CONFIG_DATA_VERSION
bumped version to 2; including automatic 'update' (versions 1 and 2 are the same except for the format itself, in which case you can't even get to the actual value)
checks automatically use the 'current' value



git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac294@2609 e5f2f494-b856-4b98-b285-d166d9295462

src/bin/cfgmgr/b10-cfgmgr.py.in

@@ -19,7 +19,7 @@
 
 import sys; sys.path.append ('@@PYTHONPATH@@')
 
-from isc.config.cfgmgr import ConfigManager
+from isc.config.cfgmgr import ConfigManager, ConfigManagerDataReadError
 from isc.cc import SessionError
 import signal
 import os
@@ -52,10 +52,15 @@ def main():
     except SessionError as se:
         print("[b10-cfgmgr] Error creating config manager, "
               "is the command channel daemon running?")
+        return 1
     except KeyboardInterrupt as kie:
         print("[b10-cfgmgr] Interrupted, exiting")
+    except ConfigManagerDataReadError as cmdre:
+        print(str(cmdre))
+        return 2
     if cm:
-        cm.write_config()
+        return cm.write_config()
+    return 0
 
 if __name__ == "__main__":
-    main()
+    sys.exit(main())

src/lib/config/Makefile.am

@@ -15,6 +15,7 @@ endif
 EXTRA_DIST =  testdata/b10-config-bad1.db
 EXTRA_DIST += testdata/b10-config-bad2.db
 EXTRA_DIST += testdata/b10-config-bad3.db
+EXTRA_DIST += testdata/b10-config-bad4.db
 EXTRA_DIST += testdata/b10-config.db
 EXTRA_DIST += testdata/data22_1.data
 EXTRA_DIST += testdata/data22_2.data

src/lib/config/testdata/b10-config-bad1.db

@@ -1 +1 @@
-{'version': 0}
+{"version": 0}

src/lib/config/testdata/b10-config-bad4.db

@@ -0,0 +1 @@
+{'version': 2}

src/lib/config/testdata/b10-config.db

@@ -1 +1 @@
-{"version": 1, "TestModule": {"test": 125}}
+{"version": 2, "TestModule": {"test": 125}}

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

@@ -37,7 +37,7 @@
 """
 
 from isc.cc import Session
-from isc.config.config_data import ConfigData, MultiConfigData
+from isc.config.config_data import ConfigData, MultiConfigData, BIND10_CONFIG_DATA_VERSION
 import isc
 
 class ModuleCCSessionError(Exception): pass
@@ -333,7 +333,7 @@ class UIModuleCCSession(MultiConfigData):
         """Requests the current configuration from the configuration
            manager through b10-cmdctl, and stores those as CURRENT"""
         config = self._conn.send_GET('/config_data')
-        if 'version' not in config or config['version'] != 1:
+        if 'version' not in config or config['version'] != BIND10_CONFIG_DATA_VERSION:
             raise ModuleCCSessionError("Bad config version")
         self._set_current_config(config)
 

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

@@ -27,7 +27,7 @@ import copy
 import tempfile
 import json
 from isc.cc import data
-from isc.config import ccsession
+from isc.config import ccsession, config_data
 
 class ConfigManagerDataReadError(Exception):
     """This exception is thrown when there is an error while reading
@@ -43,15 +43,13 @@ class ConfigManagerData:
     """This class hold the actual configuration information, and
        reads it from and writes it to persistent storage"""
 
-    CONFIG_VERSION = 1
-
     def __init__(self, data_path, file_name = "b10-config.db"):
         """Initialize the data for the configuration manager, and
            set the version and path for the data store. Initializing
            this does not yet read the database, a call to
            read_from_file is needed for that."""
         self.data = {}
-        self.data['version'] = ConfigManagerData.CONFIG_VERSION
+        self.data['version'] = config_data.BIND10_CONFIG_DATA_VERSION
         self.data_path = data_path
         self.db_filename = data_path + os.sep + file_name
 
@@ -67,19 +65,29 @@ class ConfigManagerData:
         config = ConfigManagerData(data_path, file_name)
         try:
             file = open(config.db_filename, 'r')
-            file_config = json.loads(file.read())
-            if 'version' in file_config and \
-                file_config['version'] == ConfigManagerData.CONFIG_VERSION:
-                config.data = file_config
-            else:
-                # We can put in a migration path here for old data
-                raise ConfigManagerDataReadError("[b10-cfgmgr] Old version of data found")
-            file.close()
+            try:
+                file_config = json.loads(file.read())
+                # handle different versions here
+                # 
+                if 'version' in file_config:
+                    if file_config['version'] == config_data.BIND10_CONFIG_DATA_VERSION:
+                        config.data = file_config
+                    elif file_config['version'] == 1:
+                        # only format change, no other changes necessary
+                        file_config['version'] = 2
+                        config.data = file_config
+                    else:
+                        # We can put in a migration path here for old data
+                        file.close();
+                        raise ConfigManagerDataReadError("[b10-cfgmgr] Old version of data found")
+                else:
+                    raise ConfigManagerDataReadError("No version information in configuration file " + config.db_filename)
+            except:
+                raise ConfigManagerDataReadError("Config file out of date or corrupt, please update or remove " + config.db_filename)
+            finally:
+                file.close();
         except IOError as ioe:
             raise ConfigManagerDataEmpty("No config file found")
-        except:
-            raise ConfigManagerDataReadError("Config file unreadable")
-
         return config
         
     def write_to_file(self, output_file_name = None):
@@ -243,7 +251,7 @@ class ConfigManager:
             except data.DataNotFoundError as dnfe:
                 # no data is ok, that means we have nothing that
                 # deviates from default values
-                return ccsession.create_answer(0, { 'version': self.config.CONFIG_VERSION })
+                return ccsession.create_answer(0, { 'version': config_data.BIND10_CONFIG_DATA_VERSION })
         else:
             return ccsession.create_answer(1, "Bad module_name in get_config command")
 

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

@@ -25,6 +25,8 @@ import isc.config.module_spec
 
 class ConfigDataError(Exception): pass
 
+BIND10_CONFIG_DATA_VERSION = 2
+
 def check_type(spec_part, value):
     """Does nothing if the value is of the correct type given the
        specification part relevant for the value. Raises an

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

@@ -22,6 +22,7 @@
 import unittest
 import os
 from isc.config.ccsession import *
+from isc.config.config_data import BIND10_CONFIG_DATA_VERSION
 from unittest_fakesession import FakeModuleCCSession
 
 class TestHelperFunctions(unittest.TestCase):
@@ -442,20 +443,20 @@ class TestUIModuleCCSession(unittest.TestCase):
     def create_uccs2(self, fake_conn):
         module_spec = isc.config.module_spec_from_file(self.spec_file("spec2.spec"))
         fake_conn.set_get_answer('/module_spec', { module_spec.get_module_name(): module_spec.get_full_spec()})
-        fake_conn.set_get_answer('/config_data', { 'version': 1 })
+        fake_conn.set_get_answer('/config_data', { 'version': BIND10_CONFIG_DATA_VERSION })
         return UIModuleCCSession(fake_conn)
 
     def test_init(self):
         fake_conn = fakeUIConn()
         fake_conn.set_get_answer('/module_spec', {})
-        fake_conn.set_get_answer('/config_data', { 'version': 1 })
+        fake_conn.set_get_answer('/config_data', { 'version': BIND10_CONFIG_DATA_VERSION })
         uccs = UIModuleCCSession(fake_conn)
         self.assertEqual({}, uccs._specifications)
-        self.assertEqual({ 'version': 1}, uccs._current_config)
+        self.assertEqual({ 'version': BIND10_CONFIG_DATA_VERSION}, uccs._current_config)
 
         module_spec = isc.config.module_spec_from_file(self.spec_file("spec2.spec"))
         fake_conn.set_get_answer('/module_spec', { module_spec.get_module_name(): module_spec.get_full_spec()})
-        fake_conn.set_get_answer('/config_data', { 'version': 1 })
+        fake_conn.set_get_answer('/config_data', { 'version': BIND10_CONFIG_DATA_VERSION })
         uccs = UIModuleCCSession(fake_conn)
         self.assertEqual(module_spec._module_spec, uccs._specifications['Spec2']._module_spec)
 

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

@@ -22,6 +22,7 @@
 import unittest
 import os
 from isc.config.cfgmgr import *
+from isc.config import config_data
 from unittest_fakesession import FakeModuleCCSession
 
 class TestConfigManagerData(unittest.TestCase):
@@ -32,7 +33,7 @@ class TestConfigManagerData(unittest.TestCase):
 
     def test_init(self):
         self.assertEqual(self.config_manager_data.data['version'],
-                         ConfigManagerData.CONFIG_VERSION)
+                         config_data.BIND10_CONFIG_DATA_VERSION)
         self.assertEqual(self.config_manager_data.data_path,
                          self.data_path)
         self.assertEqual(self.config_manager_data.db_filename,
@@ -52,6 +53,9 @@ class TestConfigManagerData(unittest.TestCase):
         self.assertRaises(ConfigManagerDataReadError,
                           ConfigManagerData.read_from_file,
                           self.data_path, "b10-config-bad3.db")
+        self.assertRaises(ConfigManagerDataReadError,
+                          ConfigManagerData.read_from_file,
+                          self.data_path, "b10-config-bad4.db")
 
     def test_write_to_file(self):
         output_file_name = "b10-config-write-test";
@@ -161,13 +165,13 @@ class TestConfigManager(unittest.TestCase):
         self.assertEqual(commands_spec['Spec2'], module_spec.get_commands_spec())
 
     def test_read_config(self):
-        self.assertEqual(self.cm.config.data, {'version': 1})
+        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': 1})
+        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': 1})
+        self.assertEqual(self.cm.config.data, {'version': config_data.BIND10_CONFIG_DATA_VERSION})
 
     def test_write_config(self):
         # tested in ConfigManagerData tests
@@ -190,9 +194,9 @@ class TestConfigManager(unittest.TestCase):
                                 {'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': 1} ]})
+        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': 1 }]})
+                                {'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']})
         self._handle_msg_helper({ "command": [ "get_config", { } ] },