[prev in list] [next in list] [prev in thread] [next in thread]
List: varnish-dev
Subject: Re: [PATCH] VCL temperature improvements
From: Dridi Boukelmoune <dridi () varni ! sh>
Date: 2015-12-10 15:30:42
Message-ID: CABoVN9BYw+9qz3KY=wUhX=JOPrYs8tADLMjmmAg+C3X-=+xv3Q () mail ! gmail ! com
[Download RAW message or body]
> Hi,
>
> Here is a new batch of patches fix changes requested from a first
> review. There's one more commit this time, it's number 3.
One more update for the first review, I forgot to update patch number 1.
["0001-Turn-VCL-state-magic-numbers-into-an-enum.patch" (text/x-patch)]
From 9b7b2b320b64fb45d90c340b050a144feeefccbb Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Fri, 4 Dec 2015 09:48:11 +0100
Subject: [PATCH 1/8] Turn VCL state magic numbers into an enum
Make a clear distinction between (struct vclprog).warm that is used as a
boolean, unlike the second parameter of mgt_vcl_setstate. We don't use
an actual C enum but const char[] to make debugging easier.
---
bin/varnishd/mgt/mgt_vcl.c | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c
index 37359f2..583f478 100644
--- a/bin/varnishd/mgt/mgt_vcl.c
+++ b/bin/varnishd/mgt/mgt_vcl.c
@@ -48,11 +48,15 @@
#include "mgt_cli.h"
+static const char * const vcl_state_cold = "cold";
+static const char * const vcl_state_warm = "warm";
+static const char * const vcl_state_auto = "auto";
+
struct vclprog {
VTAILQ_ENTRY(vclprog) list;
char *name;
char *fname;
- int warm;
+ unsigned warm;
char state[8];
double go_cold;
};
@@ -120,21 +124,24 @@ mgt_has_vcl(void)
}
static void
-mgt_vcl_setstate(struct vclprog *vp, int warm)
+mgt_vcl_setstate(struct vclprog *vp, const char *vs)
{
- unsigned status;
+ unsigned status, warm;
double now;
char *p;
- if (warm == -1) {
+ if (vs == vcl_state_auto) {
assert(vp != active_vcl);
now = VTIM_mono();
- warm = vp->warm;
+ vs = vp->warm ? vcl_state_warm : vcl_state_cold;
if (vp->go_cold > 0 && !strcmp(vp->state, "auto") &&
vp->go_cold + mgt_param.vcl_cooldown < now)
- warm = 0;
+ vs = vcl_state_cold;
}
+ assert(vs != vcl_state_auto);
+ warm = vs == vcl_state_warm ? 1 : 0;
+
if (vp->warm == warm)
return;
@@ -230,7 +237,7 @@ mgt_push_vcls_and_start(unsigned *status, char **p)
struct vclprog *vp;
AN(active_vcl);
- mgt_vcl_setstate(active_vcl, 1);
+ mgt_vcl_setstate(active_vcl, vcl_state_warm);
VTAILQ_FOREACH(vp, &vclhead, list) {
if (mgt_cli_askchild(status, p, "vcl.load \"%s\" %s %d%s\n",
vp->name, vp->fname, vp->warm, vp->state))
@@ -321,7 +328,7 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv)
bprintf(vp->state, "%s", "auto");
if (vp != active_vcl) {
vp->go_cold = VTIM_mono();
- mgt_vcl_setstate(vp, -1);
+ mgt_vcl_setstate(vp, vcl_state_auto);
}
} else if (!strcmp(av[3], "cold")) {
if (vp == active_vcl) {
@@ -330,10 +337,10 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv)
return;
}
bprintf(vp->state, "%s", "auto");
- mgt_vcl_setstate(vp, 0);
+ mgt_vcl_setstate(vp, vcl_state_cold);
} else if (!strcmp(av[3], "warm")) {
bprintf(vp->state, "%s", av[3]);
- mgt_vcl_setstate(vp, 1);
+ mgt_vcl_setstate(vp, vcl_state_warm);
} else {
VCLI_Out(cli, "State must be one of auto, cold or warm.");
VCLI_SetResult(cli, CLIS_PARAM);
@@ -353,20 +360,20 @@ mcf_vcl_use(struct cli *cli, const char * const *av, void *priv)
return;
if (vp == active_vcl)
return;
- mgt_vcl_setstate(vp, 1);
+ mgt_vcl_setstate(vp, vcl_state_warm);
if (child_pid >= 0 &&
mgt_cli_askchild(&status, &p, "vcl.use %s\n", av[2])) {
VCLI_SetResult(cli, status);
VCLI_Out(cli, "%s", p);
vp->go_cold = VTIM_mono();
- mgt_vcl_setstate(vp, -1);
+ mgt_vcl_setstate(vp, vcl_state_auto);
} else {
VCLI_Out(cli, "VCL '%s' now active", av[2]);
vp2 = active_vcl;
active_vcl = vp;
if (vp2 != NULL) {
vp2->go_cold = VTIM_mono();
- mgt_vcl_setstate(vp2, -1);
+ mgt_vcl_setstate(vp2, vcl_state_auto);
}
}
free(p);
@@ -388,7 +395,7 @@ mcf_vcl_discard(struct cli *cli, const char * const *av, void *priv)
VCLI_Out(cli, "Cannot discard active VCL program\n");
return;
}
- mgt_vcl_setstate(vp, 0);
+ mgt_vcl_setstate(vp, vcl_state_cold);
if (child_pid >= 0) {
/* If this fails the child is crashing, figure that later */
(void)mgt_cli_askchild(&status, &p, "vcl.discard %s\n", av[2]);
@@ -434,7 +441,7 @@ mgt_vcl_poker(const struct vev *e, int what)
e_poker->timeout = mgt_param.vcl_cooldown * .45;
VTAILQ_FOREACH(vp, &vclhead, list) {
if (vp != active_vcl)
- mgt_vcl_setstate(vp, -1);
+ mgt_vcl_setstate(vp, vcl_state_auto);
}
return (0);
}
--
2.4.3
["0002-Make-vcl_set_state-accept-a-ctx-instead-of-a-vcl.patch" (text/x-patch)]
From 3a47a058c6293f837ae003750d57e0b7559881f9 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Fri, 4 Dec 2015 11:14:29 +0100
Subject: [PATCH 2/8] Make vcl_set_state accept a ctx instead of a vcl
This paves the way towards failing VCL_EVENT_WARM events in VMODs. If
setting the temperature fails, we may need to carry a message back to
the CLI and other bits of context.
---
bin/varnishd/cache/cache_vcl.c | 43 +++++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index 54effd1..d26c9af 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -429,17 +429,16 @@ vcl_find(const char *name)
}
static void
-vcl_set_state(struct vcl *vcl, const char *state)
+vcl_set_state(VRT_CTX, const char *state)
{
- struct vrt_ctx ctx;
- unsigned hand = 0;
+ struct vcl *vcl;
ASSERT_CLI();
- AN(vcl->temp);
+ CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+ AN(ctx->handling);
- INIT_OBJ(&ctx, VRT_CTX_MAGIC);
- ctx.handling = &hand;
- ctx.vcl = vcl;
+ vcl = ctx->vcl;
+ AN(vcl->temp);
switch(state[0]) {
case '0':
@@ -449,7 +448,7 @@ vcl_set_state(struct vcl *vcl, const char *state)
vcl->temp = vcl->refcount ? vcl_temp_cooling :
vcl_temp_cold;
- AZ(vcl->conf->event_vcl(&ctx, VCL_EVENT_COLD));
+ AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
vcl_BackendEvent(vcl, VCL_EVENT_COLD);
}
else if (vcl->busy)
@@ -466,7 +465,7 @@ vcl_set_state(struct vcl *vcl, const char *state)
/* The VCL must first reach a stable cold state */
else if (vcl->temp != vcl_temp_cooling) {
vcl->temp = vcl_temp_warm;
- (void)vcl->conf->event_vcl(&ctx, VCL_EVENT_WARM);
+ (void)vcl->conf->event_vcl(ctx, VCL_EVENT_WARM);
vcl_BackendEvent(vcl, VCL_EVENT_WARM);
}
break;
@@ -529,7 +528,7 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
return (1);
}
VSB_delete(vsb);
- vcl_set_state(vcl, state);
+ vcl_set_state(&ctx, state);
bprintf(vcl->state, "%s", state + 1);
assert(hand == VCL_RET_OK);
VCLI_Out(cli, "Loaded \"%s\" as \"%s\"", fn , name);
@@ -577,13 +576,19 @@ VCL_Nuke(struct vcl *vcl)
void
VCL_Poll(void)
{
+ struct vrt_ctx ctx;
struct vcl *vcl, *vcl2;
+ unsigned hand;
ASSERT_CLI();
VTAILQ_FOREACH_SAFE(vcl, &vcl_head, list, vcl2) {
if (vcl->temp == vcl_temp_busy ||
- vcl->temp == vcl_temp_cooling)
- vcl_set_state(vcl, "0");
+ vcl->temp == vcl_temp_cooling) {
+ INIT_OBJ(&ctx, VRT_CTX_MAGIC);
+ ctx.vcl = vcl;
+ ctx.handling = &hand;
+ vcl_set_state(&ctx, "0");
+ }
if (vcl->discard && vcl->temp == vcl_temp_cold)
VCL_Nuke(vcl);
}
@@ -625,17 +630,21 @@ ccf_config_load(struct cli *cli, const char * const *av, void *priv)
static void __match_proto__(cli_func_t)
ccf_config_state(struct cli *cli, const char * const *av, void *priv)
{
- struct vcl *vcl;
+ struct vrt_ctx ctx;
+ unsigned hand;
+
+ INIT_OBJ(&ctx, VRT_CTX_MAGIC);
+ ctx.handling = &hand;
(void)cli;
AZ(priv);
ASSERT_CLI();
AN(av[2]);
AN(av[3]);
- vcl = vcl_find(av[2]);
- AN(vcl); // MGT ensures this
- vcl_set_state(vcl, av[3]);
- bprintf(vcl->state, "%s", av[3] + 1);
+ ctx.vcl = vcl_find(av[2]);
+ AN(ctx.vcl); // MGT ensures this
+ vcl_set_state(&ctx, av[3]);
+ bprintf(ctx.vcl->state, "%s", av[3] + 1);
}
static void __match_proto__(cli_func_t)
--
2.4.3
["0003-Wrap-VCL-event-calls-in-a-dedicated-function.patch" (text/x-patch)]
From a3d21158b95fbd7a6c4a840e82f61e9e62b1f2f7 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Thu, 10 Dec 2015 11:44:05 +0100
Subject: [PATCH 3/8] Wrap VCL event calls in a dedicated function
---
bin/varnishd/cache/cache_vcl.c | 45 ++++++++++++++++++++++++++++++++++++------
1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index d26c9af..9aa1259 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -428,6 +428,39 @@ vcl_find(const char *name)
return (NULL);
}
+static int
+vcl_event(VRT_CTX, enum vcl_event_e ev)
+{
+ int retval;
+
+ CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+ AN(ctx->handling);
+ AN(ctx->vcl);
+
+ retval = ctx->vcl->conf->event_vcl(ctx, ev);
+
+ switch (ev) {
+ case VCL_EVENT_LOAD:
+ AN(ctx->msg);
+ /* fall through */
+ case VCL_EVENT_WARM:
+ case VCL_EVENT_USE:
+ break;
+#define EV(event, message) \
+ case event: \
+ if (retval) \
+ WRONG("A VMOD failed the VCL " message); \
+ break;
+ EV(VCL_EVENT_COLD, "cool down");
+ EV(VCL_EVENT_DISCARD, "discard");
+#undef EV
+ default:
+ WRONG("Unexpected VCL event");
+ }
+
+ return (retval);
+}
+
static void
vcl_set_state(VRT_CTX, const char *state)
{
@@ -448,7 +481,7 @@ vcl_set_state(VRT_CTX, const char *state)
vcl->temp = vcl->refcount ? vcl_temp_cooling :
vcl_temp_cold;
- AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
+ (void)vcl_event(ctx, VCL_EVENT_COLD);
vcl_BackendEvent(vcl, VCL_EVENT_COLD);
}
else if (vcl->busy)
@@ -465,7 +498,7 @@ vcl_set_state(VRT_CTX, const char *state)
/* The VCL must first reach a stable cold state */
else if (vcl->temp != vcl_temp_cooling) {
vcl->temp = vcl_temp_warm;
- (void)vcl->conf->event_vcl(ctx, VCL_EVENT_WARM);
+ (void)vcl_event(ctx, VCL_EVENT_WARM);
vcl_BackendEvent(vcl, VCL_EVENT_WARM);
}
break;
@@ -515,13 +548,13 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
VSB_clear(vsb);
ctx.msg = vsb;
- i = vcl->conf->event_vcl(&ctx, VCL_EVENT_LOAD);
+ i = vcl_event(&ctx, VCL_EVENT_LOAD);
AZ(VSB_finish(vsb));
if (i) {
VCLI_Out(cli, "VCL \"%s\" Failed initialization", name);
if (VSB_len(vsb))
VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(vsb));
- AZ(vcl->conf->event_vcl(&ctx, VCL_EVENT_DISCARD));
+ (void)vcl_event(&ctx, VCL_EVENT_DISCARD);
vcl_KillBackends(vcl);
VCL_Close(&vcl);
VSB_delete(vsb);
@@ -563,7 +596,7 @@ VCL_Nuke(struct vcl *vcl)
ctx.method = VCL_MET_FINI;
ctx.handling = &hand;
ctx.vcl = vcl;
- AZ(vcl->conf->event_vcl(&ctx, VCL_EVENT_DISCARD));
+ (void)vcl_event(&ctx, VCL_EVENT_DISCARD);
vcl_KillBackends(vcl);
free(vcl->loaded_name);
VCL_Close(&vcl);
@@ -688,7 +721,7 @@ ccf_config_use(struct cli *cli, const char * const *av, void *priv)
AN(vsb);
ctx.msg = vsb;
ctx.vcl = vcl;
- i = vcl->conf->event_vcl(&ctx, VCL_EVENT_USE);
+ i = vcl_event(&ctx, VCL_EVENT_USE);
AZ(VSB_finish(vsb));
if (i) {
VCLI_Out(cli, "VCL \"%s\" Failed to activate", av[2]);
--
2.4.3
["0004-Allow-VMODs-to-fail-a-warm-up.patch" (text/x-patch)]
From 8f8c8aae347df5b549418836026d26553cfb8b67 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Fri, 4 Dec 2015 11:45:16 +0100
Subject: [PATCH 4/8] Allow VMODs to fail a warm-up
It is also possible to convey a message to the CLI. In case of a failure
the VCL is cooled down. VCL_Load may now fail with either CLIS_PARAM or
CLIS_CANT.
---
bin/varnishd/cache/cache_vcl.c | 83 ++++++++++++++++++++++++++++++------------
1 file changed, 60 insertions(+), 23 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index 9aa1259..0e21b78 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -441,9 +441,9 @@ vcl_event(VRT_CTX, enum vcl_event_e ev)
switch (ev) {
case VCL_EVENT_LOAD:
+ case VCL_EVENT_WARM:
AN(ctx->msg);
/* fall through */
- case VCL_EVENT_WARM:
case VCL_EVENT_USE:
break;
#define EV(event, message) \
@@ -461,14 +461,18 @@ vcl_event(VRT_CTX, enum vcl_event_e ev)
return (retval);
}
-static void
+static int
vcl_set_state(VRT_CTX, const char *state)
{
struct vcl *vcl;
+ int i = 0;
ASSERT_CLI();
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
AN(ctx->handling);
+ AN(ctx->vcl);
+ AN(state);
+ assert(ctx->msg != NULL || *state == '0');
vcl = ctx->vcl;
AN(vcl->temp);
@@ -498,16 +502,37 @@ vcl_set_state(VRT_CTX, const char *state)
/* The VCL must first reach a stable cold state */
else if (vcl->temp != vcl_temp_cooling) {
vcl->temp = vcl_temp_warm;
- (void)vcl_event(ctx, VCL_EVENT_WARM);
- vcl_BackendEvent(vcl, VCL_EVENT_WARM);
+ i = vcl_event(ctx, VCL_EVENT_WARM);
+ if (i == 0)
+ vcl_BackendEvent(vcl, VCL_EVENT_WARM);
+ else
+ AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
}
break;
default:
WRONG("Wrong enum state");
}
+ return (i);
}
-static int
+static void
+vcl_unload(VRT_CTX, struct cli *cli, const char *name, const char *step)
+{
+ struct vcl *vcl = ctx->vcl;
+
+ AZ(VSB_finish(ctx->msg));
+ VCLI_SetResult(cli, CLIS_CANT);
+ VCLI_Out(cli, "VCL \"%s\" Failed %s", name, step);
+ if (VSB_len(ctx->msg))
+ VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(ctx->msg));
+ AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_DISCARD));
+ vcl_KillBackends(vcl);
+ VCL_Close(&vcl);
+ VSB_clear(ctx->msg);
+ VSB_delete(ctx->msg);
+}
+
+static void
VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
{
struct vcl *vcl;
@@ -520,8 +545,9 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
vcl = vcl_find(name);
if (vcl != NULL) {
+ VCLI_SetResult(cli, CLIS_PARAM);
VCLI_Out(cli, "Config '%s' already loaded", name);
- return (1);
+ return;
}
vsb = VSB_new_auto();
@@ -530,9 +556,10 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
vcl = VCL_Open(fn, vsb);
if (vcl == NULL) {
AZ(VSB_finish(vsb));
+ VCLI_SetResult(cli, CLIS_PARAM);
VCLI_Out(cli, "%s", VSB_data(vsb));
VSB_delete(vsb);
- return (1);
+ return;
}
vcl->loaded_name = strdup(name);
@@ -549,19 +576,20 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
VSB_clear(vsb);
ctx.msg = vsb;
i = vcl_event(&ctx, VCL_EVENT_LOAD);
- AZ(VSB_finish(vsb));
if (i) {
- VCLI_Out(cli, "VCL \"%s\" Failed initialization", name);
- if (VSB_len(vsb))
- VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(vsb));
- (void)vcl_event(&ctx, VCL_EVENT_DISCARD);
- vcl_KillBackends(vcl);
- VCL_Close(&vcl);
- VSB_delete(vsb);
- return (1);
+ vcl_unload(&ctx, cli, name, "initialization");
+ return;
}
+ VSB_clear(vsb);
+ i = vcl_set_state(&ctx, state);
+ if (i) {
+ assert(*state == '1');
+ vcl_unload(&ctx, cli, name, "warmup");
+ return;
+ }
+ VSB_clear(vsb);
+ VSB_finish(vsb);
VSB_delete(vsb);
- vcl_set_state(&ctx, state);
bprintf(vcl->state, "%s", state + 1);
assert(hand == VCL_RET_OK);
VCLI_Out(cli, "Loaded \"%s\" as \"%s\"", fn , name);
@@ -572,7 +600,6 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
Lck_Unlock(&vcl_mtx);
VSC_C_main->n_vcl++;
VSC_C_main->n_vcl_avail++;
- return (0);
}
/*--------------------------------------------------------------------
@@ -620,7 +647,7 @@ VCL_Poll(void)
INIT_OBJ(&ctx, VRT_CTX_MAGIC);
ctx.vcl = vcl;
ctx.handling = &hand;
- vcl_set_state(&ctx, "0");
+ (void)vcl_set_state(&ctx, "0");
}
if (vcl->discard && vcl->temp == vcl_temp_cold)
VCL_Nuke(vcl);
@@ -656,8 +683,7 @@ ccf_config_load(struct cli *cli, const char * const *av, void *priv)
AZ(priv);
ASSERT_CLI();
- if (VCL_Load(cli, av[2], av[3], av[4]))
- VCLI_SetResult(cli, CLIS_PARAM);
+ VCL_Load(cli, av[2], av[3], av[4]);
}
static void __match_proto__(cli_func_t)
@@ -667,6 +693,7 @@ ccf_config_state(struct cli *cli, const char * const *av, void *priv)
unsigned hand;
INIT_OBJ(&ctx, VRT_CTX_MAGIC);
+ ctx.msg = VSB_new_auto();
ctx.handling = &hand;
(void)cli;
@@ -676,8 +703,18 @@ ccf_config_state(struct cli *cli, const char * const *av, void *priv)
AN(av[3]);
ctx.vcl = vcl_find(av[2]);
AN(ctx.vcl); // MGT ensures this
- vcl_set_state(&ctx, av[3]);
- bprintf(ctx.vcl->state, "%s", av[3] + 1);
+ if (vcl_set_state(&ctx, av[3]) == 0) {
+ bprintf(ctx.vcl->state, "%s", av[3] + 1);
+ VSB_delete(ctx.msg);
+ return;
+ }
+ VSB_finish(ctx.msg);
+ VCLI_SetResult(cli, CLIS_CANT);
+ VCLI_Out(cli, "Failed <vcl.state %s %s>", ctx.vcl->loaded_name,
+ av[3] + 1);
+ if (VSB_len(ctx.msg))
+ VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(ctx.msg));
+ VSB_delete(ctx.msg);
}
static void __match_proto__(cli_func_t)
--
2.4.3
["0005-Make-VMODs-actually-fail-warm-ups.patch" (text/x-patch)]
From 6eb7cc64a36b06f3ff65a27af93364244f40e4d9 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Mon, 7 Dec 2015 11:18:33 +0100
Subject: [PATCH 5/8] Make VMODs actually fail warm-ups
The implementation is similar to the load/discard dance when a load
fails. New VGC functions are introduced iff the VCL has at least one
VMOD handling events.
The generated code looks like this:
static unsigned vgc_inistep;
static unsigned vgc_warmupstep;
...
static int
VGC_Load(VRT_CTX)
{
...
}
static int
VGC_Discard(VRT_CTX)
{
...
}
static int
VGC_Warmup(VRT_CTX, enum vcl_event_e ev)
{
vgc_warmupstep = 0;
/* 4 */
if (Vmod_debug_Func._event(ctx, &vmod_priv_debug, ev))
return (1);
vgc_warmupstep = 4;
return (0);
}
static int
VGC_Use(VRT_CTX, enum vcl_event_e ev)
{
/* 4 */
if (Vmod_debug_Func._event(ctx, &vmod_priv_debug, ev))
return (1);
return (0);
}
static int
VGC_Cooldown(VRT_CTX, enum vcl_event_e ev)
{
int retval = 0;
/* 4 */
if (vgc_warmupstep >= 4 &&
Vmod_debug_Func._event(ctx, &vmod_priv_debug, ev) != 0)
retval = 1;
return (retval);
}
static int
VGC_Event(VRT_CTX, enum vcl_event_e ev)
{
if (ev == VCL_EVENT_LOAD)
return(VGC_Load(ctx));
if (ev == VCL_EVENT_WARM)
return(VGC_Warmup(ctx, ev));
if (ev == VCL_EVENT_USE)
return(VGC_Use(ctx, ev));
if (ev == VCL_EVENT_COLD)
return(VGC_Cooldown(ctx, ev));
if (ev == VCL_EVENT_DISCARD)
return(VGC_Discard(ctx));
return (1);
}
However, if there are no VMODs handling events, no new functions shall
be generated, leading to code looking like this:
static unsigned vgc_inistep;
static unsigned vgc_warmupstep;
...
static int
VGC_Load(VRT_CTX)
{
...
}
static int
VGC_Discard(VRT_CTX)
{
...
}
static int
VGC_Event(VRT_CTX, enum vcl_event_e ev)
{
if (ev == VCL_EVENT_LOAD)
return(VGC_Load(ctx));
if (ev == VCL_EVENT_DISCARD)
return(VGC_Discard(ctx));
(void)vgc_warmupstep;
return (0);
}
---
doc/sphinx/reference/vmod.rst | 6 +++
lib/libvcc/vcc_compile.c | 96 +++++++++++++++++++++++++++++++++++++------
lib/libvcc/vcc_vmod.c | 3 +-
3 files changed, 90 insertions(+), 15 deletions(-)
diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst
index 57fa09c..12a199d 100644
--- a/doc/sphinx/reference/vmod.rst
+++ b/doc/sphinx/reference/vmod.rst
@@ -383,6 +383,12 @@ first with a ``VCL_EVENT_WARM`` event. Unless a user decides that a given VCL
should always be warm, an inactive VMOD will eventually become cold and should
manage resources accordingly.
+An event function must return zero upon success. It is therefore possible to
+fail an initialization, the ``VCL_EVENT_LOAD`` or ``VCL_EVENT_WARM`` events.
+Should such a failure happen, a ``VCL_EVENT_DISCARD`` or ``VCL_EVENT_COLD``
+event to the VMODs that succeeded. The VMOD that failed will not receive this
+event, and therefore must not be left half-initialized should a failure occur.
+
If your VMOD is running an asynchronous background job you can hold a reference
to the VCL to prevent it from going cold too soon and get the same guarantees
as backends with ongoing requests for instance. For that, you must acquire the
diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c
index e79597f..2d08d00 100644
--- a/lib/libvcc/vcc_compile.c
+++ b/lib/libvcc/vcc_compile.c
@@ -325,20 +325,22 @@ EmitCoordinates(const struct vcc *tl, struct vsb *vsb)
/*--------------------------------------------------------------------
* Init/Fini/Event
*
- * We call Fini-s in the opposite order of init-s.
- * Other events are called in same order as init-s, no matter which
- * event it might be.
+ * We call DISCARD and COLD events in the opposite order of LOAD and
+ * WARM.
*/
static void
EmitInitFini(const struct vcc *tl)
{
struct inifin *p;
+ unsigned has_event = 0;
- Fh(tl, 0, "\nstatic unsigned vgc_inistep;\n");
+ Fh(tl, 0, "\n");
+ Fh(tl, 0, "static unsigned vgc_inistep;\n");
+ Fh(tl, 0, "static unsigned vgc_warmupstep;\n");
/*
- * INIT
+ * LOAD
*/
Fc(tl, 0, "\nstatic int\nVGC_Load(VRT_CTX)\n{\n\n");
Fc(tl, 0, "\tvgc_inistep = 0;\n\n");
@@ -349,6 +351,10 @@ EmitInitFini(const struct vcc *tl)
Fc(tl, 0, "\t/* %u */\n%s\n", p->n, VSB_data(p->ini));
Fc(tl, 0, "\tvgc_inistep = %u;\n\n", p->n);
VSB_delete(p->ini);
+
+ AZ(VSB_finish(p->event));
+ if (VSB_len(p->event))
+ has_event = 1;
}
Fc(tl, 0, "\t(void)VGC_function_vcl_init(ctx);\n");
@@ -356,7 +362,7 @@ EmitInitFini(const struct vcc *tl)
Fc(tl, 0, "}\n");
/*
- * FINI
+ * DISCARD
*/
Fc(tl, 0, "\nstatic int\nVGC_Discard(VRT_CTX)\n{\n\n");
@@ -375,6 +381,66 @@ EmitInitFini(const struct vcc *tl)
Fc(tl, 0, "\treturn(0);\n");
Fc(tl, 0, "}\n");
+ if (has_event) {
+ /*
+ * WARM
+ */
+ Fc(tl, 0, "\nstatic int\n");
+ Fc(tl, 0, "VGC_Warmup(VRT_CTX, enum vcl_event_e ev)\n{\n\n");
+
+ Fc(tl, 0, "\tvgc_warmupstep = 0;\n\n");
+ VTAILQ_FOREACH(p, &tl->inifin, list) {
+ assert(p->n > 0);
+ if (VSB_len(p->event)) {
+ Fc(tl, 0, "\t/* %u */\n", p->n);
+ Fc(tl, 0, "\tif (%s)\n", VSB_data(p->event));
+ Fc(tl, 0, "\t\treturn (1);\n");
+ Fc(tl, 0, "\tvgc_warmupstep = %u;\n\n", p->n);
+ }
+ }
+
+ Fc(tl, 0, "\treturn (0);\n");
+ Fc(tl, 0, "}\n");
+
+ /*
+ * USE (deprecated)
+ */
+ Fc(tl, 0, "\nstatic int\n");
+ Fc(tl, 0, "VGC_Use(VRT_CTX, enum vcl_event_e ev)\n{\n\n");
+
+ VTAILQ_FOREACH(p, &tl->inifin, list) {
+ assert(p->n > 0);
+ if (VSB_len(p->event)) {
+ Fc(tl, 0, "\t/* %u */\n", p->n);
+ Fc(tl, 0, "\tif (%s)\n", VSB_data(p->event));
+ Fc(tl, 0, "\t\treturn (1);\n\n");
+ }
+ }
+
+ Fc(tl, 0, "\treturn (0);\n");
+ Fc(tl, 0, "}\n");
+
+ /*
+ * COLD
+ */
+ Fc(tl, 0, "\nstatic int\n");
+ Fc(tl, 0, "VGC_Cooldown(VRT_CTX, enum vcl_event_e ev)\n{\n");
+ Fc(tl, 0, "\tint retval = 0;\n\n");
+
+ VTAILQ_FOREACH_REVERSE(p, &tl->inifin, inifinhead, list) {
+ if (VSB_len(p->event)) {
+ Fc(tl, 0, "\t/* %u */\n", p->n);
+ Fc(tl, 0, "\tif (vgc_warmupstep >= %u &&\n", p->n);
+ Fc(tl, 0, "\t %s != 0)\n", VSB_data(p->event));
+ Fc(tl, 0, "\t\tretval = 1;\n\n");
+ }
+ VSB_delete(p->event);
+ }
+
+ Fc(tl, 0, "\treturn (retval);\n");
+ Fc(tl, 0, "}\n");
+ }
+
/*
* EVENTS
*/
@@ -383,16 +449,20 @@ EmitInitFini(const struct vcc *tl)
Fc(tl, 0, "{\n");
Fc(tl, 0, "\tif (ev == VCL_EVENT_LOAD)\n");
Fc(tl, 0, "\t\treturn(VGC_Load(ctx));\n");
+ if (has_event) {
+ Fc(tl, 0, "\tif (ev == VCL_EVENT_WARM)\n");
+ Fc(tl, 0, "\t\treturn(VGC_Warmup(ctx, ev));\n");
+ Fc(tl, 0, "\tif (ev == VCL_EVENT_USE)\n");
+ Fc(tl, 0, "\t\treturn(VGC_Use(ctx, ev));\n");
+ Fc(tl, 0, "\tif (ev == VCL_EVENT_COLD)\n");
+ Fc(tl, 0, "\t\treturn(VGC_Cooldown(ctx, ev));\n");
+ }
Fc(tl, 0, "\tif (ev == VCL_EVENT_DISCARD)\n");
Fc(tl, 0, "\t\treturn(VGC_Discard(ctx));\n");
Fc(tl, 0, "\n");
- VTAILQ_FOREACH(p, &tl->inifin, list) {
- AZ(VSB_finish(p->event));
- if (VSB_len(p->event))
- Fc(tl, 0, "\t/* %u */\n%s\n", p->n, VSB_data(p->event));
- VSB_delete(p->event);
- }
- Fc(tl, 0, "\treturn (0);\n");
+ if (!has_event)
+ Fc(tl, 0, "\t(void)vgc_warmupstep;\n");
+ Fc(tl, 0, "\treturn (%d);\n", has_event ? 1 : 0);
Fc(tl, 0, "}\n");
}
diff --git a/lib/libvcc/vcc_vmod.c b/lib/libvcc/vcc_vmod.c
index 60d2ed2..6cc2645 100644
--- a/lib/libvcc/vcc_vmod.c
+++ b/lib/libvcc/vcc_vmod.c
@@ -205,8 +205,7 @@ vcc_ParseImport(struct vcc *tl)
VSB_printf(ifp->fin,
"\t\t(void)%s(ctx, &vmod_priv_%.*s,\n"
"\t\t VCL_EVENT_DISCARD);\n", p, PF(mod));
- VSB_printf(ifp->event,
- "\t(void)%s(ctx, &vmod_priv_%.*s, ev);\n",
+ VSB_printf(ifp->event, "\t%s(ctx, &vmod_priv_%.*s, ev)",
p, PF(mod));
} else {
sym = VCC_AddSymbolStr(tl, p, SYM_FUNC);
--
2.4.3
["0006-Catch-a-vcl.state-failure-on-the-manager-side.patch" (text/x-patch)]
From a59a199c32e37d29f4467165925795dd05f226b3 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Mon, 7 Dec 2015 12:07:27 +0100
Subject: [PATCH 6/8] Catch a vcl.state failure on the manager side
Don't update the state of the VCL to warm if it failed, and don't start
the child if the active VCL failed to warm up.
---
bin/varnishd/mgt/mgt.h | 2 +-
bin/varnishd/mgt/mgt_child.c | 3 ++-
bin/varnishd/mgt/mgt_vcl.c | 49 ++++++++++++++++++++++++----------------
bin/varnishtest/tests/v00044.vtc | 8 +++++++
4 files changed, 40 insertions(+), 22 deletions(-)
diff --git a/bin/varnishd/mgt/mgt.h b/bin/varnishd/mgt/mgt.h
index 5fb12d4..4756fad 100644
--- a/bin/varnishd/mgt/mgt.h
+++ b/bin/varnishd/mgt/mgt.h
@@ -167,7 +167,7 @@ void mgt_vcc_init(void);
void mgt_vcl_init(void);
void mgt_vcc_default(struct cli *, const char *b_arg, const char *vclsrc,
int Cflag);
-int mgt_push_vcls_and_start(unsigned *status, char **p);
+int mgt_push_vcls_and_start(struct cli *, unsigned *status, char **p);
int mgt_has_vcl(void);
extern char *mgt_cc_cmd;
extern const char *mgt_vcl_dir;
diff --git a/bin/varnishd/mgt/mgt_child.c b/bin/varnishd/mgt/mgt_child.c
index 8a26eb4..ae01a91 100644
--- a/bin/varnishd/mgt/mgt_child.c
+++ b/bin/varnishd/mgt/mgt_child.c
@@ -418,7 +418,8 @@ mgt_launch_child(struct cli *cli)
mgt_cli_start_child(child_cli_in, child_cli_out);
child_pid = pid;
- if (mgt_push_vcls_and_start(&u, &p)) {
+ if (mgt_push_vcls_and_start(cli, &u, &p)) {
+ VCLI_SetResult(cli, u);
MGT_complain(C_ERR, "Child (%jd) Pushing vcls failed:\n%s",
(intmax_t)child_pid, p);
free(p);
diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c
index 583f478..609aba4 100644
--- a/bin/varnishd/mgt/mgt_vcl.c
+++ b/bin/varnishd/mgt/mgt_vcl.c
@@ -123,12 +123,13 @@ mgt_has_vcl(void)
return (!VTAILQ_EMPTY(&vclhead));
}
-static void
-mgt_vcl_setstate(struct vclprog *vp, const char *vs)
+static int
+mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, const char *vs)
{
unsigned status, warm;
double now;
char *p;
+ int i;
if (vs == vcl_state_auto) {
assert(vp != active_vcl);
@@ -143,7 +144,7 @@ mgt_vcl_setstate(struct vclprog *vp, const char *vs)
warm = vs == vcl_state_warm ? 1 : 0;
if (vp->warm == warm)
- return;
+ return (0);
vp->warm = warm;
@@ -151,15 +152,19 @@ mgt_vcl_setstate(struct vclprog *vp, const char *vs)
vp->go_cold = 0;
if (child_pid < 0)
- return;
+ return (0);
- /*
- * We ignore the result here so we don't croak if the child did.
- */
- (void)mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n",
+ i = mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n",
vp->name, vp->warm, vp->state);
+ if (i) {
+ AN(cli);
+ AN(vp->warm);
+ VCLI_SetResult(cli, status);
+ VCLI_Out(cli, "%s", p);
+ }
free(p);
+ return (i);
}
/*--------------------------------------------------------------------*/
@@ -232,12 +237,15 @@ mgt_vcc_default(struct cli *cli, const char *b_arg, const char *vclsrc,
/*--------------------------------------------------------------------*/
int
-mgt_push_vcls_and_start(unsigned *status, char **p)
+mgt_push_vcls_and_start(struct cli *cli, unsigned *status, char **p)
{
struct vclprog *vp;
AN(active_vcl);
- mgt_vcl_setstate(active_vcl, vcl_state_warm);
+
+ /* The VCL has not been loaded yet, it cannot fail */
+ AZ(mgt_vcl_setstate(cli, active_vcl, vcl_state_warm));
+
VTAILQ_FOREACH(vp, &vclhead, list) {
if (mgt_cli_askchild(status, p, "vcl.load \"%s\" %s %d%s\n",
vp->name, vp->fname, vp->warm, vp->state))
@@ -328,7 +336,7 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv)
bprintf(vp->state, "%s", "auto");
if (vp != active_vcl) {
vp->go_cold = VTIM_mono();
- mgt_vcl_setstate(vp, vcl_state_auto);
+ (void)mgt_vcl_setstate(cli, vp, vcl_state_auto);
}
} else if (!strcmp(av[3], "cold")) {
if (vp == active_vcl) {
@@ -337,10 +345,10 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv)
return;
}
bprintf(vp->state, "%s", "auto");
- mgt_vcl_setstate(vp, vcl_state_cold);
+ (void)mgt_vcl_setstate(cli, vp, vcl_state_cold);
} else if (!strcmp(av[3], "warm")) {
- bprintf(vp->state, "%s", av[3]);
- mgt_vcl_setstate(vp, vcl_state_warm);
+ if (mgt_vcl_setstate(cli, vp, vcl_state_warm) == 0)
+ bprintf(vp->state, "%s", av[3]);
} else {
VCLI_Out(cli, "State must be one of auto, cold or warm.");
VCLI_SetResult(cli, CLIS_PARAM);
@@ -360,20 +368,21 @@ mcf_vcl_use(struct cli *cli, const char * const *av, void *priv)
return;
if (vp == active_vcl)
return;
- mgt_vcl_setstate(vp, vcl_state_warm);
+ if (mgt_vcl_setstate(cli, vp, vcl_state_warm))
+ return;
if (child_pid >= 0 &&
mgt_cli_askchild(&status, &p, "vcl.use %s\n", av[2])) {
VCLI_SetResult(cli, status);
VCLI_Out(cli, "%s", p);
vp->go_cold = VTIM_mono();
- mgt_vcl_setstate(vp, vcl_state_auto);
+ (void)mgt_vcl_setstate(cli, vp, vcl_state_auto);
} else {
VCLI_Out(cli, "VCL '%s' now active", av[2]);
vp2 = active_vcl;
active_vcl = vp;
if (vp2 != NULL) {
vp2->go_cold = VTIM_mono();
- mgt_vcl_setstate(vp2, vcl_state_auto);
+ (void)mgt_vcl_setstate(cli, vp2, vcl_state_auto);
}
}
free(p);
@@ -395,9 +404,9 @@ mcf_vcl_discard(struct cli *cli, const char * const *av, void *priv)
VCLI_Out(cli, "Cannot discard active VCL program\n");
return;
}
- mgt_vcl_setstate(vp, vcl_state_cold);
+ (void)mgt_vcl_setstate(cli, vp, vcl_state_cold);
if (child_pid >= 0) {
- /* If this fails the child is crashing, figure that later */
+ /* XXX If this fails the child is crashing, figure that later */
(void)mgt_cli_askchild(&status, &p, "vcl.discard %s\n", av[2]);
free(p);
}
@@ -441,7 +450,7 @@ mgt_vcl_poker(const struct vev *e, int what)
e_poker->timeout = mgt_param.vcl_cooldown * .45;
VTAILQ_FOREACH(vp, &vclhead, list) {
if (vp != active_vcl)
- mgt_vcl_setstate(vp, vcl_state_auto);
+ (void)mgt_vcl_setstate(NULL, vp, vcl_state_auto);
}
return (0);
}
diff --git a/bin/varnishtest/tests/v00044.vtc b/bin/varnishtest/tests/v00044.vtc
index aa79e6a..c056d6c 100644
--- a/bin/varnishtest/tests/v00044.vtc
+++ b/bin/varnishtest/tests/v00044.vtc
@@ -83,3 +83,11 @@ delay .4
varnish v1 -expect VBE.vcl1.default.happy >= 0
delay 4
varnish v1 -expect !VBE.vcl1.default.happy
+
+# A VMOD's warm-up can fail
+varnish v1 -cliok "param.set max_esi_depth 42"
+varnish v1 -clierr 300 "vcl.state vcl1 warm"
+
+# A warm-up failure can also fail a child start
+varnish v1 -cliok stop
+varnish v1 -clierr 300 start
--
2.4.3
["0007-Simplify-vcl.use-by-making-it-failsafe.patch" (text/x-patch)]
From 56edac247bcfd8378e307d69142a30419bb92f2e Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Mon, 7 Dec 2015 15:02:13 +0100
Subject: [PATCH 7/8] Simplify `vcl.use` by making it failsafe
By the time we decide to switch to a VCL, it must be warm and usable.
The deprecated VCL_EVENT_USE should not get in the way.
---
bin/varnishd/cache/cache_vcl.c | 26 ++++++--------------------
lib/libvcc/vcc_compile.c | 3 ++-
lib/libvmod_debug/vmod_debug.c | 1 +
3 files changed, 9 insertions(+), 21 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index 0e21b78..cc40c1f 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -443,14 +443,13 @@ vcl_event(VRT_CTX, enum vcl_event_e ev)
case VCL_EVENT_LOAD:
case VCL_EVENT_WARM:
AN(ctx->msg);
- /* fall through */
- case VCL_EVENT_USE:
break;
#define EV(event, message) \
case event: \
if (retval) \
WRONG("A VMOD failed the VCL " message); \
break;
+ EV(VCL_EVENT_USE, "use");
EV(VCL_EVENT_COLD, "cool down");
EV(VCL_EVENT_DISCARD, "discard");
#undef EV
@@ -744,33 +743,20 @@ ccf_config_use(struct cli *cli, const char * const *av, void *priv)
struct vcl *vcl;
struct vrt_ctx ctx;
unsigned hand = 0;
- struct vsb *vsb;
- int i;
ASSERT_CLI();
+ AN(cli);
AZ(priv);
vcl = vcl_find(av[2]);
AN(vcl); // MGT ensures this
assert(vcl->temp == vcl_temp_warm); // MGT ensures this
INIT_OBJ(&ctx, VRT_CTX_MAGIC);
ctx.handling = &hand;
- vsb = VSB_new_auto();
- AN(vsb);
- ctx.msg = vsb;
ctx.vcl = vcl;
- i = vcl_event(&ctx, VCL_EVENT_USE);
- AZ(VSB_finish(vsb));
- if (i) {
- VCLI_Out(cli, "VCL \"%s\" Failed to activate", av[2]);
- if (VSB_len(vsb) > 0)
- VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(vsb));
- VCLI_SetResult(cli, CLIS_CANT);
- } else {
- Lck_Lock(&vcl_mtx);
- vcl_active = vcl;
- Lck_Unlock(&vcl_mtx);
- }
- VSB_delete(vsb);
+ (void)vcl_event(&ctx, VCL_EVENT_USE);
+ Lck_Lock(&vcl_mtx);
+ vcl_active = vcl;
+ Lck_Unlock(&vcl_mtx);
}
static void __match_proto__(cli_func_t)
diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c
index 2d08d00..0c80c3e 100644
--- a/lib/libvcc/vcc_compile.c
+++ b/lib/libvcc/vcc_compile.c
@@ -326,7 +326,8 @@ EmitCoordinates(const struct vcc *tl, struct vsb *vsb)
* Init/Fini/Event
*
* We call DISCARD and COLD events in the opposite order of LOAD and
- * WARM.
+ * WARM. The child will panic if a USE event fails, since a WARM event
+ * leads to a usable state.
*/
static void
diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c
index 91b49c7..12c97c1 100644
--- a/lib/libvmod_debug/vmod_debug.c
+++ b/lib/libvmod_debug/vmod_debug.c
@@ -278,6 +278,7 @@ event_warm(VRT_CTX)
VSL(SLT_Debug, 0, "%s: VCL_EVENT_WARM", VCL_Name(ctx->vcl));
+ AN(ctx->msg);
if (cache_param->max_esi_depth == 42) {
VSB_printf(ctx->msg, "max_esi_depth is not the answer.");
return (-1);
--
2.4.3
["0008-Replace-the-VCL-refcount-by-a-self-desribing-list.patch" (text/x-patch)]
From b34e9bafefd5d8c7bd9f62bd40bc872b281556fb Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Mon, 7 Dec 2015 17:06:36 +0100
Subject: [PATCH 8/8] Replace the VCL refcount by a self-desribing list
Instead of counting the references, the VCL keeps track of them with up
to 31 characters in the description. It is the VMOD's responsibility to
keep track of the opaque struct vclref * and provide a meaningful user-
friendly description.
---
bin/varnishd/cache/cache_vcl.c | 80 ++++++++++++++++++++++++++++++++--------
bin/varnishtest/tests/v00045.vtc | 12 +++++-
doc/sphinx/reference/vmod.rst | 22 +++++++++++
include/vrt.h | 5 ++-
lib/libvmod_debug/vmod_debug.c | 43 ++++++++++++++++-----
5 files changed, 134 insertions(+), 28 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index cc40c1f..ee9a69b 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -62,10 +62,18 @@ struct vcl {
char state[8];
char *loaded_name;
unsigned busy;
- unsigned refcount;
unsigned discard;
const char *temp;
VTAILQ_HEAD(,backend) backend_list;
+ VTAILQ_HEAD(,vclref) ref_list;
+};
+
+struct vclref {
+ unsigned magic;
+#define VCLREF_MAGIC 0x47fb6848
+ const struct vcl *vcl;
+ VTAILQ_ENTRY(vclref) list;
+ char desc[32];
};
/*
@@ -252,7 +260,7 @@ vcl_KillBackends(struct vcl *vcl)
CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
AZ(vcl->busy);
- AZ(vcl->refcount);
+ assert(VTAILQ_EMPTY(&vcl->ref_list));
while (1) {
be = VTAILQ_FIRST(&vcl->backend_list);
if (be == NULL)
@@ -375,40 +383,59 @@ VRT_count(VRT_CTX, unsigned u)
ctx->vcl->conf->ref[u].line, ctx->vcl->conf->ref[u].pos);
}
-void
-VRT_ref_vcl(VRT_CTX)
+struct vclref *
+VRT_ref_vcl(VRT_CTX, const char *desc)
{
struct vcl *vcl;
+ struct vclref* ref;
ASSERT_CLI();
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+ AN(desc);
+ AN(*desc);
vcl = ctx->vcl;
CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
xxxassert(vcl->temp == vcl_temp_warm);
+ ALLOC_OBJ(ref, VCLREF_MAGIC);
+ AN(ref);
+ ref->vcl = vcl;
+ snprintf(ref->desc, sizeof ref->desc, "%s", desc);
+
Lck_Lock(&vcl_mtx);
- vcl->refcount++;
+ VTAILQ_INSERT_TAIL(&vcl->ref_list, ref, list);
Lck_Unlock(&vcl_mtx);
+
+ return (ref);
}
void
-VRT_rel_vcl(VRT_CTX)
+VRT_rel_vcl(VRT_CTX, struct vclref **refp)
{
struct vcl *vcl;
+ struct vclref *ref;
+
+ AN(refp);
+ ref = *refp;
+ *refp = NULL;
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+ CHECK_OBJ_NOTNULL(ref, VCLREF_MAGIC);
vcl = ctx->vcl;
CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
+ assert(vcl == ref->vcl);
assert(vcl->temp == vcl_temp_warm || vcl->temp == vcl_temp_busy ||
vcl->temp == vcl_temp_cooling);
Lck_Lock(&vcl_mtx);
- assert(vcl->refcount > 0);
- vcl->refcount--;
+ assert(!VTAILQ_EMPTY(&vcl->ref_list));
+ VTAILQ_REMOVE(&vcl->ref_list, ref, list);
/* No garbage collection here, for the same reasons as in VCL_Rel. */
Lck_Unlock(&vcl_mtx);
+
+ FREE_OBJ(ref);
}
/*--------------------------------------------------------------------*/
@@ -460,6 +487,23 @@ vcl_event(VRT_CTX, enum vcl_event_e ev)
return (retval);
}
+static void
+vcl_print_refs(VRT_CTX)
+{
+ struct vcl *vcl;
+ struct vclref *ref;
+
+ CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+ CHECK_OBJ_NOTNULL(ctx->vcl, VCL_MAGIC);
+ AN(ctx->msg);
+ vcl = ctx->vcl;
+ VSB_printf(ctx->msg, "VCL %s is waiting for:", vcl->loaded_name);
+ Lck_Lock(&vcl_mtx);
+ VTAILQ_FOREACH(ref, &ctx->vcl->ref_list, list)
+ VSB_printf(ctx->msg, "\n\t- %s", ref->desc);
+ Lck_Unlock(&vcl_mtx);
+}
+
static int
vcl_set_state(VRT_CTX, const char *state)
{
@@ -482,16 +526,17 @@ vcl_set_state(VRT_CTX, const char *state)
if (vcl->busy == 0 && (vcl->temp == vcl_temp_warm ||
vcl->temp == vcl_temp_busy)) {
- vcl->temp = vcl->refcount ? vcl_temp_cooling :
- vcl_temp_cold;
+ vcl->temp = VTAILQ_EMPTY(&vcl->ref_list) ?
+ vcl_temp_cold : vcl_temp_cooling;
(void)vcl_event(ctx, VCL_EVENT_COLD);
vcl_BackendEvent(vcl, VCL_EVENT_COLD);
}
else if (vcl->busy)
vcl->temp = vcl_temp_busy;
+ else if (VTAILQ_EMPTY(&vcl->ref_list))
+ vcl->temp = vcl_temp_cold;
else
- vcl->temp = vcl->refcount ? vcl_temp_cooling :
- vcl_temp_cold;
+ vcl->temp = vcl_temp_cooling;
break;
case '1':
assert(vcl->temp != vcl_temp_warm);
@@ -499,7 +544,11 @@ vcl_set_state(VRT_CTX, const char *state)
if (vcl->temp == vcl_temp_busy)
vcl->temp = vcl_temp_warm;
/* The VCL must first reach a stable cold state */
- else if (vcl->temp != vcl_temp_cooling) {
+ else if (vcl->temp == vcl_temp_cooling) {
+ vcl_print_refs(ctx);
+ i = -1;
+ }
+ else {
vcl->temp = vcl_temp_warm;
i = vcl_event(ctx, VCL_EVENT_WARM);
if (i == 0)
@@ -564,6 +613,7 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
vcl->loaded_name = strdup(name);
XXXAN(vcl->loaded_name);
VTAILQ_INIT(&vcl->backend_list);
+ VTAILQ_INIT(&vcl->ref_list);
vcl->temp = vcl_temp_init;
@@ -617,7 +667,7 @@ VCL_Nuke(struct vcl *vcl)
assert(vcl != vcl_active);
assert(vcl->discard);
AZ(vcl->busy);
- AZ(vcl->refcount);
+ assert(VTAILQ_EMPTY(&vcl->ref_list));
VTAILQ_REMOVE(&vcl_head, vcl, list);
ctx.method = VCL_MET_FINI;
ctx.handling = &hand;
@@ -733,7 +783,7 @@ ccf_config_discard(struct cli *cli, const char * const *av, void *priv)
vcl->discard = 1;
Lck_Unlock(&vcl_mtx);
- if (vcl->busy == 0 && vcl->refcount == 0)
+ if (vcl->temp == vcl_temp_cold)
VCL_Nuke(vcl);
}
diff --git a/bin/varnishtest/tests/v00045.vtc b/bin/varnishtest/tests/v00045.vtc
index 1cc1259..936dec9 100644
--- a/bin/varnishtest/tests/v00045.vtc
+++ b/bin/varnishtest/tests/v00045.vtc
@@ -6,7 +6,7 @@ server s1 -start
varnish v1 -vcl+backend {
import ${vmod_debug};
sub vcl_init {
- debug.vcl_release_delay(2s);
+ debug.vcl_release_delay(3s);
}
} -start
@@ -21,9 +21,19 @@ shell {
grep "auto/cooling.*vcl1" >/dev/null
}
+# It can't be warmed up yet
+delay 1
+shell {
+ ${varnishadm} -n ${v1_name} vcl.state vcl1 warm 2>/dev/null |
+ grep "vmod-debug ref on vcl1" >/dev/null
+}
+
# It will eventually cool down
delay 2
shell {
${varnishadm} -n ${v1_name} vcl.list |
grep "auto/cold.*vcl1" >/dev/null
}
+
+# At this point it becomes possible to warm up again
+varnish v1 -cliok "vcl.state vcl1 warm"
diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst
index 12a199d..47cb0ce 100644
--- a/doc/sphinx/reference/vmod.rst
+++ b/doc/sphinx/reference/vmod.rst
@@ -396,6 +396,28 @@ reference by calling ``VRT_ref_vcl`` when you receive a ``VCL_EVENT_WARM`` and
later calling ``VRT_rel_vcl`` once the background job is over. Receiving a
``VCL_EVENT_COLD`` is your cue to terminate any background job bound to a VCL.
+You can find an example of VCL references in vmod-debug::
+
+ priv_vcl->vclref = VRT_ref_vcl(ctx, "vmod-debug");
+ ...
+ VRT_rel_vcl(&ctx, &priv_vcl->vclref);
+
+In this simplified version, you can see that you need at least a VCL-bound data
+structure like a ``PRIV_VCL`` or a VMOD object to keep track of the reference
+and later release it. You also have to provide a description, it will be printed
+to the user if they try to warm up a cooling VCL::
+
+ $ varnishadm vcl.list
+ available auto/cooling 0 vcl1
+ active auto/warm 0 vcl2
+
+ $ varnishadm vcl.state vcl1 warm
+ Command failed with error code 300
+ Failed <vcl.state vcl1 auto>
+ Message:
+ VCL vcl1 is waiting for:
+ - vmod-debug
+
In the case where properly releasing resources may take some time, you can
opt for an asynchronous worker, either by spawning a thread and tracking it, or
by using Varnish's worker pools.
diff --git a/include/vrt.h b/include/vrt.h
index 96a3c64..4fe3067 100644
--- a/include/vrt.h
+++ b/include/vrt.h
@@ -298,8 +298,9 @@ struct vmod_priv {
typedef int vmod_event_f(VRT_CTX, struct vmod_priv *, enum vcl_event_e);
#endif
-void VRT_ref_vcl(VRT_CTX);
-void VRT_rel_vcl(VRT_CTX);
+struct vclref;
+struct vclref * VRT_ref_vcl(VRT_CTX, const char *);
+void VRT_rel_vcl(VRT_CTX, struct vclref **);
void VRT_priv_fini(const struct vmod_priv *p);
struct vmod_priv *VRT_priv_task(VRT_CTX, void *vmod_id);
diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c
index 12c97c1..6384904 100644
--- a/lib/libvmod_debug/vmod_debug.c
+++ b/lib/libvmod_debug/vmod_debug.c
@@ -45,6 +45,8 @@ struct priv_vcl {
#define PRIV_VCL_MAGIC 0x8E62FA9D
char *foo;
uintptr_t exp_cb;
+ struct vcl *vcl;
+ struct vclref *vclref;
};
VCL_DURATION vcl_release_delay = 0.0;
@@ -248,6 +250,8 @@ priv_vcl_free(void *priv)
EXP_Deregister_Callback(&priv_vcl->exp_cb);
VSL(SLT_Debug, 0, "exp_cb: deregistered");
}
+ AZ(priv_vcl->vcl);
+ AZ(priv_vcl->vclref);
FREE_OBJ(priv_vcl);
AZ(priv_vcl);
}
@@ -273,8 +277,10 @@ event_load(VRT_CTX, struct vmod_priv *priv)
}
static int
-event_warm(VRT_CTX)
+event_warm(VRT_CTX, struct vmod_priv *priv)
{
+ struct priv_vcl *priv_vcl;
+ char buf[32];
VSL(SLT_Debug, 0, "%s: VCL_EVENT_WARM", VCL_Name(ctx->vcl));
@@ -284,7 +290,13 @@ event_warm(VRT_CTX)
return (-1);
}
- VRT_ref_vcl(ctx);
+ CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC);
+ AZ(priv_vcl->vcl);
+ AZ(priv_vcl->vclref);
+
+ snprintf(buf, sizeof buf, "vmod-debug ref on %s", VCL_Name(ctx->vcl));
+ priv_vcl->vcl = ctx->vcl;
+ priv_vcl->vclref = VRT_ref_vcl(ctx, buf);
return (0);
}
@@ -292,29 +304,40 @@ static void*
cooldown_thread(void *priv)
{
struct vrt_ctx ctx;
+ struct priv_vcl *priv_vcl;
+
+ CAST_OBJ_NOTNULL(priv_vcl, priv, PRIV_VCL_MAGIC);
+ AN(priv_vcl->vcl);
+ AN(priv_vcl->vclref);
- AN(priv);
INIT_OBJ(&ctx, VRT_CTX_MAGIC);
- ctx.vcl = (struct vcl*)priv;
+ ctx.vcl = priv_vcl->vcl;
VTIM_sleep(vcl_release_delay);
- VRT_rel_vcl(&ctx);
+ VRT_rel_vcl(&ctx, &priv_vcl->vclref);
+ priv_vcl->vcl = NULL;
return (NULL);
}
static int
-event_cold(VRT_CTX)
+event_cold(VRT_CTX, struct vmod_priv *priv)
{
pthread_t thread;
+ struct priv_vcl *priv_vcl;
+
+ CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC);
+ AN(priv_vcl->vcl);
+ AN(priv_vcl->vclref);
VSL(SLT_Debug, 0, "%s: VCL_EVENT_COLD", VCL_Name(ctx->vcl));
if (vcl_release_delay == 0.0) {
- VRT_rel_vcl(ctx);
+ VRT_rel_vcl(ctx, &priv_vcl->vclref);
+ priv_vcl->vcl = NULL;
return (0);
}
- AZ(pthread_create(&thread, NULL, cooldown_thread, ctx->vcl));
+ AZ(pthread_create(&thread, NULL, cooldown_thread, priv_vcl));
AZ(pthread_detach(thread));
return (0);
}
@@ -325,8 +348,8 @@ event_function(VRT_CTX, struct vmod_priv *priv, enum vcl_event_e e)
switch (e) {
case VCL_EVENT_LOAD: return event_load(ctx, priv);
- case VCL_EVENT_COLD: return event_cold(ctx);
- case VCL_EVENT_WARM: return event_warm(ctx);
+ case VCL_EVENT_WARM: return event_warm(ctx, priv);
+ case VCL_EVENT_COLD: return event_cold(ctx, priv);
default: return (0);
}
}
--
2.4.3
_______________________________________________
varnish-dev mailing list
varnish-dev@varnish-cache.org
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic