Merge ~chad.smith/cloud-init:cc-ntp-testing into cloud-init:master

[Chad Smith]

cc_ntp: Restructure cc_ntp unit tests add with_logs class attr to unit tests.

Any CiTestCase subclass can now set a class attribute with_logs = True and tests can now make assertions on self.logs.getvalue(). This branch restructures a bit of cc_ntp module to get better test coverage of the module. It also restructures the handler_cc_ntp unit tests to avoid nested mocks where possible. Deeply nested mocks cause a couple of issues:
  - greater risk: mocks are permanent within the scope, so multiple call-sites could be affected by package mocks
  - less legible tests: each mock doesn't advertise the actual call-site
  - tight coupling: the unit test logic to tightly bound to the actual implementation in remote (unrelated) modules which makes it more costly to maintain code
  - false success: we should be testing the expected behavior not specific remote method names as we want to know if that underlying behavior changes and breaks us.

LP: #1692794

cc_ntp: Restructure cc_ntp unit tests add with_logs class attr to unit tests.

Any CiTestCase subclass can now set a class attribute with_logs = True and tests can now make assertions on self.logs.getvalue(). This branch restructures a bit of cc_ntp module to get better test coverage of the module. It also restructures the handler_cc_ntp unit tests to avoid nested mocks where possible. Deeply nested mocks cause a couple of issues:
  - greater risk: mocks are permanent within the scope, so multiple call-sites could be affected by package mocks
  - less legible tests: each mock doesn't advertise the actual call-site
  - tight coupling: the unit test logic to tightly bound to the actual implementation in remote (unrelated) modules which makes it more costly to maintain code
  - false success: we should be testing the expected behavior not specific remote method names as we want to know if that underlying behavior changes and breaks us.

LP: #1692794

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:5ededba3f222795dea69af9d7d7e187c3fdf8bba
https://jenkins.ubuntu.com/server/job/cloud-init-ci/377/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/377/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/377/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/377/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/377/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/377/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/377/rebuild

review: Needs Fixing (continuous-integration)

e4f1154... by Chad Smith on 2017-05-23

lints

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:e4f11545162dc97c8dc0abc3d03981e9e1b895d1
https://jenkins.ubuntu.com/server/job/cloud-init-ci/378/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/378
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/378
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/378
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/378
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/378

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/378/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

some comments in line.

I'm generally in agreement here.

49c4eb8... by Chad Smith on 2017-05-23

use util.read_file_or_url instead of open()read(). replace ntp_conf in write_.._template

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:49c4eb85daa77cb20a027e3199a92e3dbc773f53
https://jenkins.ubuntu.com/server/job/cloud-init-ci/381/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/381
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/381
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/381
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/381
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/381

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/381/rebuild

review: Approve (continuous-integration)

15a8bc4... by Chad Smith on 2017-05-23

add unit test for the real ntp templates for each distro. drop unused metadata param

[Chad Smith (chad.smith)]

@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?

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:15a8bc4dbb75ff9968c344cda907e0fd6ed3a0fd
https://jenkins.ubuntu.com/server/job/cloud-init-ci/386/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/386/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/386/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/386/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/386/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/386/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/386/rebuild

review: Needs Fixing (continuous-integration)

29d0bb1... by Chad Smith on 2017-05-24

calculate root_dir path for templates directory from util import as sys.path will be different on test environments

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:29d0bb1cffd00ec3df0c2effd4a649af07390911
https://jenkins.ubuntu.com/server/job/cloud-init-ci/388/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/388
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/388
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/388
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/388
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/388

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/388/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I'm happy with this if you are.
You mentioned some improvement on the root_dir path calculation. You can do that here or in a follow on.
Just say which you want.

review: Approve

[Chad Smith (chad.smith)]

+1 let's land this as it's getting bigger. I've added a card/bug to work on a better root_dir path calculation in a followup.

diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index 225f898..5cc5453 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:
+        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)
     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())
@@ -98,8 +90,10 @@ def install_ntp(install_func, packages=None, check_exe="ntpd"):
     install_func(packages)
 
 
-def rename_ntp_conf(config=NTP_CONF):
+def rename_ntp_conf(config=None):
     """Rename any existing ntp.conf file and render from template"""
+    if config is None:  # For testing
+        config = NTP_CONF
     if os.path.exists(config):
         util.rename(config, config + ".dist")
 
@@ -117,8 +111,9 @@ def write_ntp_config_template(cfg, cloud):
     pools = cfg.get('pools', [])
 
     if len(servers) == 0 and len(pools) == 0:
-        LOG.debug('Adding distro default ntp pool servers')
         pools = generate_server_names(cloud.distro.name)
+        LOG.debug(
+            'Adding distro default ntp pool servers: %s', ','.join(pools))
 
     params = {
         'servers': servers,
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
@@ -4,6 +4,7 @@ from __future__ import print_function
 
 import functools
 import json
+import logging
 import os
 import shutil
 import sys
@@ -18,6 +19,10 @@ try:
     from contextlib import ExitStack
 except ImportError:
     from contextlib2 import ExitStack
+try:
+    from cStringIO import StringIO
+except ImportError:
+    from io import StringIO
 
 from cloudinit import helpers as ch
 from cloudinit import util
@@ -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]
+
+    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..21f2ab1 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -2,351 +2,214 @@
 
 from cloudinit.config import cc_ntp
 from cloudinit.sources import DataSourceNone
-from cloudinit import templater
 from cloudinit import (distros, helpers, cloud, util)
 from ..helpers import FilesystemMockingTestCase, mock
 
-import logging
+
 import os
+from os.path import dirname
 import shutil
-import tempfile
-
-LOG = logging.getLogger(__name__)
 
-NTP_TEMPLATE = """
+NTP_TEMPLATE = b"""\
 ## template: jinja
-
-{% if pools %}# pools
-{% endif %}
-{% for pool in pools -%}
-pool {{pool}} iburst
-{% endfor %}
-{%- if servers %}# servers
-{% endif %}
-{% for server in servers -%}
-server {{server}} iburst
-{% endfor %}
-
-"""
-
-
-NTP_EXPECTED_UBUNTU = """
-# pools
-pool 0.mycompany.pool.ntp.org iburst
-# servers
-server 192.168.23.3 iburst
-
+servers {{servers}}
+pools {{pools}}
 """
 
 
 class TestNtp(FilesystemMockingTestCase):
 
+    with_logs = True
+
     def setUp(self):
         super(TestNtp, self).setUp()
         self.subp = util.subp
-        self.new_root = tempfile.mkdtemp()
-        self.addCleanup(shutil.rmtree, self.new_root)
+        self.new_root = self.tmp_dir()
 
-    def _get_cloud(self, distro, metadata=None):
+    def _get_cloud(self, distro):
         self.patchUtils(self.new_root)
-        paths = helpers.Paths({})
+        paths = helpers.Paths({'templates_dir': self.new_root})
         cls = distros.fetch(distro)
         mydist = cls(distro, {}, paths)
         myds = DataSourceNone.DataSourceNone({}, mydist, paths)
-        if metadata:
-            myds.metadata.update(metadata)
         return cloud.Cloud(myds, paths, {}, mydist, None)
 
     @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)
+        content = util.read_file_or_url('file://' + ntp_conf).contents
+        self.assertEqual(
+            "servers ['192.168.2.1', '192.168.2.2']\npools []\n",
+            content.decode())
+
+    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)
+        content = util.read_file_or_url('file://' + ntp_conf).contents
+        self.assertEqual(
+            "servers []\npools ['10.0.0.1', '10.0.0.2']\n",
+            content.decode())
+
+    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']
+        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)
+        content = util.read_file_or_url('file://' + ntp_conf).contents
+        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.decode())
+        self.assertIn(
+            "Adding distro default ntp pool servers: {}".format(
+                ",".join(default_pools)),
+            self.logs.getvalue())
+
+    def test_ntp_handler_mocked_template(self):
+        """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')
-        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)
-
-    def test_ntp_handler(self):
-        """Test ntp handler renders ubuntu ntp.conf template"""
-        pools = ['0.mycompany.pool.ntp.org']
-        servers = ['192.168.23.3']
+        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)
+
+        content = util.read_file_or_url('file://' + ntp_conf).contents
+        self.assertEqual(
+            'servers {}\npools {}\n'.format(servers, pools),
+            content.decode())
+
+    def test_ntp_handler_real_distro_templates(self):
+        """Test ntp handler renders the shipped distro ntp.conf templates."""
+        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
+        for distro in ('debian', 'ubuntu', 'fedora', 'rhel', 'sles'):
+            mycloud = self._get_cloud(distro)
+            root_dir = dirname(dirname(os.path.realpath(util.__file__)))
+            tmpl_file = os.path.join(
+                '{}/templates/ntp.conf.{}.tmpl'.format(root_dir, distro))
+            # Create a copy in our tmp_dir
+            shutil.copy(
+                tmpl_file,
+                os.path.join(self.new_root, 'ntp.conf.%s.tmpl' % distro))
+            with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+                with mock.patch.object(util, 'which', return_value=[True]):
+                    cc_ntp.handle('notimportant', cfg, mycloud, None, None)
+
+            content = util.read_file_or_url('file://' + ntp_conf).contents
+            expected_servers = '\n'.join([
+                'server {} iburst'.format(server) for server in servers])
+            self.assertIn(
+                expected_servers, content.decode(),
+                'failed to render ntp.conf for distro:{}'.format(distro))
+            expected_pools = '\n'.join([
+                'pool {} iburst'.format(pool) for pool in pools])
+            self.assertIn(
+                expected_pools, content.decode(),
+                'failed to render ntp.conf for distro:{}'.format(distro))
+
+    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