Greetings,
Let assume that I would like behavior on LB from the backend and force it to
cache only resposnes that have a X-No-Cache header with value NO.
Nginx should cache a response with any code, if it has such headers.
This works well until the backend is unavailable and nginx returns a
hardcoded 502 that doesn't have a control header, but such a response is
cached anyway.
Here is the config that allows to reproduce the issue:
http {
default_type application/octet-stream;
proxy_cache_path /tmp/nginx_cache keys_zone=the_zone:1m;
proxy_cache the_zone;
proxy_cache_valid any 15m;
proxy_cache_methods GET HEAD POST;
add_header X-Cache-Status $upstream_cache_status always;
map $upstream_http_x_no_cache $no_cache {
default 1;
"NO" 0;
}
proxy_no_cache $no_cache;
upstream echo {
server 127.127.127.127:80;
}
server {
listen 1234;
server_name localhost;
location / {
proxy_pass http://echo;
}
}
}
when I run:
curl -D - http://127.0.0.1:1234/
it returns MISS on the first request, and HIT on the second one.
Here I expect both requests to return MISS.
--
wbr, Kirill
Hello!
On Sun, Apr 07, 2024 at 01:36:21PM +0200, Kirill A. Korinsky wrote:
> Greetings,
>
> Let assume that I would like behavior on LB from the backend and force it to
> cache only resposnes that have a X-No-Cache header with value NO.
>
> Nginx should cache a response with any code, if it has such headers.
>
> This works well until the backend is unavailable and nginx returns a
> hardcoded 502 that doesn't have a control header, but such a response is
> cached anyway.
>
> Here is the config that allows to reproduce the issue:
>
> http {
> default_type application/octet-stream;
>
> proxy_cache_path /tmp/nginx_cache keys_zone=the_zone:1m;
> proxy_cache the_zone;
> proxy_cache_valid any 15m;
> proxy_cache_methods GET HEAD POST;
>
> add_header X-Cache-Status $upstream_cache_status always;
>
> map $upstream_http_x_no_cache $no_cache {
> default 1;
> "NO" 0;
> }
>
> proxy_no_cache $no_cache;
>
> upstream echo {
> server 127.127.127.127:80;
> }
>
> server {
> listen 1234;
> server_name localhost;
>
> location / {
> proxy_pass http://echo;
> }
> }
> }
>
> when I run:
>
> curl -D - http://127.0.0.1:1234/
>
> it returns MISS on the first request, and HIT on the second one.
>
> Here I expect both requests to return MISS.
Thanks for the report.
Indeed, proxy_no_cache is only checked for proper upstream
responses, but not when caching errors, including internally
generated 502/504 in ngx_http_upstream_finalize_request(), and
intercepted errors in ngx_http_upstream_intercept_errors().
Quick look suggests there will be also issues with caching errors
after proxy_cache_bypass (errors won't be cached even if they
should), as well as issues with proxy_cache_max_range_offset after
proxy_cache_bypass (it will be ignored).
This needs cleanup / fixes, added to my TODO list.
--
Maxim Dounin
http://mdounin.ru/
Hello Maxim,
Hope this helps.
We encounter disk failures fairly often and what the kernel will do most of
the time is re-mount the disk as read-only.
What I did was add this check which checks if the disk is healthy before
hand and executes the proxy_no_cache path if it is.
It doesn't cover all the possible disk failures, like sometimes you'll just
get IO errors and still return 5XX to clients. But to cover all cases a
much cleverer reshuffling of the code is needed.
diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
index d7f427d50..839ed6c0d 100644
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -8,6 +8,7 @@
#include <ngx_config.h>
#include <ngx_core.h>
#include <ngx_http.h>
+#include <sys/statvfs.h>
#include <ngx_http_proxy_module.h>
@@ -3424,6 +3425,16 @@ ngx_http_upstream_send_response(ngx_http_request_t
*r, ngx_http_upstream_t *u)
break;
default: /* NGX_OK */
+ if (r->cache) {
+ struct statvfs fs;
+ if (statvfs((char *)r->cache->file_cache->path->name.data,
&fs) == -1) {
+ return NGX_ERROR;
+ }
+ if ((fs.f_flag & ST_RDONLY) != 0) {
+ u->cacheable = 0;
+ break;
+ }
+ }
if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {
On Sun, Apr 7, 2024 at 4:56 PM Maxim Dounin <mdounin at mdounin.ru> wrote:
> Hello!
>
> On Sun, Apr 07, 2024 at 01:36:21PM +0200, Kirill A. Korinsky wrote:
>
> > Greetings,
> >
> > Let assume that I would like behavior on LB from the backend and force
> it to
> > cache only resposnes that have a X-No-Cache header with value NO.
> >
> > Nginx should cache a response with any code, if it has such headers.
> >
> > This works well until the backend is unavailable and nginx returns a
> > hardcoded 502 that doesn't have a control header, but such a response is
> > cached anyway.
> >
> > Here is the config that allows to reproduce the issue:
> >
> > http {
> > default_type application/octet-stream;
> >
> > proxy_cache_path /tmp/nginx_cache keys_zone=the_zone:1m;
> > proxy_cache the_zone;
> > proxy_cache_valid any 15m;
> > proxy_cache_methods GET HEAD POST;
> >
> > add_header X-Cache-Status $upstream_cache_status
> always;
> >
> > map $upstream_http_x_no_cache $no_cache {
> > default 1;
> > "NO" 0;
> > }
> >
> > proxy_no_cache $no_cache;
> >
> > upstream echo {
> > server 127.127.127.127:80;
> > }
> >
> > server {
> > listen 1234;
> > server_name localhost;
> >
> > location / {
> > proxy_pass http://echo;
> > }
> > }
> > }
> >
> > when I run:
> >
> > curl -D - http://127.0.0.1:1234/
> >
> > it returns MISS on the first request, and HIT on the second one.
> >
> > Here I expect both requests to return MISS.
>
> Thanks for the report.
>
> Indeed, proxy_no_cache is only checked for proper upstream
> responses, but not when caching errors, including internally
> generated 502/504 in ngx_http_upstream_finalize_request(), and
> intercepted errors in ngx_http_upstream_intercept_errors().
>
> Quick look suggests there will be also issues with caching errors
> after proxy_cache_bypass (errors won't be cached even if they
> should), as well as issues with proxy_cache_max_range_offset after
> proxy_cache_bypass (it will be ignored).
>
> This needs cleanup / fixes, added to my TODO list.
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx mailing list
> nginx at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx/attachments/20250130/f421123f/attachment.htm>
Hello!
On Thu, Jan 30, 2025 at 11:01:32AM +0200, Clima Gabriel wrote:
> Hello Maxim,
> Hope this helps.
> We encounter disk failures fairly often and what the kernel will do most of
> the time is re-mount the disk as read-only.
> What I did was add this check which checks if the disk is healthy before
> hand and executes the proxy_no_cache path if it is.
> It doesn't cover all the possible disk failures, like sometimes you'll just
> get IO errors and still return 5XX to clients. But to cover all cases a
> much cleverer reshuffling of the code is needed.
>
>
> diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> index d7f427d50..839ed6c0d 100644
> --- a/src/http/ngx_http_upstream.c
> +++ b/src/http/ngx_http_upstream.c
> @@ -8,6 +8,7 @@
> #include <ngx_config.h>
> #include <ngx_core.h>
> #include <ngx_http.h>
> +#include <sys/statvfs.h>
>
> #include <ngx_http_proxy_module.h>
>
> @@ -3424,6 +3425,16 @@ ngx_http_upstream_send_response(ngx_http_request_t
> *r, ngx_http_upstream_t *u)
> break;
>
> default: /* NGX_OK */
> + if (r->cache) {
> + struct statvfs fs;
> + if (statvfs((char *)r->cache->file_cache->path->name.data,
> &fs) == -1) {
> + return NGX_ERROR;
> + }
> + if ((fs.f_flag & ST_RDONLY) != 0) {
> + u->cacheable = 0;
> + break;
> + }
> + }
>
> if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {
I cannot say I like this approach.
Rather, I would recommend adding an explicit test via the
proxy_no_cache configuration directive if that's an expected state
in your setup. Right now this certainly can be done with the
embedded Perl module, but we can consider extending "if" with "-r"
and "-w" tests to make things easier.
Alternatively, we can consider handling (at least some) cache
access failures gracefully and ensure that we'll fall back to
normal proxying if this happens. This wasn't done previously to
keep the code simple, but can be reconsidered if there is an
understanding that this is quite important in some setups and
there is a simple enough way to handle such failures.
Please also note that right now even proxying won't work if
a temporary file is needed and cannot be created, as file access
failures are considered fatal.
OTOH, my personal preference is to keep disks mirrored, this
ensures that a single disk failure won't affect server operations
and provides better performance as a bonus.
(Note well that the code in question was modified in freenginx to
address the issue reported by Kirill in the thread you are
replying to, and your patch won't apply, see
https://freenginx.org/hg/nginx/rev/c5623963c29e for details.)
[...]
--
Maxim Dounin
http://mdounin.ru/
> my personal preference is to keep disks mirrored
Certainly, this is ideal, however Nginx is used by many businesses (cdn,
content providers, etc) whose margins are often too low to justify hardware
redundancy. And using cheap SSDs can have incredibly bad impact on
performance, as we've encountered certain ssds (870 QVO f.e.) that, at the
extremes, took (literally) seconds to return from open() / read() / write()
can be reconsidered if there is an understanding that this is quite
> important in some setups and there is a simple enough way to handle such
> failures
We'll need to implement this regardless so I'll, fingers crossed, be back
with updates.
Following are notes on my initial idea which proved too problematic to
pursue further, so feel free to skip.
Create the temp file ahead of time, before the proxy_no_cache check, and
take the proxy_no_cache code path if we fail to create the file.
Clearly not very elegant either as:
The temp file will need to be removed in the else of "if (p->cacheable) ..."
The temp file may be created successfully yet you may still encounter an IO
error a few nanoseconds later when you try to write to it.
There were several variants of this (diff file attached) which either
leaked tempfiles or broke proxy_no_cache.
On Fri, Jan 31, 2025 at 11:36 AM Maxim Dounin <mdounin at mdounin.ru> wrote:
> Hello!
>
> On Thu, Jan 30, 2025 at 11:01:32AM +0200, Clima Gabriel wrote:
>
> > Hello Maxim,
> > Hope this helps.
> > We encounter disk failures fairly often and what the kernel will do most
> of
> > the time is re-mount the disk as read-only.
> > What I did was add this check which checks if the disk is healthy before
> > hand and executes the proxy_no_cache path if it is.
> > It doesn't cover all the possible disk failures, like sometimes you'll
> just
> > get IO errors and still return 5XX to clients. But to cover all cases a
> > much cleverer reshuffling of the code is needed.
> >
> >
> > diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> > index d7f427d50..839ed6c0d 100644
> > --- a/src/http/ngx_http_upstream.c
> > +++ b/src/http/ngx_http_upstream.c
> > @@ -8,6 +8,7 @@
> > #include <ngx_config.h>
> > #include <ngx_core.h>
> > #include <ngx_http.h>
> > +#include <sys/statvfs.h>
> >
> > #include <ngx_http_proxy_module.h>
> >
> > @@ -3424,6 +3425,16 @@ ngx_http_upstream_send_response(ngx_http_request_t
> > *r, ngx_http_upstream_t *u)
> > break;
> >
> > default: /* NGX_OK */
> > + if (r->cache) {
> > + struct statvfs fs;
> > + if (statvfs((char *)r->cache->file_cache->path->name.data,
> > &fs) == -1) {
> > + return NGX_ERROR;
> > + }
> > + if ((fs.f_flag & ST_RDONLY) != 0) {
> > + u->cacheable = 0;
> > + break;
> > + }
> > + }
> >
> > if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {
>
> I cannot say I like this approach.
>
> Rather, I would recommend adding an explicit test via the
> proxy_no_cache configuration directive if that's an expected state
> in your setup. Right now this certainly can be done with the
> embedded Perl module, but we can consider extending "if" with "-r"
> and "-w" tests to make things easier.
>
> Alternatively, we can consider handling (at least some) cache
> access failures gracefully and ensure that we'll fall back to
> normal proxying if this happens. This wasn't done previously to
> keep the code simple, but can be reconsidered if there is an
> understanding that this is quite important in some setups and
> there is a simple enough way to handle such failures.
>
> Please also note that right now even proxying won't work if
> a temporary file is needed and cannot be created, as file access
> failures are considered fatal.
>
> OTOH, my personal preference is to keep disks mirrored, this
> ensures that a single disk failure won't affect server operations
> and provides better performance as a bonus.
>
> (Note well that the code in question was modified in freenginx to
> address the issue reported by Kirill in the thread you are
> replying to, and your patch won't apply, see
> https://freenginx.org/hg/nginx/rev/c5623963c29e for details.)
>
> [...]
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx mailing list
> nginx at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx/attachments/20250131/8012b338/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ideas_MISS_if_write_tempfile_failed.diff
Type: text/x-patch
Size: 3848 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx/attachments/20250131/8012b338/attachment.bin>