[master] 3dc2baed6 Loosen assertion on ctx->(req|bo), fix shard and vcl_pipe
Nils Goroll
nils.goroll at uplex.de
Mon Jul 6 16:30:08 UTC 2020
commit 3dc2baed643f7af63039b3c49d43083a5759b27f
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Mon Jul 6 17:15:03 2020 +0200
Loosen assertion on ctx->(req|bo), fix shard and vcl_pipe
in VRT_priv_task() we asserted that only one of ctx->req and ctx->bo is
set when not in vcl_pipe {}, but we also need to extend that assertion
to when ctx->method == 0 after vcl_pipe as finished because
VRT_priv_task() could be called from director resolution.
Being at it, I also noticed that our behavior in vcl_pipe {} is
inconsistent as, from the shard director perspective, it is a backend
method. So now, vcl_pipe {} is handled like vcl_backend_* {}.
We still need to make up our mind about #3329 / #3330 and depending on
the outcome we might need to touch some places again which were changed
in this commit.
Fixes #3361
diff --git a/bin/varnishd/cache/cache_vrt_priv.c b/bin/varnishd/cache/cache_vrt_priv.c
index bcc7bfca5..44692af56 100644
--- a/bin/varnishd/cache/cache_vrt_priv.c
+++ b/bin/varnishd/cache/cache_vrt_priv.c
@@ -134,8 +134,15 @@ VRT_priv_task(VRT_CTX, const void *vmod_id)
{
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+ /*
+ * XXX when coming from VRT_DirectorResolve() in pipe mode
+ * (ctx->method == 0), both req and bo are set.
+ * see #3329 #3330: we should make up our mind where
+ * pipe objects live
+ */
+
assert(ctx->req == NULL || ctx->bo == NULL ||
- ctx->method == VCL_MET_PIPE);
+ ctx->method == VCL_MET_PIPE || ctx->method == 0);
if (ctx->req) {
CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
diff --git a/bin/varnishtest/tests/d00029.vtc b/bin/varnishtest/tests/d00029.vtc
index 698f89e21..5bacdae45 100644
--- a/bin/varnishtest/tests/d00029.vtc
+++ b/bin/varnishtest/tests/d00029.vtc
@@ -1,16 +1,16 @@
varnishtest "shard director LAZY - d18.vtc"
-server s1 {
+server s1 -repeat 3 {
rxreq
txresp -body "ech3Ooj"
} -start
-server s2 {
+server s2 -repeat 3 {
rxreq
txresp -body "ieQu2qua"
} -start
-server s3 {
+server s3 -repeat 3 {
rxreq
txresp -body "xiuFi3Pe"
} -start
@@ -45,10 +45,13 @@ varnish v1 -vcl+backend {
}
sub vcl_recv {
+ if (req.http.pipe) {
+ return (pipe);
+ }
return(pass);
}
- sub vcl_backend_fetch {
+ sub shard_be {
set bereq.backend=vd.backend(resolve=LAZY);
if (bereq.url == "/1") {
@@ -64,6 +67,14 @@ varnish v1 -vcl+backend {
}
}
+ sub vcl_backend_fetch {
+ call shard_be;
+ }
+
+ sub vcl_pipe {
+ call shard_be;
+ }
+
sub vcl_backend_response {
set beresp.http.backend = bereq.backend;
}
@@ -85,4 +96,20 @@ client c1 {
rxresp
expect resp.body == "xiuFi3Pe"
expect resp.http.backend == "vd"
+
+ txreq -url /1 -hdr "pipe: true"
+ rxresp
+ expect resp.body == "ech3Ooj"
+} -run
+
+client c1 {
+ txreq -url /2 -hdr "pipe: true"
+ rxresp
+ expect resp.body == "ieQu2qua"
+} -run
+
+client c1 {
+ txreq -url /3 -hdr "pipe: true"
+ rxresp
+ expect resp.body == "xiuFi3Pe"
} -run
diff --git a/bin/varnishtest/tests/d00030.vtc b/bin/varnishtest/tests/d00030.vtc
index 3c29f33e7..912314b03 100644
--- a/bin/varnishtest/tests/d00030.vtc
+++ b/bin/varnishtest/tests/d00030.vtc
@@ -31,7 +31,7 @@ logexpect l2 -v v1 -g raw {
expect * 1001 VCL_Error {shard .backend param invalid}
} -start
logexpect l3 -v v1 -g raw {
- expect * 1003 VCL_Error {shard_param.set.. may only be used in vcl_init and in backend context}
+ expect * 1003 VCL_Error {shard_param.set.. may only be used in vcl_init and in backend/pipe context}
} -start
client c1 {
@@ -159,7 +159,7 @@ varnish v1 -errvcl {invalid warmup argument 1.1} {
}
}
-varnish v1 -errvcl {resolve=LAZY with other parameters can only be used in backend context} {
+varnish v1 -errvcl {resolve=LAZY with other parameters can only be used in backend/pipe context} {
import directors;
import blob;
diff --git a/doc/changes.rst b/doc/changes.rst
index b9d7e44ca..cfed269df 100644
--- a/doc/changes.rst
+++ b/doc/changes.rst
@@ -38,6 +38,9 @@ NEXT (scheduled 2020-09-15)
argument which allows to extend the effective privilege set of the
worker process.
+* The shard director and shard director parameter objects should now
+ work in ``vcl_pipe {}`` like in ``vcl_backend_* {}`` subs.
+
================================
Varnish Cache 6.4.0 (2020-03-16)
================================
diff --git a/lib/libvmod_directors/vmod_directors.vcc b/lib/libvmod_directors/vmod_directors.vcc
index 940b403cd..5238250f1 100644
--- a/lib/libvmod_directors/vmod_directors.vcc
+++ b/lib/libvmod_directors/vmod_directors.vcc
@@ -462,10 +462,11 @@ is _not_ the order given when backends are added.
* ``HASH``:
- * when called in backend context: Use the varnish hash value as
- set by ``vcl_hash{}``
+ * when called in backend context and in ``vcl_pipe {}``: Use the
+ varnish hash value as set by ``vcl_hash{}``
- * when called in client context: hash ``req.url``
+ * when called in client context other than ``vcl_pipe {}``: hash
+ ``req.url``
* ``URL``: hash req.url / bereq.url
@@ -556,9 +557,10 @@ is _not_ the order given when backends are added.
In ``vcl_init{}`` and on the client side, ``LAZY`` mode can not be
used with any other argument.
- On the backend side, parameters from arguments or an associated
- parameter set affect the shard director instance for the backend
- request irrespective of where it is referenced.
+ On the backend side and in ``vcl_pipe {}``, parameters from
+ arguments or an associated parameter set affect the shard director
+ instance for the backend request irrespective of where it is
+ referenced.
* *param*
@@ -607,10 +609,11 @@ Parameter sets have two scopes:
* per backend request scope
The per-VCL scope defines defaults for the per backend scope. Any
-changes to a parameter set in backend context only affect the
-respective backend request.
+changes to a parameter set in backend context and in ``vcl_pipe {}``
+only affect the respective backend request.
-Parameter sets can not be used in client context.
+Parameter sets can not be used in client context except for
+``vcl_pipe {}``.
The following example is a typical use case: A parameter set is
associated with several directors. Director choice happens on the
@@ -649,11 +652,11 @@ $Method VOID .clear()
Reset the parameter set to default values as documented for
`xshard.backend()`_.
-* in ``vcl_init{}``, resets the parameter set default for this VCL
-* in backend context, resets the parameter set for this backend
- request to the VCL defaults
+* in ``vcl_init{}``, resets the parameter set default for this VCL in
+* backend context and in ``vcl_pipe {}``, resets the parameter set for
+ this backend request to the VCL defaults
-This method may not be used in client context
+This method may not be used in client context other than ``vcl_pipe {}``.
$Method VOID .set(
[ ENUM {HASH, URL, KEY, BLOB} by ],
@@ -669,11 +672,11 @@ Change the given parameters of a parameter set as documented for
* in ``vcl_init{}``, changes the parameter set default for this VCL
-* in backend context, changes the parameter set for this backend
- request, keeping the defaults set for this VCL for unspecified
- arguments.
+* in backend context and in ``vcl_pipe {}``, changes the parameter set
+ for this backend request, keeping the defaults set for this VCL for
+ unspecified arguments.
-This method may not be used in client context
+This method may not be used in client context other than ``vcl_pipe {}``.
$Method STRING .get_by()
@@ -709,7 +712,7 @@ shard director using this parameter object would use. See
$Method BLOB .use()
-This method may only be used in backend context.
+This method may only be used in backend context and in ``vcl_pipe {}``.
For use with the *param* argument of `xshard.backend()`_
to associate this shard parameter set with a shard director.
diff --git a/lib/libvmod_directors/vmod_shard.c b/lib/libvmod_directors/vmod_shard.c
index 9779ae78d..fbcd1d132 100644
--- a/lib/libvmod_directors/vmod_shard.c
+++ b/lib/libvmod_directors/vmod_shard.c
@@ -170,6 +170,9 @@ vmod_shard_param_read(VRT_CTX, const void *id,
const struct vmod_directors_shard_param *p,
struct vmod_directors_shard_param *pstk, const char *who);
+// XXX #3329 #3330 revisit - for now, treat pipe like backend
+#define SHARD_VCL_TASK_REQ (VCL_MET_TASK_C & ~VCL_MET_PIPE)
+#define SHARD_VCL_TASK_BEREQ (VCL_MET_TASK_B | VCL_MET_PIPE)
/* -------------------------------------------------------------------------
* shard vmod interface
*/
@@ -617,14 +620,14 @@ vmod_shard_backend(VRT_CTX, struct vmod_directors_shard *vshard,
return (vshard->dir);
}
- if ((ctx->method & VCL_MET_TASK_B) == 0) {
+ if ((ctx->method & SHARD_VCL_TASK_BEREQ) == 0) {
VRT_fail(ctx, "shard .backend resolve=LAZY with other "
- "parameters can only be used in backend "
+ "parameters can only be used in backend/pipe "
"context");
return (NULL);
}
- assert(ctx->method & VCL_MET_TASK_B);
+ assert(ctx->method & SHARD_VCL_TASK_BEREQ);
pp = shard_param_task(ctx, vshard->shardd,
vshard->shardd->param);
@@ -918,11 +921,11 @@ shard_param_prep(VRT_CTX, struct vmod_directors_shard_param *p,
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
CHECK_OBJ_NOTNULL(p, VMOD_SHARD_SHARD_PARAM_MAGIC);
- if (ctx->method & VCL_MET_TASK_C) {
+ if (ctx->method & SHARD_VCL_TASK_REQ) {
VRT_fail(ctx, "%s may only be used "
- "in vcl_init and in backend context", who);
+ "in vcl_init and in backend/pipe context", who);
return (NULL);
- } else if (ctx->method & VCL_MET_TASK_B)
+ } else if (ctx->method & SHARD_VCL_TASK_BEREQ)
p = shard_param_task(ctx, p, p);
else
assert(ctx->method & VCL_MET_TASK_H);
@@ -967,7 +970,7 @@ vmod_shard_param_read(VRT_CTX, const void *id,
CHECK_OBJ_NOTNULL(p, VMOD_SHARD_SHARD_PARAM_MAGIC);
(void) who; // XXX
- if (ctx->method == 0 || (ctx->method & VCL_MET_TASK_B))
+ if (ctx->method == 0 || (ctx->method & SHARD_VCL_TASK_BEREQ))
p = shard_param_task(ctx, id, p);
if (p == NULL)
More information about the varnish-commit
mailing list