Merge ~ballot/content-cache-charm/+git/content-cache-charm:add_lua into content-cache-charm:master
- Git
- lp:~ballot/content-cache-charm/+git/content-cache-charm
- add_lua
- Merge into master
Status: | Merged |
---|---|
Approved by: | Benjamin Allot |
Approved revision: | a388316ee133b9454a3298c9d464a6df9386c9f0 |
Merged at revision: | 3fa2dbc5cd782969d2b2bd3011f50275f4871dae |
Proposed branch: | ~ballot/content-cache-charm/+git/content-cache-charm:add_lua |
Merge into: | content-cache-charm:master |
Diff against target: |
624 lines (+388/-11) 10 files modified
config.yaml (+6/-0) lib/nginx.py (+75/-6) nginx_config_rendered_test_output-nginx_metrics.txt (+32/-0) reactive/content_cache.py (+36/-0) templates/nginx_cfg.tmpl (+8/-0) templates/nginx_metrics_cfg.tmpl (+23/-0) tests/unit/files/config_test_basic_config.txt (+6/-0) tests/unit/files/nginx_config_rendered_test_output-basic_site.txt (+32/-0) tests/unit/test_content_cache.py (+87/-3) tests/unit/test_nginx.py (+83/-2) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Joel Sing (community) | +1 | Approve | |
Canonical IS Reviewers | Pending | ||
Review via email: mp+372309@code.launchpad.net |
Commit message
Add nginx metrics for cache hits
A new option `enable_
This metrics are available on port 9145.
Update tests
All the tests have been adapted with the new bits of configuration and
new tests have been developed for the new feature.
Description of the change
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Joel Sing (jsing) : | # |
Benjamin Allot (ballot) wrote : | # |
Ah damn, I was rereading yesterday night but I missed quite a lot of consistency/debug stnzas. My bad.
About the separated tests:
I had done so in my previous configuration. However, it had his share of probem:
* a lot lot of files needed to be modified without adding any value (listen, backend haproxy ones notably)
* I had introduced a specific clause based on the name of the key to inject the enable_
After weighting the pros and cons, I ended up doing a separate file to test only the part relevant to the metrics exposure.
About the noqa, I had an issue using the form
```
patcher = mock.patch.
patcher.start()
.
.
patcher.stop()
```
For some reason, the patcher.stop() was not removing the patch and I was ending with others tests failing.
I spent time trying to figure out why but could not find out so I use the context manager since it was "working".
I'm still looking for a more elegant solution to handle this but I figured it should not block the progress.
Joel Sing (jsing) : | # |
Joel Sing (jsing) wrote : | # |
LGTM - per previous comment, before landing please rename config_
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Change successfully merged at revision 3fa2dbc5cd78296
Preview Diff
1 | diff --git a/config.yaml b/config.yaml |
2 | index 7f6d083..cdc2408 100644 |
3 | --- a/config.yaml |
4 | +++ b/config.yaml |
5 | @@ -40,3 +40,9 @@ options: |
6 | origin-headers: |
7 | X-Origin-Key: my-origin-secret-key |
8 | signed-url-hmac-key: my-signed-url-secret-key |
9 | + enable_prometheus_metrics: |
10 | + default: true |
11 | + type: boolean |
12 | + description: | |
13 | + Export metrics for the number of requests and the number of cache hits. |
14 | + Prometheus metrics are exposed at /metrics on port 9145. |
15 | diff --git a/lib/nginx.py b/lib/nginx.py |
16 | index 055acc8..48ef4a9 100644 |
17 | --- a/lib/nginx.py |
18 | +++ b/lib/nginx.py |
19 | @@ -6,8 +6,10 @@ import jinja2 |
20 | from lib import utils |
21 | |
22 | |
23 | -NGINX_BASE_PATH = '/etc/nginx' |
24 | INDENT = ' ' * 4 |
25 | +METRICS_PORT = 9145 |
26 | +METRICS_SITE = 'nginx_metrics' |
27 | +NGINX_BASE_PATH = '/etc/nginx' |
28 | # Subset of http://nginx.org/en/docs/http/ngx_http_proxy_module.html |
29 | PROXY_CACHE_DEFAULTS = { |
30 | 'background-update': 'on', |
31 | @@ -23,8 +25,17 @@ class NginxConf: |
32 | def __init__(self, conf_path=None): |
33 | if not conf_path: |
34 | conf_path = NGINX_BASE_PATH |
35 | - self._conf_path = os.path.join(conf_path, 'conf.d') |
36 | - self._sites_path = os.path.join(conf_path, 'sites-available') |
37 | + self._base_path = conf_path |
38 | + self._conf_path = os.path.join(self.base_path, 'conf.d') |
39 | + self._sites_path = os.path.join(self.base_path, 'sites-available') |
40 | + script_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) |
41 | + self.jinja_env = jinja2.Environment(loader=jinja2.FileSystemLoader(script_dir)) |
42 | + |
43 | + # Expose base_path as a property to allow mocking in indirect calls to |
44 | + # this class. |
45 | + @property |
46 | + def base_path(self): |
47 | + return self._base_path |
48 | |
49 | # Expose conf_path as a property to allow mocking in indirect calls to |
50 | # this class. |
51 | @@ -103,13 +114,71 @@ class NginxConf: |
52 | data = { |
53 | 'address': conf['listen_address'], |
54 | 'cache_max_size': conf['cache_max_size'], |
55 | + 'enable_prometheus_metrics': conf['enable_prometheus_metrics'], |
56 | 'cache_path': conf['cache_path'], |
57 | 'locations': self._process_locations(conf['locations']), |
58 | 'name': self._generate_name(conf['site']), |
59 | 'port': conf['listen_port'], |
60 | 'site': conf['site'], |
61 | } |
62 | - base = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) |
63 | - env = jinja2.Environment(loader=jinja2.FileSystemLoader(base)) |
64 | - template = env.get_template('templates/nginx_cfg.tmpl') |
65 | + template = self.jinja_env.get_template('templates/nginx_cfg.tmpl') |
66 | return template.render(data) |
67 | + |
68 | + def _remove_metrics_site(self, available, enabled): |
69 | + """Remove the configuration exposing metrics. |
70 | + |
71 | + :param str available: Path of the "available" site exposing the metrics |
72 | + :param str enabled: Path of the "enabled" symlink to the "available" configuration |
73 | + :returns: True if any change was made, False otherwise |
74 | + :rtype: bool |
75 | + """ |
76 | + changed = False |
77 | + try: |
78 | + os.remove(available) |
79 | + changed = True |
80 | + except FileNotFoundError: |
81 | + pass |
82 | + try: |
83 | + os.remove(enabled) |
84 | + changed = True |
85 | + except FileNotFoundError: |
86 | + pass |
87 | + |
88 | + return changed |
89 | + |
90 | + def toggle_metrics_site(self, enable_prometheus_metrics): |
91 | + """Create/delete the metrics site configuration and links. |
92 | + |
93 | + :param bool enable_prometheus_metrics: True if metrics are exposed to prometheus |
94 | + :returns: True if any change was made, False otherwise |
95 | + :rtype: bool |
96 | + """ |
97 | + changed = False |
98 | + metrics_site_conf = '{0}.conf'.format(METRICS_SITE) |
99 | + available = os.path.join(self.sites_path, metrics_site_conf) |
100 | + enabled = os.path.join(self.base_path, 'sites-enabled', metrics_site_conf) |
101 | + # If no cache metrics, remove the site |
102 | + if not enable_prometheus_metrics: |
103 | + return self._remove_metrics_site(available, enabled) |
104 | + template = self.jinja_env.get_template('templates/nginx_metrics_cfg.tmpl') |
105 | + content = template.render({'nginx_conf_path': self.conf_path, 'port': METRICS_PORT}) |
106 | + # Check if contents changed |
107 | + try: |
108 | + with open(available, 'r', encoding='utf-8') as f: |
109 | + current = f.read() |
110 | + except FileNotFoundError: |
111 | + current = '' |
112 | + if content != current: |
113 | + with open(available, 'w', encoding='utf-8') as f: |
114 | + f.write(content) |
115 | + changed = True |
116 | + os.listdir(self.sites_path) |
117 | + if not os.path.exists(enabled): |
118 | + os.symlink(available, enabled) |
119 | + changed = True |
120 | + if os.path.realpath(available) != os.path.realpath(enabled): |
121 | + os.remove(enabled) |
122 | + os.symlink(available, enabled) |
123 | + changed = True |
124 | + |
125 | + return changed |
126 | diff --git a/nginx_config_rendered_test_output-nginx_metrics.txt b/nginx_config_rendered_test_output-nginx_metrics.txt |
127 | new file mode 100644 |
128 | index 0000000..a58a258 |
129 | --- /dev/null |
130 | +++ b/nginx_config_rendered_test_output-nginx_metrics.txt |
131 | @@ -0,0 +1,32 @@ |
132 | +proxy_cache_path /var/lib/nginx/proxy/nginx_metrics use_temp_path=off levels=1:2 keys_zone=nginx_metrics-cache:10m max_size=1g; |
133 | + |
134 | +server { |
135 | + server_name nginx_metrics; |
136 | + listen 127.0.0.1:6080; |
137 | + |
138 | + port_in_redirect off; |
139 | + absolute_redirect off; |
140 | + |
141 | + location / { |
142 | + proxy_pass http://localhost:8080; |
143 | + proxy_set_header Host "nginx_metrics"; |
144 | + proxy_cache nginx_metrics-cache; |
145 | + proxy_cache_background_update on; |
146 | + proxy_cache_lock on; |
147 | + proxy_cache_min_uses 1; |
148 | + proxy_cache_revalidate on; |
149 | + proxy_cache_use_stale error timeout updating http_500 http_502 http_503 http_504; |
150 | + proxy_cache_valid 200 1d; |
151 | + |
152 | + log_by_lua_block { |
153 | + http_request_total:inc(1, {ngx.var.server_name, ngx.var.status}) |
154 | + if ngx.var.upstream_cache_status == "HIT" then |
155 | + cache_request_hit_total:inc(1, {ngx.var.server_name}) |
156 | + end |
157 | + } |
158 | + } |
159 | + |
160 | + |
161 | + access_log /var/log/nginx/nginx_metrics-access.log content_cache; |
162 | + error_log /var/log/nginx/nginx_metrics-error.log; |
163 | +} |
164 | diff --git a/reactive/content_cache.py b/reactive/content_cache.py |
165 | index 03cd7bf..ae90822 100644 |
166 | --- a/reactive/content_cache.py |
167 | +++ b/reactive/content_cache.py |
168 | @@ -65,6 +65,32 @@ def service_start_or_restart(name): |
169 | host.service_start(name) |
170 | |
171 | |
172 | +def configure_nginx_metrics(ngx_conf, enable_prometheus_metrics): |
173 | + """Configure nginx to expose metrics. |
174 | + |
175 | + Create the dedicated server exposing the metrics and add the logging of the cache hits for the other sites. |
176 | + |
177 | + :param bool enable_prometheus_metrics: True is the metrics should be exposed, False otherwise |
178 | + :returns: True if any change was made, False otherwise |
179 | + :rtype: bool |
180 | + """ |
181 | + changed = False |
182 | + if copy_file('files/prometheus.lua', os.path.join(ngx_conf.conf_path, 'prometheus.lua')): |
183 | + changed = True |
184 | + if ngx_conf.toggle_metrics_site(enable_prometheus_metrics): |
185 | + changed = True |
186 | + old_ports = [int(port.split('/')[0]) for port in hookenv.opened_ports()] |
187 | + hookenv.log("Current opened ports: {}".format(old_ports)) |
188 | + if enable_prometheus_metrics and nginx.METRICS_PORT not in old_ports: |
189 | + hookenv.log("Opening port {0}".format(nginx.METRICS_PORT)) |
190 | + hookenv.open_port(nginx.METRICS_PORT, 'TCP') |
191 | + elif not enable_prometheus_metrics and nginx.METRICS_PORT in old_ports: |
192 | + hookenv.log("Closing port {0}".format(nginx.METRICS_PORT)) |
193 | + hookenv.close_port(nginx.METRICS_PORT, 'TCP') |
194 | + |
195 | + return changed |
196 | + |
197 | + |
198 | @reactive.when_not('content_cache.nginx.configured') |
199 | def configure_nginx(conf_path=None): |
200 | status.maintenance('setting up Nginx as caching layer') |
201 | @@ -76,6 +102,8 @@ def configure_nginx(conf_path=None): |
202 | status.blocked('requires list of sites to configure') |
203 | return |
204 | |
205 | + enable_prometheus_metrics = config.get('enable_prometheus_metrics', False) |
206 | + |
207 | ngx_conf = nginx.NginxConf(conf_path) |
208 | sites_secrets = secrets_from_config(config.get('sites_secrets')) |
209 | sites = sites_from_config(config.get('sites'), sites_secrets) |
210 | @@ -95,11 +123,19 @@ def configure_nginx(conf_path=None): |
211 | conf['site'] = site_conf.get('site-name') or site |
212 | conf['listen_port'] = site_conf['cache_port'] |
213 | conf['locations'] = site_conf.get('locations', {}) |
214 | + conf['enable_prometheus_metrics'] = enable_prometheus_metrics |
215 | |
216 | if ngx_conf.write_site(site, ngx_conf.render(conf)): |
217 | hookenv.log('Wrote out new configs for site: {}'.format(site)) |
218 | changed = True |
219 | |
220 | + if configure_nginx_metrics(ngx_conf, enable_prometheus_metrics): |
221 | + hookenv.log('nginx metrics exposed to prometheus') |
222 | + changed = True |
223 | + |
224 | + # Include the site exposing metrics if needed |
225 | + if enable_prometheus_metrics: |
226 | + sites[nginx.METRICS_SITE] = None |
227 | if ngx_conf.sync_sites(sites.keys()): |
228 | hookenv.log('Enabled sites: {}'.format(' '.join(sites.keys()))) |
229 | changed = True |
230 | diff --git a/templates/nginx_cfg.tmpl b/templates/nginx_cfg.tmpl |
231 | index 18d5488..031b3e0 100644 |
232 | --- a/templates/nginx_cfg.tmpl |
233 | +++ b/templates/nginx_cfg.tmpl |
234 | @@ -100,7 +100,15 @@ server { |
235 | args["token"] = nil |
236 | ngx.req.set_uri_args(args) |
237 | } |
238 | +{%- endif %} |
239 | +{%- if enable_prometheus_metrics %} |
240 | |
241 | + log_by_lua_block { |
242 | + http_request_total:inc(1, {ngx.var.server_name, ngx.var.status}) |
243 | + if ngx.var.upstream_cache_status == "HIT" then |
244 | + cache_request_hit_total:inc(1, {ngx.var.server_name}) |
245 | + end |
246 | + } |
247 | {%- endif %} |
248 | } |
249 | {% endfor %} |
250 | diff --git a/templates/nginx_metrics_cfg.tmpl b/templates/nginx_metrics_cfg.tmpl |
251 | new file mode 100644 |
252 | index 0000000..e048ffc |
253 | --- /dev/null |
254 | +++ b/templates/nginx_metrics_cfg.tmpl |
255 | @@ -0,0 +1,23 @@ |
256 | +lua_shared_dict prometheus_metrics 10M; |
257 | +lua_package_path "{{ nginx_conf_path }}/?.lua"; |
258 | +init_by_lua_block { |
259 | + prometheus = require("prometheus").init("prometheus_metrics") |
260 | + http_request_total = prometheus:counter( |
261 | + "nginx_http_request_total", "Number of HTTP requests per site", {"host", "status"}) |
262 | + cache_request_hit_total = prometheus:counter( |
263 | + "nginx_cache_request_hit_total", "Number of cache hits per site", {"host"}) |
264 | +} |
265 | + |
266 | +server { |
267 | + listen {{ port }}; |
268 | + allow all; |
269 | + location /metrics { |
270 | + content_by_lua_block { |
271 | + prometheus:collect() |
272 | + } |
273 | + } |
274 | + |
275 | + access_log /var/log/nginx/nginx_metrics-access.log; |
276 | + error_log /var/log/nginx/nginx_metrics-error.log; |
277 | +} |
278 | + |
279 | diff --git a/tests/unit/files/config_test_basic_config.txt b/tests/unit/files/config_test_basic_config.txt |
280 | new file mode 100644 |
281 | index 0000000..6707563 |
282 | --- /dev/null |
283 | +++ b/tests/unit/files/config_test_basic_config.txt |
284 | @@ -0,0 +1,6 @@ |
285 | +basic_site: |
286 | + port: 80 |
287 | + locations: |
288 | + /: |
289 | + backends: |
290 | + - 127.0.1.10:80 |
291 | diff --git a/tests/unit/files/nginx_config_rendered_test_output-basic_site.txt b/tests/unit/files/nginx_config_rendered_test_output-basic_site.txt |
292 | new file mode 100644 |
293 | index 0000000..90693e5 |
294 | --- /dev/null |
295 | +++ b/tests/unit/files/nginx_config_rendered_test_output-basic_site.txt |
296 | @@ -0,0 +1,32 @@ |
297 | +proxy_cache_path /var/lib/nginx/proxy/basic_site use_temp_path=off levels=1:2 keys_zone=basic_site-cache:10m max_size=1g; |
298 | + |
299 | +server { |
300 | + server_name basic_site; |
301 | + listen 127.0.0.1:6080; |
302 | + |
303 | + port_in_redirect off; |
304 | + absolute_redirect off; |
305 | + |
306 | + location / { |
307 | + proxy_pass http://localhost:8080; |
308 | + proxy_set_header Host "basic_site"; |
309 | + proxy_cache basic_site-cache; |
310 | + proxy_cache_background_update on; |
311 | + proxy_cache_lock on; |
312 | + proxy_cache_min_uses 1; |
313 | + proxy_cache_revalidate on; |
314 | + proxy_cache_use_stale error timeout updating http_500 http_502 http_503 http_504; |
315 | + proxy_cache_valid 200 1d; |
316 | + |
317 | + log_by_lua_block { |
318 | + http_request_total:inc(1, {ngx.var.server_name, ngx.var.status}) |
319 | + if ngx.var.upstream_cache_status == "HIT" then |
320 | + cache_request_hit_total:inc(1, {ngx.var.server_name}) |
321 | + end |
322 | + } |
323 | + } |
324 | + |
325 | + |
326 | + access_log /var/log/nginx/basic_site-access.log content_cache; |
327 | + error_log /var/log/nginx/basic_site-error.log; |
328 | +} |
329 | diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py |
330 | index 3ac3749..c071022 100644 |
331 | --- a/tests/unit/test_content_cache.py |
332 | +++ b/tests/unit/test_content_cache.py |
333 | @@ -6,6 +6,7 @@ import unittest |
334 | from unittest import mock |
335 | |
336 | import freezegun |
337 | +import jinja2 |
338 | |
339 | # We also need to mock up charms.layer so we can run unit tests without having |
340 | # to build the charm and pull in layers such as layer-status. |
341 | @@ -16,6 +17,7 @@ from charms.layer import status # NOQA: E402 |
342 | # Add path to where our reactive layer lives and import. |
343 | sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))) |
344 | from reactive import content_cache # NOQA: E402 |
345 | +from lib import nginx # NOQA: E402 |
346 | |
347 | |
348 | class TestCharm(unittest.TestCase): |
349 | @@ -131,14 +133,17 @@ class TestCharm(unittest.TestCase): |
350 | self.assertFalse(status.blocked.assert_called()) |
351 | self.assertFalse(clear_flag.assert_called_once_with('content_cache.active')) |
352 | |
353 | + @mock.patch('charmhelpers.core.hookenv.close_port') |
354 | + @mock.patch('charmhelpers.core.hookenv.opened_ports') |
355 | @mock.patch('reactive.content_cache.service_start_or_restart') |
356 | - def test_configure_nginx_sites(self, service_start_or_restart): |
357 | + def test_configure_nginx_sites(self, service_start_or_restart, opened_ports, close_port): |
358 | '''Test configuration of Nginx sites''' |
359 | with open('tests/unit/files/config_test_config.txt', 'r', encoding='utf-8') as f: |
360 | ngx_config = f.read() |
361 | self.mock_config.return_value = { |
362 | 'cache_max_size': '1g', |
363 | 'cache_path': '/var/lib/nginx/proxy', |
364 | + 'enable_prometheus_metrics': False, |
365 | 'sites': ngx_config, |
366 | } |
367 | |
368 | @@ -149,13 +154,18 @@ class TestCharm(unittest.TestCase): |
369 | os.mkdir(os.path.join(self.tmpdir, 'conf.d')) |
370 | os.mkdir(os.path.join(self.tmpdir, 'sites-available')) |
371 | os.mkdir(os.path.join(self.tmpdir, 'sites-enabled')) |
372 | + opened_ports.return_value = ['80/tcp', '{0}/tcp'.format(nginx.METRICS_PORT)] |
373 | content_cache.configure_nginx(self.tmpdir) |
374 | self.assertFalse(service_start_or_restart.assert_called_once_with('nginx')) |
375 | + close_port.assert_called_once_with(nginx.METRICS_PORT, 'TCP') |
376 | |
377 | # Re-run with same set of sites, no change so shouldn't need to restart Nginx |
378 | service_start_or_restart.reset_mock() |
379 | + close_port.reset_mock() |
380 | + opened_ports.return_value = ['80/tcp'] |
381 | content_cache.configure_nginx(self.tmpdir) |
382 | self.assertFalse(service_start_or_restart.assert_not_called()) |
383 | + close_port.assert_not_called() |
384 | |
385 | for site in [ |
386 | 'site1.local', |
387 | @@ -165,6 +175,7 @@ class TestCharm(unittest.TestCase): |
388 | 'site5', |
389 | 'site6.local', |
390 | 'site7.local', |
391 | + 'site8.local', |
392 | ]: |
393 | with open( |
394 | 'tests/unit/files/nginx_config_rendered_test_output-{}.txt'.format(site), 'r', encoding='utf-8' |
395 | @@ -183,8 +194,10 @@ class TestCharm(unittest.TestCase): |
396 | got = f.read() |
397 | self.assertEqual(got, want) |
398 | |
399 | + @mock.patch('charmhelpers.core.hookenv.close_port') |
400 | + @mock.patch('charmhelpers.core.hookenv.opened_ports') |
401 | @mock.patch('reactive.content_cache.service_start_or_restart') |
402 | - def test_configure_nginx_sites_secrets(self, service_start_or_restart): |
403 | + def test_configure_nginx_sites_secrets(self, service_start_or_restart, opened_ports, close_port): |
404 | with open('tests/unit/files/config_test_secrets.txt', 'r', encoding='utf-8') as f: |
405 | secrets = f.read() |
406 | config = ''' |
407 | @@ -227,9 +240,11 @@ site1.local: |
408 | got = f.read() |
409 | self.assertEqual(got, want) |
410 | |
411 | + @mock.patch('charmhelpers.core.hookenv.close_port') |
412 | + @mock.patch('charmhelpers.core.hookenv.opened_ports') |
413 | @mock.patch('shutil.disk_usage') |
414 | @mock.patch('reactive.content_cache.service_start_or_restart') |
415 | - def test_configure_nginx_cache_config(self, service_start_or_restart, disk_usage): |
416 | + def test_configure_nginx_cache_config(self, service_start_or_restart, disk_usage, opened_ports, close_port): |
417 | config = ''' |
418 | site1.local: |
419 | locations: |
420 | @@ -671,3 +686,72 @@ site1.local: |
421 | content='test content\n', group='somegroup', owner='somedude', path=dest, perms=444 |
422 | ) |
423 | ) |
424 | + |
425 | + @mock.patch('charmhelpers.core.hookenv.open_port') |
426 | + @mock.patch('charmhelpers.core.hookenv.opened_ports') |
427 | + @mock.patch('reactive.content_cache.service_start_or_restart') |
428 | + def test_configure_nginx_metrics_sites(self, service_start_or_restart, opened_ports, open_port): |
429 | + """Test configuration of Nginx sites with enable_prometheus_metrics activated.""" |
430 | + with open('tests/unit/files/config_test_basic_config.txt', 'r', encoding='utf-8') as f: |
431 | + ngx_config = f.read() |
432 | + self.mock_config.return_value = { |
433 | + 'cache_max_size': '1g', |
434 | + 'enable_prometheus_metrics': True, |
435 | + 'cache_path': '/var/lib/nginx/proxy', |
436 | + 'sites': ngx_config, |
437 | + } |
438 | + |
439 | + with mock.patch.multiple( |
440 | + 'lib.nginx.NginxConf', |
441 | + sites_path=os.path.join(self.tmpdir, 'sites-available'), |
442 | + base_path=self.tmpdir, |
443 | + ) as nginxconf_mock: # noqa: F841 |
444 | + # conf.d, sites-available, and sites-enabled won't exist in our |
445 | + # temporary directory. |
446 | + os.mkdir(os.path.join(self.tmpdir, 'conf.d')) |
447 | + os.mkdir(os.path.join(self.tmpdir, 'sites-available')) |
448 | + os.mkdir(os.path.join(self.tmpdir, 'sites-enabled')) |
449 | + opened_ports.return_value = ['80/tcp', '443/tcp'] |
450 | + content_cache.configure_nginx(self.tmpdir) |
451 | + self.assertFalse(service_start_or_restart.assert_called_once_with('nginx')) |
452 | + open_port.assert_called_once_with(nginx.METRICS_PORT, 'TCP') |
453 | + |
454 | + # Re-run with same set of sites, no change so shouldn't need to restart Nginx |
455 | + service_start_or_restart.reset_mock() |
456 | + open_port.reset_mock() |
457 | + opened_ports.return_value = ['80/tcp', '443/tcp', '{0}/tcp'.format(nginx.METRICS_PORT)] |
458 | + content_cache.configure_nginx(self.tmpdir) |
459 | + self.assertFalse(service_start_or_restart.assert_not_called()) |
460 | + open_port.assert_not_called() |
461 | + |
462 | + # Test the site with cache HIT logging |
463 | + site = 'basic_site' |
464 | + test_file = 'tests/unit/files/nginx_config_rendered_test_output-{0}.txt'.format(site) |
465 | + with open(test_file, 'r', encoding='utf-8') as f: |
466 | + want = f.read() |
467 | + |
468 | + test_file = os.path.join(self.tmpdir, 'sites-available/{0}.conf'.format(site)) |
469 | + with open(test_file, 'r', encoding='utf-8') as f: |
470 | + got = f.read() |
471 | + self.assertEqual(got, want) |
472 | + |
473 | + # Test the site exposing the metrics |
474 | + site = 'nginx_metrics' |
475 | + script_dir = os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..') |
476 | + jinja_env = jinja2.Environment(loader=jinja2.FileSystemLoader(script_dir)) |
477 | + template = jinja_env.get_template('templates/nginx_metrics_cfg.tmpl') |
478 | + content = template.render({ |
479 | + 'nginx_conf_path': os.path.join(self.tmpdir, 'conf.d'), |
480 | + 'port': nginx.METRICS_PORT}) |
481 | + want = content |
482 | + test_file = os.path.join(self.tmpdir, 'sites-available/{0}.conf'.format(nginx.METRICS_SITE)) |
483 | + with open(test_file, 'r', encoding='utf-8') as f: |
484 | + got = f.read() |
485 | + self.assertEqual(got, want) |
486 | + |
487 | + # Prometheus.lua library |
488 | + with open('files/prometheus.lua', 'r') as f: |
489 | + want = f.read() |
490 | + with open(os.path.join(self.tmpdir, 'conf.d', 'prometheus.lua'), 'r') as f: |
491 | + got = f.read() |
492 | + self.assertEqual(got, want) |
493 | diff --git a/tests/unit/test_nginx.py b/tests/unit/test_nginx.py |
494 | index e9ec2f1..7097f7e 100644 |
495 | --- a/tests/unit/test_nginx.py |
496 | +++ b/tests/unit/test_nginx.py |
497 | @@ -5,6 +5,8 @@ import tempfile |
498 | import unittest |
499 | import yaml |
500 | |
501 | +import jinja2 |
502 | + |
503 | sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))) |
504 | from lib import nginx # NOQA: E402 |
505 | |
506 | @@ -21,10 +23,13 @@ class TestLibNginx(unittest.TestCase): |
507 | |
508 | def test_nginx_config_conf_path(self): |
509 | conf_path = '/etc/nginx/conf.d' |
510 | + alternate_conf_path = '/var/lib/snap/nginx/etc' |
511 | ngx_conf = nginx.NginxConf() |
512 | self.assertEqual(ngx_conf.conf_path, conf_path) |
513 | ngx_conf = nginx.NginxConf(None) |
514 | self.assertEqual(ngx_conf.conf_path, conf_path) |
515 | + ngx_conf = nginx.NginxConf(alternate_conf_path) |
516 | + self.assertEqual(ngx_conf.base_path, alternate_conf_path) |
517 | |
518 | def test_nginx_config_sites_path(self): |
519 | sites_path = '/etc/nginx/sites-available' |
520 | @@ -34,7 +39,7 @@ class TestLibNginx(unittest.TestCase): |
521 | self.assertEqual(ngx_conf.sites_path, sites_path) |
522 | |
523 | def test_nginx_config_render(self): |
524 | - '''Test parsing a YAML-formatted list of sites''' |
525 | + """Test parsing a YAML-formatted list of sites.""" |
526 | ngx_conf = nginx.NginxConf() |
527 | |
528 | with open('tests/unit/files/config_test_config.txt', 'r', encoding='utf-8') as f: |
529 | @@ -42,6 +47,7 @@ class TestLibNginx(unittest.TestCase): |
530 | |
531 | conf = {} |
532 | conf['cache_max_size'] = '1g' |
533 | + conf['enable_prometheus_metrics'] = False |
534 | conf['cache_path'] = '/var/lib/nginx/proxy' |
535 | conf['listen_address'] = '127.0.0.1' |
536 | # From the given YAML-formatted list of sites, check that each individual |
537 | @@ -66,7 +72,7 @@ class TestLibNginx(unittest.TestCase): |
538 | self.assertEqual(ngx_conf.render(conf), output) |
539 | |
540 | def test_nginx_config_write_sites(self): |
541 | - '''Test writing out sites to individual Nginx site config files''' |
542 | + """Test writing out sites to individual Nginx site config files.""" |
543 | ngx_conf = nginx.NginxConf(self.tmpdir) |
544 | os.mkdir(os.path.join(self.tmpdir, 'sites-available')) |
545 | os.mkdir(os.path.join(self.tmpdir, 'sites-enabled')) |
546 | @@ -113,3 +119,78 @@ class TestLibNginx(unittest.TestCase): |
547 | |
548 | # Re-run, no change this time. |
549 | self.assertFalse(ngx_conf.sync_sites(['site1.local', 'site2.local'])) |
550 | + |
551 | + def test_nginx_config_render_with_metrics(self): |
552 | + """Test rendering with metrics exposed.""" |
553 | + ngx_conf = nginx.NginxConf() |
554 | + |
555 | + with open('tests/unit/files/config_test_basic_config.txt', 'r', encoding='utf-8') as f: |
556 | + sites = yaml.safe_load(f.read()) |
557 | + |
558 | + conf = { |
559 | + 'cache_max_size': '1g', |
560 | + 'cache_path': '/var/lib/nginx/proxy', |
561 | + 'listen_address': '127.0.0.1', |
562 | + 'enable_prometheus_metrics': True, |
563 | + } |
564 | + for site, site_conf in sites.items(): |
565 | + conf['site'] = site_conf.get('site-name') or site |
566 | + conf['listen_port'] = BASE_LISTEN_PORT |
567 | + conf['locations'] = site_conf.get('locations', {}) |
568 | + |
569 | + for location, loc_conf in conf['locations'].items(): |
570 | + if loc_conf.get('backends'): |
571 | + loc_conf['backend_port'] = BASE_BACKEND_PORT |
572 | + |
573 | + output_file = 'tests/unit/files/nginx_config_rendered_test_output-{}.txt'.format(site) |
574 | + with open(output_file, 'r', encoding='utf-8') as f: |
575 | + output = f.read() |
576 | + |
577 | + self.assertEqual(ngx_conf.render(conf), output) |
578 | + |
579 | + def test_nginx_config_toggle_metrics_site(self): |
580 | + """Test the metrics site. |
581 | + |
582 | + Check that the activation fo the cache metrics activate the dedicated site for exposing prometheus metrics. |
583 | + """ |
584 | + ngx_conf = nginx.NginxConf(self.tmpdir) |
585 | + os.mkdir(os.path.join(self.tmpdir, 'sites-available')) |
586 | + os.mkdir(os.path.join(self.tmpdir, 'sites-enabled')) |
587 | + |
588 | + metrics_site_conf = '{0}.conf'.format(nginx.METRICS_SITE) |
589 | + |
590 | + script_dir = os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..') |
591 | + env = jinja2.Environment(loader=jinja2.FileSystemLoader(script_dir)) |
592 | + template = env.get_template('templates/nginx_metrics_cfg.tmpl') |
593 | + content = template.render({ |
594 | + 'nginx_conf_path': os.path.join(self.tmpdir, 'conf.d'), |
595 | + 'port': nginx.METRICS_PORT}) |
596 | + nginx_metrics_conf = content |
597 | + |
598 | + metrics_site_available = os.path.join(self.tmpdir, 'sites-available', metrics_site_conf) |
599 | + metrics_site_enabled = os.path.join(self.tmpdir, 'sites-enabled', metrics_site_conf) |
600 | + self.assertTrue(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=True)) |
601 | + # Write again with same contents, this time it should return 'False' |
602 | + # as there's no change, thus no need to restart/reload Nginx. |
603 | + self.assertFalse(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=True)) |
604 | + self.assertTrue(os.path.exists(metrics_site_available)) |
605 | + |
606 | + # Compare what's been written out matches what's in tests/unit/files. |
607 | + with open(metrics_site_available, 'r', encoding='utf-8') as f: |
608 | + output = f.read() |
609 | + self.assertEqual(nginx_metrics_conf, output) |
610 | + |
611 | + # Check link existence |
612 | + self.assertTrue(os.path.islink(metrics_site_enabled)) |
613 | + |
614 | + # Mess up with the target of the link and check that a toggle fixes it |
615 | + os.remove(metrics_site_enabled) |
616 | + os.symlink('/dev/null', metrics_site_enabled) |
617 | + self.assertTrue(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=True)) |
618 | + self.assertTrue(os.path.realpath(metrics_site_available) == os.path.realpath(metrics_site_enabled)) |
619 | + |
620 | + # Remove the site |
621 | + self.assertTrue(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=False)) |
622 | + self.assertFalse(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=False)) |
623 | + self.assertFalse(os.path.exists(metrics_site_available)) |
624 | + self.assertFalse(os.path.exists(metrics_site_enabled)) |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.