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

[Haw Loeung]

Allow overriding the default proxy cache background update

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

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

[Stuart Bishop (stub)]

Looks fine. Pedantic grammatical corrections inline.

review: Approve

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

Change successfully merged at revision 0d60ed4081322396ce2101e75ebf1eaa9cf706d2

[Joel Sing (jsing)]

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

[Haw Loeung (hloeung)]

MP with fixes:

| https://code.launchpad.net/~hloeung/content-cache-charm/+git/content-cache-charm/+merge/388285

diff --git a/config.yaml b/config.yaml
index 03bf2eb..0a81a0d 100644
--- a/config.yaml
+++ b/config.yaml
@@ -25,6 +25,14 @@ options:
     default: "/var/lib/nginx/proxy"
     description: >
       Path or directory to store cached objects.
+  disable_cache_background_update:
+    default: false
+    type: boolean
+    description: >
+      Default is to enable serving contents from the cache with a
+      background subrequest to update objects in cache. This works for
+      large objects, but can cause issues with sites updated recently
+      to redirects that still have objects in the cache.
   enable_prometheus_metrics:
     default: true
     type: boolean
diff --git a/lib/nginx.py b/lib/nginx.py
index ff58d60..74f3e58 100644
--- a/lib/nginx.py
+++ b/lib/nginx.py
@@ -24,13 +24,16 @@ PROXY_CACHE_DEFAULTS = {
 
 
 class NginxConf:
-    def __init__(self, conf_path=None, unit='content-cache'):
+    def __init__(self, conf_path=None, unit='content-cache', disable_cache_bg_update=False):
         if not conf_path:
             conf_path = NGINX_BASE_PATH
         self.unit = unit
+
         self._base_path = conf_path
         self._conf_path = os.path.join(self.base_path, 'conf.d')
+        self._disable_cache_bg_update = disable_cache_bg_update
         self._sites_path = os.path.join(self.base_path, 'sites-available')
+
         script_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
         self.jinja_env = jinja2.Environment(loader=jinja2.FileSystemLoader(script_dir))
 
@@ -110,6 +113,10 @@ class NginxConf:
                         continue
                     lc.setdefault(cache_key, v)
 
+                if self._disable_cache_bg_update:
+                    lc['cache-background-update'] = 'off'
+                    lc['cache-use-stale'] = lc['cache-use-stale'].replace('updating ', '')
+
                 cache_val = self.proxy_cache_configs['valid']
                 # Backwards compatibility
                 if 'cache-validity' in lc:
diff --git a/reactive/content_cache.py b/reactive/content_cache.py
index 03d012c..6c6eeef 100644
--- a/reactive/content_cache.py
+++ b/reactive/content_cache.py
@@ -140,7 +140,8 @@ def configure_nginx(conf_path=None):
 
     enable_prometheus_metrics = config.get('enable_prometheus_metrics')
 
-    ngx_conf = nginx.NginxConf(conf_path, hookenv.local_unit())
+    disable_cache_bg_update = config.get('disable_cache_background_update', False)
+    ngx_conf = nginx.NginxConf(conf_path, hookenv.local_unit(), disable_cache_bg_update=disable_cache_bg_update)
     sites_secrets = secrets_from_config(config.get('sites_secrets'))
     blacklist_ports = [int(x.strip()) for x in config.get('blacklist_ports', '').split(',') if x.strip()]
     sites = sites_from_config(config.get('sites'), sites_secrets, blacklist_ports=blacklist_ports)
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
index 7add210..98a3042 100644
--- a/tests/unit/files/nginx_config_rendered_test_output-basic_site.txt
+++ b/tests/unit/files/nginx_config_rendered_test_output-basic_site.txt
@@ -20,11 +20,11 @@ server {
         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 on;
+        proxy_cache_background_update off;
         proxy_cache_lock on;
         proxy_cache_min_uses 1;
         proxy_cache_revalidate on;
-        proxy_cache_use_stale error timeout updating http_500 http_502 http_503 http_504;
+        proxy_cache_use_stale error timeout http_500 http_502 http_503 http_504;
         proxy_cache_valid 200 1d;
 
         access_by_lua_block {
diff --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
index 3eed240..21638c3 100644
--- a/tests/unit/files/nginx_config_rendered_test_output-token_site.txt
+++ b/tests/unit/files/nginx_config_rendered_test_output-token_site.txt
@@ -20,11 +20,11 @@ server {
         add_header X-Cache-Status "$upstream_cache_status from mock-content-cache/0";
         proxy_force_ranges on;
         proxy_cache c27ba6753fee-cache;
-        proxy_cache_background_update on;
+        proxy_cache_background_update off;
         proxy_cache_lock on;
         proxy_cache_min_uses 1;
         proxy_cache_revalidate on;
-        proxy_cache_use_stale error timeout updating http_500 http_502 http_503 http_504;
+        proxy_cache_use_stale error timeout http_500 http_502 http_503 http_504;
         proxy_cache_valid 200 1d;
 
         access_by_lua_block {
diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
index 39f92c8..ad9964f 100644
--- a/tests/unit/test_content_cache.py
+++ b/tests/unit/test_content_cache.py
@@ -192,6 +192,7 @@ class TestCharm(unittest.TestCase):
             'cache_inactive_time': '2h',
             'cache_max_size': '1g',
             'cache_path': '/var/lib/nginx/proxy',
+            'disable_cache_background_update': False,
             'enable_prometheus_metrics': False,
             'sites': ngx_config,
             'worker_connections': 768,
@@ -1154,6 +1155,7 @@ site1.local:
             'cache_inactive_time': '2h',
             'cache_max_size': '1g',
             'cache_path': '/var/lib/nginx/proxy',
+            'disable_cache_background_update': True,
             'enable_prometheus_metrics': True,
             'sites': ngx_config,
             'worker_connections': 768,
diff --git a/tests/unit/test_nginx.py b/tests/unit/test_nginx.py
index 5123f6e..7f95ba8 100644
--- a/tests/unit/test_nginx.py
+++ b/tests/unit/test_nginx.py
@@ -124,7 +124,7 @@ class TestLibNginx(unittest.TestCase):
 
     def test_nginx_config_render_with_metrics(self):
         """Test rendering with metrics exposed."""
-        ngx_conf = nginx.NginxConf(unit='mock-content-cache/0')
+        ngx_conf = nginx.NginxConf(unit='mock-content-cache/0', disable_cache_bg_update=True)
 
         with open('tests/unit/files/config_test_basic_config.txt', 'r', encoding='utf-8') as f:
             sites = yaml.safe_load(f.read())
@@ -133,8 +133,9 @@ class TestLibNginx(unittest.TestCase):
             'cache_inactive_time': '2h',
             'cache_max_size': '1g',
             'cache_path': '/var/lib/nginx/proxy',
-            'listen_address': '127.0.0.1',
+            'disable_cache_background_update': True,
             'enable_prometheus_metrics': True,
+            'listen_address': '127.0.0.1',
         }
         for site, site_conf in sites.items():
             conf['site'] = site