> Same patches as last time, rebased against current master. I sneaked > in a trivial patch (0001) taking advantage of the review.
Hi, As requested I pushed the patches 1 to 4 from the previous batch. The patch 2 was slightly amended to have peuso-enum symbols in upper case, I have pushed another similar change (see ad8d91f). The patch 4 was also amended to introduce two functions with distinct signatures instead of a single catch-all. Please find attached the remaining patches for the next review. The patches that have been accepted so far are only the prep work, actual fixes ahead! Cheers, Dridi
From bd8d7ab8c5d3738ef7f980daade34f18f171a9d6 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Fri, 4 Dec 2015 11:45:16 +0100 Subject: [PATCH 1/5] 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 b1b7756..b297172 100644 --- a/bin/varnishd/cache/cache_vcl.c +++ b/bin/varnishd/cache/cache_vcl.c @@ -438,7 +438,7 @@ vcl_setup_event(VRT_CTX, enum vcl_event_e ev) assert(ev == VCL_EVENT_LOAD || ev == VCL_EVENT_WARM || ev == VCL_EVENT_USE); - if (ev == VCL_EVENT_LOAD) + if (ev != VCL_EVENT_USE) AN(ctx->msg); return (ctx->vcl->conf->event_vcl(ctx, ev)); @@ -457,14 +457,18 @@ vcl_failsafe_event(VRT_CTX, enum vcl_event_e ev) WRONG("A VMOD cannot fail COLD or DISCARD events"); } -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); @@ -494,16 +498,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_setup_event(ctx, VCL_EVENT_WARM); - vcl_BackendEvent(vcl, VCL_EVENT_WARM); + i = vcl_setup_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_cancel_load(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; @@ -516,8 +541,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(); @@ -526,9 +552,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); @@ -545,19 +572,20 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) VSB_clear(vsb); ctx.msg = vsb; i = vcl_setup_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)); - vcl_failsafe_event(&ctx, VCL_EVENT_DISCARD); - vcl_KillBackends(vcl); - VCL_Close(&vcl); - VSB_delete(vsb); - return (1); + vcl_cancel_load(&ctx, cli, name, "initialization"); + return; + } + VSB_clear(vsb); + i = vcl_set_state(&ctx, state); + if (i) { + assert(*state == '1'); + vcl_cancel_load(&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); @@ -568,7 +596,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); } /*-------------------------------------------------------------------- @@ -616,7 +643,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); @@ -652,8 +679,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) @@ -663,6 +689,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; @@ -672,8 +699,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 506681a7281e2eaba3baebd47e2829b0e95c1202 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Mon, 7 Dec 2015 11:18:33 +0100 Subject: [PATCH 2/5] 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 62aaeae..da6d8b4 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 d1b8f26..3a522a5 100644 --- a/lib/libvcc/vcc_vmod.c +++ b/lib/libvcc/vcc_vmod.c @@ -228,8 +228,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 262afd1f53e1554769b4b231c12c839d6ea84064 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Mon, 7 Dec 2015 12:07:27 +0100 Subject: [PATCH 3/5] 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 1aeba55..fe96360 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_startup(struct cli *, const char *b_arg, const char *f_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 3766eb8..cc2a9b5 100644 --- a/bin/varnishd/mgt/mgt_vcl.c +++ b/bin/varnishd/mgt/mgt_vcl.c @@ -122,12 +122,13 @@ mgt_has_vcl(void) return (!VTAILQ_EMPTY(&vclhead)); } -static void -mgt_vcl_setstate(struct vclprog *vp, const char *vs) +static int +mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, const char *vs) { unsigned status, warm; double now; char *p; + int i; if (vs == VCL_STATE_AUTO) { assert(vp != active_vcl); @@ -142,7 +143,7 @@ mgt_vcl_setstate(struct vclprog *vp, const char *vs) warm = vs == VCL_STATE_WARM ? 1 : 0; if (vp->warm == warm) - return; + return (0); vp->warm = warm; @@ -150,15 +151,19 @@ mgt_vcl_setstate(struct vclprog *vp, const char *vs) vp->go_cold = 0; if (child_pid < 0) - return; + return (0); - /* - * We ignore the result here so we don't croak if the child did. - */ - (void)mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n", + i = mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n", vp->name, vp->warm, vp->state); + if (i) { + AN(cli); + AN(vp->warm); + VCLI_SetResult(cli, status); + VCLI_Out(cli, "%s", p); + } free(p); + return (i); } /*--------------------------------------------------------------------*/ @@ -232,12 +237,15 @@ mgt_vcc_startup(struct cli *cli, const char *b_arg, const char *f_arg, /*--------------------------------------------------------------------*/ 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)) @@ -319,7 +327,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) { @@ -328,10 +336,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); @@ -351,20 +359,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); @@ -386,9 +395,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); } @@ -432,7 +441,7 @@ mgt_vcl_poker(const struct vev *e, int what) e_poker->timeout = mgt_param.vcl_cooldown * .45; VTAILQ_FOREACH(vp, &vclhead, list) { if (vp != active_vcl) - mgt_vcl_setstate(vp, 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 8949fad5c8574e3396416c55a6c8fd05140d26ae Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Mon, 7 Dec 2015 15:02:13 +0100 Subject: [PATCH 4/5] 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 | 35 ++++++++++------------------------- lib/libvcc/vcc_compile.c | 3 ++- lib/libvmod_debug/vmod_debug.c | 1 + 3 files changed, 13 insertions(+), 26 deletions(-) diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c index b297172..b044f73 100644 --- a/bin/varnishd/cache/cache_vcl.c +++ b/bin/varnishd/cache/cache_vcl.c @@ -435,11 +435,8 @@ vcl_setup_event(VRT_CTX, enum vcl_event_e ev) CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); AN(ctx->handling); AN(ctx->vcl); - assert(ev == VCL_EVENT_LOAD || ev == VCL_EVENT_WARM || - ev == VCL_EVENT_USE); - - if (ev != VCL_EVENT_USE) - AN(ctx->msg); + AN(ctx->msg); + assert(ev == VCL_EVENT_LOAD || ev == VCL_EVENT_WARM); return (ctx->vcl->conf->event_vcl(ctx, ev)); } @@ -451,10 +448,11 @@ vcl_failsafe_event(VRT_CTX, enum vcl_event_e ev) CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); AN(ctx->handling); AN(ctx->vcl); - assert(ev == VCL_EVENT_COLD || ev == VCL_EVENT_DISCARD); + assert(ev == VCL_EVENT_COLD || ev == VCL_EVENT_DISCARD || + ev == VCL_EVENT_USE); if (ctx->vcl->conf->event_vcl(ctx, ev) != 0) - WRONG("A VMOD cannot fail COLD or DISCARD events"); + WRONG("A VMOD cannot fail USE, COLD or DISCARD events"); } static int @@ -740,33 +738,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_setup_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); + vcl_failsafe_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 da6d8b4..cc22496 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 a9e2cde..9485268 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 1841ac3e05f4b878d5c7da44654b06a84c3d731e Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Mon, 7 Dec 2015 17:06:36 +0100 Subject: [PATCH 5/5] 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 b044f73..fd19192 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); } /*--------------------------------------------------------------------*/ @@ -455,6 +482,23 @@ vcl_failsafe_event(VRT_CTX, enum vcl_event_e ev) WRONG("A VMOD cannot fail USE, COLD or DISCARD events"); } +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) { @@ -477,16 +521,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; vcl_failsafe_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); @@ -494,7 +539,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_setup_event(ctx, VCL_EVENT_WARM); if (i == 0) @@ -559,6 +608,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; @@ -612,7 +662,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; @@ -728,7 +778,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 9485268..76287df 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; }; static 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
