Merge ~raharper/cloud-init:bug-lp-1645644-ntp into cloud-init:master
- Git
- lp:~raharper/cloud-init
- bug-lp-1645644-ntp
- Merge into master
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | e11d3899d47ec5fcb545e0c7820af9d3995cb574 | ||||
Proposed branch: | ~raharper/cloud-init:bug-lp-1645644-ntp | ||||
Merge into: | cloud-init:master | ||||
Diff against target: |
352 lines (+183/-28) 8 files modified
cloudinit/config/cc_ntp.py (+22/-2) tests/cloud_tests/configs/modules/ntp_pools.yaml (+6/-4) tests/cloud_tests/configs/modules/ntp_servers.yaml (+10/-6) tests/cloud_tests/testcases/modules/ntp.py (+2/-2) tests/cloud_tests/testcases/modules/ntp_pools.py (+19/-6) tests/cloud_tests/testcases/modules/ntp_servers.py (+14/-5) tests/unittests/test_handler/test_handler_ntp.py (+76/-2) tox.ini (+34/-1) |
||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Server Team CI bot | continuous-integration | Needs Fixing | |
Scott Moser | Needs Fixing | ||
Review via email: mp+314539@code.launchpad.net |
Commit message
Description of the change
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
- f5fdb6c... by Ryan Harper
-
test_handler_ntp: mock out systemd check
Signed-off-by: Ryan Harper <email address hidden>
- 95f8e85... by Ryan Harper
-
test_handler_ntp: mock out systemd check
Signed-off-by: Ryan Harper <email address hidden>
- 7ca55db... by Ryan Harper
-
test_handler_ntp: mock out systemd check
Signed-off-by: Ryan Harper <email address hidden>
Scott Moser (smoser) : | # |
- d801528... by Ryan Harper
-
Address merge feedback
Signed-off-by: Ryan Harper <email address hidden>
- 62e56aa... by Ryan Harper
-
Address merge feedback
Signed-off-by: Ryan Harper <email address hidden>
- b97c262... by Ryan Harper
-
cc_ntp: Add unittest to test for exception if restart fails
Add a unittest to ensure we raise an exception if we fail
when restarting ntp service.Signed-off-by: Ryan Harper <email address hidden>
- 2571946... by Ryan Harper
-
cc_ntp: Add unittest to test for exception if restart fails
Add a unittest to ensure we raise an exception if we fail
when restarting ntp service.Signed-off-by: Ryan Harper <email address hidden>
- 97b0ad4... by Ryan Harper
-
Merge branch 'bug-lp-
1645644- ntp' of git.launchpad. net:~raharper/ cloud-init into bug-lp-1645644-ntp
Server Team CI bot (server-team-bot) wrote : | # |
Scott Moser (smoser) wrote : | # |
i rebased to master, dropped the tox changes and then submitted a MP back here.
https:/
i think its sane, would appreciate your input.
the tox changes are welcome in a separate commit.
Ryan Harper (raharper) wrote : | # |
pretty sure the tox changes weren't needed for the fix, but it was useful when exercising this fix under ci. I'll re-run that and propose separately
- 06008d2... by Ryan Harper
-
cloud_test: add integration test
Signed-off-by: Ryan Harper <email address hidden>
- 61fa4b0... by Ryan Harper
-
cloud_tests: ntp_pools: remove duplicate test I accidentally left in
Signed-off-by: Ryan Harper <email address hidden>
- 039fad4... by Ryan Harper
-
Merge branch 'bug-lp-
1645644- ntp' of git.launchpad. net:~raharper/ cloud-init into bug-lp-1645644-ntp
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:039fad44ce2
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
Preview Diff
1 | diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py |
2 | index e33032f..a982b6e 100644 |
3 | --- a/cloudinit/config/cc_ntp.py |
4 | +++ b/cloudinit/config/cc_ntp.py |
5 | @@ -74,10 +74,19 @@ def handle(name, cfg, cloud, log, _args): |
6 | "not present or disabled by cfg", name) |
7 | return True |
8 | |
9 | - install_ntp(cloud.distro.install_packages, packages=['ntp'], |
10 | - check_exe="ntpd") |
11 | rename_ntp_conf() |
12 | + # ensure when ntp is installed it has a configuration file |
13 | + # to use instead of starting up with packaged defaults |
14 | write_ntp_config_template(ntp_cfg, cloud) |
15 | + install_ntp(cloud.distro.install_packages, packages=['ntp'], |
16 | + check_exe="ntpd") |
17 | + |
18 | + # if ntp was already installed, it may not have started |
19 | + try: |
20 | + reload_ntp(systemd=cloud.distro.uses_systemd()) |
21 | + except util.ProcessExecutionError as e: |
22 | + LOG.exception("Failed to reload/start ntp service: %s", e) |
23 | + raise |
24 | |
25 | |
26 | def install_ntp(install_func, packages=None, check_exe="ntpd"): |
27 | @@ -90,6 +99,7 @@ def install_ntp(install_func, packages=None, check_exe="ntpd"): |
28 | |
29 | |
30 | def rename_ntp_conf(config=NTP_CONF): |
31 | + """ Rename any existing ntp.conf file and render from template""" |
32 | if os.path.exists(config): |
33 | util.rename(config, config + ".dist") |
34 | |
35 | @@ -125,4 +135,14 @@ def write_ntp_config_template(cfg, cloud): |
36 | |
37 | templater.render_to_file(template_fn, NTP_CONF, params) |
38 | |
39 | + |
40 | +def reload_ntp(systemd=False): |
41 | + service = 'ntp' |
42 | + if systemd: |
43 | + cmd = ['systemctl', 'reload-or-restart', service] |
44 | + else: |
45 | + cmd = ['service', service, 'restart'] |
46 | + util.subp(cmd, capture=True) |
47 | + |
48 | + |
49 | # vi: ts=4 expandtab |
50 | diff --git a/tests/cloud_tests/configs/modules/ntp_pools.yaml b/tests/cloud_tests/configs/modules/ntp_pools.yaml |
51 | index bd0ac29..e040cc3 100644 |
52 | --- a/tests/cloud_tests/configs/modules/ntp_pools.yaml |
53 | +++ b/tests/cloud_tests/configs/modules/ntp_pools.yaml |
54 | @@ -5,10 +5,9 @@ cloud_config: | |
55 | #cloud-config |
56 | ntp: |
57 | pools: |
58 | - - 0.pool.ntp.org |
59 | - - 1.pool.ntp.org |
60 | - - 2.pool.ntp.org |
61 | - - 3.pool.ntp.org |
62 | + - 0.cloud-init.mypool |
63 | + - 1.cloud-init.mypool |
64 | + - 172.16.15.14 |
65 | collect_scripts: |
66 | ntp_installed_pools: | |
67 | #!/bin/bash |
68 | @@ -19,5 +18,8 @@ collect_scripts: |
69 | ntp_conf_pools: | |
70 | #!/bin/bash |
71 | grep '^pool' /etc/ntp.conf |
72 | + ntpq_servers: | |
73 | + #!/bin/sh |
74 | + ntpq -p -w |
75 | |
76 | # vi: ts=4 expandtab |
77 | diff --git a/tests/cloud_tests/configs/modules/ntp_servers.yaml b/tests/cloud_tests/configs/modules/ntp_servers.yaml |
78 | index 934b9c5..e0564a0 100644 |
79 | --- a/tests/cloud_tests/configs/modules/ntp_servers.yaml |
80 | +++ b/tests/cloud_tests/configs/modules/ntp_servers.yaml |
81 | @@ -5,16 +5,20 @@ cloud_config: | |
82 | #cloud-config |
83 | ntp: |
84 | servers: |
85 | - - pool.ntp.org |
86 | + - 172.16.15.14 |
87 | + - 172.16.17.18 |
88 | collect_scripts: |
89 | ntp_installed_servers: | |
90 | - #!/bin/bash |
91 | - dpkg -l | grep ntp | wc -l |
92 | + #!/bin/sh |
93 | + dpkg -l | grep -c ntp |
94 | ntp_conf_dist_servers: | |
95 | - #!/bin/bash |
96 | - ls /etc/ntp.conf.dist | wc -l |
97 | + #!/bin/sh |
98 | + cat /etc/ntp.conf.dist | wc -l |
99 | ntp_conf_servers: | |
100 | - #!/bin/bash |
101 | + #!/bin/sh |
102 | grep '^server' /etc/ntp.conf |
103 | + ntpq_servers: | |
104 | + #!/bin/sh |
105 | + ntpq -p -w |
106 | |
107 | # vi: ts=4 expandtab |
108 | diff --git a/tests/cloud_tests/testcases/modules/ntp.py b/tests/cloud_tests/testcases/modules/ntp.py |
109 | index b111925..82d3288 100644 |
110 | --- a/tests/cloud_tests/testcases/modules/ntp.py |
111 | +++ b/tests/cloud_tests/testcases/modules/ntp.py |
112 | @@ -13,9 +13,9 @@ class TestNtp(base.CloudTestCase): |
113 | self.assertEqual(1, int(out)) |
114 | |
115 | def test_ntp_dist_entries(self): |
116 | - """Test dist config file has one entry""" |
117 | + """Test dist config file is empty""" |
118 | out = self.get_data_file('ntp_conf_dist_empty') |
119 | - self.assertEqual(1, int(out)) |
120 | + self.assertEqual(0, int(out)) |
121 | |
122 | def test_ntp_entires(self): |
123 | """Test config entries""" |
124 | diff --git a/tests/cloud_tests/testcases/modules/ntp_pools.py b/tests/cloud_tests/testcases/modules/ntp_pools.py |
125 | index d80cb67..7f4be76 100644 |
126 | --- a/tests/cloud_tests/testcases/modules/ntp_pools.py |
127 | +++ b/tests/cloud_tests/testcases/modules/ntp_pools.py |
128 | @@ -13,16 +13,29 @@ class TestNtpPools(base.CloudTestCase): |
129 | self.assertEqual(1, int(out)) |
130 | |
131 | def test_ntp_dist_entries(self): |
132 | - """Test dist config file has one entry""" |
133 | + """Test dist config file is empty""" |
134 | out = self.get_data_file('ntp_conf_dist_pools') |
135 | - self.assertEqual(1, int(out)) |
136 | + self.assertEqual(0, int(out)) |
137 | |
138 | def test_ntp_entires(self): |
139 | """Test config entries""" |
140 | out = self.get_data_file('ntp_conf_pools') |
141 | - self.assertIn('pool 0.pool.ntp.org iburst', out) |
142 | - self.assertIn('pool 1.pool.ntp.org iburst', out) |
143 | - self.assertIn('pool 2.pool.ntp.org iburst', out) |
144 | - self.assertIn('pool 3.pool.ntp.org iburst', out) |
145 | + pools = self.cloud_config.get('ntp').get('pools') |
146 | + for pool in pools: |
147 | + self.assertIn('pool %s iburst' % pool, out) |
148 | + |
149 | + def test_ntpq_servers(self): |
150 | + """Test ntpq output has configured servers""" |
151 | + out = self.get_data_file('ntpq_servers') |
152 | + pools = self.cloud_config.get('ntp').get('pools') |
153 | + for pool in pools: |
154 | + self.assertIn('pool %s iburst' % pool, out) |
155 | + |
156 | + def test_ntpq_servers(self): |
157 | + """Test ntpq output has configured servers""" |
158 | + out = self.get_data_file('ntpq_servers') |
159 | + pools = self.cloud_config.get('ntp').get('pools') |
160 | + for pool in pools: |
161 | + self.assertIn(pool, out) |
162 | |
163 | # vi: ts=4 expandtab |
164 | diff --git a/tests/cloud_tests/testcases/modules/ntp_servers.py b/tests/cloud_tests/testcases/modules/ntp_servers.py |
165 | index 4879bb6..9ef270e 100644 |
166 | --- a/tests/cloud_tests/testcases/modules/ntp_servers.py |
167 | +++ b/tests/cloud_tests/testcases/modules/ntp_servers.py |
168 | @@ -13,13 +13,22 @@ class TestNtpServers(base.CloudTestCase): |
169 | self.assertEqual(1, int(out)) |
170 | |
171 | def test_ntp_dist_entries(self): |
172 | - """Test dist config file has one entry""" |
173 | + """Test dist config file is empty""" |
174 | out = self.get_data_file('ntp_conf_dist_servers') |
175 | - self.assertEqual(1, int(out)) |
176 | + self.assertEqual(0, int(out)) |
177 | |
178 | def test_ntp_entires(self): |
179 | - """Test config entries""" |
180 | - out = self.get_data_file('ntp_conf_servers') |
181 | - self.assertIn('server pool.ntp.org iburst', out) |
182 | + """Test config pools entries""" |
183 | + out = self.get_data_file('ntp_conf_pools') |
184 | + servers = self.cloud_config.get('ntp').get('servers') |
185 | + for server in servers: |
186 | + self.assertIn('server %s iburst' % server, out) |
187 | + |
188 | + def test_ntpq_servers(self): |
189 | + """Test ntpq output has configured servers""" |
190 | + out = self.get_data_file('ntpq_servers') |
191 | + servers = self.cloud_config.get('ntp').get('servers') |
192 | + for server in servers: |
193 | + self.assertIn(server, out) |
194 | |
195 | # vi: ts=4 expandtab |
196 | diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py |
197 | index ec60007..02bd304 100644 |
198 | --- a/tests/unittests/test_handler/test_handler_ntp.py |
199 | +++ b/tests/unittests/test_handler/test_handler_ntp.py |
200 | @@ -249,6 +249,8 @@ class TestNtp(FilesystemMockingTestCase): |
201 | } |
202 | } |
203 | mycloud = self._get_cloud('ubuntu') |
204 | + mycloud.distro = mock.MagicMock() |
205 | + mycloud.distro.uses_systemd.return_value = True |
206 | side_effect = [NTP_TEMPLATE.lstrip()] |
207 | |
208 | with mock.patch.object(util, 'which', return_value=None): |
209 | @@ -259,13 +261,70 @@ class TestNtp(FilesystemMockingTestCase): |
210 | with mock.patch.object(os.path, 'isfile', |
211 | return_value=True): |
212 | with mock.patch.object(util, 'rename'): |
213 | - cc_ntp.handle("notimportant", cfg, |
214 | - mycloud, LOG, None) |
215 | + with mock.patch.object(util, 'subp') as msubp: |
216 | + cc_ntp.handle("notimportant", cfg, |
217 | + mycloud, LOG, None) |
218 | |
219 | mockwrite.assert_called_once_with( |
220 | '/etc/ntp.conf', |
221 | NTP_EXPECTED_UBUNTU, |
222 | mode=420) |
223 | + msubp.assert_any_call(['systemctl', 'reload-or-restart', 'ntp'], |
224 | + capture=True) |
225 | + |
226 | + @mock.patch("cloudinit.config.cc_ntp.install_ntp") |
227 | + @mock.patch("cloudinit.config.cc_ntp.write_ntp_config_template") |
228 | + @mock.patch("cloudinit.config.cc_ntp.rename_ntp_conf") |
229 | + def test_write_config_before_install(self, mock_ntp_rename, |
230 | + mock_ntp_write_config, |
231 | + mock_install_ntp): |
232 | + pools = ['0.mycompany.pool.ntp.org'] |
233 | + servers = ['192.168.23.3'] |
234 | + cfg = { |
235 | + 'ntp': { |
236 | + 'pools': pools, |
237 | + 'servers': servers, |
238 | + } |
239 | + } |
240 | + cc = self._get_cloud('ubuntu') |
241 | + cc.distro = mock.MagicMock() |
242 | + mock_parent = mock.MagicMock() |
243 | + mock_parent.attach_mock(mock_ntp_rename, 'mock_ntp_rename') |
244 | + mock_parent.attach_mock(mock_ntp_write_config, 'mock_ntp_write_config') |
245 | + mock_parent.attach_mock(mock_install_ntp, 'mock_install_ntp') |
246 | + |
247 | + cc_ntp.handle('cc_ntp', cfg, cc, LOG, None) |
248 | + |
249 | + """Check call order""" |
250 | + mock_parent.assert_has_calls([ |
251 | + mock.call.mock_ntp_rename(), |
252 | + mock.call.mock_ntp_write_config(cfg.get('ntp'), cc), |
253 | + mock.call.mock_install_ntp(cc.distro.install_packages, |
254 | + packages=['ntp'], check_exe="ntpd")]) |
255 | + |
256 | + @mock.patch("cloudinit.config.cc_ntp.reload_ntp") |
257 | + @mock.patch("cloudinit.config.cc_ntp.install_ntp") |
258 | + @mock.patch("cloudinit.config.cc_ntp.write_ntp_config_template") |
259 | + @mock.patch("cloudinit.config.cc_ntp.rename_ntp_conf") |
260 | + def test_reload_ntp_fail_raises_exception(self, mock_rename, |
261 | + mock_write_conf, |
262 | + mock_install, |
263 | + mock_reload): |
264 | + pools = ['0.mycompany.pool.ntp.org'] |
265 | + servers = ['192.168.23.3'] |
266 | + cfg = { |
267 | + 'ntp': { |
268 | + 'pools': pools, |
269 | + 'servers': servers, |
270 | + } |
271 | + } |
272 | + cc = self._get_cloud('ubuntu') |
273 | + cc.distro = mock.MagicMock() |
274 | + |
275 | + mock_reload.side_effect = [util.ProcessExecutionError] |
276 | + self.assertRaises(util.ProcessExecutionError, |
277 | + cc_ntp.handle, 'cc_ntp', |
278 | + cfg, cc, LOG, None) |
279 | |
280 | @mock.patch("cloudinit.config.cc_ntp.util") |
281 | def test_no_ntpcfg_does_nothing(self, mock_util): |
282 | @@ -275,4 +334,19 @@ class TestNtp(FilesystemMockingTestCase): |
283 | self.assertFalse(cc.distro.install_packages.called) |
284 | self.assertFalse(mock_util.subp.called) |
285 | |
286 | + @mock.patch("cloudinit.config.cc_ntp.util") |
287 | + def test_reload_ntp_systemd(self, mock_util): |
288 | + cc_ntp.reload_ntp(systemd=True) |
289 | + self.assertTrue(mock_util.subp.called) |
290 | + mock_util.subp.assert_called_with( |
291 | + ['systemctl', 'reload-or-restart', 'ntp'], capture=True) |
292 | + |
293 | + @mock.patch("cloudinit.config.cc_ntp.util") |
294 | + def test_reload_ntp_service(self, mock_util): |
295 | + cc_ntp.reload_ntp(systemd=False) |
296 | + self.assertTrue(mock_util.subp.called) |
297 | + mock_util.subp.assert_called_with( |
298 | + ['service', 'ntp', 'restart'], capture=True) |
299 | + |
300 | + |
301 | # vi: ts=4 expandtab |
302 | diff --git a/tox.ini b/tox.ini |
303 | index bf9046a..3667ce8 100644 |
304 | --- a/tox.ini |
305 | +++ b/tox.ini |
306 | @@ -1,5 +1,5 @@ |
307 | [tox] |
308 | -envlist = py27, py3, flake8, xenial, pylint |
309 | +envlist = py27, py3, flake8, xenial, pylint, citest |
310 | recreate = True |
311 | |
312 | [testenv] |
313 | @@ -68,6 +68,39 @@ deps = |
314 | flake8==2.5.4 |
315 | hacking==0.10.2 |
316 | |
317 | +[testenv:citest] |
318 | +basepython = python3 |
319 | +commands = {envpython} -m tests.cloud_tests {posargs} |
320 | +deps = |
321 | + # requirements |
322 | + jinja2==2.8 |
323 | + pyyaml==3.11 |
324 | + PrettyTable==0.7.2 |
325 | + oauthlib==1.0.3 |
326 | + pyserial==3.0.1 |
327 | + configobj==5.0.6 |
328 | + requests==2.9.1 |
329 | + # jsonpatch ubuntu is 1.10, not 1.19 (#839779) |
330 | + jsonpatch==1.10 |
331 | + six==1.10.0 |
332 | + # test-requirements |
333 | + httpretty==0.8.6 |
334 | + mock==1.3.0 |
335 | + nose==1.3.7 |
336 | + unittest2==1.1.0 |
337 | + contextlib2==0.5.1 |
338 | + pep8==1.7.0 |
339 | + pyflakes==1.1.0 |
340 | + flake8==2.5.4 |
341 | + hacking==0.10.2 |
342 | + pylxd==2.1.3 |
343 | + babel==1.3 |
344 | + python-dateutil==2.4.2 |
345 | + pbr==1.8.0 |
346 | + pyOpenSSL==0.15.1 |
347 | + requests-unixsocket==0.1.5 |
348 | + ws4py==0.3.4 |
349 | + |
350 | [testenv:centos6] |
351 | basepython = python2.6 |
352 | commands = nosetests {posargs:tests} |
FAILED: Continuous integration, rev:97b0ad442bb 251a519a5ed8344 32d1a75c20bb9f /jenkins. ubuntu. com/server/ job/cloud- init-ci/ 73/ /jenkins. ubuntu. com/server/ job/cloud- init-ci/ nodes=metal- arm64/73/ console /jenkins. ubuntu. com/server/ job/cloud- init-ci/ nodes=metal- ppc64el/ 73/console /jenkins. ubuntu. com/server/ job/cloud- init-ci/ nodes=metal- s390x/73/ console /jenkins. ubuntu. com/server/ job/cloud- init-ci/ nodes=vm- amd64/73/ console /jenkins. ubuntu. com/server/ job/cloud- init-ci/ nodes=vm- i386/73/ console
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild: /jenkins. ubuntu. com/server/ job/cloud- init-ci/ 73/rebuild
https:/