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

Proposed by Benjamin Allot
Status: Needs review
Proposed branch: ~ballot/content-cache-charm/+git/content-cache-charm:reuseport
Merge into: content-cache-charm:master
Diff against target: 238 lines (+112/-1)
7 files modified
config.yaml (+7/-0)
lib/nginx.py (+1/-0)
reactive/content_cache.py (+1/-0)
templates/nginx_cfg.tmpl (+1/-1)
tests/unit/files/nginx_config_rendered_test_output-reuseport.txt (+49/-0)
tests/unit/test_content_cache.py (+51/-0)
tests/unit/test_nginx.py (+2/-0)
Reviewer Review Type Date Requested Status
Joel Sing +1 Approve
Haw Loeung +1 Approve
Review via email: mp+400068@code.launchpad.net

Commit message

Add the reuseport directive in nginx.

To post a comment you must log in.
Revision history for this message
Canonical IS Mergebot (canonical-is-mergebot) wrote :

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

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

LGTM

review: Approve (+1)
Revision history for this message
Joel Sing (jsing) wrote :

LGTM

review: Approve (+1)

Unmerged commits

cbdb480... by Benjamin Allot

Add the reuseport directive in nginx.

According to several resources, the way Linux handles TCP connection
with epoll is responsible for the way the load is balanced between the
workers.

This is an option turned off by default for now.

See [0] and [1] for reference.

[0]: https://blog.cloudflare.com/the-sad-state-of-linux-socket-balancing/
[1]: https://www.nginx.com/blog/performance-tuning-tips-tricks/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/config.yaml b/config.yaml
2index 84f8834..3ce504a 100644
3--- a/config.yaml
4+++ b/config.yaml
5@@ -102,6 +102,13 @@ options:
6 description: >
7 A comma-separated list of nagios servicegroups.
8 If left empty, the nagios_context will be used as the servicegroup
9+ reuseport:
10+ default: false
11+ type: boolean
12+ description: >
13+ Use the SO_REUSEPORT socket option, allowing a better load balance on the nginx workers.
14+ Can result in performance drop due to increased latency on heavy workload.
15+ See https://www.nginx.com/blog/socket-sharding-nginx-release-1-9-1/.
16 sites:
17 default: ""
18 type: string
19diff --git a/lib/nginx.py b/lib/nginx.py
20index 2b733a3..4a81f75 100644
21--- a/lib/nginx.py
22+++ b/lib/nginx.py
23@@ -162,6 +162,7 @@ class NginxConf:
24 'keys_zone': self._generate_keys_zone(conf['site']),
25 'locations': self._process_locations(conf['locations']),
26 'port': conf['listen_port'],
27+ 'reuseport': conf['reuseport'],
28 'site': conf['site'],
29 'site_name': conf['site_name'],
30 }
31diff --git a/reactive/content_cache.py b/reactive/content_cache.py
32index d396779..15cf56d 100644
33--- a/reactive/content_cache.py
34+++ b/reactive/content_cache.py
35@@ -175,6 +175,7 @@ def configure_nginx(conf_path=None):
36 conf['cache_max_size'] = config['cache_max_size'] or utils.cache_max_size(config['cache_path'])
37 conf['cache_path'] = config['cache_path']
38 conf['listen_address'] = '127.0.0.1'
39+ conf['reuseport'] = config['reuseport']
40 changed = False
41 for site, site_conf in sites.items():
42 conf['site'] = site
43diff --git a/templates/nginx_cfg.tmpl b/templates/nginx_cfg.tmpl
44index fae0d85..4751a43 100644
45--- a/templates/nginx_cfg.tmpl
46+++ b/templates/nginx_cfg.tmpl
47@@ -2,7 +2,7 @@ proxy_cache_path {{cache_path}}/{{site}} use_temp_path=off levels=1:2 keys_zone=
48
49 server {
50 server_name {{site_name}};
51- listen {% if address %}{{address}}:{% endif %}{{port}};
52+ listen {% if address %}{{address}}:{% endif %}{{port}}{% if reuseport %} reuseport{% endif %};
53
54 port_in_redirect off;
55 absolute_redirect off;
56diff --git a/tests/unit/files/nginx_config_rendered_test_output-reuseport.txt b/tests/unit/files/nginx_config_rendered_test_output-reuseport.txt
57new file mode 100644
58index 0000000..59b9418
59--- /dev/null
60+++ b/tests/unit/files/nginx_config_rendered_test_output-reuseport.txt
61@@ -0,0 +1,49 @@
62+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;
63+
64+server {
65+ server_name basic_site;
66+ listen 127.0.0.1:6080 reuseport;
67+
68+ port_in_redirect off;
69+ absolute_redirect off;
70+
71+ location / {
72+ proxy_pass http://localhost:8080;
73+ proxy_set_header Host "basic_site";
74+ # Removed the following headers to avoid cache poisoning.
75+ proxy_set_header Forwarded "";
76+ proxy_set_header X-Forwarded-Host "";
77+ proxy_set_header X-Forwarded-Port "";
78+ proxy_set_header X-Forwarded-Proto "";
79+ proxy_set_header X-Forwarded-Scheme "";
80+
81+ add_header X-Cache-Status "$upstream_cache_status from mock-content-cache/0";
82+ proxy_force_ranges on;
83+ proxy_cache e176ab9d1f16-cache;
84+ proxy_cache_background_update off;
85+ proxy_cache_lock off;
86+ proxy_cache_min_uses 1;
87+ proxy_cache_revalidate on;
88+ proxy_cache_use_stale error timeout http_500 http_502 http_503 http_504;
89+ proxy_cache_valid 200 1d;
90+
91+ access_by_lua_block {
92+ -- Exclude healthchecks from nginx_cache_request_total metric.
93+ from, to = ngx.re.find(ngx.var.uri, "^/check/?")
94+ if not from then
95+ cache_request_total:inc(1, {ngx.var.server_name})
96+ end
97+ }
98+
99+ log_by_lua_block {
100+ http_request_total:inc(1, {ngx.var.server_name, ngx.var.status})
101+ if ngx.var.upstream_cache_status == "HIT" then
102+ cache_request_hit_total:inc(1, {ngx.var.server_name})
103+ end
104+ }
105+ }
106+
107+
108+ access_log /var/log/nginx/basic_site-access.log content_cache;
109+ error_log /var/log/nginx/basic_site-error.log;
110+}
111diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
112index 4d2a8e8..b8de692 100644
113--- a/tests/unit/test_content_cache.py
114+++ b/tests/unit/test_content_cache.py
115@@ -7,6 +7,7 @@ from unittest import mock
116
117 import freezegun
118 import jinja2
119+import pytest
120 import yaml
121
122 # We also need to mock up charms.layer so we can run unit tests without having
123@@ -214,6 +215,7 @@ class TestCharm(unittest.TestCase):
124 'enable_cache_background_update': True,
125 'enable_cache_lock': True,
126 'enable_prometheus_metrics': False,
127+ 'reuseport': False,
128 'sites': ngx_config,
129 'worker_connections': 768,
130 'worker_processes': 0,
131@@ -291,6 +293,7 @@ site1.local:
132 'cache_inactive_time': '',
133 'cache_max_size': '1g',
134 'cache_path': '/var/lib/nginx/proxy',
135+ 'reuseport': False,
136 'sites': config,
137 'sites_secrets': secrets,
138 'worker_connections': 768,
139@@ -340,6 +343,7 @@ site1.local:
140 'cache_inactive_time': '2h',
141 'cache_max_size': '1g',
142 'cache_path': '/var/lib/nginx/proxy',
143+ 'reuseport': False,
144 'sites': config,
145 'worker_connections': 768,
146 'worker_processes': 0,
147@@ -357,6 +361,7 @@ site1.local:
148 'cache_inactive_time': '2h',
149 'cache_max_size': '20g',
150 'cache_path': '/srv/cache',
151+ 'reuseport': False,
152 'sites': config,
153 'worker_connections': 768,
154 'worker_processes': 0,
155@@ -375,6 +380,7 @@ site1.local:
156 'cache_inactive_time': '2h',
157 'cache_max_size': '',
158 'cache_path': '/srv/cache',
159+ 'reuseport': False,
160 'sites': config,
161 'worker_connections': 768,
162 'worker_processes': 0,
163@@ -1300,6 +1306,7 @@ site1.local:
164 'enable_cache_lock': False,
165 'enable_prometheus_metrics': True,
166 'metrics_listen_address': '127.0.0.2',
167+ 'reuseport': False,
168 'sites': ngx_config,
169 'worker_connections': 768,
170 'worker_processes': 0,
171@@ -1400,3 +1407,47 @@ site1.local:
172 }
173 content_cache.configure_haproxy()
174 close_port.assert_not_called()
175+
176+ @mock.patch('charms.reactive.set_flag')
177+ @mock.patch('charmhelpers.core.hookenv.close_port')
178+ @mock.patch('charmhelpers.core.hookenv.opened_ports')
179+ @mock.patch('reactive.content_cache.update_logrotate')
180+ def test_configure_nginx_reuseport(self, logrotation, opened_ports, close_port, set_flag):
181+ '''Test configuration of Nginx sites.'''
182+ with open('tests/unit/files/config_test_basic_config.txt', 'r', encoding='utf-8') as f:
183+ ngx_config = f.read()
184+ self.mock_config.return_value = {
185+ 'cache_inactive_time': '2h',
186+ 'cache_max_size': '1g',
187+ 'cache_path': '/var/lib/nginx/proxy',
188+ 'enable_cache_background_update': False,
189+ 'enable_cache_lock': False,
190+ 'enable_prometheus_metrics': True,
191+ 'metrics_listen_address': '127.0.0.2',
192+ 'reuseport': True,
193+ 'sites': ngx_config,
194+ 'worker_connections': 768,
195+ 'worker_processes': 0,
196+ }
197+
198+ with mock.patch('lib.nginx.NginxConf.sites_path', new_callable=mock.PropertyMock) as mock_site_path:
199+ mock_site_path.return_value = os.path.join(self.tmpdir, 'sites-available')
200+ # conf.d, sites-available, and sites-enabled won't exist in our
201+ # temporary directory.
202+ os.mkdir(os.path.join(self.tmpdir, 'conf.d'))
203+ os.mkdir(os.path.join(self.tmpdir, 'sites-available'))
204+ os.mkdir(os.path.join(self.tmpdir, 'sites-enabled'))
205+ shutil.copyfile('tests/unit/files/nginx.conf', os.path.join(self.tmpdir, 'nginx.conf'))
206+
207+ opened_ports.return_value = ['80/tcp', '{0}/tcp'.format(nginx.METRICS_PORT)]
208+ content_cache.configure_nginx(self.tmpdir)
209+
210+ site = 'basic_site'
211+ test_file = 'tests/unit/files/nginx_config_rendered_test_output-reuseport.txt'
212+ with open(test_file, 'r', encoding='utf-8') as f:
213+ want = f.read()
214+
215+ test_file = os.path.join(self.tmpdir, 'sites-available/{0}.conf'.format(site))
216+ with open(test_file, 'r', encoding='utf-8') as f:
217+ got = f.read()
218+ self.assertEqual(got, want)
219diff --git a/tests/unit/test_nginx.py b/tests/unit/test_nginx.py
220index b0abde9..2734123 100644
221--- a/tests/unit/test_nginx.py
222+++ b/tests/unit/test_nginx.py
223@@ -53,6 +53,7 @@ class TestLibNginx(unittest.TestCase):
224 conf['enable_prometheus_metrics'] = False
225 conf['cache_path'] = '/var/lib/nginx/proxy'
226 conf['listen_address'] = '127.0.0.1'
227+ conf['reuseport'] = False
228 # From the given YAML-formatted list of sites, check that each individual
229 # Nginx config rendered matches what's in tests/unit/files.
230 port = BASE_LISTEN_PORT - 1
231@@ -139,6 +140,7 @@ class TestLibNginx(unittest.TestCase):
232 'cache_path': '/var/lib/nginx/proxy',
233 'enable_prometheus_metrics': True,
234 'listen_address': '127.0.0.1',
235+ 'reuseport': False,
236 }
237 for site, site_conf in sites.items():
238 conf['site'] = site

Subscribers

People subscribed via source and target branches