cc_ntp: write template before installing and add service restart

On systems which installed ntp and specified servers or pools in the
config ntpd didn't notice the updated configuration file and didn't
use the correct configuration. Resolve this by rendering the template
first which allows the package install to use the existing
configuration. Additionally add a service restart to handle the case
where ntp does not need to be installed but it may not have started.

Add an integration test to confirm that cc_ntp enables ntp to use the
specific servers and pools in the cloud-config.

LP: #1645644

6 files changed, 120 insertions, 25 deletions

diff --git a/tests/cloud_tests/configs/modules/ntp_pools.yaml b/tests/cloud_tests/configs/modules/ntp_pools.yaml
index bd0ac292..e040cc32 100644
--- a/tests/cloud_tests/configs/modules/ntp_pools.yaml
+++ b/tests/cloud_tests/configs/modules/ntp_pools.yaml
@@ -5,10 +5,9 @@ cloud_config: |
   #cloud-config
   ntp:
     pools:
-        - 0.pool.ntp.org
-        - 1.pool.ntp.org
-        - 2.pool.ntp.org
-        - 3.pool.ntp.org
+        - 0.cloud-init.mypool
+        - 1.cloud-init.mypool
+        - 172.16.15.14
 collect_scripts:
   ntp_installed_pools: |
     #!/bin/bash
@@ -19,5 +18,8 @@ collect_scripts:
   ntp_conf_pools: |
     #!/bin/bash
     grep '^pool' /etc/ntp.conf
+  ntpq_servers: |
+    #!/bin/sh
+    ntpq -p -w
 
 # vi: ts=4 expandtab
diff --git a/tests/cloud_tests/configs/modules/ntp_servers.yaml b/tests/cloud_tests/configs/modules/ntp_servers.yaml
index 934b9c5d..e0564a03 100644
--- a/tests/cloud_tests/configs/modules/ntp_servers.yaml
+++ b/tests/cloud_tests/configs/modules/ntp_servers.yaml
@@ -5,16 +5,20 @@ cloud_config: |
   #cloud-config
   ntp:
     servers:
-        - pool.ntp.org
+        - 172.16.15.14
+        - 172.16.17.18
 collect_scripts:
   ntp_installed_servers: |
-    #!/bin/bash
-    dpkg -l | grep ntp | wc -l
+    #!/bin/sh
+    dpkg -l | grep -c ntp
   ntp_conf_dist_servers: |
-    #!/bin/bash
-    ls /etc/ntp.conf.dist | wc -l
+    #!/bin/sh
+    cat /etc/ntp.conf.dist | wc -l
   ntp_conf_servers: |
-    #!/bin/bash
+    #!/bin/sh
     grep '^server' /etc/ntp.conf
+  ntpq_servers: |
+    #!/bin/sh
+    ntpq -p -w
 
 # vi: ts=4 expandtab
diff --git a/tests/cloud_tests/testcases/modules/ntp.py b/tests/cloud_tests/testcases/modules/ntp.py
index b1119257..82d32880 100644
--- a/tests/cloud_tests/testcases/modules/ntp.py
+++ b/tests/cloud_tests/testcases/modules/ntp.py
@@ -13,9 +13,9 @@ class TestNtp(base.CloudTestCase):
         self.assertEqual(1, int(out))
 
     def test_ntp_dist_entries(self):
-        """Test dist config file has one entry"""
+        """Test dist config file is empty"""
         out = self.get_data_file('ntp_conf_dist_empty')
-        self.assertEqual(1, int(out))
+        self.assertEqual(0, int(out))
 
     def test_ntp_entires(self):
         """Test config entries"""
diff --git a/tests/cloud_tests/testcases/modules/ntp_pools.py b/tests/cloud_tests/testcases/modules/ntp_pools.py
index d80cb673..ff6d8fa4 100644
--- a/tests/cloud_tests/testcases/modules/ntp_pools.py
+++ b/tests/cloud_tests/testcases/modules/ntp_pools.py
@@ -13,16 +13,22 @@ class TestNtpPools(base.CloudTestCase):
         self.assertEqual(1, int(out))
 
     def test_ntp_dist_entries(self):
-        """Test dist config file has one entry"""
+        """Test dist config file is empty"""
         out = self.get_data_file('ntp_conf_dist_pools')
-        self.assertEqual(1, int(out))
+        self.assertEqual(0, int(out))
 
     def test_ntp_entires(self):
         """Test config entries"""
         out = self.get_data_file('ntp_conf_pools')
-        self.assertIn('pool 0.pool.ntp.org iburst', out)
-        self.assertIn('pool 1.pool.ntp.org iburst', out)
-        self.assertIn('pool 2.pool.ntp.org iburst', out)
-        self.assertIn('pool 3.pool.ntp.org iburst', out)
+        pools = self.cloud_config.get('ntp').get('pools')
+        for pool in pools:
+            self.assertIn('pool %s iburst' % pool, out)
+
+    def test_ntpq_servers(self):
+        """Test ntpq output has configured servers"""
+        out = self.get_data_file('ntpq_servers')
+        pools = self.cloud_config.get('ntp').get('pools')
+        for pool in pools:
+            self.assertIn(pool, out)
 
 # vi: ts=4 expandtab
diff --git a/tests/cloud_tests/testcases/modules/ntp_servers.py b/tests/cloud_tests/testcases/modules/ntp_servers.py
index 4879bb6f..9ef270ee 100644
--- a/tests/cloud_tests/testcases/modules/ntp_servers.py
+++ b/tests/cloud_tests/testcases/modules/ntp_servers.py
@@ -13,13 +13,22 @@ class TestNtpServers(base.CloudTestCase):
         self.assertEqual(1, int(out))
 
     def test_ntp_dist_entries(self):
-        """Test dist config file has one entry"""
+        """Test dist config file is empty"""
         out = self.get_data_file('ntp_conf_dist_servers')
-        self.assertEqual(1, int(out))
+        self.assertEqual(0, int(out))
 
     def test_ntp_entires(self):
-        """Test config entries"""
-        out = self.get_data_file('ntp_conf_servers')
-        self.assertIn('server pool.ntp.org iburst', out)
+        """Test config pools entries"""
+        out = self.get_data_file('ntp_conf_pools')
+        servers = self.cloud_config.get('ntp').get('servers')
+        for server in servers:
+            self.assertIn('server %s iburst' % server, out)
+
+    def test_ntpq_servers(self):
+        """Test ntpq output has configured servers"""
+        out = self.get_data_file('ntpq_servers')
+        servers = self.cloud_config.get('ntp').get('servers')
+        for server in servers:
+            self.assertIn(server, out)
 
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
index ec600077..02bd3046 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -249,6 +249,8 @@ class TestNtp(FilesystemMockingTestCase):
             }
         }
         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):
@@ -259,13 +261,70 @@ class TestNtp(FilesystemMockingTestCase):
                         with mock.patch.object(os.path, 'isfile',
                                                return_value=True):
                             with mock.patch.object(util, 'rename'):
-                                cc_ntp.handle("notimportant", cfg,
-                                              mycloud, LOG, None)
+                                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):
@@ -275,4 +334,19 @@ class TestNtp(FilesystemMockingTestCase):
         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)
+
+
 # vi: ts=4 expandtab