[6.0] ae00a9c1f change semantics of the vcl 'auto' state and centralize vcl mgt
Dridi Boukelmoune
dridi.boukelmoune at gmail.com
Thu Dec 19 18:25:07 UTC 2019
commit ae00a9c1f1d4045bf2dc7eb0053d3ecc01e552c8
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Sat Nov 17 19:19:48 2018 +0100
change semantics of the vcl 'auto' state and centralize vcl mgt
Conceptually, the auto state was a variant of the warm state which
would automatically cool the vcl. Yet, cooling did not only transition
the temperature, but also the state, so 'auto' only worked one way -
except that vcl.use or moving a label (by labeling another vcl) would
also set 'auto', so a manual warm/cold setting would get lost.
Now the auto-state will remain no matter the actual temperature or
labeling, so when a vcl needs to implicitly change temperature (due to
being used or being labeled), an auto vcl will remain auto, and a
cold/warm vcl will change state, but never become 'auto' implicitly.
The vcl state/temperature test v00003.vtc, besides testing the new
auto semantics, now also checks for the right vcl.list output and has
been reduced by a duplicate check (warm event check has been
integrated into an existing warm event).
On other code changes:
* mgt_vcl_setstate
is now only concerned with the state, the temperature will be
changed implicitly if so required. The state will either end up
changed or restored, depending on success.
owner of changes to the (struct vclprog).state member
* mgt_vcl_settemp
responsible for the right action to change the temperature. For auto,
it will only change the temperature, for non-auto, also the state.
owner of changes to the (struct vclprog).warm member
* mgt_vcl_tellchild
Inform the child about a change and/or temperature change
* mgt_vcl_set_cooldown
Update the cooldown (go_cold) appropriately, should be called after
a change/temperature change.
Fixes #2834
Closes #2801
Conflicts:
bin/varnishtest/tests/v00003.vtc
diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c
index 85bd56f5f..de8513c55 100644
--- a/bin/varnishd/mgt/mgt_vcl.c
+++ b/bin/varnishd/mgt/mgt_vcl.c
@@ -104,6 +104,9 @@ static struct vclprog *active_vcl;
static struct vev *e_poker;
static int mgt_vcl_setstate(struct cli *, struct vclprog *, const char *);
+static int mgt_vcl_settemp(struct cli *, struct vclprog *, unsigned);
+static int mgt_vcl_tellchild(struct cli *, struct vclprog *, unsigned);
+static void mgt_vcl_set_cooldown(struct vclprog *, vtim_mono);
/*--------------------------------------------------------------------*/
@@ -197,13 +200,19 @@ mgt_vcl_dep_add(struct vclprog *vp_from, struct vclprog *vp_to)
CHECK_OBJ_NOTNULL(vp_from, VCLPROG_MAGIC);
CHECK_OBJ_NOTNULL(vp_to, VCLPROG_MAGIC);
ALLOC_OBJ(vd, VCLDEP_MAGIC);
+
+ mgt_vcl_set_cooldown(vp_from, -1);
+ mgt_vcl_set_cooldown(vp_to, -1);
+
XXXAN(vd);
vd->from = vp_from;
VTAILQ_INSERT_TAIL(&vp_from->dfrom, vd, lfrom);
vd->to = vp_to;
VTAILQ_INSERT_TAIL(&vp_to->dto, vd, lto);
vp_to->nto++;
+
assert(vp_to->state == VCL_STATE_WARM || /* vcl.label ... */
+ vp_to->state == VCL_STATE_AUTO || /* vcl.label ... */
vp_to->state == VCL_STATE_LABEL); /* return(vcl(...)) */
}
@@ -215,12 +224,8 @@ mgt_vcl_dep_del(struct vcldep *vd)
VTAILQ_REMOVE(&vd->from->dfrom, vd, lfrom);
VTAILQ_REMOVE(&vd->to->dto, vd, lto);
vd->to->nto--;
- if (vd->to->nto == 0 && vd->to->state == VCL_STATE_WARM) {
- vd->to->state = VCL_STATE_AUTO;
- AZ(vd->to->go_cold);
- (void)mgt_vcl_setstate(NULL, vd->to, VCL_STATE_AUTO);
- AN(vd->to->go_cold);
- }
+ if (vd->to->nto == 0)
+ mgt_vcl_set_cooldown(vd->to, VTIM_mono());
FREE_OBJ(vd);
}
@@ -393,50 +398,79 @@ mgt_has_vcl(void)
return (!VTAILQ_EMPTY(&vclhead));
}
+/*
+ * go_cold
+ *
+ * -1: leave alone
+ * 0: timer not started - not currently used
+ * >0: when timer started
+ */
+static void
+mgt_vcl_set_cooldown(struct vclprog *vp, vtim_mono now)
+{
+ CHECK_OBJ_NOTNULL(vp, VCLPROG_MAGIC);
+
+ if (vp == active_vcl ||
+ vp->state != VCL_STATE_AUTO ||
+ vp->warm == 0 ||
+ !VTAILQ_EMPTY(&vp->dto) ||
+ !VTAILQ_EMPTY(&vp->dfrom))
+ vp->go_cold = -1;
+ else
+ vp->go_cold = now;
+}
+
static unsigned
-mgt_vcl_cooldown(struct vclprog *vp)
+mgt_vcl_cooldown(struct vclprog *vp, vtim_mono now)
{
- double now;
+ CHECK_OBJ_NOTNULL(vp, VCLPROG_MAGIC);
- if (vp->state != VCL_STATE_AUTO)
+ if (vp->go_cold < 0)
return (0);
- now = VTIM_mono();
- if (vp->go_cold > 0 && vp->go_cold + mgt_param.vcl_cooldown < now)
- return (1);
+ if (vp->go_cold == 0) {
+ mgt_vcl_set_cooldown(vp, now);
+ return (0);
+ }
- if (vp->go_cold == 0 && vp != active_vcl)
- vp->go_cold = now;
+ assert(vp->go_cold > 0);
+
+ if (vp->go_cold + mgt_param.vcl_cooldown < now)
+ return (1);
return (0);
}
static int
-mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, const char *vs)
+mgt_vcl_settemp(struct cli *cli, struct vclprog *vp, unsigned warm)
{
- unsigned status, warm;
- char *p;
int i;
- assert(vs != VCL_STATE_LABEL);
+ CHECK_OBJ_NOTNULL(vp, VCLPROG_MAGIC);
- if (vp == active_vcl || mcf_is_label(vp)) {
- AN(vp->warm);
- /* Only the poker sends COLD indiscriminately, ignore it */
- if (vs == VCL_STATE_COLD)
- AZ(cli);
+ if (warm == vp->warm)
return (0);
+
+ if (vp->state == VCL_STATE_AUTO || vp->state == VCL_STATE_LABEL) {
+ mgt_vcl_set_cooldown(vp, -1);
+ i = mgt_vcl_tellchild(cli, vp, warm);
+ mgt_vcl_set_cooldown(vp, VTIM_mono());
+ } else {
+ i = mgt_vcl_setstate(cli, vp,
+ warm ? VCL_STATE_WARM : VCL_STATE_COLD);
}
- if (vs == VCL_STATE_AUTO)
- vs = (mgt_vcl_cooldown(vp) ? VCL_STATE_COLD : VCL_STATE_WARM);
- else
- vp->go_cold = 0;
+ return (i);
+}
- warm = (vs == VCL_STATE_WARM ? 1 : 0);
+static int
+mgt_vcl_tellchild(struct cli *cli, struct vclprog *vp, unsigned warm)
+{
+ unsigned status;
+ char *p;
+ int i;
- if (vp->warm == warm)
- return (0);
+ CHECK_OBJ_NOTNULL(vp, VCLPROG_MAGIC);
if (!MCH_Running()) {
vp->warm = warm;
@@ -444,7 +478,7 @@ mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, const char *vs)
}
i = mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n",
- vp->name, warm, vs);
+ vp->name, warm, vp->state);
if (i && cli != NULL) {
VCLI_SetResult(cli, status);
VCLI_Out(cli, "%s", p);
@@ -463,6 +497,47 @@ mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, const char *vs)
return (i);
}
+static int
+mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, const char *vs)
+{
+ unsigned warm;
+ int i;
+ const char *os;
+
+ CHECK_OBJ_NOTNULL(vp, VCLPROG_MAGIC);
+
+ assert(vs != VCL_STATE_LABEL);
+
+ if (mcf_is_label(vp)) {
+ AN(vp->warm);
+ /* do not touch labels */
+ return (0);
+ }
+
+ if (vp->state == vs)
+ return (0);
+
+ os = vp->state;
+ vp->state = vs;
+
+ if (vp == active_vcl) {
+ assert (vs == VCL_STATE_WARM || vs == VCL_STATE_AUTO);
+ AN(vp->warm);
+ warm = 1;
+ } else if (vs == VCL_STATE_AUTO) {
+ warm = vp->warm;
+ } else {
+ warm = (vs == VCL_STATE_WARM ? 1 : 0);
+ }
+
+ i = mgt_vcl_tellchild(cli, vp, warm);
+ if (i == 0)
+ mgt_vcl_set_cooldown(vp, VTIM_mono());
+ else
+ vp->state = os;
+ return (i);
+}
+
/*--------------------------------------------------------------------*/
static void
@@ -518,8 +593,7 @@ mgt_new_vcl(struct cli *cli, const char *vclname, const char *vclsrc,
}
free(p);
- if (vp->warm && vp->state == VCL_STATE_AUTO)
- vp->go_cold = VTIM_mono();
+ mgt_vcl_set_cooldown(vp, VTIM_mono());
}
/*--------------------------------------------------------------------*/
@@ -537,8 +611,8 @@ mgt_vcl_startup(struct cli *cli, const char *vclsrc, const char *vclname,
bprintf(buf, "boot%d", n++);
vclname = buf;
}
+ active_vcl = NULL;
mgt_new_vcl(cli, vclname, vclsrc, origin, NULL, C_flag);
- active_vcl = mcf_vcl_byname(vclname);
}
/*--------------------------------------------------------------------*/
@@ -565,7 +639,7 @@ mgt_push_vcls(struct cli *cli, unsigned *status, char **p)
AN(active_vcl);
/* The VCL has not been loaded yet, it cannot fail */
- AZ(mgt_vcl_setstate(cli, active_vcl, VCL_STATE_WARM));
+ (void)cli;
VTAILQ_FOREACH(vp, &vclhead, list)
vp->loaded = 0;
@@ -658,36 +732,21 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv)
return;
}
- if (!VTAILQ_EMPTY(&vp->dto)) {
- assert(vp->state != VCL_STATE_COLD);
- if (state == VCL_STATE_COLD) {
+ if (state == VCL_STATE_COLD) {
+ if (!VTAILQ_EMPTY(&vp->dto)) {
+ assert(vp->state != VCL_STATE_COLD);
VCLI_Out(cli, "A labeled VCL cannot be set cold");
VCLI_SetResult(cli, CLIS_CANT);
return;
}
- }
-
- if (vp->state == state)
- return;
-
- if (state == VCL_STATE_AUTO) {
- vp->state = VCL_STATE_AUTO;
- (void)mgt_vcl_setstate(cli, vp, VCL_STATE_AUTO);
- } else if (state == VCL_STATE_COLD) {
if (vp == active_vcl) {
VCLI_Out(cli, "Cannot set the active VCL cold.");
VCLI_SetResult(cli, CLIS_CANT);
return;
}
- vp->state = VCL_STATE_AUTO;
- (void)mgt_vcl_setstate(cli, vp, VCL_STATE_COLD);
- } else if (state == VCL_STATE_WARM) {
- if (mgt_vcl_setstate(cli, vp, VCL_STATE_WARM) == 0)
- vp->state = VCL_STATE_WARM;
- } else {
- VCLI_Out(cli, "State must be one of auto, cold or warm.");
- VCLI_SetResult(cli, CLIS_PARAM);
}
+
+ (void)mgt_vcl_setstate(cli, vp, state);
}
static void v_matchproto_(cli_func_t)
@@ -696,6 +755,7 @@ mcf_vcl_use(struct cli *cli, const char * const *av, void *priv)
unsigned status;
char *p = NULL;
struct vclprog *vp, *vp2;
+ vtim_mono now;
(void)priv;
vp = mcf_find_vcl(cli, av[2]);
@@ -703,22 +763,22 @@ mcf_vcl_use(struct cli *cli, const char * const *av, void *priv)
return;
if (vp == active_vcl)
return;
- if (mgt_vcl_setstate(cli, vp, VCL_STATE_WARM))
+
+ if (mgt_vcl_settemp(cli, vp, 1))
return;
+
if (MCH_Running() &&
mgt_cli_askchild(&status, &p, "vcl.use %s\n", av[2])) {
VCLI_SetResult(cli, status);
VCLI_Out(cli, "%s", p);
- AZ(vp->go_cold);
- (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) {
- AZ(vp2->go_cold);
- (void)mgt_vcl_setstate(cli, vp2, VCL_STATE_AUTO);
- }
+ now = VTIM_mono();
+ mgt_vcl_set_cooldown(vp, now);
+ if (vp2 != NULL)
+ mgt_vcl_set_cooldown(vp2, now);
}
free(p);
}
@@ -760,12 +820,14 @@ mcf_vcl_discard(struct cli *cli, const char * const *av, void *priv)
}
return;
}
- if (mcf_is_label(vp))
+ if (mcf_is_label(vp)) {
AN(vp->warm);
- else
+ vp->warm = 0;
+ } else {
(void)mgt_vcl_setstate(cli, vp, VCL_STATE_COLD);
+ }
if (MCH_Running()) {
- assert(vp->state != VCL_STATE_WARM);
+ AZ(vp->warm);
if (mgt_cli_askchild(&status, &p, "vcl.discard %s\n", av[2]))
assert(status == CLIS_OK || status == CLIS_COMMS);
free(p);
@@ -913,9 +975,8 @@ mcf_vcl_label(struct cli *cli, const char * const *av, void *priv)
}
}
- if (mgt_vcl_setstate(cli, vpt, VCL_STATE_WARM))
+ if (mgt_vcl_settemp(cli, vpt, 1))
return;
- vpt->state = VCL_STATE_WARM; /* XXX: race with the poker? */
if (vpl != NULL) {
mgt_vcl_dep_del(VTAILQ_FIRST(&vpl->dfrom));
@@ -925,7 +986,8 @@ mcf_vcl_label(struct cli *cli, const char * const *av, void *priv)
}
AN(vpl);
- vpl->warm = 1;
+ if (mgt_vcl_settemp(cli, vpl, 1))
+ return;
mgt_vcl_dep_add(vpl, vpt);
if (!MCH_Running())
@@ -945,13 +1007,16 @@ static int v_matchproto_(vev_cb_f)
mgt_vcl_poker(const struct vev *e, int what)
{
struct vclprog *vp;
+ vtim_mono now;
(void)e;
(void)what;
e_poker->timeout = mgt_param.vcl_cooldown * .45;
- VTAILQ_FOREACH(vp, &vclhead, list)
- if (mgt_vcl_cooldown(vp))
- (void)mgt_vcl_setstate(NULL, vp, VCL_STATE_COLD);
+ now = VTIM_mono();
+ VTAILQ_FOREACH(vp, &vclhead, list) {
+ if (mgt_vcl_cooldown(vp, now))
+ (void)mgt_vcl_settemp(NULL, vp, 0);
+ }
return (0);
}
diff --git a/bin/varnishtest/tests/c00077.vtc b/bin/varnishtest/tests/c00077.vtc
index 49ed76797..8b2c35df1 100644
--- a/bin/varnishtest/tests/c00077.vtc
+++ b/bin/varnishtest/tests/c00077.vtc
@@ -13,6 +13,8 @@ varnish v1 -vcl+backend {
varnish v1 -clierr 106 "vcl.label vcl.A vcl1"
varnish v1 -cliok "vcl.label vclA vcl1"
+# labeling twice #2834
+varnish v1 -cliok "vcl.label vclA vcl1"
varnish v1 -vcl+backend {
sub vcl_recv {
diff --git a/bin/varnishtest/tests/r02432.vtc b/bin/varnishtest/tests/r02432.vtc
index 97b5d30d9..2365f1725 100644
--- a/bin/varnishtest/tests/r02432.vtc
+++ b/bin/varnishtest/tests/r02432.vtc
@@ -11,6 +11,7 @@ varnish v1 -vcl+backend {
} -start
varnish v1 -cliok "vcl.label label1 vcl1"
+varnish v1 -cliok "vcl.list"
varnish v1 -vcl+backend {
sub vcl_recv {
diff --git a/bin/varnishtest/tests/v00003.vtc b/bin/varnishtest/tests/v00003.vtc
index 6236f28f1..2dcd423e6 100644
--- a/bin/varnishtest/tests/v00003.vtc
+++ b/bin/varnishtest/tests/v00003.vtc
@@ -4,7 +4,6 @@ server s1 -repeat 20 {
rxreq
txresp
delay .2
- close
} -start
# The debug vmod logs temperature vcl events
@@ -12,7 +11,8 @@ varnish v1 -arg "-p vcl_cooldown=1" -vcl {
import debug;
backend default {
.host = "${s1_addr}";
- .probe = { .interval = 1s; .initial = 1;}
+ .port = "${s1_port}";
+ .probe = { .interval = 1s; }
}
} -start
@@ -24,7 +24,8 @@ varnish v1 -cliok "backend.list -p *.*"
varnish v1 -vcl {
backend default {
.host = "${s1_addr}";
- .probe = { .interval = 1s; .initial = 1;}
+ .port = "${s1_port}";
+ .probe = { .interval = 1s; }
}
}
@@ -33,64 +34,64 @@ delay .4
varnish v1 -expect VBE.vcl1.default.happy >= 0
varnish v1 -expect VBE.vcl2.default.happy >= 0
-# Freeze the first VCL
-varnish v1 -cliok "vcl.state vcl1 cold"
-delay .4
-varnish v1 -expect !VBE.vcl1.default.happy
+# We are about to freeze vcl, then implicitly thaw it via use.
+# check that we see the events
-# Set it auto should be a no-op
-varnish v1 -cliok "vcl.state vcl1 auto"
+logexpect l1 -v v1 -g raw {
+ expect * 0 Debug "vcl1: VCL_EVENT_COLD"
+ expect * 0 Debug "vcl1: VCL_EVENT_WARM"
+} -start
+
+# Freeze vcl1
+varnish v1 -cliok "vcl.state vcl1 cold"
+varnish v1 -cliexpect "available *cold/cold *[0-9]+ *vcl1\\s+active *auto/warm *[0-9]+ *vcl2" "vcl.list"
delay .4
varnish v1 -expect !VBE.vcl1.default.happy
-# Use it, and it should come alive
+# Use it, and it should come warm (but not auto)
varnish v1 -cliok "vcl.use vcl1"
+varnish v1 -cliexpect "active *warm/warm *[0-9]+ *vcl1\\s+available *auto/warm *[0-9]+ *vcl2" "vcl.list"
delay .4
varnish v1 -expect VBE.vcl1.default.happy >= 0
varnish v1 -expect VBE.vcl2.default.happy >= 0
+logexpect l1 -wait
+
# and the unused one should go cold
delay 4
+varnish v1 -cliexpect "active *warm/warm *[0-9]+ *vcl1\\s+available *auto/cold *[0-9]+ *vcl2" "vcl.list"
varnish v1 -expect !VBE.vcl2.default.happy
-# Mark the used warm and use the other
-varnish v1 -cliok "vcl.state vcl1 warm"
+# use the other
varnish v1 -cliok "vcl.use vcl2"
+varnish v1 -cliexpect "available *warm/warm *[0-9]+ *vcl1\\s+active *auto/warm *[0-9]+ *vcl2" "vcl.list"
-# It will stay warm even after the cooldown period
+# the non-auto vcl will stay warm even after the cooldown period
delay 4
+varnish v1 -cliexpect "available *warm/warm *[0-9]+ *vcl1\\s+active *auto/warm *[0-9]+ *vcl2" "vcl.list"
varnish v1 -expect VBE.vcl1.default.happy >= 0
varnish v1 -expect VBE.vcl2.default.happy >= 0
# You can't freeze the active VCL
varnish v1 -clierr 300 "vcl.state vcl2 cold"
-# However a warm event is guaranteed...
-logexpect l1 -v v1 -g raw {
- expect * 0 Debug "vcl1: VCL_EVENT_COLD"
- expect * 0 Debug "vcl1: VCL_EVENT_WARM"
-} -start
-
-# ...when you use a cold VCL
-varnish v1 -cliok "vcl.state vcl1 cold"
-varnish v1 -cliok "vcl.use vcl1"
-
-logexpect l1 -wait
-
-# It will apply the cooldown period once inactive
-varnish v1 -cliok "vcl.use vcl2"
+# the non-auto vcl will apply the cooldown again once changed back to auto
+varnish v1 -cliok "vcl.state vcl1 auto"
+varnish v1 -cliexpect "available *auto/warm *[0-9]+ *vcl1\\s+active *auto/warm *[0-9]+ *vcl2" "vcl.list"
delay .4
varnish v1 -expect VBE.vcl1.default.happy >= 0
delay 4
+varnish v1 -cliexpect "available *auto/cold *[0-9]+ *vcl1\\s+active *auto/warm *[0-9]+ *vcl2" "vcl.list"
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"
-varnish v1 -cliexpect "available *cold/cold *[0-9]+ *vcl1\\s+active *warm/warm *[0-9]+ *vcl2" "vcl.list"
+varnish v1 -cliexpect "available *auto/cold *[0-9]+ *vcl1\\s+active *auto/warm *[0-9]+ *vcl2" "vcl.list"
# A warm-up failure can also fail a child start
varnish v1 -cliok stop
varnish v1 -cliok "vcl.state vcl1 warm"
+varnish v1 -cliok "vcl.list"
varnish v1 -clierr 300 start
More information about the varnish-commit
mailing list