On Mon, Dec 7, 2015 at 9:03 PM, Dridi Boukelmoune <[email protected]> wrote: > Hi, > > This is hopefully the closure of work I started during VDD15Q3, with a > design refined during VDD15Q4. Some of the post-Q3 patches have > already been pushed to the master branch, and the remaining have been > split and should be easier to review.
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. Best, Dridi
From b2880d14c49f08756bf230f6d1b2461890a15bdd Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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. --- bin/varnishd/mgt/mgt_vcl.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c index 37359f2..673f9e1 100644 --- a/bin/varnishd/mgt/mgt_vcl.c +++ b/bin/varnishd/mgt/mgt_vcl.c @@ -48,11 +48,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; }; @@ -120,21 +126,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; @@ -230,7 +239,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 +330,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 +339,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 +362,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 +397,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 +443,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 470409e5f5dd4f05f4f6ab2f8f37ec7e498763b7 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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
From 6172daa6276474e06b1f7cef4174a8862949f8ab Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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
From 239f85204eb71bc546d4522bf4a18406b843e624 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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
From 97af95ac66bb8a9d0fb18452315542ad067ef8d6 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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
From fdd3db437524fce4622c055f34de0b1c66756a66 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 673f9e1..227b9fc 100644 --- a/bin/varnishd/mgt/mgt_vcl.c +++ b/bin/varnishd/mgt/mgt_vcl.c @@ -125,12 +125,13 @@ mgt_has_vcl(void) return (!VTAILQ_EMPTY(&vclhead)); } -static void -mgt_vcl_setstate(struct vclprog *vp, enum vclstate vs) +static int +mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, enum vclstate vs) { unsigned status, warm; double now; char *p; + int i; if (vs == VCL_STATE_AUTO) { assert(vp != active_vcl); @@ -145,7 +146,7 @@ mgt_vcl_setstate(struct vclprog *vp, enum vclstate vs) warm = vs == VCL_STATE_WARM ? 1 : 0; if (vp->warm == warm) - return; + return (0); vp->warm = warm; @@ -153,15 +154,19 @@ mgt_vcl_setstate(struct vclprog *vp, enum vclstate 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); } /*--------------------------------------------------------------------*/ @@ -234,12 +239,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)) @@ -330,7 +338,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) { @@ -339,10 +347,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); @@ -362,20 +370,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); @@ -397,9 +406,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); } @@ -443,7 +452,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
From bc73103da78b6d60762b1e99f63458492082e83e Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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
From 7816e345447f4c80b1f005b0a2b4d6492a9357c3 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> 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 [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
