Hi,
This is Varnish 4.1 material.
I have finally done and tested the changes we had decided to make
during the last VDD:
- removing the USE event
- allowing VMODs to fail a WARM event
- VCL reference counting for VMODs
- relaxing the "cold" restrictions on cooling VCLs
I have found and fixed more issues than anticipated after I updated my
DNS director draft to comply to the new rules. I'm happy to say that
the director is actually simpler to implement now. I have also updated
the documentation along with the code, but I may have missed some
changes and I'm running late for tonight's rehearsal :)
Best Regards,
Dridi
From 60fe90aea611bc2b20945b611fafc9eb2b28197b Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Tue, 1 Sep 2015 15:39:09 +0200
Subject: [PATCH 01/10] Get rid of VCL_EVENT_USE
---
bin/varnishd/cache/cache_vcl.c | 27 ++++-----------------------
bin/varnishtest/tests/v00044.vtc | 3 +--
doc/sphinx/reference/vmod.rst | 14 +++++++-------
lib/libvcc/generate.py | 1 -
lib/libvmod_debug/vmod_debug.c | 1 -
5 files changed, 12 insertions(+), 34 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index c747ead..d20c5d1 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -610,35 +610,16 @@ static void __match_proto__(cli_func_t)
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();
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->conf->event_vcl(&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);
+ Lck_Lock(&vcl_mtx);
+ vcl_active = vcl;
+ Lck_Unlock(&vcl_mtx);
+ return;
}
static void __match_proto__(cli_func_t)
diff --git a/bin/varnishtest/tests/v00044.vtc b/bin/varnishtest/tests/v00044.vtc
index c3fd107..aa79e6a 100644
--- a/bin/varnishtest/tests/v00044.vtc
+++ b/bin/varnishtest/tests/v00044.vtc
@@ -7,7 +7,7 @@ server s1 -repeat 20 {
close
} -start
-# The debug vmod logs some vcl events
+# The debug vmod logs temperature vcl events
varnish v1 -arg "-p vcl_cooldown=1" -vcl {
import ${vmod_debug};
backend default {
@@ -69,7 +69,6 @@ varnish v1 -clierr 106 "vcl.state vcl2 cold"
logexpect l1 -v v1 -g raw {
expect * 0 Debug "vcl1: VCL_EVENT_COLD"
expect * 0 Debug "vcl1: VCL_EVENT_WARM"
- expect * 0 Debug "vcl1: VCL_EVENT_USE"
} -start
# ...when you use a cold VCL
diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst
index b3ccefa..f0a75fd 100644
--- a/doc/sphinx/reference/vmod.rst
+++ b/doc/sphinx/reference/vmod.rst
@@ -357,10 +357,10 @@ Event functions
VMODs can have an "event" function which is called when a VCL which imports
the VMOD is loaded, made active, or discarded. This corresponds to the
-``VCL_EVENT_LOAD``, ``VCL_EVENT_USE``, and ``VCL_EVENT_DISCARD`` events,
-respectively. In addition, this function will be called when the VCL state is
-changed to cold or warm, corresponding to the ``VCL_EVENT_COLD`` and
-``VCL_EVENT_WARM`` events.
+``VCL_EVENT_LOAD``, and ``VCL_EVENT_DISCARD`` events, respectively. In
+addition, this function will be called when the VCL temperature is changed to
+cold or warm, corresponding to the ``VCL_EVENT_COLD`` and ``VCL_EVENT_WARM``
+events.
The first argument to the event function is a VRT context.
@@ -378,9 +378,9 @@ discarded and free this global state when the count reaches zero.
VMOD writers are *strongly* encouraged to release all per-VCL resources for a
given VCL when it emits a ``VCL_EVENT_COLD`` event. You will get a chance to
reacquire the resources before the VCL becomes active again and be notified
-first with a ``VCL_EVENT_WARM`` event, and then a ``VCL_EVENT_USE`` 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.
+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.
.. _ref-vmod-objects:
diff --git a/lib/libvcc/generate.py b/lib/libvcc/generate.py
index 6d6915a..06043bf 100755
--- a/lib/libvcc/generate.py
+++ b/lib/libvcc/generate.py
@@ -1020,7 +1020,6 @@ struct worker;
enum vcl_event_e {
VCL_EVENT_LOAD,
VCL_EVENT_WARM,
- VCL_EVENT_USE,
VCL_EVENT_COLD,
VCL_EVENT_DISCARD,
};
diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c
index 732d89a..b065b7f 100644
--- a/lib/libvmod_debug/vmod_debug.c
+++ b/lib/libvmod_debug/vmod_debug.c
@@ -258,7 +258,6 @@ event_function(VRT_CTX, struct vmod_priv *priv, enum vcl_event_e e)
switch (e) {
case VCL_EVENT_COLD: ev = "VCL_EVENT_COLD"; break;
case VCL_EVENT_WARM: ev = "VCL_EVENT_WARM"; break;
- case VCL_EVENT_USE: ev = "VCL_EVENT_USE"; break;
default: ev = NULL;
}
--
2.4.3
From d65fb7305761b80ba84ccba6ab345f951eca2013 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Tue, 1 Sep 2015 19:27:00 +0200
Subject: [PATCH 02/10] 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.
---
bin/varnishd/mgt/mgt_vcl.c | 43 ++++++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c
index 7971346..8abc342 100644
--- a/bin/varnishd/mgt/mgt_vcl.c
+++ b/bin/varnishd/mgt/mgt_vcl.c
@@ -49,11 +49,17 @@
#include "mgt_cli.h"
+enum vclstate {
+ VCL_STATE_AUTO,
+ VCL_STATE_COLD,
+ VCL_STATE_WARM,
+};
+
struct vclprog {
VTAILQ_ENTRY(vclprog) list;
char *name;
char *fname;
- int warm;
+ unsigned warm;
char state[8];
double go_cold;
};
@@ -121,21 +127,24 @@ mgt_has_vcl(void)
}
static void
-mgt_vcl_setstate(struct vclprog *vp, int warm)
+mgt_vcl_setstate(struct vclprog *vp, enum vclstate 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;
@@ -231,7 +240,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))
@@ -322,7 +331,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) {
@@ -331,10 +340,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);
@@ -354,20 +363,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);
@@ -389,7 +398,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]);
@@ -417,8 +426,8 @@ mcf_vcl_list(struct cli *cli, const char * const *av, void *priv)
VTAILQ_FOREACH(vp, &vclhead, list) {
VCLI_Out(cli, "%-10s %4s/%s %6s %s\n",
vp == active_vcl ? "active" : "available",
- vp->state,
- vp->warm ? "warm" : "cold", "", vp->name);
+ vp->state, vp->warm ? "warm" : "cold", "",
+ vp->name);
}
}
}
@@ -435,7 +444,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
From 9b76ff14ad893a7f0e0255b0c176fb8db7cd7153 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Tue, 1 Sep 2015 22:26:22 +0200
Subject: [PATCH 03/10] Remove unused handling
---
bin/varnishd/cache/cache_vcl.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index d20c5d1..df7ec32 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -387,13 +387,11 @@ static void
vcl_set_state(struct vcl *vcl, const char *state)
{
struct vrt_ctx ctx;
- unsigned hand = 0;
ASSERT_CLI();
AN(vcl->temp);
INIT_OBJ(&ctx, VRT_CTX_MAGIC);
- ctx.handling = &hand;
ctx.vcl = vcl;
switch(state[0]) {
--
2.4.3
From bdbe6d46d21957e10a7647979f59354ef535310c Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Sun, 6 Sep 2015 02:35:56 +0200
Subject: [PATCH 04/10] Reserve space for the "cooling" state in vcl.list
---
bin/varnishd/cache/cache_vcl.c | 2 +-
bin/varnishd/mgt/mgt_vcl.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index df7ec32..fa7e539 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -552,7 +552,7 @@ ccf_config_list(struct cli *cli, const char * const *av, void *priv)
flg = "discarded";
} else
flg = "available";
- VCLI_Out(cli, "%-10s %4s/%s %6u %s\n",
+ VCLI_Out(cli, "%-10s %4s/%-8s %6u %s\n",
flg, vcl->state, vcl->temp, vcl->busy, vcl->loaded_name);
}
}
diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c
index 8abc342..99d47bc 100644
--- a/bin/varnishd/mgt/mgt_vcl.c
+++ b/bin/varnishd/mgt/mgt_vcl.c
@@ -424,7 +424,7 @@ mcf_vcl_list(struct cli *cli, const char * const *av, void *priv)
free(p);
} else {
VTAILQ_FOREACH(vp, &vclhead, list) {
- VCLI_Out(cli, "%-10s %4s/%s %6s %s\n",
+ VCLI_Out(cli, "%-10s %4s/%-8s %6s %s\n",
vp == active_vcl ? "active" : "available",
vp->state, vp->warm ? "warm" : "cold", "",
vp->name);
--
2.4.3
From 8765dbe0ec8518f73a1fde8983cd352eeebe4cd0 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Mon, 7 Sep 2015 14:45:04 +0200
Subject: [PATCH 05/10] VMODs can now fail a VCL warm-up
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_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_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);
}
---
bin/varnishd/cache/cache_vcl.c | 85 +++++++++++++++++++++++++++++-----------
bin/varnishd/mgt/mgt.h | 2 +-
bin/varnishd/mgt/mgt_child.c | 2 +-
bin/varnishd/mgt/mgt_vcl.c | 36 +++++++++--------
bin/varnishtest/tests/v00044.vtc | 4 ++
doc/sphinx/reference/vmod.rst | 6 +++
lib/libvcc/vcc_compile.c | 71 ++++++++++++++++++++++++++++-----
lib/libvcc/vcc_vmod.c | 3 +-
lib/libvmod_debug/vmod_debug.c | 11 +++++-
9 files changed, 165 insertions(+), 55 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index fa7e539..544c317 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -383,16 +383,16 @@ vcl_find(const char *name)
return (NULL);
}
-static void
-vcl_set_state(struct vcl *vcl, const char *state)
+static int
+vcl_set_state(VRT_CTX, const char *state)
{
- struct vrt_ctx ctx;
+ struct vcl *vcl;
+ int i = 0;
ASSERT_CLI();
- AN(vcl->temp);
- INIT_OBJ(&ctx, VRT_CTX_MAGIC);
- ctx.vcl = vcl;
+ vcl = ctx->vcl;
+ AN(vcl->temp);
switch(state[0]) {
case '0':
@@ -402,7 +402,7 @@ vcl_set_state(struct vcl *vcl, const char *state)
break;
if (vcl->busy == 0) {
vcl->temp = vcl_temp_cold;
- (void)vcl->conf->event_vcl(&ctx, VCL_EVENT_COLD);
+ (void)vcl->conf->event_vcl(ctx, VCL_EVENT_COLD);
vcl_BackendEvent(vcl, VCL_EVENT_COLD);
} else {
vcl->temp = vcl_temp_cooling;
@@ -413,13 +413,34 @@ vcl_set_state(struct vcl *vcl, const char *state)
vcl->temp = vcl_temp_warm;
else {
vcl->temp = vcl_temp_warm;
- (void)vcl->conf->event_vcl(&ctx, VCL_EVENT_WARM);
- vcl_BackendEvent(vcl, VCL_EVENT_WARM);
+ i = vcl->conf->event_vcl(ctx, VCL_EVENT_WARM);
+ if (i == 0)
+ vcl_BackendEvent(vcl, VCL_EVENT_WARM);
+ else
+ (void)vcl->conf->event_vcl(ctx, VCL_EVENT_COLD);
}
break;
default:
WRONG("Wrong enum state");
}
+ return (i);
+}
+
+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 int
@@ -464,19 +485,20 @@ 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);
- 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));
- vcl_KillBackends(vcl);
- VCL_Close(&vcl);
- VSB_delete(vsb);
+ vcl_unload(&ctx, cli, name, "initialization");
return (1);
}
+ VSB_clear(vsb);
+ i = vcl_set_state(&ctx, state);
+ if (i) {
+ assert(*state == '1');
+ vcl_unload(&ctx, cli, name, "warmup");
+ return (1);
+ }
+ VSB_clear(vsb);
+ VSB_finish(vsb);
VSB_delete(vsb);
- vcl_set_state(vcl, state);
bprintf(vcl->state, "%s", state + 1);
assert(hand == VCL_RET_OK);
VCLI_Out(cli, "Loaded \"%s\" as \"%s\"", fn , name);
@@ -523,12 +545,17 @@ VCL_Nuke(struct vcl *vcl)
void
VCL_Poll(void)
{
+ struct vrt_ctx ctx;
struct vcl *vcl, *vcl2;
+ INIT_OBJ(&ctx, VRT_CTX_MAGIC);
+
ASSERT_CLI();
VTAILQ_FOREACH_SAFE(vcl, &vcl_head, list, vcl2) {
- if (vcl->temp == vcl_temp_cooling)
- vcl_set_state(vcl, "0");
+ if (vcl->temp == vcl_temp_cooling) {
+ ctx.vcl = vcl;
+ (void)vcl_set_state(&ctx, "0");
+ }
if (vcl->discard && vcl->busy == 0)
VCL_Nuke(vcl);
}
@@ -570,8 +597,12 @@ 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 vrt_ctx ctx;
struct vcl *vcl;
+ INIT_OBJ(&ctx, VRT_CTX_MAGIC);
+ ctx.msg = VSB_new_auto();
+
(void)cli;
AZ(priv);
ASSERT_CLI();
@@ -579,8 +610,18 @@ ccf_config_state(struct cli *cli, const char * const *av, void *priv)
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;
+ if (vcl_set_state(&ctx, av[3]) == 0) {
+ bprintf(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>", 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)
diff --git a/bin/varnishd/mgt/mgt.h b/bin/varnishd/mgt/mgt.h
index 18356dc..22fd0b9 100644
--- a/bin/varnishd/mgt/mgt.h
+++ b/bin/varnishd/mgt/mgt.h
@@ -159,7 +159,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 69c90d8..0fdf199 100644
--- a/bin/varnishd/mgt/mgt_child.c
+++ b/bin/varnishd/mgt/mgt_child.c
@@ -433,7 +433,7 @@ 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)) {
REPORT(LOG_ERR, "Pushing vcls failed:\n%s", p);
free(p);
child_state = CH_RUNNING;
diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c
index 99d47bc..d645df9 100644
--- a/bin/varnishd/mgt/mgt_vcl.c
+++ b/bin/varnishd/mgt/mgt_vcl.c
@@ -127,7 +127,7 @@ mgt_has_vcl(void)
}
static void
-mgt_vcl_setstate(struct vclprog *vp, enum vclstate vs)
+mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, enum vclstate vs)
{
unsigned status, warm;
double now;
@@ -156,11 +156,13 @@ mgt_vcl_setstate(struct vclprog *vp, enum vclstate vs)
if (child_pid < 0)
return;
- /*
- * 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",
- vp->name, vp->warm, vp->state);
+ if (mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n",
+ vp->name, vp->warm, vp->state)) {
+ AN(cli);
+ AN(vp->warm);
+ VCLI_SetResult(cli, status);
+ VCLI_Out(cli, "%s", p);
+ }
free(p);
}
@@ -235,12 +237,12 @@ 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);
+ 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))
@@ -331,7 +333,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);
+ mgt_vcl_setstate(NULL, vp, VCL_STATE_AUTO);
}
} else if (!strcmp(av[3], "cold")) {
if (vp == active_vcl) {
@@ -340,10 +342,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);
+ mgt_vcl_setstate(NULL, vp, VCL_STATE_COLD);
} else if (!strcmp(av[3], "warm")) {
bprintf(vp->state, "%s", av[3]);
- mgt_vcl_setstate(vp, VCL_STATE_WARM);
+ mgt_vcl_setstate(cli, vp, VCL_STATE_WARM);
} else {
VCLI_Out(cli, "State must be one of auto, cold or warm.");
VCLI_SetResult(cli, CLIS_PARAM);
@@ -363,20 +365,20 @@ 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);
+ mgt_vcl_setstate(cli, 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, VCL_STATE_AUTO);
+ mgt_vcl_setstate(NULL, 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);
+ mgt_vcl_setstate(NULL, vp2, VCL_STATE_AUTO);
}
}
free(p);
@@ -398,9 +400,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);
+ mgt_vcl_setstate(NULL, 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);
}
@@ -444,7 +446,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);
+ 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..e9eeecc 100644
--- a/bin/varnishtest/tests/v00044.vtc
+++ b/bin/varnishtest/tests/v00044.vtc
@@ -83,3 +83,7 @@ delay .4
varnish v1 -expect VBE.vcl1.default.happy >= 0
delay 4
varnish v1 -expect !VBE.vcl1.default.happy
+
+# A VMOD's warmup can fail
+varnish v1 -cliok "param.set max_esi_depth 42"
+varnish v1 -clierr 300 "vcl.state vcl1 warm"
diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst
index f0a75fd..14b4918 100644
--- a/doc/sphinx/reference/vmod.rst
+++ b/doc/sphinx/reference/vmod.rst
@@ -382,6 +382,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.
+
.. _ref-vmod-objects:
VMOD Objects
diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c
index 36f813a..4153665 100644
--- a/lib/libvcc/vcc_compile.c
+++ b/lib/libvcc/vcc_compile.c
@@ -334,11 +334,14 @@ 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 +352,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 +363,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 +382,48 @@ 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");
+
+ /*
+ * 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 +432,18 @@ 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_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 e7a7f4f..fa2ac8e 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, "%s(ctx, &vmod_priv_%.*s, ev)",
p, PF(mod));
} else {
sym = VCC_AddSymbolStr(tl, p, SYM_FUNC);
diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c
index b065b7f..b80ca74 100644
--- a/lib/libvmod_debug/vmod_debug.c
+++ b/lib/libvmod_debug/vmod_debug.c
@@ -264,15 +264,22 @@ event_function(VRT_CTX, struct vmod_priv *priv, enum vcl_event_e e)
if (ev != NULL)
VSL(SLT_Debug, 0, "%s: %s", VCL_Name(ctx->vcl), ev);
- if (e != VCL_EVENT_LOAD)
+ if (e != VCL_EVENT_LOAD && e != VCL_EVENT_WARM)
return (0);
AN(ctx->msg);
- if (cache_param->nuke_limit == 42) {
+ if (e == VCL_EVENT_LOAD && cache_param->nuke_limit == 42) {
VSB_printf(ctx->msg, "nuke_limit is not the answer.");
return (-1);
}
+ if (e == VCL_EVENT_WARM && cache_param->max_esi_depth == 42) {
+ VSB_printf(ctx->msg, "max_esi_depth is not the answer.");
+ return (-1);
+ }
+ else if (e == VCL_EVENT_WARM)
+ return (0);
+
ALLOC_OBJ(priv_vcl, PRIV_VCL_MAGIC);
AN(priv_vcl);
priv_vcl->foo = strdup("FOO");
--
2.4.3
From cd2095dcef329dcc0affd9873005f1009534b8f9 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Tue, 8 Sep 2015 12:21:46 +0200
Subject: [PATCH 06/10] VMODs handling of VCL_EVENT_COLD must be failsafe
---
bin/varnishd/cache/cache_vcl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index 544c317..af24d67 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -402,7 +402,7 @@ vcl_set_state(VRT_CTX, const char *state)
break;
if (vcl->busy == 0) {
vcl->temp = vcl_temp_cold;
- (void)vcl->conf->event_vcl(ctx, VCL_EVENT_COLD);
+ AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
vcl_BackendEvent(vcl, VCL_EVENT_COLD);
} else {
vcl->temp = vcl_temp_cooling;
@@ -417,7 +417,7 @@ vcl_set_state(VRT_CTX, const char *state)
if (i == 0)
vcl_BackendEvent(vcl, VCL_EVENT_WARM);
else
- (void)vcl->conf->event_vcl(ctx, VCL_EVENT_COLD);
+ AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
}
break;
default:
--
2.4.3
From 6f9f423ff8b8c55c76f5643365d1807f55adf8ba Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Tue, 8 Sep 2015 14:43:22 +0200
Subject: [PATCH 07/10] Allow VMODs to hold a reference on a warm VCL
---
bin/varnishd/cache/cache_vcl.c | 46 +++++++++++++++++++++++++++++++++++++++---
doc/sphinx/reference/vmod.rst | 9 ++++++++-
include/vrt.h | 3 +++
3 files changed, 54 insertions(+), 4 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index af24d67..d95345c 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -61,6 +61,7 @@ struct vcl {
char state[8];
char *loaded_name;
unsigned busy;
+ unsigned refcount;
unsigned discard;
const char *temp;
VTAILQ_HEAD(,backend) backend_list;
@@ -366,6 +367,41 @@ 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 vcl *vcl;
+
+ ASSERT_CLI();
+ CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+
+ vcl = ctx->vcl;
+ CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
+ xxxassert(vcl->temp == vcl_temp_warm);
+
+ Lck_Lock(&vcl_mtx);
+ vcl->refcount++;
+ Lck_Unlock(&vcl_mtx);
+}
+
+void
+VRT_rel_vcl(VRT_CTX)
+{
+ struct vcl *vcl;
+
+ CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+
+ vcl = ctx->vcl;
+ CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
+ assert(vcl->temp == vcl_temp_warm || vcl->temp == vcl_temp_cooling);
+
+ Lck_Lock(&vcl_mtx);
+ assert(vcl->refcount > 0);
+ vcl->refcount--;
+ /* No garbage collection here, for the same reasons as in VCL_Rel. */
+ Lck_Unlock(&vcl_mtx);
+}
+
/*--------------------------------------------------------------------*/
static struct vcl *
@@ -400,7 +436,7 @@ vcl_set_state(VRT_CTX, const char *state)
vcl->temp = vcl_temp_cold;
if (vcl->temp == vcl_temp_cold)
break;
- if (vcl->busy == 0) {
+ if (vcl->busy == 0 && vcl->refcount == 0) {
vcl->temp = vcl_temp_cold;
AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
vcl_BackendEvent(vcl, VCL_EVENT_COLD);
@@ -528,6 +564,7 @@ VCL_Nuke(struct vcl *vcl)
assert(vcl != vcl_active);
assert(vcl->discard);
AZ(vcl->busy);
+ AZ(vcl->refcount);
VTAILQ_REMOVE(&vcl_head, vcl, list);
ctx.method = VCL_MET_FINI;
ctx.handling = &hand;
@@ -556,7 +593,7 @@ VCL_Poll(void)
ctx.vcl = vcl;
(void)vcl_set_state(&ctx, "0");
}
- if (vcl->discard && vcl->busy == 0)
+ if (vcl->discard && vcl->busy == 0 && vcl->refcount == 0)
VCL_Nuke(vcl);
}
}
@@ -579,6 +616,9 @@ ccf_config_list(struct cli *cli, const char * const *av, void *priv)
flg = "discarded";
} else
flg = "available";
+ /* XXX: Should the refcount be displayed?
+ * If yes, mcf_vcl_list needs to be updated too.
+ */
VCLI_Out(cli, "%-10s %4s/%-8s %6u %s\n",
flg, vcl->state, vcl->temp, vcl->busy, vcl->loaded_name);
}
@@ -641,7 +681,7 @@ ccf_config_discard(struct cli *cli, const char * const *av, void *priv)
vcl->discard = 1;
Lck_Unlock(&vcl_mtx);
- if (vcl->busy == 0)
+ if (vcl->busy == 0 && vcl->refcount == 0)
VCL_Nuke(vcl);
}
diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst
index 14b4918..1263aec 100644
--- a/doc/sphinx/reference/vmod.rst
+++ b/doc/sphinx/reference/vmod.rst
@@ -388,6 +388,13 @@ 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
+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.
+
.. _ref-vmod-objects:
VMOD Objects
@@ -404,7 +411,7 @@ their own locking to protect shared resources.
When a VCL is loaded or unloaded, the event and priv->free are
run sequentially all in a single thread, and there is guaranteed
to be no other activity related to this particular VCL, nor are
-there init/fini activity in any other VCL or VMOD at this time.
+there init/fini activity in any other VCL or VMOD at this time.
That means that the VMOD init, and any object init/fini functions
are already serialized in sensible order, and won't need any locking,
diff --git a/include/vrt.h b/include/vrt.h
index 2d0a012..2203bd8 100644
--- a/include/vrt.h
+++ b/include/vrt.h
@@ -295,6 +295,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);
+
void VRT_priv_fini(const struct vmod_priv *p);
struct vmod_priv *VRT_priv_task(VRT_CTX, void *vmod_id);
struct vmod_priv *VRT_priv_top(VRT_CTX, void *vmod_id);
--
2.4.3
From 7b07e557cd992d7237ebf97d3a7f28d3918967ca Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Tue, 8 Sep 2015 17:34:45 +0200
Subject: [PATCH 08/10] Don't allow a VCL transition from cooling to warm
A cooling VCL may have imported VMOD, and the VMODs might be releasing
resources. It might create a race condition in VMODs with asynchronous
jobs (outside of both a transaction and the CLI).
Allowed transitions:
- init -> cold
- init -> warm
- cold -> warm
- warm -> cold
- warm -> cooling
- cooling -> cold
---
bin/varnishd/cache/cache_vcl.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index d95345c..ae68075 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -445,16 +445,17 @@ vcl_set_state(VRT_CTX, const char *state)
}
break;
case '1':
- if (vcl->temp == vcl_temp_cooling)
- vcl->temp = vcl_temp_warm;
- else {
- vcl->temp = vcl_temp_warm;
- i = vcl->conf->event_vcl(ctx, VCL_EVENT_WARM);
- if (i == 0)
- vcl_BackendEvent(vcl, VCL_EVENT_WARM);
- else
- AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
+ /* The VCL must first reach a stable cold state */
+ if (vcl->temp == vcl_temp_cooling) {
+ VSB_printf(ctx->msg, "Can't warm up a cooling VCL.\n");
+ return (1);
}
+ vcl->temp = vcl_temp_warm;
+ i = vcl->conf->event_vcl(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");
--
2.4.3
From 9e0982f7fcb7ea8416c5658850f9f6f0c573e506 Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Tue, 8 Sep 2015 23:12:15 +0200
Subject: [PATCH 09/10] Allow VMODs to create backends for a cooling VCL
They are supposed to soon notice the temperature change and stop doing
that. If a VMOD holds a VCL reference, it must see a cold event *before*
the VCL temperature transitions to "cooling".
---
bin/varnishd/cache/cache_vcl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index ae68075..a40388d 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -206,7 +206,7 @@ VCL_AddBackend(struct vcl *vcl, struct backend *be)
if (vcl->temp == vcl_temp_warm) {
/* Only when adding backend to already warm VCL */
VBE_Event(be, VCL_EVENT_WARM);
- } else if (vcl->temp != vcl_temp_init)
+ } else if (vcl->temp != vcl_temp_init && vcl->temp != vcl_temp_cooling)
WRONG("Dynamic Backends can only be added to warm VCLs");
}
@@ -437,10 +437,12 @@ vcl_set_state(VRT_CTX, const char *state)
if (vcl->temp == vcl_temp_cold)
break;
if (vcl->busy == 0 && vcl->refcount == 0) {
+ if (vcl->temp == vcl_temp_warm)
+ AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
vcl->temp = vcl_temp_cold;
- AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
vcl_BackendEvent(vcl, VCL_EVENT_COLD);
} else {
+ AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD));
vcl->temp = vcl_temp_cooling;
}
break;
--
2.4.3
From d539cc48a34ebcbd9fc9139d4a6dc532b26d052e Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmo...@gmail.com>
Date: Thu, 10 Sep 2015 18:24:54 +0200
Subject: [PATCH 10/10] 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/cache/cache_vcl.c | 7 ++++---
bin/varnishd/mgt/mgt_child.c | 1 +
bin/varnishd/mgt/mgt_vcl.c | 33 ++++++++++++++++++++-------------
bin/varnishtest/tests/v00044.vtc | 6 +++++-
4 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index a40388d..3de3216 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -482,7 +482,7 @@ vcl_unload(VRT_CTX, struct cli *cli, const char *name, const char *step)
VSB_delete(ctx->msg);
}
-static int
+static void
VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
{
struct vcl *vcl;
@@ -495,6 +495,7 @@ 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);
}
@@ -504,6 +505,7 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
vcl = VCL_Open(fn, vsb);
if (vcl == NULL) {
+ VCLI_SetResult(cli, CLIS_PARAM);
AZ(VSB_finish(vsb));
VCLI_Out(cli, "%s", VSB_data(vsb));
VSB_delete(vsb);
@@ -633,8 +635,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)
diff --git a/bin/varnishd/mgt/mgt_child.c b/bin/varnishd/mgt/mgt_child.c
index 0fdf199..68c1e6d 100644
--- a/bin/varnishd/mgt/mgt_child.c
+++ b/bin/varnishd/mgt/mgt_child.c
@@ -434,6 +434,7 @@ 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(cli, &u, &p)) {
+ VCLI_SetResult(cli, u);
REPORT(LOG_ERR, "Pushing vcls failed:\n%s", p);
free(p);
child_state = CH_RUNNING;
diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c
index d645df9..6d610f4 100644
--- a/bin/varnishd/mgt/mgt_vcl.c
+++ b/bin/varnishd/mgt/mgt_vcl.c
@@ -126,7 +126,7 @@ mgt_has_vcl(void)
return (!VTAILQ_EMPTY(&vclhead));
}
-static void
+static int
mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, enum vclstate vs)
{
unsigned status, warm;
@@ -146,7 +146,7 @@ mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, enum vclstate vs)
warm = vs == VCL_STATE_WARM ? 1 : 0;
if (vp->warm == warm)
- return;
+ return (0);
vp->warm = warm;
@@ -154,7 +154,7 @@ mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, enum vclstate vs)
vp->go_cold = 0;
if (child_pid < 0)
- return;
+ return (0);
if (mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n",
vp->name, vp->warm, vp->state)) {
@@ -162,9 +162,12 @@ mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, enum vclstate vs)
AN(vp->warm);
VCLI_SetResult(cli, status);
VCLI_Out(cli, "%s", p);
+ free(p);
+ return (1);
}
free(p);
+ return (0);
}
/*--------------------------------------------------------------------*/
@@ -242,7 +245,10 @@ mgt_push_vcls_and_start(struct cli *cli, unsigned *status, char **p)
struct vclprog *vp;
AN(active_vcl);
- mgt_vcl_setstate(cli, 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))
@@ -333,7 +339,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(NULL, vp, VCL_STATE_AUTO);
+ (void)mgt_vcl_setstate(NULL, vp, VCL_STATE_AUTO);
}
} else if (!strcmp(av[3], "cold")) {
if (vp == active_vcl) {
@@ -342,10 +348,10 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv)
return;
}
bprintf(vp->state, "%s", "auto");
- mgt_vcl_setstate(NULL, vp, VCL_STATE_COLD);
+ (void)mgt_vcl_setstate(NULL, vp, VCL_STATE_COLD);
} else if (!strcmp(av[3], "warm")) {
- bprintf(vp->state, "%s", av[3]);
- mgt_vcl_setstate(cli, 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);
@@ -365,20 +371,21 @@ mcf_vcl_use(struct cli *cli, const char * const *av, void *priv)
return;
if (vp == active_vcl)
return;
- mgt_vcl_setstate(cli, 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(NULL, vp, VCL_STATE_AUTO);
+ (void)mgt_vcl_setstate(NULL, 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(NULL, vp2, VCL_STATE_AUTO);
+ (void)mgt_vcl_setstate(NULL, vp2, VCL_STATE_AUTO);
}
}
free(p);
@@ -400,7 +407,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(NULL, vp, VCL_STATE_COLD);
+ (void)mgt_vcl_setstate(NULL, vp, VCL_STATE_COLD);
if (child_pid >= 0) {
/* XXX If this fails the child is crashing, figure that later */
(void)mgt_cli_askchild(&status, &p, "vcl.discard %s\n", av[2]);
@@ -446,7 +453,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(NULL, 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 e9eeecc..9f5a9c1 100644
--- a/bin/varnishtest/tests/v00044.vtc
+++ b/bin/varnishtest/tests/v00044.vtc
@@ -84,6 +84,10 @@ varnish v1 -expect VBE.vcl1.default.happy >= 0
delay 4
varnish v1 -expect !VBE.vcl1.default.happy
-# A VMOD's warmup can fail
+# 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 fail can also fail a child start
+varnish v1 -cliok stop
+varnish v1 -clierr 300 start
--
2.4.3
_______________________________________________
varnish-dev mailing list
varnish-dev@varnish-cache.org
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
