Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fastcgi caching #234

Merged
merged 2 commits into from
Jun 20, 2015
Merged

Fastcgi caching #234

merged 2 commits into from
Jun 20, 2015

Conversation

louim
Copy link
Contributor

@louim louim commented Jun 5, 2015

Hello, as mentioned in #186 this is a "microcaching" setup we have used in the past with Bedrock.

I have a couple of question about where should I go from now.

  • I implemented the cache globally, as in "enabled" or "disabled" for all sites on the server. Should it stays like that or should that be handled for each sites separately?
  • I initially toyed with the cache residing in /var/run since it is in memory instead of being on the filesystem. However I decided to scrap the idea, because it would require changing the Nginx init script to create the folder after each server reboot, since /var/run is cleared. What do you think?
  • Cache size: should this be a fixed size, a size based on the number of sites on the server, or just user configurable?
  • Should the cache be enabled by default? (I think it should)
  • Should this feature be documented in the Readme or the wiki?

Comments are welcome.

I will be following shortly with another pull request that aim to prevent website DDOS using 404s. That was one of the weak point we saw when using the cache.

set $skip_cache 0;

# POST requests and urls with a query string should always go to PHP
if ($request_method = POST) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your logic is backwards. The caching methods should be a whitelist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words: you should only be caching GET calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@austinpray You are totally right, in fact it is not even used as Nginx default cached method are only GET and HEAD. http://nginx.org/en/docs/http/ngx_http_fastcgi_module.html#fastcgi_cache_methods

I updated accordingly.

@swalkinshaw
Copy link
Member

@louim thanks for this. It's looking mostly good. For your questions:

  1. I'm okay with just making it globally enabled for now and not per-site.
  2. I think I'd prefer /var/run. I don't mind that the cache won't be persisted across restarts since it's just a temporary micro cache anyway. Does this directory really need to be created? I think Nginx just handles it automatically. But there's a bigger problem with /var/run. I think it's set to 10% of RAM which means that the Nginx cache would need to be set as less than that. So might just be best to avoid all this.
  3. Let's pick a good default and make it a variable. I'd probably go with less than 1GB though.
  4. I'll have to think about making it default.
  5. Mentioned in README as a feature + explained in Wiki I think.

@louim
Copy link
Contributor Author

louim commented Jun 6, 2015

@swalkinshaw thanks for the comments, I updated the PR to fix what you mentioned. Just to follow up on you answers:

  1. It would actually be easy to add as per site if that's what you think is best. Juste need to add a key in the config dict to turn it on, and tweak the global condition for the nginx.conf
  2. I would too prefer /var/run for performances reason. But I tested it and no folder are persisted in /var/run. Most Deamon create their own folders when starting. See http://askubuntu.com/questions/303120/how-folders-created-in-var-run-on-each-reboot. The solution would imply having the directory creation added to the init script. As you mentioned, the size is also an issue. For small VPS with 1GB RAM, this could be an issue as the cache will be really small.
  3. 250MB would be good I think. HTML doesn't really take much space.
  4. I actually pondered about it and I think it should not be enabled by default. Even if the cache is only a small period of time, It can make people not aware of it wonder why their modifications don't appear right away. Best leave the choice to the user to knowingly enable it.
  5. 👍

@swalkinshaw
Copy link
Member

@louim:

  1. per site would be better so let's do that 👍. Maybe the keys_zone should have the site name in it too?
  2. -
  3. 250MB is a good default
  4. Yep, agreed.

@louim
Copy link
Contributor Author

louim commented Jun 9, 2015

@swalkinshaw I made the config site specific. It required me to move the location ~ \.php$ block from the generic include to the site-specific file since I the content now change depending of the sites.

Settings can be used like :

...
ssl:
  enabled: false
cache:
  enabled: true
  duration: 30s
  skip_cache_uri: "some string here"
  skip_cache_cookie: "some other string here"
system_cron: true
...

All params will take the default global value when not specifically defined in a site. I also added fastcgi_cache_lock on; so only one request hit the backend when generating a new cache.

Just let me know if everything looks good to you, and Ill write the documentation and squash my commits.

As a side note, If we one day decide to compile NGINX from source (saw that mentioned in #235), it would be interesting to compile it with https://github.com/FRiCKLE/ngx_cache_purge, so the setup can be used with longer cache time. This would allow people to use plugin like https://wordpress.org/plugins/nginx-helper/ to purge their cache from the wordpress admin.

@swalkinshaw
Copy link
Member

@louim this looks great. Want to squash and update readme/wiki?

@louim
Copy link
Contributor Author

louim commented Jun 17, 2015

@swalkinshaw I'm unable to edit / create new page to the wiki. Would you mind opening the wiki for public edit or adding me as a collaborator so I can edit it?

@swalkinshaw
Copy link
Member

@louim it's public now

louim added 2 commits June 18, 2015 00:15
Make fastcgi_cache a template, since we have variables in it.

Correct indentation and add default parameter that can be overridden
@louim
Copy link
Contributor Author

louim commented Jun 18, 2015

@swalkinshaw Updated the wiki and the readme, rebased against the current master. Should be good to go! 👍

swalkinshaw added a commit that referenced this pull request Jun 20, 2015
@swalkinshaw swalkinshaw merged commit 67655aa into roots:master Jun 20, 2015
@swalkinshaw
Copy link
Member

Tested this out and it's working great. Thanks @louim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants