Add volume for nginx cache

[glensc]

Use volume for writable data, as it's supposedly faster because no need to update overlayfs layers on volumes.

NOTE: nginx itself does chown nginx on startup according to my tests:

# docker-compose exec nginx ls -la /var/cache/nginx
total 0
drwxr-xr-x    1 root     root            98 Jun 12 17:05 .
drwxr-xr-x    1 root     root            19 Nov 13  2021 ..
drwxr-xr-x    2 nginx    root             6 Jun 12 17:05 client_temp
drwx------    2 nginx    root             6 Jun 12 17:05 fastcgi_temp
drwx------    2 nginx    root             6 Jun 12 17:05 proxy_temp
drwx------    2 nginx    root             6 Jun 12 17:05 scgi_temp
drwx------    2 nginx    root             6 Jun 12 17:05 uwsgi_temp

Refs:

• Logs flooded with client request body is buffered to a temporary file messages #1381 (comment)

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

[aminvakil]

Thanks for doing this!

I think sentry-nginx-cache is a more appropriate name for this volume and will reduce the effort later if we wanted to have another volume for a different part of our nginx.

Out of curiosity, is there a reason not to mount the entire /var/cache/nginx instead of only client_temp folder, even though it will not give us any value right now, it may do in future and also it's more suitable for our naming :)

[glensc]

Out of curiosity, is there a reason not to mount the entire /var/cache/nginx instead of only client_temp folder, even though it will not give us any value right now, it may do in future and also it's more suitable for our naming :)

My concern was that with VOLUME I was sure docker creates the empty directory, so with mounting the volume may end up missing dirs, but seems nginx creates (and sets permissions) the directories if they are missing:

root@docker2 sentry/21.12.0# docker-compose exec nginx ls -la /var/cache/nginx/
total 0
drwxr-xr-x    7 root     root            98 Jun 13 08:47 .
drwxr-xr-x    1 root     root            31 Nov 13  2021 ..
drwx------    2 nginx    root             6 Jun 13 08:47 client_temp
drwx------    2 nginx    root             6 Jun 13 08:47 fastcgi_temp
drwx------    2 nginx    root             6 Jun 13 08:47 proxy_temp
drwx------    2 nginx    root             6 Jun 13 08:47 scgi_temp
drwx------    2 nginx    root             6 Jun 13 08:47 uwsgi_temp

[glensc]

Changed to add volume for whole /var/cache/nginx.

[BYK]

I think it would be better to use tmpfs for this then: https://docs.docker.com/compose/compose-file/compose-file-v3/#tmpfs

[glensc]

not tmpfs, as if you want to keep in memory, set client_body_buffer_size:

• Logs flooded with client request body is buffered to a temporary file messages #1381 (comment)

this is for cases when client_body_buffer_size is exceeded and need to persist on disk.

[aminvakil]

@glensc client_body_buffer_size does not have to do anything with this, we can or cannot change the value of client_body_buffer_size by default in repository or manually.

But we can use tmpfs "for cases when client_body_buffer_size is exceeded and need to persist on disk" anyway, right?

[glensc]

@aminvakil client_body_buffer_size has everything to do with this. it's what the related issue is for:

• Logs flooded with client request body is buffered to a temporary file messages #1381

the disk will be used for requests larger than client_body_buffer_size, if you want to keep data in memory you increase that value. setting nginx to write to disk but then hacking kernel to use tmpfs seems odd approach if you can just change the client_body_buffer_size setting.

if the request is smaller than the buffer it's not written to disk at all but kept in memory:

• https://nginx.org/en/docs/http/ngx_http_core_module.html#client_body_buffer_size
• https://serverfault.com/a/511790/318658

[BYK]

I think @glensc has a good point there. Since the speed of tmpfs comes from it being based on the memory, using that here doesn't make much sense. Based on that I think this patch is good to go.

[aminvakil]

Aside from being tmpfs or not, this is definitely an improvement.

@glensc Thanks!

[chadwhitacre]

Thanks @glensc, merging, should go out in #1515 tomorrow. 👍