Merge ~ballot/content-cache-charm/+git/content-cache-charm:reuseport into content-cache-charm:master

[Benjamin Allot]

Add the reuseport directive in nginx.

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

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

[Haw Loeung (hloeung)]

LGTM

review: Approve (+1)

[Joel Sing (jsing)]

LGTM

review: Approve (+1)

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

Change successfully merged at revision f13eeb0394a217e536b01f7d31ef413f55fce5cd

diff --git a/config.yaml b/config.yaml
index 84f8834..3ce504a 100644
--- a/config.yaml
+++ b/config.yaml
@@ -102,6 +102,13 @@ options:
     description: >
       A comma-separated list of nagios servicegroups.
       If left empty, the nagios_context will be used as the servicegroup
+  reuseport:
+    default: false
+    type: boolean
+    description: >
+      Use the SO_REUSEPORT socket option, allowing a better load balance on the nginx workers.
+      Can result in performance drop due to increased latency on heavy workload.
+      See https://www.nginx.com/blog/socket-sharding-nginx-release-1-9-1/.
   sites:
     default: ""
     type: string
diff --git a/lib/nginx.py b/lib/nginx.py
index 2b733a3..4a81f75 100644
--- a/lib/nginx.py
+++ b/lib/nginx.py
@@ -162,6 +162,7 @@ class NginxConf:
             'keys_zone': self._generate_keys_zone(conf['site']),
             'locations': self._process_locations(conf['locations']),
             'port': conf['listen_port'],
+            'reuseport': conf['reuseport'],
             'site': conf['site'],
             'site_name': conf['site_name'],
         }
diff --git a/reactive/content_cache.py b/reactive/content_cache.py
index d396779..15cf56d 100644
--- a/reactive/content_cache.py
+++ b/reactive/content_cache.py
@@ -175,6 +175,7 @@ def configure_nginx(conf_path=None):
     conf['cache_max_size'] = config['cache_max_size'] or utils.cache_max_size(config['cache_path'])
     conf['cache_path'] = config['cache_path']
     conf['listen_address'] = '127.0.0.1'
+    conf['reuseport'] = config['reuseport']
     changed = False
     for site, site_conf in sites.items():
         conf['site'] = site
diff --git a/templates/nginx_cfg.tmpl b/templates/nginx_cfg.tmpl
index fae0d85..4751a43 100644
--- a/templates/nginx_cfg.tmpl
+++ b/templates/nginx_cfg.tmpl
@@ -2,7 +2,7 @@ proxy_cache_path {{cache_path}}/{{site}} use_temp_path=off levels=1:2 keys_zone=
 
 server {
     server_name {{site_name}};
-    listen {% if address %}{{address}}:{% endif %}{{port}};
+    listen {% if address %}{{address}}:{% endif %}{{port}}{% if reuseport %} reuseport{% endif %};
 
     port_in_redirect off;
     absolute_redirect off;
diff --git a/tests/unit/files/nginx_config_rendered_test_output-reuseport.txt b/tests/unit/files/nginx_config_rendered_test_output-reuseport.txt
 new file mode 100644
index 0000000..59b9418
--- /dev/null
+++ b/tests/unit/files/nginx_config_rendered_test_output-reuseport.txt
@@ -0,0 +1,49 @@
+proxy_cache_path /var/lib/nginx/proxy/basic_site use_temp_path=off levels=1:2 keys_zone=e176ab9d1f16-cache:10m inactive=2h max_size=1g;
+
+server {
+    server_name basic_site;
+    listen 127.0.0.1:6080 reuseport;
+
+    port_in_redirect off;
+    absolute_redirect off;
+
+    location / {
+        proxy_pass http://localhost:8080;
+        proxy_set_header Host "basic_site";
+        # Removed the following headers to avoid cache poisoning.
+        proxy_set_header Forwarded "";
+        proxy_set_header X-Forwarded-Host "";
+        proxy_set_header X-Forwarded-Port "";
+        proxy_set_header X-Forwarded-Proto "";
+        proxy_set_header X-Forwarded-Scheme "";
+
+        add_header X-Cache-Status "$upstream_cache_status from mock-content-cache/0";
+        proxy_force_ranges on;
+        proxy_cache e176ab9d1f16-cache;
+        proxy_cache_background_update off;
+        proxy_cache_lock off;
+        proxy_cache_min_uses 1;
+        proxy_cache_revalidate on;
+        proxy_cache_use_stale error timeout http_500 http_502 http_503 http_504;
+        proxy_cache_valid 200 1d;
+
+        access_by_lua_block {
+            -- Exclude healthchecks from nginx_cache_request_total metric.
+            from, to = ngx.re.find(ngx.var.uri, "^/check/?")
+            if not from then
+                cache_request_total:inc(1, {ngx.var.server_name})
+            end
+        }
+
+        log_by_lua_block {
+          http_request_total:inc(1, {ngx.var.server_name, ngx.var.status})
+          if ngx.var.upstream_cache_status == "HIT" then
+              cache_request_hit_total:inc(1, {ngx.var.server_name})
+          end
+        }
+    }
+
+
+    access_log /var/log/nginx/basic_site-access.log content_cache;
+    error_log /var/log/nginx/basic_site-error.log;
+}
diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
index 4d2a8e8..b8de692 100644
--- a/tests/unit/test_content_cache.py
+++ b/tests/unit/test_content_cache.py
@@ -7,6 +7,7 @@ from unittest import mock
 
 import freezegun
 import jinja2
+import pytest
 import yaml
 
 # We also need to mock up charms.layer so we can run unit tests without having
@@ -214,6 +215,7 @@ class TestCharm(unittest.TestCase):
             'enable_cache_background_update': True,
             'enable_cache_lock': True,
             'enable_prometheus_metrics': False,
+            'reuseport': False,
             'sites': ngx_config,
             'worker_connections': 768,
             'worker_processes': 0,
@@ -291,6 +293,7 @@ site1.local:
             'cache_inactive_time': '',
             'cache_max_size': '1g',
             'cache_path': '/var/lib/nginx/proxy',
+            'reuseport': False,
             'sites': config,
             'sites_secrets': secrets,
             'worker_connections': 768,
@@ -340,6 +343,7 @@ site1.local:
                 'cache_inactive_time': '2h',
                 'cache_max_size': '1g',
                 'cache_path': '/var/lib/nginx/proxy',
+                'reuseport': False,
                 'sites': config,
                 'worker_connections': 768,
                 'worker_processes': 0,
@@ -357,6 +361,7 @@ site1.local:
                 'cache_inactive_time': '2h',
                 'cache_max_size': '20g',
                 'cache_path': '/srv/cache',
+                'reuseport': False,
                 'sites': config,
                 'worker_connections': 768,
                 'worker_processes': 0,
@@ -375,6 +380,7 @@ site1.local:
                 'cache_inactive_time': '2h',
                 'cache_max_size': '',
                 'cache_path': '/srv/cache',
+                'reuseport': False,
                 'sites': config,
                 'worker_connections': 768,
                 'worker_processes': 0,
@@ -1300,6 +1306,7 @@ site1.local:
             'enable_cache_lock': False,
             'enable_prometheus_metrics': True,
             'metrics_listen_address': '127.0.0.2',
+            'reuseport': False,
             'sites': ngx_config,
             'worker_connections': 768,
             'worker_processes': 0,
@@ -1400,3 +1407,47 @@ site1.local:
             }
             content_cache.configure_haproxy()
             close_port.assert_not_called()
+
+    @mock.patch('charms.reactive.set_flag')
+    @mock.patch('charmhelpers.core.hookenv.close_port')
+    @mock.patch('charmhelpers.core.hookenv.opened_ports')
+    @mock.patch('reactive.content_cache.update_logrotate')
+    def test_configure_nginx_reuseport(self, logrotation, opened_ports, close_port, set_flag):
+        '''Test configuration of Nginx sites.'''
+        with open('tests/unit/files/config_test_basic_config.txt', 'r', encoding='utf-8') as f:
+            ngx_config = f.read()
+        self.mock_config.return_value = {
+            'cache_inactive_time': '2h',
+            'cache_max_size': '1g',
+            'cache_path': '/var/lib/nginx/proxy',
+            'enable_cache_background_update': False,
+            'enable_cache_lock': False,
+            'enable_prometheus_metrics': True,
+            'metrics_listen_address': '127.0.0.2',
+            'reuseport': True,
+            'sites': ngx_config,
+            'worker_connections': 768,
+            'worker_processes': 0,
+        }
+
+        with mock.patch('lib.nginx.NginxConf.sites_path', new_callable=mock.PropertyMock) as mock_site_path:
+            mock_site_path.return_value = os.path.join(self.tmpdir, 'sites-available')
+            # conf.d, sites-available, and sites-enabled won't exist in our
+            # temporary directory.
+            os.mkdir(os.path.join(self.tmpdir, 'conf.d'))
+            os.mkdir(os.path.join(self.tmpdir, 'sites-available'))
+            os.mkdir(os.path.join(self.tmpdir, 'sites-enabled'))
+            shutil.copyfile('tests/unit/files/nginx.conf', os.path.join(self.tmpdir, 'nginx.conf'))
+
+            opened_ports.return_value = ['80/tcp', '{0}/tcp'.format(nginx.METRICS_PORT)]
+            content_cache.configure_nginx(self.tmpdir)
+
+            site = 'basic_site'
+            test_file = 'tests/unit/files/nginx_config_rendered_test_output-reuseport.txt'
+            with open(test_file, 'r', encoding='utf-8') as f:
+                want = f.read()
+
+            test_file = os.path.join(self.tmpdir, 'sites-available/{0}.conf'.format(site))
+            with open(test_file, 'r', encoding='utf-8') as f:
+                got = f.read()
+            self.assertEqual(got, want)
diff --git a/tests/unit/test_nginx.py b/tests/unit/test_nginx.py
index b0abde9..2734123 100644
--- a/tests/unit/test_nginx.py
+++ b/tests/unit/test_nginx.py
@@ -53,6 +53,7 @@ class TestLibNginx(unittest.TestCase):
         conf['enable_prometheus_metrics'] = False
         conf['cache_path'] = '/var/lib/nginx/proxy'
         conf['listen_address'] = '127.0.0.1'
+        conf['reuseport'] = False
         # From the given YAML-formatted list of sites, check that each individual
         # Nginx config rendered matches what's in tests/unit/files.
         port = BASE_LISTEN_PORT - 1
@@ -139,6 +140,7 @@ class TestLibNginx(unittest.TestCase):
             'cache_path': '/var/lib/nginx/proxy',
             'enable_prometheus_metrics': True,
             'listen_address': '127.0.0.1',
+            'reuseport': False,
         }
         for site, site_conf in sites.items():
             conf['site'] = site