Merge lp:~hloeung/ubuntu-repository-cache/only-reload-apache2-when-needed into lp:ubuntu-repository-cache

Proposed by Haw Loeung on 2021-02-19
Status: Merged
Approved by: Haw Loeung on 2021-02-23
Approved revision: 338
Merged at revision: 316
Proposed branch: lp:~hloeung/ubuntu-repository-cache/only-reload-apache2-when-needed
Merge into: lp:ubuntu-repository-cache
Diff against target: 544 lines (+278/-87)
4 files modified
.bzrignore (+1/-0)
.coveragerc (+2/-0)
lib/ubuntu_repository_cache/apache.py (+111/-45)
tests/unit/test_apache.py (+164/-42)
To merge this branch: bzr merge lp:~hloeung/ubuntu-repository-cache/only-reload-apache2-when-needed
Reviewer Review Type Date Requested Status
Joel Sing +1 2021-02-19 Approve on 2021-02-23
Canonical IS Reviewers 2021-02-19 Pending
Review via email: mp+398325@code.launchpad.net

Commit message

Only reload/graceful apache2 when required - LP:1916084

To post a comment you must log in.

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

333. By Haw Loeung on 2021-02-22

Ignore .juju-persistent-config used with config.save()

Joel Sing (jsing) wrote :

Generally looks good, but see comments inline.

334. By Haw Loeung on 2021-02-22

Don't call apache.start() in service.unpause()

335. By Haw Loeung on 2021-02-22

Merge upstream changes

336. By Haw Loeung on 2021-02-22

Reworked configure_remoteip() and enable module on install

337. By Haw Loeung on 2021-02-22

Fixed based on review

Haw Loeung (hloeung) :
338. By Haw Loeung on 2021-02-22

Fixed to use one set of config keys to retrieve values and check config.changed() as per review

Haw Loeung (hloeung) wrote :

> Generally looks good, but see comments inline.

Okay, all addressed. Thanks for the review.

Haw Loeung (hloeung) :
Joel Sing (jsing) wrote :

LGTM

review: Approve (+1)

Change successfully merged at revision 316

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2020-05-07 05:29:31 +0000
3+++ .bzrignore 2021-02-22 22:51:46 +0000
4@@ -1,6 +1,7 @@
5 *.pyc
6 *.swp
7 *~
8+.juju-persistent-config
9 .unit-state.db
10 lib/test.py
11 .venv
12
13=== added file '.coveragerc'
14--- .coveragerc 1970-01-01 00:00:00 +0000
15+++ .coveragerc 2021-02-22 22:51:46 +0000
16@@ -0,0 +1,2 @@
17+[run]
18+omit = lib/ubuntu_repository_cache/tests/*
19
20=== modified file 'lib/ubuntu_repository_cache/apache.py'
21--- lib/ubuntu_repository_cache/apache.py 2021-02-22 03:11:12 +0000
22+++ lib/ubuntu_repository_cache/apache.py 2021-02-22 22:51:46 +0000
23@@ -41,6 +41,8 @@
24 config = hookenv.config()
25 if config['enable_healthcheck']:
26 subprocess.check_call(['a2enmod', 'cgid'])
27+ if config['remoteip_logging']:
28+ subprocess.check_call(['a2enmod', 'remoteip'])
29
30 start()
31
32@@ -123,20 +125,37 @@
33 return config
34
35
36-def create_mpm_workerfile():
37+def create_mpm_workerfile(config=None):
38 '''Create the multi-processing module (MPM) configuration'''
39
40- config = hookenv.config()
41- mpm_context = {
42- 'startservers': config['apache2_mpm_startservers'],
43- 'minsparethreads': config['apache2_mpm_minsparethreads'],
44- 'maxsparethreads': config['apache2_mpm_maxsparethreads'],
45- 'threadlimit': config['apache2_mpm_threadlimit'],
46- 'threadsperchild': config['apache2_mpm_threadsperchild'],
47- 'serverlimit': config['apache2_mpm_serverlimit'],
48- 'maxrequestworkers': config['apache2_mpm_maxrequestworkers'],
49- 'maxconnectionsperchild': config['apache2_mpm_maxconnectionsperchild'],
50- }
51+ reload_required = False
52+
53+ if not config:
54+ config = hookenv.config()
55+
56+ mpm_configs = [
57+ 'maxconnectionsperchild',
58+ 'maxrequestworkers',
59+ 'maxsparethreads',
60+ 'minsparethreads',
61+ 'serverlimit',
62+ 'startservers',
63+ 'threadlimit',
64+ 'threadsperchild',
65+ ]
66+ mpm_context = {}
67+ for k in mpm_configs:
68+ cfkey = 'apache2_mpm_{}'.format(k)
69+ mpm_context[k] = config[cfkey]
70+ if config.changed(cfkey):
71+ reload_required = True
72+
73+ # We'll only proceed when any one of the MPM configs have changed.
74+ if not reload_required:
75+ return reload_required
76+
77+ reload_required = True
78+
79 mpm_context = tune_mpm_configs(mpm_context)
80 conf_mpm_worker = '/etc/apache2/conf-available/000mpm-worker.conf'
81 templating.render('apache2/mpm_worker.template', conf_mpm_worker, mpm_context)
82@@ -148,24 +167,44 @@
83 os.unlink(mods_mpm_worker)
84 os.symlink(conf_mpm_worker, mods_mpm_worker)
85
86-
87-def create_security():
88+ return reload_required
89+
90+
91+def create_security(config=None):
92 '''Create the security configuration'''
93
94- config = hookenv.config()
95- security_context = {
96- 'server_tokens': config['apache2_server_tokens'],
97- 'server_signature': config['apache2_server_signature'],
98- 'trace_enabled': config['apache2_trace_enabled'],
99- }
100+ reload_required = False
101+
102+ if not config:
103+ config = hookenv.config()
104+
105+ security_configs = ['server_signature', 'server_tokens', 'trace_enabled']
106+ security_context = {}
107+ for k in security_configs:
108+ cfkey = 'apache2_{}'.format(k)
109+ security_context[k] = config[cfkey]
110+ if config.changed(cfkey):
111+ reload_required = True
112+
113+ # We'll only proceed when any one of the MPM configs have changed.
114+ if not reload_required:
115+ return reload_required
116+
117+ reload_required = True
118+
119 templating.render('apache2/security.template', '/etc/apache2/conf-available/security.conf', security_context)
120 subprocess.check_call(['a2enconf', 'security'])
121-
122-
123-def create_metadata_site():
124+ return reload_required
125+
126+
127+def create_metadata_site(config=None, apache2_conf_path='/etc/apache2'):
128 '''Create the site config for serving (or proxying) repository metadata'''
129
130- config = hookenv.config()
131+ reload_required = False
132+
133+ if not config:
134+ config = hookenv.config()
135+
136 apache_context = {}
137 apache_context['DocumentRoot'] = unitdata.kv().get('apache-root')
138 apache_context['Sync_Host'] = config['sync-host']
139@@ -188,38 +227,62 @@
140 apache_context['MetadataProxy'] = metaproxy_conf
141 apache_context['Enable_Healthcheck'] = config['enable_healthcheck']
142
143- templating.render(
144- 'apache2/archive_ubuntu_com.conf', '/etc/apache2/sites-available/archive_ubuntu_com.conf', apache_context
145- )
146- subprocess.call(['a2ensite', 'archive_ubuntu_com'])
147- start()
148+ apache_config_file = os.path.join(apache2_conf_path, 'sites-available', 'archive_ubuntu_com.conf')
149+ cur_conf = ''
150+ if os.path.exists(apache_config_file):
151+ with open(apache_config_file) as f:
152+ cur_conf = f.read()
153+ new_conf = templating.render('apache2/archive_ubuntu_com.conf', apache_config_file, apache_context)
154+
155+ if new_conf != cur_conf:
156+ reload_required = True
157+ if not os.path.exists(os.path.join(apache2_conf_path, 'sites-enabled', 'archive_ubuntu_com.conf')):
158+ subprocess.call(['a2ensite', 'archive_ubuntu_com'])
159+
160+ return reload_required
161
162
163 @util.run_as_user('root')
164-def configure_remoteip_logging():
165- config = hookenv.config()
166+def configure_remoteip_logging(config=None, apache2_conf_path='/etc/apache2'):
167+ reload_required = False
168+
169+ if not config:
170+ config = hookenv.config()
171+
172+ if config['remoteip_logging']:
173+ apache_config_file = os.path.join(apache2_conf_path, 'conf-available', 'remoteip.conf')
174+ cur_conf = ''
175+ if os.path.exists(apache_config_file):
176+ with open(apache_config_file) as f:
177+ cur_conf = f.read()
178+ new_conf = templating.render('apache2/remoteip.conf', apache_config_file, {})
179+
180+ if new_conf != cur_conf:
181+ reload_required = True
182+ if not os.path.exists(os.path.join(apache2_conf_path, 'conf-enabled', 'remoteip.conf')):
183+ subprocess.check_call(['a2enconf', 'remoteip'])
184+
185 if config.changed('remoteip_logging'):
186 LOG('Apache remoteip_logging changed')
187- if config['remoteip_logging'] is True:
188+ if config['remoteip_logging']:
189 subprocess.check_call(['a2enmod', 'remoteip'])
190- templating.render('apache2/remoteip.conf', '/etc/apache2/conf-available/remoteip.conf', {})
191- subprocess.check_call(['a2enconf', 'remoteip'])
192 LOG('Enabled remoteip logging')
193 else:
194 subprocess.check_call(['a2dismod', 'remoteip'])
195 if os.path.exists('/etc/apache2/conf-enabled/remoteip.conf'):
196 subprocess.check_call(['a2disconf', 'remoteip'])
197 LOG('Disabled remoteip logging')
198+ reload_required = True
199
200- start()
201+ return reload_required
202
203
204 def configure_health_check(config=None):
205+ reload_required = False
206+
207 if not config:
208 config = hookenv.config()
209
210- restart_required = False
211-
212 if config.changed('enable_healthcheck'):
213 LOG('Apache enable_healthcheck changed')
214 if config['enable_healthcheck']:
215@@ -231,7 +294,7 @@
216
217 subprocess.check_call([cmd, 'cgid'])
218 LOG('{} health check endpoint'.format(action))
219- restart_required = True
220+ reload_required = True
221
222 subprocess.call(['touch', HEALTH_CHECK_LOG])
223 subprocess.call(['chown', HEALTH_CHECK_LOG_OWNER, HEALTH_CHECK_LOG])
224@@ -244,16 +307,19 @@
225 content = f.read()
226 host.write_file(path=healthcheck_script, content=content, perms=0o755)
227
228- return restart_required
229+ return reload_required
230
231
232 @util.run_as_user('root')
233 def render_configs():
234 '''Render the configuration templates for apache'''
235
236+ reload_required = False
237+
238 LOG('Rendering apache2 configuration templates')
239 config = hookenv.config()
240 if config.changed('apache2_mpm_type'):
241+ reload_required = True
242 if config['apache2_mpm_type'] == 'worker':
243 LOG('Using apache2-mpm-worker')
244 subprocess.check_call(['a2dismod', 'mpm_prefork'])
245@@ -264,13 +330,13 @@
246 subprocess.check_call(['a2dismod', 'mpm_worker'])
247 subprocess.check_call(['a2enmod', 'mpm_prefork'])
248
249- create_metadata_site()
250- create_mpm_workerfile()
251- create_security()
252- configure_remoteip_logging()
253- configure_health_check()
254+ reload_required = create_metadata_site() or reload_required
255+ reload_required = create_mpm_workerfile() or reload_required
256+ reload_required = create_security() or reload_required
257+ reload_required = configure_remoteip_logging() or reload_required
258+ reload_required = configure_health_check() or reload_required
259
260- start()
261+ start(start_only=(not reload_required))
262
263
264 def update_checks(nrpe_config):
265
266=== modified file 'tests/unit/test_apache.py'
267--- tests/unit/test_apache.py 2021-02-22 02:59:25 +0000
268+++ tests/unit/test_apache.py 2021-02-22 22:51:46 +0000
269@@ -43,11 +43,6 @@
270 self.addCleanup(patcher.stop)
271 self.mock_local_unit.return_value = 'mock-ubuntu-repository-cache/0'
272
273- patcher = mock.patch('charmhelpers.core.hookenv.config')
274- self.mock_config = patcher.start()
275- self.addCleanup(patcher.stop)
276- self.mock_config.return_value = {'nagios_context': 'juju'}
277-
278 patcher = mock.patch('charmhelpers.core.host.log')
279 self.mock_log = patcher.start()
280 self.addCleanup(patcher.stop)
281@@ -65,46 +60,53 @@
282 @mock.patch('subprocess.check_call')
283 def test_configure_health_check(self, check_call, call, seteuid, setegid, chown, write_file):
284 config = hookenv.Config({'enable_healthcheck': True})
285- self.assertEqual(apache.configure_health_check(config), True)
286+ self.assertTrue(apache.configure_health_check(config))
287 check_call.assert_called_with(['a2enmod', 'cgid'])
288
289 check_call.reset_mock()
290 config = hookenv.Config({'enable_healthcheck': False})
291- self.assertEqual(apache.configure_health_check(config), True)
292+ self.assertTrue(apache.configure_health_check(config))
293 check_call.assert_called_with(['a2dismod', 'cgid'])
294
295+ # No change
296 check_call.reset_mock()
297 config = hookenv.Config({'enable_healthcheck': True})
298 config.save()
299 config = hookenv.Config({'enable_healthcheck': True})
300- self.assertEqual(apache.configure_health_check(config), False)
301+ self.assertFalse(apache.configure_health_check(config))
302
303- @mock.patch('charmhelpers.core.templating.render')
304 @mock.patch('lib.ubuntu_repository_cache.apache.start')
305 @mock.patch('lib.ubuntu_repository_cache.util.get_failover_host')
306+ @mock.patch('os.chown')
307+ @mock.patch('os.fchown')
308 @mock.patch('subprocess.call')
309- def test_create_metadata_site(self, call, get_failover_host, start, render):
310+ def test_create_metadata_site(self, call, fchown, chown, get_failover_host, start):
311 get_failover_host.return_value = '10.1.1.1'
312- self.mock_config.return_value = {
313- 'display-host': 'archive.ubuntu.com',
314- 'enable_healthcheck': True,
315- 'path-base': 'ubuntu',
316- 'sync-host': 'us.archive.ubuntu.com',
317- }
318- apache.create_metadata_site()
319- render.assert_called_with(
320- 'apache2/archive_ubuntu_com.conf',
321- '/etc/apache2/sites-available/archive_ubuntu_com.conf',
322+ config = hookenv.Config(
323 {
324- 'DocumentRoot': os.path.join(self.tmpdir, 'apache'),
325- 'Sync_Host': 'us.archive.ubuntu.com',
326- 'Display_Host': 'archive.ubuntu.com',
327- 'Path_Base': 'ubuntu',
328- 'MetadataProxy': '# No failover configured',
329- 'Enable_Healthcheck': True,
330- },
331+ 'display-host': 'archive.ubuntu.com',
332+ 'enable_healthcheck': True,
333+ 'path-base': 'ubuntu',
334+ 'sync-host': 'us.archive.ubuntu.com',
335+ }
336 )
337+ self.assertTrue(apache.create_metadata_site(config, apache2_conf_path=self.tmpdir))
338 call.assert_called_with(['a2ensite', 'archive_ubuntu_com'])
339+ chown.assert_called()
340+
341+ # No change, no need to reload
342+ call.reset_mock()
343+ config.save()
344+ config = hookenv.Config(
345+ {
346+ 'display-host': 'archive.ubuntu.com',
347+ 'enable_healthcheck': True,
348+ 'path-base': 'ubuntu',
349+ 'sync-host': 'us.archive.ubuntu.com',
350+ }
351+ )
352+ self.assertFalse(apache.create_metadata_site(config, apache2_conf_path=self.tmpdir))
353+ call.assert_not_called()
354
355 @mock.patch('charmhelpers.core.templating.render')
356 @mock.patch('lib.ubuntu_repository_cache.apache.start')
357@@ -113,12 +115,14 @@
358 @mock.patch('os.mknod')
359 @mock.patch('subprocess.call')
360 def test_create_metadata_site_in_failover(self, call, mknod, in_failover, get_failover_host, start, render):
361- self.mock_config.return_value = {
362- 'display-host': 'archive.ubuntu.com',
363- 'enable_healthcheck': True,
364- 'path-base': 'ubuntu',
365- 'sync-host': 'us.archive.ubuntu.com',
366- }
367+ config = hookenv.Config(
368+ {
369+ 'display-host': 'archive.ubuntu.com',
370+ 'enable_healthcheck': True,
371+ 'path-base': 'ubuntu',
372+ 'sync-host': 'us.archive.ubuntu.com',
373+ }
374+ )
375 get_failover_host.return_value = '10.1.1.1'
376 in_failover.return_value = True
377
378@@ -127,7 +131,7 @@
379 os.mkdir(os.path.join(self.tmpdir, 'apache'))
380 os.mkdir(health_check_path)
381
382- apache.create_metadata_site()
383+ apache.create_metadata_site(config)
384 mknod.assert_called_with(disabled_flag)
385 render.assert_called_with(
386 'apache2/archive_ubuntu_com.conf',
387@@ -148,7 +152,7 @@
388 # If the disabled flag exists, no need to create it.
389 mknod.reset_mock()
390 open(disabled_flag, 'a').close()
391- apache.create_metadata_site()
392+ apache.create_metadata_site(config)
393 mknod.assert_not_called()
394
395 @mock.patch('charmhelpers.core.templating.render')
396@@ -157,19 +161,21 @@
397 @mock.patch('os.unlink')
398 @mock.patch('subprocess.call')
399 def test_create_metadata_site_no_failover(self, call, unlink, get_failover_host, start, render):
400- self.mock_config.return_value = {
401- 'display-host': 'archive.ubuntu.com',
402- 'enable_healthcheck': True,
403- 'path-base': 'ubuntu',
404- 'sync-host': 'us.archive.ubuntu.com',
405- }
406+ config = hookenv.Config(
407+ {
408+ 'display-host': 'archive.ubuntu.com',
409+ 'enable_healthcheck': True,
410+ 'path-base': 'ubuntu',
411+ 'sync-host': 'us.archive.ubuntu.com',
412+ }
413+ )
414
415 health_check_path = os.path.join(self.tmpdir, 'apache', 'health-check')
416 disabled_flag = os.path.join(health_check_path, 'health-check-disabled.flag')
417 os.mkdir(os.path.join(self.tmpdir, 'apache'))
418 os.mkdir(health_check_path)
419 os.mknod(disabled_flag)
420- apache.create_metadata_site()
421+ apache.create_metadata_site(config)
422 unlink.assert_called_with(disabled_flag)
423
424 @mock.patch('charmhelpers.core.host.service_start')
425@@ -204,3 +210,119 @@
426 apache.start(start_only=True)
427 service_reload.assert_not_called()
428 service_start.assert_called_with('apache2')
429+
430+ @mock.patch('charmhelpers.core.templating.render')
431+ @mock.patch('os.symlink')
432+ @mock.patch('subprocess.check_call')
433+ def test_create_mpm_workerfile(self, check_call, symlink, render):
434+ config = hookenv.Config(
435+ {
436+ 'apache2_mpm_maxconnectionsperchild': 10000,
437+ 'apache2_mpm_maxrequestworkers': 0,
438+ 'apache2_mpm_maxsparethreads': 0,
439+ 'apache2_mpm_minsparethreads': 0,
440+ 'apache2_mpm_serverlimit': 0,
441+ 'apache2_mpm_startservers': 0,
442+ 'apache2_mpm_threadlimit': 0,
443+ 'apache2_mpm_threadsperchild': 0,
444+ }
445+ )
446+ self.assertTrue(apache.create_mpm_workerfile(config))
447+ render.assert_called_with(
448+ 'apache2/mpm_worker.template',
449+ '/etc/apache2/conf-available/000mpm-worker.conf',
450+ {
451+ 'maxconnectionsperchild': 10000,
452+ 'maxrequestworkers': 512,
453+ 'maxsparethreads': 512,
454+ 'minsparethreads': 256,
455+ 'serverlimit': 1,
456+ 'startservers': 1,
457+ 'threadlimit': 512,
458+ 'threadsperchild': 512,
459+ },
460+ )
461+ check_call.assert_called_with(['a2enconf', '000mpm-worker'])
462+ symlink.assert_called_with(
463+ '/etc/apache2/conf-available/000mpm-worker.conf', '/etc/apache2/mods-available/mpm_worker.conf'
464+ )
465+
466+ # No change, no need to reload
467+ render.reset_mock()
468+ check_call.reset_mock()
469+ symlink.reset_mock()
470+ config.save()
471+ config = hookenv.Config(
472+ {
473+ 'apache2_mpm_maxconnectionsperchild': 10000,
474+ 'apache2_mpm_maxrequestworkers': 0,
475+ 'apache2_mpm_maxsparethreads': 0,
476+ 'apache2_mpm_minsparethreads': 0,
477+ 'apache2_mpm_serverlimit': 0,
478+ 'apache2_mpm_startservers': 0,
479+ 'apache2_mpm_threadlimit': 0,
480+ 'apache2_mpm_threadsperchild': 0,
481+ }
482+ )
483+ self.assertFalse(apache.create_mpm_workerfile(config))
484+
485+ @mock.patch('charmhelpers.core.templating.render')
486+ @mock.patch('subprocess.check_call')
487+ def test_create_security(self, check_call, render):
488+ config = hookenv.Config(
489+ {
490+ 'apache2_server_tokens': 'OS',
491+ 'apache2_server_signature': 'On',
492+ 'apache2_trace_enabled': 'Off',
493+ }
494+ )
495+ self.assertTrue(apache.create_security(config))
496+ render.assert_called_with(
497+ 'apache2/security.template',
498+ '/etc/apache2/conf-available/security.conf',
499+ {
500+ 'server_tokens': 'OS',
501+ 'server_signature': 'On',
502+ 'trace_enabled': 'Off',
503+ },
504+ )
505+ check_call.assert_called_with(['a2enconf', 'security'])
506+
507+ # No change, no need to reload
508+ render.reset_mock()
509+ check_call.reset_mock()
510+ config.save()
511+ config = hookenv.Config(
512+ {
513+ 'apache2_server_tokens': 'OS',
514+ 'apache2_server_signature': 'On',
515+ 'apache2_trace_enabled': 'Off',
516+ }
517+ )
518+ self.assertFalse(apache.create_security(config))
519+
520+ @mock.patch('os.chown')
521+ @mock.patch('os.fchown')
522+ # For @util.run_as_user()
523+ @mock.patch('os.setegid')
524+ @mock.patch('os.seteuid')
525+ @mock.patch('subprocess.check_call')
526+ def test_configure_remoteip_logging(self, check_call, seteuid, setegid, fchown, chown):
527+ config = hookenv.Config({'remoteip_logging': True})
528+ self.assertTrue(apache.configure_remoteip_logging(config, apache2_conf_path=self.tmpdir))
529+ check_call.assert_called_with(['a2enmod', 'remoteip'])
530+ with open(os.path.join(self.tmpdir, 'conf-available', 'remoteip.conf'), 'r') as f:
531+ got = f.read()
532+ with open('templates/apache2/remoteip.conf', 'r') as f:
533+ want = f.read().strip()
534+ self.assertEqual(want, got)
535+
536+ # No change
537+ config.save()
538+ config = hookenv.Config({'remoteip_logging': True})
539+ self.assertFalse(apache.configure_remoteip_logging(config, apache2_conf_path=self.tmpdir))
540+
541+ check_call.reset_mock()
542+ config = hookenv.Config({'remoteip_logging': False})
543+ self.assertTrue(apache.configure_remoteip_logging(config, apache2_conf_path=self.tmpdir))
544+ check_call.assert_called_with(['a2dismod', 'remoteip'])

Subscribers

People subscribed via source and target branches