Merge lp:~james-page/charm-helpers/fixup-service-running into lp:charm-helpers
- fixup-service-running
- Merge into devel
Proposed by
James Page
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 574 | ||||
Proposed branch: | lp:~james-page/charm-helpers/fixup-service-running | ||||
Merge into: | lp:charm-helpers | ||||
Diff against target: |
364 lines (+78/-94) 2 files modified
charmhelpers/core/host.py (+16/-23) tests/core/test_host.py (+62/-71) |
||||
To merge this branch: | bzr merge lp:~james-page/charm-helpers/fixup-service-running | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ryan Beisner (community) | Approve | ||
Edward Hope-Morley | Approve | ||
Review via email: mp+294942@code.launchpad.net |
Commit message
Refactor service_running to determine init approach
Use of "service --status-all" has some challenges on trusty where
a recent lsb-base update resulted in all calls being diverted to
upstart, where return codes for non-running daemons are still 0.
Detect whether to use upstart or the base init.d script to
determine whether a service is running, and look at either the
upstart output, or for a non-zero return code from an init.d
status call.
Description of the change
To post a comment you must log in.
- 574. By James Page
-
Avoid conditional for sysv init check
Revision history for this message
James Page (james-page) wrote : | # |
Test sync: https:/
Revision history for this message
Edward Hope-Morley (hopem) wrote : | # |
lgtm +1
review:
Needs Resubmitting
Revision history for this message
Edward Hope-Morley (hopem) : | # |
review:
Approve
Revision history for this message
Ryan Beisner (1chb1n) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'charmhelpers/core/host.py' |
2 | --- charmhelpers/core/host.py 2016-05-13 16:04:36 +0000 |
3 | +++ charmhelpers/core/host.py 2016-05-17 17:50:07 +0000 |
4 | @@ -128,11 +128,8 @@ |
5 | return subprocess.call(cmd) == 0 |
6 | |
7 | |
8 | -def systemv_services_running(): |
9 | - output = subprocess.check_output( |
10 | - ['service', '--status-all'], |
11 | - stderr=subprocess.STDOUT).decode('UTF-8') |
12 | - return [row.split()[-1] for row in output.split('\n') if '[ + ]' in row] |
13 | +_UPSTART_CONF = "/etc/init/{}.conf" |
14 | +_INIT_D_CONF = "/etc/init.d/{}" |
15 | |
16 | |
17 | def service_running(service_name): |
18 | @@ -140,26 +137,22 @@ |
19 | if init_is_systemd(): |
20 | return service('is-active', service_name) |
21 | else: |
22 | - try: |
23 | - output = subprocess.check_output( |
24 | - ['service', service_name, 'status'], |
25 | - stderr=subprocess.STDOUT).decode('UTF-8') |
26 | - except subprocess.CalledProcessError: |
27 | - return False |
28 | - else: |
29 | - # This works for upstart scripts where the 'service' command |
30 | - # returns a consistent string to represent running 'start/running' |
31 | - if ("start/running" in output or "is running" in output or |
32 | - "up and running" in output): |
33 | - return True |
34 | - # Actively check for upstart stopped message(s) as the fall through |
35 | - # SystemV check can return false positives |
36 | - if ("stop/waiting" in output): |
37 | + if os.path.exists(_UPSTART_CONF.format(service_name)): |
38 | + try: |
39 | + output = subprocess.check_output( |
40 | + ['status', service_name], |
41 | + stderr=subprocess.STDOUT).decode('UTF-8') |
42 | + except subprocess.CalledProcessError: |
43 | return False |
44 | + else: |
45 | + # This works for upstart scripts where the 'service' command |
46 | + # returns a consistent string to represent running 'start/running' |
47 | + if "start/running" in output: |
48 | + return True |
49 | + elif os.path.exists(_INIT_D_CONF.format(service_name)): |
50 | # Check System V scripts init script return codes |
51 | - if service_name in systemv_services_running(): |
52 | - return True |
53 | - return False |
54 | + return service('status', service_name) |
55 | + return False |
56 | |
57 | |
58 | def service_available(service_name): |
59 | |
60 | === modified file 'tests/core/test_host.py' |
61 | --- tests/core/test_host.py 2016-05-13 16:29:19 +0000 |
62 | +++ tests/core/test_host.py 2016-05-17 17:50:07 +0000 |
63 | @@ -60,13 +60,6 @@ |
64 | link/ether 08:00:27:16:b9:5f brd ff:ff:ff:ff:ff:ff |
65 | """ |
66 | |
67 | -SERVICE_STATUS_ALL = b""" |
68 | - [ + ] rabbitmq-server |
69 | - [ ? ] rc.local |
70 | - [ + ] resolvconf |
71 | - [ - ] rsync |
72 | -""" |
73 | - |
74 | |
75 | class HelpersTest(TestCase): |
76 | |
77 | @@ -176,15 +169,16 @@ |
78 | call('enable', service_name), |
79 | call('start', service_name)]) |
80 | |
81 | + @patch.object(host, 'service_running') |
82 | @patch.object(host, 'init_is_systemd') |
83 | - @patch('subprocess.check_output') |
84 | @patch.object(host, 'service') |
85 | - def test_pauses_a_running_upstart_service(self, service, check_output, systemd): |
86 | + def test_pauses_a_running_upstart_service(self, service, systemd, |
87 | + service_running): |
88 | """Pause on a running service will call service stop.""" |
89 | - check_output.return_value = b'foo-service start/running, process 123' |
90 | service_name = 'foo-service' |
91 | service.side_effect = [True] |
92 | systemd.return_value = False |
93 | + service_running.return_value = True |
94 | tempdir = mkdtemp(prefix="test_pauses_an_upstart_service") |
95 | conf_path = os.path.join(tempdir, "{}.conf".format(service_name)) |
96 | # Just needs to exist |
97 | @@ -200,15 +194,16 @@ |
98 | override_contents = fh.read() |
99 | self.assertEqual("manual\n", override_contents) |
100 | |
101 | + @patch.object(host, 'service_running') |
102 | @patch.object(host, 'init_is_systemd') |
103 | - @patch('subprocess.check_output') |
104 | @patch.object(host, 'service') |
105 | - def test_pauses_a_stopped_upstart_service(self, service, check_output, systemd): |
106 | + def test_pauses_a_stopped_upstart_service(self, service, systemd, |
107 | + service_running): |
108 | """Pause on a stopped service will not call service stop.""" |
109 | - check_output.return_value = b'foo-service stop/waiting' |
110 | service_name = 'foo-service' |
111 | service.side_effect = [True] |
112 | systemd.return_value = False |
113 | + service_running.return_value = False |
114 | tempdir = mkdtemp(prefix="test_pauses_an_upstart_service") |
115 | conf_path = os.path.join(tempdir, "{}.conf".format(service_name)) |
116 | # Just needs to exist |
117 | @@ -226,17 +221,17 @@ |
118 | override_contents = fh.read() |
119 | self.assertEqual("manual\n", override_contents) |
120 | |
121 | + @patch.object(host, 'service_running') |
122 | @patch.object(host, 'init_is_systemd') |
123 | - @patch('subprocess.check_output') |
124 | @patch('subprocess.check_call') |
125 | @patch.object(host, 'service') |
126 | def test_pauses_a_running_sysv_service(self, service, check_call, |
127 | - check_output, systemd): |
128 | + systemd, service_running): |
129 | """Pause calls service stop on a running sysv service.""" |
130 | - check_output.return_value = b'foo-service start/running, process 123' |
131 | service_name = 'foo-service' |
132 | service.side_effect = [True] |
133 | systemd.return_value = False |
134 | + service_running.return_value = True |
135 | tempdir = mkdtemp(prefix="test_pauses_a_sysv_service") |
136 | sysv_path = os.path.join(tempdir, service_name) |
137 | # Just needs to exist |
138 | @@ -249,17 +244,17 @@ |
139 | service.assert_called_with('stop', service_name) |
140 | check_call.assert_called_with(["update-rc.d", service_name, "disable"]) |
141 | |
142 | + @patch.object(host, 'service_running') |
143 | @patch.object(host, 'init_is_systemd') |
144 | - @patch('subprocess.check_output') |
145 | @patch('subprocess.check_call') |
146 | @patch.object(host, 'service') |
147 | def test_pauses_a_stopped_sysv_service(self, service, check_call, |
148 | - check_output, systemd): |
149 | + systemd, service_running): |
150 | """Pause does not call service stop on a stopped sysv service.""" |
151 | - check_output.return_value = b'foo-service stop/waiting' |
152 | service_name = 'foo-service' |
153 | service.side_effect = [True] |
154 | systemd.return_value = False |
155 | + service_running.return_value = False |
156 | tempdir = mkdtemp(prefix="test_pauses_a_sysv_service") |
157 | sysv_path = os.path.join(tempdir, service_name) |
158 | # Just needs to exist |
159 | @@ -289,15 +284,17 @@ |
160 | "Unable to detect {0}".format(service_name), str(exception)) |
161 | self.assertIn(tempdir, str(exception)) |
162 | |
163 | + @patch.object(host, 'service_running') |
164 | @patch.object(host, 'init_is_systemd') |
165 | @patch('subprocess.check_output') |
166 | @patch.object(host, 'service') |
167 | - def test_resumes_a_running_upstart_service(self, service, check_output, systemd): |
168 | + def test_resumes_a_running_upstart_service(self, service, check_output, systemd, |
169 | + service_running): |
170 | """When the service is already running, service start isn't called.""" |
171 | - check_output.return_value = b'foo-service start/running, process 111' |
172 | service_name = 'foo-service' |
173 | service.side_effect = [True] |
174 | systemd.return_value = False |
175 | + service_running.return_value = True |
176 | tempdir = mkdtemp(prefix="test_resumes_an_upstart_service") |
177 | conf_path = os.path.join(tempdir, "{}.conf".format(service_name)) |
178 | with open(conf_path, "w") as fh: |
179 | @@ -306,21 +303,23 @@ |
180 | self.assertTrue(host.service_resume(service_name, init_dir=tempdir)) |
181 | |
182 | # Start isn't called because service is already running |
183 | - self.assertRaises( |
184 | - AssertionError, service.assert_called_with, 'start', service_name) |
185 | + self.assertFalse(service.called) |
186 | override_path = os.path.join( |
187 | tempdir, "{}.override".format(service_name)) |
188 | self.assertFalse(os.path.exists(override_path)) |
189 | |
190 | + @patch.object(host, 'service_running') |
191 | @patch.object(host, 'init_is_systemd') |
192 | @patch('subprocess.check_output') |
193 | @patch.object(host, 'service') |
194 | - def test_resumes_a_stopped_upstart_service(self, service, check_output, systemd): |
195 | + def test_resumes_a_stopped_upstart_service(self, service, check_output, systemd, |
196 | + service_running): |
197 | """When the service is stopped, service start is called.""" |
198 | check_output.return_value = b'foo-service stop/waiting' |
199 | service_name = 'foo-service' |
200 | service.side_effect = [True] |
201 | systemd.return_value = False |
202 | + service_running.return_value = False |
203 | tempdir = mkdtemp(prefix="test_resumes_an_upstart_service") |
204 | conf_path = os.path.join(tempdir, "{}.conf".format(service_name)) |
205 | with open(conf_path, "w") as fh: |
206 | @@ -333,16 +332,17 @@ |
207 | tempdir, "{}.override".format(service_name)) |
208 | self.assertFalse(os.path.exists(override_path)) |
209 | |
210 | + @patch.object(host, 'service_running') |
211 | @patch.object(host, 'init_is_systemd') |
212 | - @patch('subprocess.check_output') |
213 | @patch('subprocess.check_call') |
214 | @patch.object(host, 'service') |
215 | - def test_resumes_a_sysv_service(self, service, check_call, check_output, systemd): |
216 | + def test_resumes_a_sysv_service(self, service, check_call, systemd, |
217 | + service_running): |
218 | """When process is in a stop/waiting state, service start is called.""" |
219 | - check_output.return_value = b'foo-service stop/waiting' |
220 | service_name = 'foo-service' |
221 | service.side_effect = [True] |
222 | systemd.return_value = False |
223 | + service_running.return_value = False |
224 | tempdir = mkdtemp(prefix="test_resumes_a_sysv_service") |
225 | sysv_path = os.path.join(tempdir, service_name) |
226 | # Just needs to exist |
227 | @@ -355,17 +355,16 @@ |
228 | service.assert_called_with('start', service_name) |
229 | check_call.assert_called_with(["update-rc.d", service_name, "enable"]) |
230 | |
231 | + @patch.object(host, 'service_running') |
232 | @patch.object(host, 'init_is_systemd') |
233 | - @patch('subprocess.check_output') |
234 | @patch('subprocess.check_call') |
235 | @patch.object(host, 'service') |
236 | def test_resume_a_running_sysv_service(self, service, check_call, |
237 | - check_output, systemd): |
238 | + systemd, service_running): |
239 | """When process is already running, service start isn't called.""" |
240 | - check_output.return_value = b'foo-service start/running, process 123' |
241 | service_name = 'foo-service' |
242 | - service.side_effect = [True] |
243 | systemd.return_value = False |
244 | + service_running.return_value = True |
245 | tempdir = mkdtemp(prefix="test_resumes_a_sysv_service") |
246 | sysv_path = os.path.join(tempdir, service_name) |
247 | # Just needs to exist |
248 | @@ -376,16 +375,18 @@ |
249 | service_name, init_dir=tempdir, initd_dir=tempdir)) |
250 | |
251 | # Start isn't called because service is already running |
252 | - self.assertRaises( |
253 | - AssertionError, service.assert_called_with, 'start', service_name) |
254 | + self.assertFalse(service.called) |
255 | check_call.assert_called_with(["update-rc.d", service_name, "enable"]) |
256 | |
257 | + @patch.object(host, 'service_running') |
258 | @patch.object(host, 'init_is_systemd') |
259 | @patch.object(host, 'service') |
260 | - def test_resume_with_unknown_service(self, service, systemd): |
261 | + def test_resume_with_unknown_service(self, service, systemd, |
262 | + service_running): |
263 | service_name = 'foo-service' |
264 | service.side_effect = [True] |
265 | systemd.return_value = False |
266 | + service_running.return_value = False |
267 | tempdir = mkdtemp(prefix="test_resumes_with_unknown_service") |
268 | self.addCleanup(rmtree, tempdir) |
269 | exception = self.assertRaises( |
270 | @@ -467,67 +468,57 @@ |
271 | call('restart', service_name) |
272 | ]) |
273 | |
274 | - @patch.object(host, 'systemv_services_running') |
275 | + @patch.object(host, 'os') |
276 | @patch.object(host, 'init_is_systemd') |
277 | @patch('subprocess.check_output') |
278 | def test_service_running_on_stopped_service(self, check_output, systemd, |
279 | - systemv_services_running): |
280 | + os): |
281 | systemd.return_value = False |
282 | + os.path.exists.return_value = True |
283 | check_output.return_value = b'foo stop/waiting' |
284 | self.assertFalse(host.service_running('foo')) |
285 | - self.assertFalse(systemv_services_running.called) |
286 | |
287 | + @patch.object(host, 'os') |
288 | @patch.object(host, 'init_is_systemd') |
289 | @patch('subprocess.check_output') |
290 | - def test_service_running_on_running_service(self, check_output, systemd): |
291 | + def test_service_running_on_running_service(self, check_output, systemd, |
292 | + os): |
293 | systemd.return_value = False |
294 | + os.path.exists.return_value = True |
295 | check_output.return_value = b'foo start/running, process 23871' |
296 | self.assertTrue(host.service_running('foo')) |
297 | |
298 | - @patch.object(host, 'init_is_systemd') |
299 | - @patch('subprocess.check_output') |
300 | - def test_service_running_on_running_service_pxc(self, check_output, systemd): |
301 | - systemd.return_value = False |
302 | - check_output.return_value = b' * Percona XtraDB Cluster up and running' |
303 | - self.assertTrue(host.service_running('foo')) |
304 | - |
305 | - @patch.object(host, 'init_is_systemd') |
306 | - @patch('subprocess.check_output') |
307 | - def test_service_running_on_unknown_service(self, check_output, systemd): |
308 | - systemd.return_value = False |
309 | + @patch.object(host, 'os') |
310 | + @patch.object(host, 'init_is_systemd') |
311 | + @patch('subprocess.check_output') |
312 | + def test_service_running_on_unknown_service(self, check_output, systemd, |
313 | + os): |
314 | + systemd.return_value = False |
315 | + os.path.exists.return_value = True |
316 | exc = subprocess.CalledProcessError(1, ['status']) |
317 | check_output.side_effect = exc |
318 | self.assertFalse(host.service_running('foo')) |
319 | |
320 | - @patch.object(host, 'systemv_services_running') |
321 | + @patch.object(host, 'os') |
322 | + @patch.object(host, 'service') |
323 | @patch.object(host, 'init_is_systemd') |
324 | - @patch('subprocess.check_output') |
325 | - def test_service_systemv_running(self, check_output, systemd, |
326 | - systemv_services_running): |
327 | + def test_service_systemv_running(self, systemd, service, os): |
328 | systemd.return_value = False |
329 | - check_output.return_value = b' * Unhelpfull guff, thanks a lot rabbit' |
330 | - systemv_services_running.return_value = [u'udev', u'rabbitmq-server'] |
331 | + service.return_value = True |
332 | + os.path.exists.side_effect = [False, True] |
333 | self.assertTrue(host.service_running('rabbitmq-server')) |
334 | - self.assertTrue(systemv_services_running.called) |
335 | + service.assert_called_with('status', 'rabbitmq-server') |
336 | |
337 | - @patch.object(host, 'systemv_services_running') |
338 | + @patch.object(host, 'os') |
339 | + @patch.object(host, 'service') |
340 | @patch.object(host, 'init_is_systemd') |
341 | - @patch('subprocess.check_output') |
342 | - def test_service_systemv_not_running(self, check_output, systemd, |
343 | - systemv_services_running): |
344 | + def test_service_systemv_not_running(self, systemd, service, |
345 | + os): |
346 | systemd.return_value = False |
347 | - check_output.return_value = b' * Unhelpfull guff, thanks a lot rabbit' |
348 | - systemv_services_running.return_value = [u'udev', u'rabbitmq-server'] |
349 | + service.return_value = False |
350 | + os.path.exists.side_effect = [False, True] |
351 | self.assertFalse(host.service_running('keystone')) |
352 | - self.assertTrue(systemv_services_running.called) |
353 | - |
354 | - @patch('subprocess.check_output') |
355 | - def test_systemv_services_running(self, check_output): |
356 | - check_output.return_value = SERVICE_STATUS_ALL |
357 | - self.assertEqual( |
358 | - host.systemv_services_running(), |
359 | - [u'rabbitmq-server', u'resolvconf'] |
360 | - ) |
361 | + service.assert_called_with('status', 'keystone') |
362 | |
363 | @patch('grp.getgrnam') |
364 | @patch('pwd.getpwnam') |
I'm syncing this into a few charms @master and stable/16.04 to exercise. Will report back here.