Merge ~hloeung/content-cache-charm:master into content-cache-charm:master

Proposed by Haw Loeung
Status: Merged
Approved by: Haw Loeung
Approved revision: 8b03e579d5b473f8f068fb4a1848a474ec4a5169
Merged at revision: 0d60ed4081322396ce2101e75ebf1eaa9cf706d2
Proposed branch: ~hloeung/content-cache-charm:master
Merge into: content-cache-charm:master
Diff against target: 146 lines (+27/-8)
7 files modified
config.yaml (+8/-0)
lib/nginx.py (+8/-1)
reactive/content_cache.py (+2/-1)
tests/unit/files/nginx_config_rendered_test_output-basic_site.txt (+2/-2)
tests/unit/files/nginx_config_rendered_test_output-token_site.txt (+2/-2)
tests/unit/test_content_cache.py (+2/-0)
tests/unit/test_nginx.py (+3/-2)
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Canonical IS Reviewers Pending
Review via email: mp+388277@code.launchpad.net

Commit message

Allow overriding the default proxy cache background update

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.

Revision history for this message
Stuart Bishop (stub) wrote :

Looks fine. Pedantic grammatical corrections inline.

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

Change successfully merged at revision 0d60ed4081322396ce2101e75ebf1eaa9cf706d2

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

This has already been landed, however the naming seems a bit backwards - IMO it would be better to have background_cache_updates (even if that defaults to true), rather than a disable_cache_background_update that defaults to false (even enable_background_cache_updates would at least be consistent with the following option).

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

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 03bf2eb..0a81a0d 100644
3--- a/config.yaml
4+++ b/config.yaml
5@@ -25,6 +25,14 @@ options:
6 default: "/var/lib/nginx/proxy"
7 description: >
8 Path or directory to store cached objects.
9+ disable_cache_background_update:
10+ default: false
11+ type: boolean
12+ description: >
13+ Default is to enable serving contents from the cache with a
14+ background subrequest to update objects in cache. This works for
15+ large objects, but can cause issues with sites updated recently
16+ to redirects that still have objects in the cache.
17 enable_prometheus_metrics:
18 default: true
19 type: boolean
20diff --git a/lib/nginx.py b/lib/nginx.py
21index ff58d60..74f3e58 100644
22--- a/lib/nginx.py
23+++ b/lib/nginx.py
24@@ -24,13 +24,16 @@ PROXY_CACHE_DEFAULTS = {
25
26
27 class NginxConf:
28- def __init__(self, conf_path=None, unit='content-cache'):
29+ def __init__(self, conf_path=None, unit='content-cache', disable_cache_bg_update=False):
30 if not conf_path:
31 conf_path = NGINX_BASE_PATH
32 self.unit = unit
33+
34 self._base_path = conf_path
35 self._conf_path = os.path.join(self.base_path, 'conf.d')
36+ self._disable_cache_bg_update = disable_cache_bg_update
37 self._sites_path = os.path.join(self.base_path, 'sites-available')
38+
39 script_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
40 self.jinja_env = jinja2.Environment(loader=jinja2.FileSystemLoader(script_dir))
41
42@@ -110,6 +113,10 @@ class NginxConf:
43 continue
44 lc.setdefault(cache_key, v)
45
46+ if self._disable_cache_bg_update:
47+ lc['cache-background-update'] = 'off'
48+ lc['cache-use-stale'] = lc['cache-use-stale'].replace('updating ', '')
49+
50 cache_val = self.proxy_cache_configs['valid']
51 # Backwards compatibility
52 if 'cache-validity' in lc:
53diff --git a/reactive/content_cache.py b/reactive/content_cache.py
54index 03d012c..6c6eeef 100644
55--- a/reactive/content_cache.py
56+++ b/reactive/content_cache.py
57@@ -140,7 +140,8 @@ def configure_nginx(conf_path=None):
58
59 enable_prometheus_metrics = config.get('enable_prometheus_metrics')
60
61- ngx_conf = nginx.NginxConf(conf_path, hookenv.local_unit())
62+ disable_cache_bg_update = config.get('disable_cache_background_update', False)
63+ ngx_conf = nginx.NginxConf(conf_path, hookenv.local_unit(), disable_cache_bg_update=disable_cache_bg_update)
64 sites_secrets = secrets_from_config(config.get('sites_secrets'))
65 blacklist_ports = [int(x.strip()) for x in config.get('blacklist_ports', '').split(',') if x.strip()]
66 sites = sites_from_config(config.get('sites'), sites_secrets, blacklist_ports=blacklist_ports)
67diff --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
68index 7add210..98a3042 100644
69--- a/tests/unit/files/nginx_config_rendered_test_output-basic_site.txt
70+++ b/tests/unit/files/nginx_config_rendered_test_output-basic_site.txt
71@@ -20,11 +20,11 @@ server {
72 add_header X-Cache-Status "$upstream_cache_status from mock-content-cache/0";
73 proxy_force_ranges on;
74 proxy_cache e176ab9d1f16-cache;
75- proxy_cache_background_update on;
76+ proxy_cache_background_update off;
77 proxy_cache_lock on;
78 proxy_cache_min_uses 1;
79 proxy_cache_revalidate on;
80- proxy_cache_use_stale error timeout updating http_500 http_502 http_503 http_504;
81+ proxy_cache_use_stale error timeout http_500 http_502 http_503 http_504;
82 proxy_cache_valid 200 1d;
83
84 access_by_lua_block {
85diff --git a/tests/unit/files/nginx_config_rendered_test_output-token_site.txt b/tests/unit/files/nginx_config_rendered_test_output-token_site.txt
86index 3eed240..21638c3 100644
87--- a/tests/unit/files/nginx_config_rendered_test_output-token_site.txt
88+++ b/tests/unit/files/nginx_config_rendered_test_output-token_site.txt
89@@ -20,11 +20,11 @@ server {
90 add_header X-Cache-Status "$upstream_cache_status from mock-content-cache/0";
91 proxy_force_ranges on;
92 proxy_cache c27ba6753fee-cache;
93- proxy_cache_background_update on;
94+ proxy_cache_background_update off;
95 proxy_cache_lock on;
96 proxy_cache_min_uses 1;
97 proxy_cache_revalidate on;
98- proxy_cache_use_stale error timeout updating http_500 http_502 http_503 http_504;
99+ proxy_cache_use_stale error timeout http_500 http_502 http_503 http_504;
100 proxy_cache_valid 200 1d;
101
102 access_by_lua_block {
103diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
104index 39f92c8..ad9964f 100644
105--- a/tests/unit/test_content_cache.py
106+++ b/tests/unit/test_content_cache.py
107@@ -192,6 +192,7 @@ class TestCharm(unittest.TestCase):
108 'cache_inactive_time': '2h',
109 'cache_max_size': '1g',
110 'cache_path': '/var/lib/nginx/proxy',
111+ 'disable_cache_background_update': False,
112 'enable_prometheus_metrics': False,
113 'sites': ngx_config,
114 'worker_connections': 768,
115@@ -1154,6 +1155,7 @@ site1.local:
116 'cache_inactive_time': '2h',
117 'cache_max_size': '1g',
118 'cache_path': '/var/lib/nginx/proxy',
119+ 'disable_cache_background_update': True,
120 'enable_prometheus_metrics': True,
121 'sites': ngx_config,
122 'worker_connections': 768,
123diff --git a/tests/unit/test_nginx.py b/tests/unit/test_nginx.py
124index 5123f6e..7f95ba8 100644
125--- a/tests/unit/test_nginx.py
126+++ b/tests/unit/test_nginx.py
127@@ -124,7 +124,7 @@ class TestLibNginx(unittest.TestCase):
128
129 def test_nginx_config_render_with_metrics(self):
130 """Test rendering with metrics exposed."""
131- ngx_conf = nginx.NginxConf(unit='mock-content-cache/0')
132+ ngx_conf = nginx.NginxConf(unit='mock-content-cache/0', disable_cache_bg_update=True)
133
134 with open('tests/unit/files/config_test_basic_config.txt', 'r', encoding='utf-8') as f:
135 sites = yaml.safe_load(f.read())
136@@ -133,8 +133,9 @@ class TestLibNginx(unittest.TestCase):
137 'cache_inactive_time': '2h',
138 'cache_max_size': '1g',
139 'cache_path': '/var/lib/nginx/proxy',
140- 'listen_address': '127.0.0.1',
141+ 'disable_cache_background_update': True,
142 'enable_prometheus_metrics': True,
143+ 'listen_address': '127.0.0.1',
144 }
145 for site, site_conf in sites.items():
146 conf['site'] = site

Subscribers

People subscribed via source and target branches