cloud-init-dev team mailing list archive
-
cloud-init-dev team
-
Mailing list archive
-
Message #02274
Re: [Merge] ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master
@scott Addressed your review comments thanks a lot. So unittest runtimes 1m30sec versus 2m20sec? Do we want to make the switch to have all CiTestCase subclasses logged?
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index 225f898..257bb40 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -53,14 +53,12 @@ distros = ['centos', 'debian', 'fedora', 'opensuse', 'ubuntu']
>
>
> def handle(name, cfg, cloud, log, _args):
> - """
> - Enable and configure ntp
> + """Enable and configure ntp."""
>
> - ntp:
> - pools: ['0.{{distro}}.pool.ntp.org', '1.{{distro}}.pool.ntp.org']
> - servers: ['192.168.2.1']
> -
> - """
> + if 'ntp' not in cfg:
This is the same logic unchanged which was below, just hoisted up to the top of the module. It would process {"ntp": {}} and install ntp. The ntp_cfg local variable we create just below isn't actually referenced by the original check.
In this branch, and the followup cc-ntp-schema branch, we are allowing any "ntp" key of any value in cfg. I understood from Ryan a couple days ago in IRC that we actually want to allow for an empty ntp: configuration. The expected behavior in this case would be to install ntp and leave it with the default 4 pool servers. This is also defined in the schema more strictly in the cc-ntp-schema followup branch which declares "ntp" key as:
'ntp': {
'type': ['object', 'null'], # allow for a defined map or no definition.
> + LOG.debug(
> + "Skipping module named %s, not present or disabled by cfg", name)
> + return
>
> ntp_cfg = cfg.get('ntp', {})
>
> @@ -69,18 +67,12 @@ def handle(name, cfg, cloud, log, _args):
> " but not a dictionary type,"
> " is a %s %instead"), type_utils.obj_name(ntp_cfg))
>
> - if 'ntp' not in cfg:
> - LOG.debug("Skipping module named %s,"
> - "not present or disabled by cfg", name)
> - return True
> -
> rename_ntp_conf()
> # ensure when ntp is installed it has a configuration file
> # to use instead of starting up with packaged defaults
> - write_ntp_config_template(ntp_cfg, cloud)
> + write_ntp_config_template(cfg['ntp'], cloud)
fixed.
> install_ntp(cloud.distro.install_packages, packages=['ntp'],
> check_exe="ntpd")
> -
> # if ntp was already installed, it may not have started
> try:
> reload_ntp(systemd=cloud.distro.uses_systemd())
> diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
> index d24f817..9ff1599 100644
> --- a/tests/unittests/helpers.py
> +++ b/tests/unittests/helpers.py
> @@ -87,6 +92,27 @@ class TestCase(unittest2.TestCase):
> class CiTestCase(TestCase):
> """This is the preferred test case base class unless user
> needs other test case classes below."""
> +
> + # Subclass overrides for specific test behavior
> + # Whether or not a unit test needs logfile setup
> + with_logs = False
> +
> + def setUp(self):
> + super(CiTestCase, self).setUp()
> + if self.with_logs:
> + # Create a log handler so unit tests can search expected logs.
> + logger = logging.getLogger()
> + self.logs = StringIO()
> + handler = logging.StreamHandler(self.logs)
> + self.old_handlers = logger.handlers
> + logger.handlers = [handler]
2m20 seconds on avg for tox vs 1m30 seconds to add logs to all CiTestCase subclasses
> +
> + def tearDown(self):
> + if self.with_logs:
> + # Remove the handler we setup
> + logging.getLogger().handlers = self.old_handlers
> + super(CiTestCase, self).tearDown()
> +
> def tmp_dir(self, dir=None, cleanup=True):
> # return a full path to a temporary directory that will be cleaned up.
> if dir is None:
> diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
> index 02bd304..aa6c8ae 100644
> --- a/tests/unittests/test_handler/test_handler_ntp.py
> +++ b/tests/unittests/test_handler/test_handler_ntp.py
> @@ -59,294 +36,148 @@ class TestNtp(FilesystemMockingTestCase):
>
> @mock.patch("cloudinit.config.cc_ntp.util")
> def test_ntp_install(self, mock_util):
> - cc = self._get_cloud('ubuntu')
> - cc.distro = mock.MagicMock()
> - cc.distro.name = 'ubuntu'
> - mock_util.which.return_value = None
> + """ntp_install installs via install_func when check_exe is absent."""
> + mock_util.which.return_value = None # check_exe not found.
> install_func = mock.MagicMock()
> -
> cc_ntp.install_ntp(install_func, packages=['ntpx'], check_exe='ntpdx')
>
> - self.assertTrue(install_func.called)
> mock_util.which.assert_called_with('ntpdx')
> - install_pkg = install_func.call_args_list[0][0][0]
> - self.assertEqual(sorted(install_pkg), ['ntpx'])
> + install_func.assert_called_once_with(['ntpx'])
>
> @mock.patch("cloudinit.config.cc_ntp.util")
> def test_ntp_install_not_needed(self, mock_util):
> - cc = self._get_cloud('ubuntu')
> - cc.distro = mock.MagicMock()
> - cc.distro.name = 'ubuntu'
> - mock_util.which.return_value = ["/usr/sbin/ntpd"]
> - cc_ntp.install_ntp(cc)
> - self.assertFalse(cc.distro.install_packages.called)
> + """ntp_install doesn't attempt install when check_exe is found."""
> + mock_util.which.return_value = ["/usr/sbin/ntpd"] # check_exe found.
> + install_func = mock.MagicMock()
> + cc_ntp.install_ntp(install_func, packages=['ntp'], check_exe='ntpd')
> + install_func.assert_not_called()
>
> def test_ntp_rename_ntp_conf(self):
> - with mock.patch.object(os.path, 'exists',
> - return_value=True) as mockpath:
> - with mock.patch.object(util, 'rename') as mockrename:
> - cc_ntp.rename_ntp_conf()
> -
> - mockpath.assert_called_with('/etc/ntp.conf')
> - mockrename.assert_called_with('/etc/ntp.conf', '/etc/ntp.conf.dist')
> + """When NTP_CONF exists, rename_ntp moves it."""
> + ntpconf = self.tmp_path("ntp.conf", self.new_root)
> + os.mknod(ntpconf)
> + with mock.patch("cloudinit.config.cc_ntp.NTP_CONF", ntpconf):
> + cc_ntp.rename_ntp_conf()
> + self.assertFalse(os.path.exists(ntpconf))
> + self.assertTrue(os.path.exists("{}.dist".format(ntpconf)))
>
> def test_ntp_rename_ntp_conf_skip_missing(self):
> - with mock.patch.object(os.path, 'exists',
> - return_value=False) as mockpath:
> - with mock.patch.object(util, 'rename') as mockrename:
> - cc_ntp.rename_ntp_conf()
> -
> - mockpath.assert_called_with('/etc/ntp.conf')
> - mockrename.assert_not_called()
> -
> - def ntp_conf_render(self, distro):
> - """ntp_conf_render
> - Test rendering of a ntp.conf from template for a given distro
> + """When NTP_CONF doesn't exist rename_ntp doesn't create a file."""
> + ntpconf = self.tmp_path("ntp.conf", self.new_root)
> + self.assertFalse(os.path.exists(ntpconf))
> + with mock.patch("cloudinit.config.cc_ntp.NTP_CONF", ntpconf):
> + cc_ntp.rename_ntp_conf()
> + self.assertFalse(os.path.exists("{}.dist".format(ntpconf)))
> + self.assertFalse(os.path.exists(ntpconf))
> +
> + def test_write_ntp_config_template_from_ntp_conf_tmpl_with_servers(self):
> + """write_ntp_config_template reads content from ntp.conf.tmpl.
> +
> + It reads ntp.conf.tmpl if present and renders the value from servers
> + key. When no pools key is defined, template is rendered using an empty
> + list for pools.
> """
> -
> - cfg = {'ntp': {}}
> - mycloud = self._get_cloud(distro)
> - distro_names = cc_ntp.generate_server_names(distro)
> -
> - with mock.patch.object(templater, 'render_to_file') as mocktmpl:
> - with mock.patch.object(os.path, 'isfile', return_value=True):
> - with mock.patch.object(util, 'rename'):
> - cc_ntp.write_ntp_config_template(cfg, mycloud)
> -
> - mocktmpl.assert_called_once_with(
> - ('/etc/cloud/templates/ntp.conf.%s.tmpl' % distro),
> - '/etc/ntp.conf',
> - {'servers': [], 'pools': distro_names})
> -
> - def test_ntp_conf_render_rhel(self):
> - """Test templater.render_to_file() for rhel"""
> - self.ntp_conf_render('rhel')
> -
> - def test_ntp_conf_render_debian(self):
> - """Test templater.render_to_file() for debian"""
> - self.ntp_conf_render('debian')
> -
> - def test_ntp_conf_render_fedora(self):
> - """Test templater.render_to_file() for fedora"""
> - self.ntp_conf_render('fedora')
> -
> - def test_ntp_conf_render_sles(self):
> - """Test templater.render_to_file() for sles"""
> - self.ntp_conf_render('sles')
> -
> - def test_ntp_conf_render_ubuntu(self):
> - """Test templater.render_to_file() for ubuntu"""
> - self.ntp_conf_render('ubuntu')
> -
> - def test_ntp_conf_servers_no_pools(self):
> distro = 'ubuntu'
> - pools = []
> - servers = ['192.168.2.1']
> cfg = {
> - 'ntp': {
> - 'pools': pools,
> - 'servers': servers,
> - }
> + 'servers': ['192.168.2.1', '192.168.2.2']
> }
> mycloud = self._get_cloud(distro)
> -
> - with mock.patch.object(templater, 'render_to_file') as mocktmpl:
> - with mock.patch.object(os.path, 'isfile', return_value=True):
> - with mock.patch.object(util, 'rename'):
> - cc_ntp.write_ntp_config_template(cfg.get('ntp'), mycloud)
> -
> - mocktmpl.assert_called_once_with(
> - ('/etc/cloud/templates/ntp.conf.%s.tmpl' % distro),
> - '/etc/ntp.conf',
> - {'servers': servers, 'pools': pools})
> -
> - def test_ntp_conf_custom_pools_no_server(self):
> + ntp_conf = self.tmp_path("ntp.conf", self.new_root) # Doesn't exist
> + # Create ntp.conf.tmpl
> + with open('{}.tmpl'.format(ntp_conf), 'wb') as stream:
> + stream.write(NTP_TEMPLATE)
> + with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
> + cc_ntp.write_ntp_config_template(cfg, mycloud)
> + with open(ntp_conf) as stream:
> + content = stream.read()
> + self.assertEqual(
> + "servers ['192.168.2.1', '192.168.2.2']\npools []\n",
> + content)
> +
> + def test_write_ntp_config_template_uses_ntp_conf_distro_no_servers(self):
> + """write_ntp_config_template reads content from ntp.conf.distro.tmpl.
> +
> + It reads ntp.conf.<distro>.tmpl before attempting ntp.conf.tmpl. It
> + renders the value from the keys servers and pools. When no
> + servers value is present, template is rendered using an empty list.
> + """
> distro = 'ubuntu'
> - pools = ['0.mycompany.pool.ntp.org']
> - servers = []
> cfg = {
> - 'ntp': {
> - 'pools': pools,
> - 'servers': servers,
> - }
> + 'pools': ['10.0.0.1', '10.0.0.2']
> }
> mycloud = self._get_cloud(distro)
> -
> - with mock.patch.object(templater, 'render_to_file') as mocktmpl:
> - with mock.patch.object(os.path, 'isfile', return_value=True):
> - with mock.patch.object(util, 'rename'):
> - cc_ntp.write_ntp_config_template(cfg.get('ntp'), mycloud)
> -
> - mocktmpl.assert_called_once_with(
> - ('/etc/cloud/templates/ntp.conf.%s.tmpl' % distro),
> - '/etc/ntp.conf',
> - {'servers': servers, 'pools': pools})
> -
> - def test_ntp_conf_custom_pools_and_server(self):
> + ntp_conf = self.tmp_path('ntp.conf', self.new_root) # Doesn't exist
> + # Create ntp.conf.tmpl which isn't read
> + with open('{}.tmpl'.format(ntp_conf), 'wb') as stream:
> + stream.write(b'NOT READ: ntp.conf.<distro>.tmpl is primary')
> + # Create ntp.conf.tmpl.<distro>
> + with open('{}.{}.tmpl'.format(ntp_conf, distro), 'wb') as stream:
> + stream.write(NTP_TEMPLATE)
> + with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
> + cc_ntp.write_ntp_config_template(cfg, mycloud)
> + with open(ntp_conf) as stream:
> + content = stream.read()
> + self.assertEqual(
> + "servers []\npools ['10.0.0.1', '10.0.0.2']\n",
> + content)
> +
> + def test_write_ntp_config_template_defaults_pools_when_empty_lists(self):
> + """write_ntp_config_template defaults pools servers upon empty config.
> +
> + When both pools and servers are empty, default NR_POOL_SERVERS get
> + configured.
> + """
> distro = 'ubuntu'
> - pools = ['0.mycompany.pool.ntp.org']
> - servers = ['192.168.23.3']
> - cfg = {
> - 'ntp': {
> - 'pools': pools,
> - 'servers': servers,
> - }
> - }
> mycloud = self._get_cloud(distro)
> -
> - with mock.patch.object(templater, 'render_to_file') as mocktmpl:
> - with mock.patch.object(os.path, 'isfile', return_value=True):
> - with mock.patch.object(util, 'rename'):
> - cc_ntp.write_ntp_config_template(cfg.get('ntp'), mycloud)
> -
> - mocktmpl.assert_called_once_with(
> - ('/etc/cloud/templates/ntp.conf.%s.tmpl' % distro),
> - '/etc/ntp.conf',
> - {'servers': servers, 'pools': pools})
> -
> - def test_ntp_conf_contents_match(self):
> - """Test rendered contents of /etc/ntp.conf for ubuntu"""
> - pools = ['0.mycompany.pool.ntp.org']
> - servers = ['192.168.23.3']
> - cfg = {
> - 'ntp': {
> - 'pools': pools,
> - 'servers': servers,
> - }
> - }
> - mycloud = self._get_cloud('ubuntu')
> - side_effect = [NTP_TEMPLATE.lstrip()]
> -
> - # work backwards from util.write_file and mock out call path
> - # write_ntp_config_template()
> - # cloud.get_template_filename()
> - # os.path.isfile()
> - # templater.render_to_file()
> - # templater.render_from_file()
> - # util.load_file()
> - # util.write_file()
> - #
> - with mock.patch.object(util, 'write_file') as mockwrite:
> - with mock.patch.object(util, 'load_file', side_effect=side_effect):
> - with mock.patch.object(os.path, 'isfile', return_value=True):
> - with mock.patch.object(util, 'rename'):
> - cc_ntp.write_ntp_config_template(cfg.get('ntp'),
> - mycloud)
> -
> - mockwrite.assert_called_once_with(
> - '/etc/ntp.conf',
> - NTP_EXPECTED_UBUNTU,
> - mode=420)
> + ntp_conf = self.tmp_path('ntp.conf', self.new_root) # Doesn't exist
> + # Create ntp.conf.tmpl
> + with open('{}.tmpl'.format(ntp_conf), 'wb') as stream:
> + stream.write(NTP_TEMPLATE)
> + with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
> + cc_ntp.write_ntp_config_template({}, mycloud)
> + with open(ntp_conf) as stream:
> + content = stream.read()
used read_file_or_url for these. thanks for that.
> + default_pools = [
> + "{}.{}.pool.ntp.org".format(x, distro)
> + for x in range(0, cc_ntp.NR_POOL_SERVERS)]
> + self.assertEqual(
> + "servers []\npools {}\n".format(default_pools),
> + content)
> + self.assertIn(
> + "Adding distro default ntp pool servers: {}".format(
> + ",".join(default_pools)),
> + self.logs.getvalue())
>
> def test_ntp_handler(self):
> - """Test ntp handler renders ubuntu ntp.conf template"""
> - pools = ['0.mycompany.pool.ntp.org']
> - servers = ['192.168.23.3']
> + """Test ntp handler renders ubuntu ntp.conf template."""
> + pools = ['0.mycompany.pool.ntp.org', '3.mycompany.pool.ntp.org']
> + servers = ['192.168.23.3', '192.168.23.4']
> cfg = {
> 'ntp': {
> 'pools': pools,
> - 'servers': servers,
> + 'servers': servers
> }
> }
> mycloud = self._get_cloud('ubuntu')
> - mycloud.distro = mock.MagicMock()
> - mycloud.distro.uses_systemd.return_value = True
> - side_effect = [NTP_TEMPLATE.lstrip()]
> -
> - with mock.patch.object(util, 'which', return_value=None):
> - with mock.patch.object(os.path, 'exists'):
> - with mock.patch.object(util, 'write_file') as mockwrite:
> - with mock.patch.object(util, 'load_file',
> - side_effect=side_effect):
> - with mock.patch.object(os.path, 'isfile',
> - return_value=True):
> - with mock.patch.object(util, 'rename'):
> - with mock.patch.object(util, 'subp') as msubp:
> - cc_ntp.handle("notimportant", cfg,
> - mycloud, LOG, None)
> -
> - mockwrite.assert_called_once_with(
> - '/etc/ntp.conf',
> - NTP_EXPECTED_UBUNTU,
> - mode=420)
> - msubp.assert_any_call(['systemctl', 'reload-or-restart', 'ntp'],
> - capture=True)
> -
> - @mock.patch("cloudinit.config.cc_ntp.install_ntp")
> - @mock.patch("cloudinit.config.cc_ntp.write_ntp_config_template")
> - @mock.patch("cloudinit.config.cc_ntp.rename_ntp_conf")
> - def test_write_config_before_install(self, mock_ntp_rename,
> - mock_ntp_write_config,
> - mock_install_ntp):
> - pools = ['0.mycompany.pool.ntp.org']
> - servers = ['192.168.23.3']
> - cfg = {
> - 'ntp': {
> - 'pools': pools,
> - 'servers': servers,
> - }
> - }
> - cc = self._get_cloud('ubuntu')
> - cc.distro = mock.MagicMock()
> - mock_parent = mock.MagicMock()
> - mock_parent.attach_mock(mock_ntp_rename, 'mock_ntp_rename')
> - mock_parent.attach_mock(mock_ntp_write_config, 'mock_ntp_write_config')
> - mock_parent.attach_mock(mock_install_ntp, 'mock_install_ntp')
> -
> - cc_ntp.handle('cc_ntp', cfg, cc, LOG, None)
> -
> - """Check call order"""
> - mock_parent.assert_has_calls([
> - mock.call.mock_ntp_rename(),
> - mock.call.mock_ntp_write_config(cfg.get('ntp'), cc),
> - mock.call.mock_install_ntp(cc.distro.install_packages,
> - packages=['ntp'], check_exe="ntpd")])
> -
> - @mock.patch("cloudinit.config.cc_ntp.reload_ntp")
> - @mock.patch("cloudinit.config.cc_ntp.install_ntp")
> - @mock.patch("cloudinit.config.cc_ntp.write_ntp_config_template")
> - @mock.patch("cloudinit.config.cc_ntp.rename_ntp_conf")
> - def test_reload_ntp_fail_raises_exception(self, mock_rename,
> - mock_write_conf,
> - mock_install,
> - mock_reload):
> - pools = ['0.mycompany.pool.ntp.org']
> - servers = ['192.168.23.3']
> - cfg = {
> - 'ntp': {
> - 'pools': pools,
> - 'servers': servers,
> - }
> - }
> - cc = self._get_cloud('ubuntu')
> - cc.distro = mock.MagicMock()
> -
> - mock_reload.side_effect = [util.ProcessExecutionError]
> - self.assertRaises(util.ProcessExecutionError,
> - cc_ntp.handle, 'cc_ntp',
> - cfg, cc, LOG, None)
> -
> - @mock.patch("cloudinit.config.cc_ntp.util")
> - def test_no_ntpcfg_does_nothing(self, mock_util):
> - cc = self._get_cloud('ubuntu')
> - cc.distro = mock.MagicMock()
> - cc_ntp.handle('cc_ntp', {}, cc, LOG, [])
> - self.assertFalse(cc.distro.install_packages.called)
> - self.assertFalse(mock_util.subp.called)
> -
> - @mock.patch("cloudinit.config.cc_ntp.util")
> - def test_reload_ntp_systemd(self, mock_util):
> - cc_ntp.reload_ntp(systemd=True)
> - self.assertTrue(mock_util.subp.called)
> - mock_util.subp.assert_called_with(
> - ['systemctl', 'reload-or-restart', 'ntp'], capture=True)
> -
> - @mock.patch("cloudinit.config.cc_ntp.util")
> - def test_reload_ntp_service(self, mock_util):
> - cc_ntp.reload_ntp(systemd=False)
> - self.assertTrue(mock_util.subp.called)
> - mock_util.subp.assert_called_with(
> - ['service', 'ntp', 'restart'], capture=True)
> -
> + ntp_conf = self.tmp_path('ntp.conf', self.new_root) # Doesn't exist
> + # Create ntp.conf.tmpl
> + with open('{}.tmpl'.format(ntp_conf), 'wb') as stream:
> + stream.write(NTP_TEMPLATE)
> + with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
> + with mock.patch.object(util, 'which', return_value=None):
> + cc_ntp.handle('notimportant', cfg, mycloud, None, None)
> +
> + with open(ntp_conf) as stream:
> + content = stream.read()
> + self.assertEqual(
> + 'servers {}\npools {}\n'.format(servers, pools),
> + content)
> +
> + def test_no_ntpcfg_does_nothing(self):
> + """When no ntp section is defined handler logs a warning and noops."""
> + cc_ntp.handle('cc_ntp', {}, None, None, [])
> + self.assertEqual(
> + 'Skipping module named cc_ntp, not present or disabled by cfg\n',
> + self.logs.getvalue())
>
> # vi: ts=4 expandtab
--
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324450
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master.
References
