Ubuntu Repository Cache Charm

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

  1. only-reload-apache2-when-needed
  2. Merge into layer-ubuntu-repository-cache
Proposed by Haw Loeung
Status: Merged
Approved by: Haw Loeung
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 (community) +1 Approve
Canonical IS Reviewers 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.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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

lp:~hloeung/ubuntu-repository-cache/only-reload-apache2-when-needed updated
333. By Haw Loeung

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

Revision history for this message
Joel Sing (jsing) wrote :

Generally looks good, but see comments inline.

lp:~hloeung/ubuntu-repository-cache/only-reload-apache2-when-needed updated
334. By Haw Loeung

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

335. By Haw Loeung

Merge upstream changes

336. By Haw Loeung

Reworked configure_remoteip() and enable module on install

337. By Haw Loeung

Fixed based on review

Revision history for this message
Haw Loeung (hloeung) :
lp:~hloeung/ubuntu-repository-cache/only-reload-apache2-when-needed updated
338. By Haw Loeung

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

Revision history for this message
Haw Loeung (hloeung) wrote :

> Generally looks good, but see comments inline.

Okay, all addressed. Thanks for the review.

Revision history for this message
Haw Loeung (hloeung) :
Revision history for this message
Joel Sing (jsing) wrote :

LGTM

review: Approve (+1)
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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