> Following today's discussion on IRC, there's an error I introduced > when cleaning up the patch set before submission, and did not notice. > > Attached is the fix for the 10th patch.
Reattaching the whole patch set again, rebased against the current master to fix documentation conflicts. Best Regards, Dridi
From dec49ffde61c843ec6302c75ee102c4fed0e99f4 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 697fc6a..1c169bf 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. .. TODO 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 21d023b23290f336b38bf3aa10edf981e70580c0 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 31f722508c34d4f7b4ebc52180418aa8cc10e3ad Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 82a857a52a2c01ee6f1a7b443471794b09bc13e9 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 e7ffa05d21904fef4c868a879dec315fb277d1d8 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 | 7 ++++ lib/libvcc/vcc_compile.c | 71 ++++++++++++++++++++++++++++----- lib/libvcc/vcc_vmod.c | 3 +- lib/libvmod_debug/vmod_debug.c | 11 +++++- 9 files changed, 166 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 1d2a7a3..c0a9108 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 1c169bf..a5bbad9 100644 --- a/doc/sphinx/reference/vmod.rst +++ b/doc/sphinx/reference/vmod.rst @@ -382,6 +382,13 @@ 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. + + .. TODO vmod objects When to lock, and when not to lock diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c index 4bd7821..910d466 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 e8bab4f..8d94532 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 9dc4fbebeb04610ff18d15f620cf638ad0233265 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 ea1bd64f1149e03a1e4a4742876018586c8d78f6 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 a5bbad9..0d8ec41 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. + .. TODO vmod objects @@ -400,7 +407,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 96ccd79954e48df7c5aecb3a45efe68800ecbef0 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 91aa17e163bc8146545a786a622fa64befbad6e1 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 0a9988e88b7b02f4c40c47bc23127a8309b1d706 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 | 5 +++-- bin/varnishd/mgt/mgt_child.c | 1 + bin/varnishd/mgt/mgt_vcl.c | 33 ++++++++++++++++++++------------- bin/varnishtest/tests/v00044.vtc | 6 +++++- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c index a40388d..110d75e 100644 --- a/bin/varnishd/cache/cache_vcl.c +++ b/bin/varnishd/cache/cache_vcl.c @@ -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 c0a9108..f4581d6 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 [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
