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

[Haw Loeung]

Only reload/graceful apache2 when required - LP:1916084

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

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]

333. By Haw Loeung on 2021-02-22

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

[Joel Sing (jsing)]

Generally looks good, but see comments inline.

[lp:~hloeung/ubuntu-repository-cache/only-reload-apache2-when-needed]

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

[lp:~hloeung/ubuntu-repository-cache/only-reload-apache2-when-needed]

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)]

> Generally looks good, but see comments inline.

Okay, all addressed. Thanks for the review.

[Joel Sing (jsing)]

LGTM

review: Approve (+1)

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 316

=== modified file '.bzrignore'
--- .bzrignore	2020-05-07 05:29:31 +0000
+++ .bzrignore	2021-02-22 22:51:46 +0000
@@ -1,6 +1,7 @@
 *.pyc
 *.swp
 *~
+.juju-persistent-config
 .unit-state.db
 lib/test.py
 .venv
 
=== added file '.coveragerc'
--- .coveragerc	1970-01-01 00:00:00 +0000
+++ .coveragerc	2021-02-22 22:51:46 +0000
@@ -0,0 +1,2 @@
+[run]
+omit = lib/ubuntu_repository_cache/tests/*
 
=== modified file 'lib/ubuntu_repository_cache/apache.py'
--- lib/ubuntu_repository_cache/apache.py	2021-02-22 03:11:12 +0000
+++ lib/ubuntu_repository_cache/apache.py	2021-02-22 22:51:46 +0000
@@ -41,6 +41,8 @@
     config = hookenv.config()
     if config['enable_healthcheck']:
         subprocess.check_call(['a2enmod', 'cgid'])
+    if config['remoteip_logging']:
+        subprocess.check_call(['a2enmod', 'remoteip'])
 
     start()
 
@@ -123,20 +125,37 @@
     return config
 
 
-def create_mpm_workerfile():
+def create_mpm_workerfile(config=None):
     '''Create the multi-processing module (MPM) configuration'''
 
-    config = hookenv.config()
-    mpm_context = {
-        'startservers': config['apache2_mpm_startservers'],
-        'minsparethreads': config['apache2_mpm_minsparethreads'],
-        'maxsparethreads': config['apache2_mpm_maxsparethreads'],
-        'threadlimit': config['apache2_mpm_threadlimit'],
-        'threadsperchild': config['apache2_mpm_threadsperchild'],
-        'serverlimit': config['apache2_mpm_serverlimit'],
-        'maxrequestworkers': config['apache2_mpm_maxrequestworkers'],
-        'maxconnectionsperchild': config['apache2_mpm_maxconnectionsperchild'],
-    }
+    reload_required = False
+
+    if not config:
+        config = hookenv.config()
+
+    mpm_configs = [
+        'maxconnectionsperchild',
+        'maxrequestworkers',
+        'maxsparethreads',
+        'minsparethreads',
+        'serverlimit',
+        'startservers',
+        'threadlimit',
+        'threadsperchild',
+    ]
+    mpm_context = {}
+    for k in mpm_configs:
+        cfkey = 'apache2_mpm_{}'.format(k)
+        mpm_context[k] = config[cfkey]
+        if config.changed(cfkey):
+            reload_required = True
+
+    # We'll only proceed when any one of the MPM configs have changed.
+    if not reload_required:
+        return reload_required
+
+    reload_required = True
+
     mpm_context = tune_mpm_configs(mpm_context)
     conf_mpm_worker = '/etc/apache2/conf-available/000mpm-worker.conf'
     templating.render('apache2/mpm_worker.template', conf_mpm_worker, mpm_context)
@@ -148,24 +167,44 @@
         os.unlink(mods_mpm_worker)
         os.symlink(conf_mpm_worker, mods_mpm_worker)
 
-
-def create_security():
+    return reload_required
+
+
+def create_security(config=None):
     '''Create the security configuration'''
 
-    config = hookenv.config()
-    security_context = {
-        'server_tokens': config['apache2_server_tokens'],
-        'server_signature': config['apache2_server_signature'],
-        'trace_enabled': config['apache2_trace_enabled'],
-    }
+    reload_required = False
+
+    if not config:
+        config = hookenv.config()
+
+    security_configs = ['server_signature', 'server_tokens', 'trace_enabled']
+    security_context = {}
+    for k in security_configs:
+        cfkey = 'apache2_{}'.format(k)
+        security_context[k] = config[cfkey]
+        if config.changed(cfkey):
+            reload_required = True
+
+    # We'll only proceed when any one of the MPM configs have changed.
+    if not reload_required:
+        return reload_required
+
+    reload_required = True
+
     templating.render('apache2/security.template', '/etc/apache2/conf-available/security.conf', security_context)
     subprocess.check_call(['a2enconf', 'security'])
-
-
-def create_metadata_site():
+    return reload_required
+
+
+def create_metadata_site(config=None, apache2_conf_path='/etc/apache2'):
     '''Create the site config for serving (or proxying) repository metadata'''
 
-    config = hookenv.config()
+    reload_required = False
+
+    if not config:
+        config = hookenv.config()
+
     apache_context = {}
     apache_context['DocumentRoot'] = unitdata.kv().get('apache-root')
     apache_context['Sync_Host'] = config['sync-host']
@@ -188,38 +227,62 @@
     apache_context['MetadataProxy'] = metaproxy_conf
     apache_context['Enable_Healthcheck'] = config['enable_healthcheck']
 
-    templating.render(
-        'apache2/archive_ubuntu_com.conf', '/etc/apache2/sites-available/archive_ubuntu_com.conf', apache_context
-    )
-    subprocess.call(['a2ensite', 'archive_ubuntu_com'])
-    start()
+    apache_config_file = os.path.join(apache2_conf_path, 'sites-available', 'archive_ubuntu_com.conf')
+    cur_conf = ''
+    if os.path.exists(apache_config_file):
+        with open(apache_config_file) as f:
+            cur_conf = f.read()
+    new_conf = templating.render('apache2/archive_ubuntu_com.conf', apache_config_file, apache_context)
+
+    if new_conf != cur_conf:
+        reload_required = True
+        if not os.path.exists(os.path.join(apache2_conf_path, 'sites-enabled', 'archive_ubuntu_com.conf')):
+            subprocess.call(['a2ensite', 'archive_ubuntu_com'])
+
+    return reload_required
 
 
 @util.run_as_user('root')
-def configure_remoteip_logging():
-    config = hookenv.config()
+def configure_remoteip_logging(config=None, apache2_conf_path='/etc/apache2'):
+    reload_required = False
+
+    if not config:
+        config = hookenv.config()
+
+    if config['remoteip_logging']:
+        apache_config_file = os.path.join(apache2_conf_path, 'conf-available', 'remoteip.conf')
+        cur_conf = ''
+        if os.path.exists(apache_config_file):
+            with open(apache_config_file) as f:
+                cur_conf = f.read()
+        new_conf = templating.render('apache2/remoteip.conf', apache_config_file, {})
+
+        if new_conf != cur_conf:
+            reload_required = True
+            if not os.path.exists(os.path.join(apache2_conf_path, 'conf-enabled', 'remoteip.conf')):
+                subprocess.check_call(['a2enconf', 'remoteip'])
+
     if config.changed('remoteip_logging'):
         LOG('Apache remoteip_logging changed')
-        if config['remoteip_logging'] is True:
+        if config['remoteip_logging']:
             subprocess.check_call(['a2enmod', 'remoteip'])
-            templating.render('apache2/remoteip.conf', '/etc/apache2/conf-available/remoteip.conf', {})
-            subprocess.check_call(['a2enconf', 'remoteip'])
             LOG('Enabled remoteip logging')
         else:
             subprocess.check_call(['a2dismod', 'remoteip'])
             if os.path.exists('/etc/apache2/conf-enabled/remoteip.conf'):
                 subprocess.check_call(['a2disconf', 'remoteip'])
             LOG('Disabled remoteip logging')
+        reload_required = True
 
-        start()
+    return reload_required
 
 
 def configure_health_check(config=None):
+    reload_required = False
+
     if not config:
         config = hookenv.config()
 
-    restart_required = False
-
     if config.changed('enable_healthcheck'):
         LOG('Apache enable_healthcheck changed')
         if config['enable_healthcheck']:
@@ -231,7 +294,7 @@
 
         subprocess.check_call([cmd, 'cgid'])
         LOG('{} health check endpoint'.format(action))
-        restart_required = True
+        reload_required = True
 
     subprocess.call(['touch', HEALTH_CHECK_LOG])
     subprocess.call(['chown', HEALTH_CHECK_LOG_OWNER, HEALTH_CHECK_LOG])
@@ -244,16 +307,19 @@
         content = f.read()
     host.write_file(path=healthcheck_script, content=content, perms=0o755)
 
-    return restart_required
+    return reload_required
 
 
 @util.run_as_user('root')
 def render_configs():
     '''Render the configuration templates for apache'''
 
+    reload_required = False
+
     LOG('Rendering apache2 configuration templates')
     config = hookenv.config()
     if config.changed('apache2_mpm_type'):
+        reload_required = True
         if config['apache2_mpm_type'] == 'worker':
             LOG('Using apache2-mpm-worker')
             subprocess.check_call(['a2dismod', 'mpm_prefork'])
@@ -264,13 +330,13 @@
             subprocess.check_call(['a2dismod', 'mpm_worker'])
             subprocess.check_call(['a2enmod', 'mpm_prefork'])
 
-    create_metadata_site()
-    create_mpm_workerfile()
-    create_security()
-    configure_remoteip_logging()
-    configure_health_check()
+    reload_required = create_metadata_site() or reload_required
+    reload_required = create_mpm_workerfile() or reload_required
+    reload_required = create_security() or reload_required
+    reload_required = configure_remoteip_logging() or reload_required
+    reload_required = configure_health_check() or reload_required
 
-    start()
+    start(start_only=(not reload_required))
 
 
 def update_checks(nrpe_config):
 
=== modified file 'tests/unit/test_apache.py'
--- tests/unit/test_apache.py	2021-02-22 02:59:25 +0000
+++ tests/unit/test_apache.py	2021-02-22 22:51:46 +0000
@@ -43,11 +43,6 @@
         self.addCleanup(patcher.stop)
         self.mock_local_unit.return_value = 'mock-ubuntu-repository-cache/0'
 
-        patcher = mock.patch('charmhelpers.core.hookenv.config')
-        self.mock_config = patcher.start()
-        self.addCleanup(patcher.stop)
-        self.mock_config.return_value = {'nagios_context': 'juju'}
-
         patcher = mock.patch('charmhelpers.core.host.log')
         self.mock_log = patcher.start()
         self.addCleanup(patcher.stop)
@@ -65,46 +60,53 @@
     @mock.patch('subprocess.check_call')
     def test_configure_health_check(self, check_call, call, seteuid, setegid, chown, write_file):
         config = hookenv.Config({'enable_healthcheck': True})
-        self.assertEqual(apache.configure_health_check(config), True)
+        self.assertTrue(apache.configure_health_check(config))
         check_call.assert_called_with(['a2enmod', 'cgid'])
 
         check_call.reset_mock()
         config = hookenv.Config({'enable_healthcheck': False})
-        self.assertEqual(apache.configure_health_check(config), True)
+        self.assertTrue(apache.configure_health_check(config))
         check_call.assert_called_with(['a2dismod', 'cgid'])
 
+        # No change
         check_call.reset_mock()
         config = hookenv.Config({'enable_healthcheck': True})
         config.save()
         config = hookenv.Config({'enable_healthcheck': True})
-        self.assertEqual(apache.configure_health_check(config), False)
+        self.assertFalse(apache.configure_health_check(config))
 
-    @mock.patch('charmhelpers.core.templating.render')
     @mock.patch('lib.ubuntu_repository_cache.apache.start')
     @mock.patch('lib.ubuntu_repository_cache.util.get_failover_host')
+    @mock.patch('os.chown')
+    @mock.patch('os.fchown')
     @mock.patch('subprocess.call')
-    def test_create_metadata_site(self, call, get_failover_host, start, render):
+    def test_create_metadata_site(self, call, fchown, chown, get_failover_host, start):
         get_failover_host.return_value = '10.1.1.1'
-        self.mock_config.return_value = {
-            'display-host': 'archive.ubuntu.com',
-            'enable_healthcheck': True,
-            'path-base': 'ubuntu',
-            'sync-host': 'us.archive.ubuntu.com',
-        }
-        apache.create_metadata_site()
-        render.assert_called_with(
-            'apache2/archive_ubuntu_com.conf',
-            '/etc/apache2/sites-available/archive_ubuntu_com.conf',
+        config = hookenv.Config(
             {
-                'DocumentRoot': os.path.join(self.tmpdir, 'apache'),
-                'Sync_Host': 'us.archive.ubuntu.com',
-                'Display_Host': 'archive.ubuntu.com',
-                'Path_Base': 'ubuntu',
-                'MetadataProxy': '# No failover configured',
-                'Enable_Healthcheck': True,
-            },
+                'display-host': 'archive.ubuntu.com',
+                'enable_healthcheck': True,
+                'path-base': 'ubuntu',
+                'sync-host': 'us.archive.ubuntu.com',
+            }
         )
+        self.assertTrue(apache.create_metadata_site(config, apache2_conf_path=self.tmpdir))
         call.assert_called_with(['a2ensite', 'archive_ubuntu_com'])
+        chown.assert_called()
+
+        # No change, no need to reload
+        call.reset_mock()
+        config.save()
+        config = hookenv.Config(
+            {
+                'display-host': 'archive.ubuntu.com',
+                'enable_healthcheck': True,
+                'path-base': 'ubuntu',
+                'sync-host': 'us.archive.ubuntu.com',
+            }
+        )
+        self.assertFalse(apache.create_metadata_site(config, apache2_conf_path=self.tmpdir))
+        call.assert_not_called()
 
     @mock.patch('charmhelpers.core.templating.render')
     @mock.patch('lib.ubuntu_repository_cache.apache.start')
@@ -113,12 +115,14 @@
     @mock.patch('os.mknod')
     @mock.patch('subprocess.call')
     def test_create_metadata_site_in_failover(self, call, mknod, in_failover, get_failover_host, start, render):
-        self.mock_config.return_value = {
-            'display-host': 'archive.ubuntu.com',
-            'enable_healthcheck': True,
-            'path-base': 'ubuntu',
-            'sync-host': 'us.archive.ubuntu.com',
-        }
+        config = hookenv.Config(
+            {
+                'display-host': 'archive.ubuntu.com',
+                'enable_healthcheck': True,
+                'path-base': 'ubuntu',
+                'sync-host': 'us.archive.ubuntu.com',
+            }
+        )
         get_failover_host.return_value = '10.1.1.1'
         in_failover.return_value = True
 
@@ -127,7 +131,7 @@
         os.mkdir(os.path.join(self.tmpdir, 'apache'))
         os.mkdir(health_check_path)
 
-        apache.create_metadata_site()
+        apache.create_metadata_site(config)
         mknod.assert_called_with(disabled_flag)
         render.assert_called_with(
             'apache2/archive_ubuntu_com.conf',
@@ -148,7 +152,7 @@
         # If the disabled flag exists, no need to create it.
         mknod.reset_mock()
         open(disabled_flag, 'a').close()
-        apache.create_metadata_site()
+        apache.create_metadata_site(config)
         mknod.assert_not_called()
 
     @mock.patch('charmhelpers.core.templating.render')
@@ -157,19 +161,21 @@
     @mock.patch('os.unlink')
     @mock.patch('subprocess.call')
     def test_create_metadata_site_no_failover(self, call, unlink, get_failover_host, start, render):
-        self.mock_config.return_value = {
-            'display-host': 'archive.ubuntu.com',
-            'enable_healthcheck': True,
-            'path-base': 'ubuntu',
-            'sync-host': 'us.archive.ubuntu.com',
-        }
+        config = hookenv.Config(
+            {
+                'display-host': 'archive.ubuntu.com',
+                'enable_healthcheck': True,
+                'path-base': 'ubuntu',
+                'sync-host': 'us.archive.ubuntu.com',
+            }
+        )
 
         health_check_path = os.path.join(self.tmpdir, 'apache', 'health-check')
         disabled_flag = os.path.join(health_check_path, 'health-check-disabled.flag')
         os.mkdir(os.path.join(self.tmpdir, 'apache'))
         os.mkdir(health_check_path)
         os.mknod(disabled_flag)
-        apache.create_metadata_site()
+        apache.create_metadata_site(config)
         unlink.assert_called_with(disabled_flag)
 
     @mock.patch('charmhelpers.core.host.service_start')
@@ -204,3 +210,119 @@
         apache.start(start_only=True)
         service_reload.assert_not_called()
         service_start.assert_called_with('apache2')
+
+    @mock.patch('charmhelpers.core.templating.render')
+    @mock.patch('os.symlink')
+    @mock.patch('subprocess.check_call')
+    def test_create_mpm_workerfile(self, check_call, symlink, render):
+        config = hookenv.Config(
+            {
+                'apache2_mpm_maxconnectionsperchild': 10000,
+                'apache2_mpm_maxrequestworkers': 0,
+                'apache2_mpm_maxsparethreads': 0,
+                'apache2_mpm_minsparethreads': 0,
+                'apache2_mpm_serverlimit': 0,
+                'apache2_mpm_startservers': 0,
+                'apache2_mpm_threadlimit': 0,
+                'apache2_mpm_threadsperchild': 0,
+            }
+        )
+        self.assertTrue(apache.create_mpm_workerfile(config))
+        render.assert_called_with(
+            'apache2/mpm_worker.template',
+            '/etc/apache2/conf-available/000mpm-worker.conf',
+            {
+                'maxconnectionsperchild': 10000,
+                'maxrequestworkers': 512,
+                'maxsparethreads': 512,
+                'minsparethreads': 256,
+                'serverlimit': 1,
+                'startservers': 1,
+                'threadlimit': 512,
+                'threadsperchild': 512,
+            },
+        )
+        check_call.assert_called_with(['a2enconf', '000mpm-worker'])
+        symlink.assert_called_with(
+            '/etc/apache2/conf-available/000mpm-worker.conf', '/etc/apache2/mods-available/mpm_worker.conf'
+        )
+
+        # No change, no need to reload
+        render.reset_mock()
+        check_call.reset_mock()
+        symlink.reset_mock()
+        config.save()
+        config = hookenv.Config(
+            {
+                'apache2_mpm_maxconnectionsperchild': 10000,
+                'apache2_mpm_maxrequestworkers': 0,
+                'apache2_mpm_maxsparethreads': 0,
+                'apache2_mpm_minsparethreads': 0,
+                'apache2_mpm_serverlimit': 0,
+                'apache2_mpm_startservers': 0,
+                'apache2_mpm_threadlimit': 0,
+                'apache2_mpm_threadsperchild': 0,
+            }
+        )
+        self.assertFalse(apache.create_mpm_workerfile(config))
+
+    @mock.patch('charmhelpers.core.templating.render')
+    @mock.patch('subprocess.check_call')
+    def test_create_security(self, check_call, render):
+        config = hookenv.Config(
+            {
+                'apache2_server_tokens': 'OS',
+                'apache2_server_signature': 'On',
+                'apache2_trace_enabled': 'Off',
+            }
+        )
+        self.assertTrue(apache.create_security(config))
+        render.assert_called_with(
+            'apache2/security.template',
+            '/etc/apache2/conf-available/security.conf',
+            {
+                'server_tokens': 'OS',
+                'server_signature': 'On',
+                'trace_enabled': 'Off',
+            },
+        )
+        check_call.assert_called_with(['a2enconf', 'security'])
+
+        # No change, no need to reload
+        render.reset_mock()
+        check_call.reset_mock()
+        config.save()
+        config = hookenv.Config(
+            {
+                'apache2_server_tokens': 'OS',
+                'apache2_server_signature': 'On',
+                'apache2_trace_enabled': 'Off',
+            }
+        )
+        self.assertFalse(apache.create_security(config))
+
+    @mock.patch('os.chown')
+    @mock.patch('os.fchown')
+    # For @util.run_as_user()
+    @mock.patch('os.setegid')
+    @mock.patch('os.seteuid')
+    @mock.patch('subprocess.check_call')
+    def test_configure_remoteip_logging(self, check_call, seteuid, setegid, fchown, chown):
+        config = hookenv.Config({'remoteip_logging': True})
+        self.assertTrue(apache.configure_remoteip_logging(config, apache2_conf_path=self.tmpdir))
+        check_call.assert_called_with(['a2enmod', 'remoteip'])
+        with open(os.path.join(self.tmpdir, 'conf-available', 'remoteip.conf'), 'r') as f:
+            got = f.read()
+        with open('templates/apache2/remoteip.conf', 'r') as f:
+            want = f.read().strip()
+        self.assertEqual(want, got)
+
+        # No change
+        config.save()
+        config = hookenv.Config({'remoteip_logging': True})
+        self.assertFalse(apache.configure_remoteip_logging(config, apache2_conf_path=self.tmpdir))
+
+        check_call.reset_mock()
+        config = hookenv.Config({'remoteip_logging': False})
+        self.assertTrue(apache.configure_remoteip_logging(config, apache2_conf_path=self.tmpdir))
+        check_call.assert_called_with(['a2dismod', 'remoteip'])