[master] 07a09db90 Only let VRT_AddDirector undo a COOLING attempt
Dridi Boukelmoune
dridi.boukelmoune at gmail.com
Thu Dec 26 17:19:06 UTC 2019
commit 07a09db90be92ebd018d1799c865534c9e78242f
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date: Thu Dec 26 17:29:21 2019 +0100
Only let VRT_AddDirector undo a COOLING attempt
When a VCL is in the COOLING state it is supposed to delete the director
that was being added, but at the same time when a VBE backend fails to
be added as a director it would also undo itself.
This is in general a code path very hard to reach because either this
happens in a worker and the opportunistic check always kicks in to hide
the problem or this is done asynchronously (e.g. non-blocking lookups to
create dynamic backends) and the race window is very small. In order to
solve this the opportunistic check is skipped in VTC mode.
The test coverage with vmod_debug is done synchronously to make things
easier, but for this reason vbe_destroy can no longer expect to only run
in the CLI thread.
This is inspired by a similar patch by Martin for a different branch,
since VRT_AddDirector deletes the backend upon failure, everything needs
to be set up beforehand.
We have to tolerate backend creation attempts in a COOLING VCL because
in the asynchronous case the VCL temperature will change before COLD
events are dispatched so there's no way to prevent a race.
Closes #3176
diff --git a/bin/varnishd/cache/cache_backend.c b/bin/varnishd/cache/cache_backend.c
index 4bc367c22..84f42b21a 100644
--- a/bin/varnishd/cache/cache_backend.c
+++ b/bin/varnishd/cache/cache_backend.c
@@ -396,7 +396,6 @@ vbe_destroy(const struct director *d)
{
struct backend *be;
- ASSERT_CLI();
CAST_OBJ_NOTNULL(be, d->priv, BACKEND_MAGIC);
if (be->probe != NULL)
@@ -520,7 +519,7 @@ static const struct vdi_methods vbe_methods_noprobe[1] = {{
* Create a new static or dynamic director::backend instance.
*/
-size_t v_matchproto_()
+size_t
VRT_backend_vsm_need(VRT_CTX)
{
(void)ctx;
@@ -547,13 +546,15 @@ vrt_hash_be(const struct vrt_backend *vrt)
return (vbe64dec(ident));
}
-VCL_BACKEND v_matchproto_()
+VCL_BACKEND
VRT_new_backend_clustered(VRT_CTX, struct vsmw_cluster *vc,
const struct vrt_backend *vrt)
{
+ VCL_BACKEND d;
struct backend *be;
struct vcl *vcl;
const struct vrt_backend_probe *vbp;
+ const struct vdi_methods *m;
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
CHECK_OBJ_NOTNULL(vrt, VRT_BACKEND_MAGIC);
@@ -593,45 +594,34 @@ VRT_new_backend_clustered(VRT_CTX, struct vsmw_cluster *vc,
if (vbp == NULL)
vbp = VCL_DefaultProbe(vcl);
- if (vbp != NULL)
+ if (vbp != NULL) {
VBP_Insert(be, vbp, be->tcp_pool);
- else
+ m = vbe_methods;
+ } else {
be->sick = 0;
+ m = vbe_methods_noprobe;
+ }
+
+ Lck_Lock(&backends_mtx);
+ VTAILQ_INSERT_TAIL(&backends, be, list);
+ VSC_C_main->n_backend++;
+ Lck_Unlock(&backends_mtx);
+
+ d = VRT_AddDirector(ctx, m, be, "%s", vrt->vcl_name);
- be->director = VRT_AddDirector(ctx,
- vbp != NULL ? vbe_methods : vbe_methods_noprobe, be,
- "%s", vrt->vcl_name);
+ /* NB: if VRT_AddDirector failed, be was already freed. */
+ if (d != NULL) {
+ be->director = d;
- if (be->director != NULL) {
/* for cold VCL, update initial director state */
if (be->probe != NULL && ! vcl->temp->is_warm)
VBP_Update_Backend(be->probe);
-
- Lck_Lock(&backends_mtx);
- VTAILQ_INSERT_TAIL(&backends, be, list);
- VSC_C_main->n_backend++;
- Lck_Unlock(&backends_mtx);
- return (be->director);
}
- /* undo */
- if (vbp != NULL)
- VBP_Remove(be);
-
- VTP_Rel(&be->tcp_pool);
-
- VSC_vbe_Destroy(&be->vsc_seg);
-#define DA(x) do { if (be->x != NULL) free(be->x); } while (0)
-#define DN(x) /**/
- VRT_BACKEND_HANDLE();
-#undef DA
-#undef DN
- Lck_Delete(&be->mtx);
- FREE_OBJ(be);
- return (NULL);
+ return (d);
}
-VCL_BACKEND v_matchproto_()
+VCL_BACKEND
VRT_new_backend(VRT_CTX, const struct vrt_backend *vrt)
{
return (VRT_new_backend_clustered(ctx, NULL, vrt));
diff --git a/bin/varnishd/cache/cache_vrt_vcl.c b/bin/varnishd/cache/cache_vrt_vcl.c
index 42bbf3b9b..cc332e96c 100644
--- a/bin/varnishd/cache/cache_vrt_vcl.c
+++ b/bin/varnishd/cache/cache_vrt_vcl.c
@@ -149,7 +149,6 @@ VRT_AddDirector(VRT_CTX, const struct vdi_methods *m, void *priv,
struct vsb *vsb;
struct vcl *vcl;
struct vcldir *vdir;
- struct director *d;
const struct vcltemp *temp;
va_list ap;
int i;
@@ -161,18 +160,17 @@ VRT_AddDirector(VRT_CTX, const struct vdi_methods *m, void *priv,
CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
// opportunistic, re-checked again under lock
- if (vcl->temp == VCL_TEMP_COOLING)
+ if (vcl->temp == VCL_TEMP_COOLING && !DO_DEBUG(DBG_VTC_MODE))
return (NULL);
- ALLOC_OBJ(d, DIRECTOR_MAGIC);
- AN(d);
ALLOC_OBJ(vdir, VCLDIR_MAGIC);
AN(vdir);
- vdir->dir = d;
- d->vdir = vdir;
+ ALLOC_OBJ(vdir->dir, DIRECTOR_MAGIC);
+ AN(vdir->dir);
+ vdir->dir->vdir = vdir;
vdir->methods = m;
- d->priv = priv;
+ vdir->dir->priv = priv;
vsb = VSB_new_auto();
AN(vsb);
VSB_printf(vsb, "%s.", VCL_Name(vcl));
@@ -183,18 +181,24 @@ VRT_AddDirector(VRT_CTX, const struct vdi_methods *m, void *priv,
AZ(VSB_finish(vsb));
REPLACE(vdir->cli_name, VSB_data(vsb));
VSB_destroy(&vsb);
- d->vcl_name = vdir->cli_name + i;
+ vdir->dir->vcl_name = vdir->cli_name + i;
vdir->vcl = vcl;
vdir->admin_health = VDI_AH_AUTO;
vdir->health_changed = VTIM_real();
+ /* NB: at this point we look at the VCL temperature after getting
+ * through the trouble of creating the director even though it might
+ * not be legal to do so. Because we change the VCL temperature before
+ * sending COLD events we have to tolerate and undo attempts for the
+ * COOLING case.
+ */
Lck_Lock(&vcl_mtx);
temp = vcl->temp;
if (temp != VCL_TEMP_COOLING)
VTAILQ_INSERT_TAIL(&vcl->director_list, vdir, list);
if (temp->is_warm)
- VDI_Event(d, VCL_EVENT_WARM);
+ VDI_Event(vdir->dir, VCL_EVENT_WARM);
Lck_Unlock(&vcl_mtx);
if (temp == VCL_TEMP_COOLING) {
@@ -204,7 +208,7 @@ VRT_AddDirector(VRT_CTX, const struct vdi_methods *m, void *priv,
if (!temp->is_warm && temp != VCL_TEMP_INIT)
WRONG("Dynamic Backends can only be added to warm VCLs");
- return (d);
+ return (vdir->dir);
}
static void
diff --git a/bin/varnishtest/tests/v00063.vtc b/bin/varnishtest/tests/v00063.vtc
index 6e8508530..974794eac 100644
--- a/bin/varnishtest/tests/v00063.vtc
+++ b/bin/varnishtest/tests/v00063.vtc
@@ -14,7 +14,19 @@ varnish v1 -vcl+backend {
# load and use a new VCL
varnish v1 -vcl+backend {}
-# expect a panic during the COLD event
+# expect a panic during the COLD event, COLD state
varnish v1 -clierr 400 "vcl.state vcl1 cold"
varnish v1 -cliexpect "Dynamic Backends can only be added to warm VCLs" panic.show
varnish v1 -cliok panic.clear
+
+varnish v1 -vcl+backend {
+ import debug;
+ sub vcl_init {
+ debug.cooling_backend();
+ }
+}
+
+varnish v1 -cliok "vcl.use vcl2"
+
+# expect no panic during the COLD event, COOLING state
+varnish v1 -cliok "vcl.state vcl3 cold"
diff --git a/lib/libvmod_debug/vmod.vcc b/lib/libvmod_debug/vmod.vcc
index b8fe56f73..e28bbd204 100644
--- a/lib/libvmod_debug/vmod.vcc
+++ b/lib/libvmod_debug/vmod.vcc
@@ -255,7 +255,11 @@ Allow VCL to go cold.
$Function VOID cold_backend(PRIV_VCL)
-Schedule a backend creation attempt when the VCL cools down, panic guaranteed.
+Schedule a backend creation attempt when the VCL is COLD, panic guaranteed.
+
+$Function VOID cooling_backend(PRIV_VCL)
+
+Schedule a backend creation attempt when the VCL is COOLING, failure guaranteed.
$Function VOID sndbuf(BYTES sndbuf)
diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c
index e379f5c08..2c601fe1c 100644
--- a/lib/libvmod_debug/vmod_debug.c
+++ b/lib/libvmod_debug/vmod_debug.c
@@ -52,6 +52,7 @@ struct priv_vcl {
VCL_DURATION vcl_discard_delay;
VCL_BACKEND be;
unsigned cold_be;
+ unsigned cooling_be;
};
@@ -418,6 +419,17 @@ xyzzy_cold_backend(VRT_CTX, struct vmod_priv *priv)
priv_vcl->cold_be = 1;
}
+VCL_VOID
+xyzzy_cooling_backend(VRT_CTX, struct vmod_priv *priv)
+{
+ struct priv_vcl *priv_vcl;
+
+ CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+ AN(priv);
+ CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC);
+ priv_vcl->cooling_be = 1;
+}
+
static const struct vdi_methods empty_methods[1] = {{
.magic = VDI_METHODS_MAGIC,
.type = "debug.dummy"
@@ -466,12 +478,22 @@ cooldown_thread(void *priv)
return (NULL);
}
+static VCL_BACKEND
+create_cold_backend(VRT_CTX)
+{
+ struct vrt_backend be[1];
+
+ INIT_OBJ(be, VRT_BACKEND_MAGIC);
+ be->path = "/";
+ be->vcl_name = "doomed";
+ return (VRT_new_backend(ctx, be));
+}
+
static int
event_cold(VRT_CTX, const struct vmod_priv *priv)
{
pthread_t thread;
struct priv_vcl *priv_vcl;
- struct vrt_backend be[1];
CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC);
@@ -481,13 +503,16 @@ event_cold(VRT_CTX, const struct vmod_priv *priv)
if (priv_vcl->cold_be) {
AZ(priv_vcl->vclref_discard);
- INIT_OBJ(be, VRT_BACKEND_MAGIC);
- be->path = "/";
- be->vcl_name = "doomed";
- priv_vcl->be = VRT_new_backend(ctx, be);
+ priv_vcl->be = create_cold_backend(ctx);
WRONG("unreachable");
}
+ if (priv_vcl->cooling_be) {
+ AN(priv_vcl->vclref_discard);
+ priv_vcl->be = create_cold_backend(ctx);
+ AZ(priv_vcl->be);
+ }
+
if (priv_vcl->vcl_discard_delay == 0.0) {
AN(priv_vcl->vclref_discard);
VRT_VCL_Allow_Discard(&priv_vcl->vclref_discard);
More information about the varnish-commit
mailing list