Add option to enable FastCGI background updates #962

bradleyess · 2018-03-09T02:09:33Z

Options in development,staging,production group_vars
Conditionally add parameter in NGINX site host file wordpress-site.conf.j2

swalkinshaw · 2018-03-09T02:38:13Z

roles/wordpress-setup/templates/wordpress-site.conf.j2

@@ -46,6 +47,11 @@ server {
    set $skip_cache 1;
  }

+  # Update an expired cache item in the background, serve stale cache to client.
+  {% if item.value.cache.enabled and item.value.cache.background_updates %}


background_update not background_updates

swalkinshaw · 2018-03-10T21:54:42Z

I wonder if there's any reason not to make this the default?

partounian · 2018-03-11T17:32:06Z

roles/wordpress-setup/templates/wordpress-site.conf.j2

@@ -46,6 +47,11 @@ server {
    set $skip_cache 1;
  }

+  # Update an expired cache item in the background, serve stale cache to client.
+  {% if item.value.cache.enabled and item.value.cache.background_update %}
+    fastcgi_cache_background_update on


forgot the semi-colon at the end

Also should this be in the cache_config block?

yeah I'd prefer this to be grouped in cache_config 👍

partounian · 2018-03-11T17:34:09Z

In what cases does it make sense to enable background_update?

swalkinshaw · 2018-03-11T17:55:00Z

The better question is the opposite to me.

Right now the cache is time based. Say it's 60s, and a client hits the site 60.1s after it was last cached. This would result in a cache miss and that client would incur the cost of a full request hitting WP.

With background updates, and the same scenario above, that client would still result in a cache hit, but the cache would be updated in the background. This means no actual client would ever incur a cache miss. They'd either get stale cached data, or the next cache hit. This is obviously preferable as long as it works as we'd expect. Meaning if we trust Nginx to properly update the cache in the background within a reasonable time frame, it's a better user experience with barely any more cache time added.

Here's a good article on this: https://siipo.la/blog/never-miss-the-cache-with-nginx-microcaching

partounian · 2018-03-11T18:35:00Z

Then according to that article should we also consider the following?

fastcgi_cache_lock on;
fastcgi_cache_use_stale updating;

Also, currently fastcgi_cache_valid is set to 30s, should we specify 200 301 302 404 and possibly increase caching to 1m?

swalkinshaw · 2018-03-11T18:37:44Z

We already set those.

Time is just a default, anyone can change it. Maybe we should only specific 200 though or make it a variable. I'm not really sure.

partounian · 2018-03-11T18:40:11Z

My mistake I only checked site config file and not the general nginx.conf

bradleyess · 2018-03-13T00:29:46Z

Updated PR with cache_config grouping and missing ;

I don't know where I stand on making this default behaviour or not, I think it could lead to some confusion - like I said in my initial suggestion - this isn't a perfect solution for all applications. Which makes me think it should be opt in behaviour.

partounian · 2018-03-13T17:56:00Z

roles/wordpress-setup/templates/wordpress-site.conf.j2

@@ -175,6 +176,11 @@ server {
    fastcgi_cache_bypass $skip_cache;
    fastcgi_no_cache $skip_cache;

+    # Update an expired cache item in the background, serve stale cache to client.
+    {% if item.value.cache.background_update %}
+    fastcgi_cache_background_update on


; is still missing

partounian · 2018-03-13T17:58:25Z

I vote for it being default behavior and replacing if item.value.cache.background_update with setting the value as {{ item.value.cache.background_update | default(true) }}. Even if we set it false we want an if check but a {{ value | default(false) }}

swalkinshaw · 2018-03-14T00:53:39Z

like I said in my initial suggestion - this isn't a perfect solution for all applications.

@InTheDeepEnd I'm still curious why you think this is the case. Is there a downside/con I'm missing?

bradleyess · 2018-03-14T01:27:49Z

@swalkinshaw - maybe I'm missing something actually, I guess this works in conjunction to whatever the cache expiry is in the group_vars, and that's set at the discretion/requirements of the project. +1 for default. My bad.

bradleyess · 2018-03-14T01:30:44Z

Set to default to true. :)

fullyint · 2018-03-14T04:25:37Z

@InTheDeepEnd This is great. Thank you!

It works as is, but more style opinions incoming! 😱

For the sake of consistency with existing Trellis caching options, I think there are two patterns this new fastcgi_cache_background_update option should follow.

Where defaults are defined. Notice that the cache options in the docs are absent from wordpress_sites (except enabled) which works because they are in the role defaults. This keeps wordpress_sites streamlined.

How defaults are defined. The cache directives in wordpress-site.conf.j2 follow this pattern:

directive {{ item.value.cache.directive | default(nginx_cache_directive) }};

A user could define nginx_cache_directive as a global override and/or set a per-site override by defining the cache.directive in wordpress_sites.

⭐ To be consistent with these patterns, could you...

remove background_update setting from wordpress_sites
add nginx_cache_background_update: on to Fastcgi cache params defaults

drop all changes from wordpress-site.conf.j2 (including the new blank lines, comments, and {% if %} blocks), leaving only the following:

  fastcgi_no_cache $skip_cache;
+ fastcgi_cache_background_update {{ item.value.cache.background_update | default(nginx_cache_background_update) }};

I'm inclined to drop the explanatory comment because no other Fastcgi cache settings have comments, but I don't feel too strongly about it.

I'm inclined to not add the extra blank lines because then the templated conf on the server would have double blank lines in some spots. Of course, it could be argued that the goal should be readability in the Trellis wordpress-site.conf.j2 template (hence your new blank lines) rather than perfect formatting in the conf on the server (the current focus).

http codes.

should we specify 200 301 302 404 and possibly increase caching to 1m? (comment above)

Looks like 200, 301, and 302 are covered by default (nginx docs). I don't know much about why and when to also include 404, but I'm inclined to continue with the default because desire for an alternative hasn't ever come up that I remember. Users could change it using child templates.

partounian · 2018-03-23T19:46:59Z

Hey @InTheDeepEnd any chance you can update this with the fixes? I would love for this to be merged.
If you are not able to, I wonder what is etiquette for me to apply these changes.

…onditional option in NGINX site host file.

swalkinshaw · 2018-05-21T03:29:39Z

Thanks everyone ❤️

tangrufus · 2018-05-21T10:46:55Z

Note that it is necessary to allow the usage of a stale cached response when it is being updated.

--- https://nginx.org/en/doc/http/ngx_http_fastcgi_module.html#fastcgi_cache_background_update

The blog post @swalkinshaw linked also updated about the issues with 404 pages on 17 May 2018.

Do we need to add fastcgi_cache_use_stale as well?

swalkinshaw · 2018-05-21T16:40:17Z

We already have fastcgi_cache_use_stale updating error timeout invalid_header http_500; in the base Nginx conf.

fastcgi_cache_use_stale updating should take care of background updates I assume?

fullyint · 2018-05-21T17:15:26Z

@tangrufus thanks for pointing out the article update regarding 404 pages. I haven't tested the need nor the solution, but the description makes sense to me. Do you envision a change like this?

- fastcgi_cache_valid {{ item.value.cache.duration | default(nginx_cache_duration) }};
+ fastcgi_cache_valid 200 301 302 404 {{ item.value.cache.duration | default(nginx_cache_duration) }};

Or we could do shorter expiration for 404s, but I doubt it makes a big difference:

  fastcgi_cache_valid {{ item.value.cache.duration | default(nginx_cache_duration) }};
+ fastcgi_cache_valid 404 1s;

tangrufus · 2018-05-21T18:24:39Z

I vote for the second solution because it could prevent troubles when nginx_cache_duration is a unexpectedly high value.

craigpearson · 2018-06-06T14:22:02Z

On one of our installs nginx was running on version 1.11.5, as this feature was added in 1.11.10 this causes an emergency log of:

nginx: [emerg] unknown directive "fastcgi_cache_background_update" in /etc/nginx/sites-enabled/example.com.conf:78

To update nginx manually you can run:

sudo add-apt-repository ppa:nginx/stable
sudo apt-get update
sudo apt-get install nginx

Not opening this as issue as I presume it's an edge case, but may be helpful for others

* Add xdebug.remote_autostart to simplify xdebug sessions * Update logrotate doc URL [ci skip] * Update WP-CLI to 1.5.1. * Update changelog. [ci skip] * Update geerlingguy.composer 1.6.1->1.7.0 (roots#983) Update from `1.6.1` -> `1.7.0` which addresses roots#943 ([DEPRECATION WARNING]: The use of 'include' for tasks has been deprecated.) * Update geerlingguy.ntp 1.5.2->1.6.0 (roots#984) Avoids deprecation warnings introduced in Ansible 2.4: "The use of 'include' for tasks has been deprecated." * Enable nginx to start on boot (roots#980) * update changelog * 'yarn run' -> 'yarn' [ci skip] * Issue warning for all Ubuntu releases that are not Xenial (roots#986) * Clarify that changelog entry indicates Trellis version (roots#987) * Validate python version on control machine (roots#988) * Common: Install `git` instead of `git-core` Because `git-core` is now a dummy package of `git`. See: http://git.661346.n2.nabble.com/git-core-vs-git-package-on-ubuntu-tp7576083p7576085.html * Add CSP frame-ancestors, make X-Frame-Options conditional (roots#977) The X-Frame-Options header has been obsoleted by the frame-ancestors directive. Retain the X-Frame-Options header for older browsers. Return empty X-Frame-Options header for WordPress Customizer content to prevent the conflict that SAMEORIGIN would have with the ALLOW-FROM option that WordPress adds on its own (Safari browser). Discussion in https://core.trac.wordpress.org/ticket/40020 * Improve failed_when rule for Wordpress Installed check (roots#991) In rare cases the wp_installed registered var may be missing the stderr attribute, so add a default to avoid related error. The `wp core is-installed` command return code is 1 if WP is simply not installed. However, in rare cases the command may return some other return code indicative of true failure, so fail if rc > 1. * deploy.sh: Return non-zero exit code when misuse (roots#990) - Exit with `127` when not enough arguments - Exit with `1` when hosts file not exist See: http://www.tldp.org/LDP/abs/html/exitcodes.html * Skip Acme Challenge failure message for non-failed sites (roots#993) * Bump Ansible version_tested_max to 2.5.3 (roots#981) * Bump Ansible version_tested_max to 2.5.3 Convert Jinja2 tests from filter format to `var is testname` format. Encourage users on Ansible 2.5.0 to upgrade to avoid erroneous warnings fixed in ansible/ansible 37538 * Add option to enable FastCGI background updates (roots#962) Enabled by default * Add quotes to nginx_cache_background_update value "on" Quotes prevent Ansible from interpolating the variable value as True. True is an invalid value for fastcgi_cache_background_update and would would make Nginx unable to reload. * Verify `wp-cli.phar` checksum

swalkinshaw reviewed Mar 9, 2018

View reviewed changes

partounian reviewed Mar 11, 2018

View reviewed changes

partounian reviewed Mar 13, 2018

View reviewed changes

Pls mentioned this pull request Apr 1, 2018

Advanced Nginx microcaching to improve cache HITs omega8cc/boa#1271

Closed

bradleyess and others added 4 commits May 20, 2018 16:12

Add option to enable FastCGI background updates in group vars + add c…

4a49a19

…onditional option in NGINX site host file.

Add option to enable FastCGI background updates in group vars + add c…

1dcc0cf

…onditional option in NGINX site host file.

Use Trellis convention for Nginx conf defaults

c6e4f2b

Add CHANGELOG entry

5d905b0

swalkinshaw merged commit eb20979 into roots:master May 21, 2018

devotoare mentioned this pull request May 23, 2018

Nginx Critical Error with fastcgi_cache_background_update #994

Closed

6 tasks

swalkinshaw mentioned this pull request Jun 1, 2018

Feature Suggestion : fastcgi_cache_background_update #961

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to enable FastCGI background updates #962

Add option to enable FastCGI background updates #962

bradleyess commented Mar 9, 2018

swalkinshaw Mar 9, 2018

swalkinshaw commented Mar 10, 2018

partounian Mar 11, 2018

partounian Mar 11, 2018

swalkinshaw Mar 11, 2018

partounian commented Mar 11, 2018

swalkinshaw commented Mar 11, 2018

partounian commented Mar 11, 2018

swalkinshaw commented Mar 11, 2018

partounian commented Mar 11, 2018

bradleyess commented Mar 13, 2018

partounian Mar 13, 2018

partounian commented Mar 13, 2018

swalkinshaw commented Mar 14, 2018

bradleyess commented Mar 14, 2018

bradleyess commented Mar 14, 2018

fullyint commented Mar 14, 2018

partounian commented Mar 23, 2018

swalkinshaw commented May 21, 2018

tangrufus commented May 21, 2018

swalkinshaw commented May 21, 2018

fullyint commented May 21, 2018

tangrufus commented May 21, 2018

craigpearson commented Jun 6, 2018 •

edited

Loading

Add option to enable FastCGI background updates #962

Add option to enable FastCGI background updates #962

Conversation

bradleyess commented Mar 9, 2018

swalkinshaw Mar 9, 2018

Choose a reason for hiding this comment

swalkinshaw commented Mar 10, 2018

partounian Mar 11, 2018

Choose a reason for hiding this comment

partounian Mar 11, 2018

Choose a reason for hiding this comment

swalkinshaw Mar 11, 2018

Choose a reason for hiding this comment

partounian commented Mar 11, 2018

swalkinshaw commented Mar 11, 2018

partounian commented Mar 11, 2018

swalkinshaw commented Mar 11, 2018

partounian commented Mar 11, 2018

bradleyess commented Mar 13, 2018

partounian Mar 13, 2018

Choose a reason for hiding this comment

partounian commented Mar 13, 2018

swalkinshaw commented Mar 14, 2018

bradleyess commented Mar 14, 2018

bradleyess commented Mar 14, 2018

fullyint commented Mar 14, 2018

partounian commented Mar 23, 2018

swalkinshaw commented May 21, 2018

tangrufus commented May 21, 2018

swalkinshaw commented May 21, 2018

fullyint commented May 21, 2018

tangrufus commented May 21, 2018

craigpearson commented Jun 6, 2018 • edited Loading

craigpearson commented Jun 6, 2018 •

edited

Loading