shard: use a configuration context per instance

The shard director had the limitation to only support adding/removing
backends of one instance before `.reconfigure()` on that instance had
to be called, then the next instance could be changed.

This limitation came from the time when it was still unclear that
calling `VRT_priv_task()` on a per-object pointer was the accepted
interface for per-instance privs (varnish historians' recommended read:
https://github.com/varnishcache/varnish-cache/wiki/VIP1:-PRIV_*-visibility-and-lifetime-control
)

It is about time to lift that limitation now. o/ Dridi

bin/varnishtest/tests/d00015.vtc

@@ -107,6 +107,7 @@ varnish v1 -vcl+backend {
 		vd.add_backend(s3, "2");
 		vd.add_backend(s3, "3");
 		vd2.clear();
+		vd2.add_backend(s3, "4");
 		vd.add_backend(s3, "4");
 		vd.add_backend(s3, "5");
 		vd.add_backend(s3, "6");
@@ -241,7 +242,6 @@ logexpect l1 -v v1 -g raw -d 1 {
 	expect 0 0    Debug   {^shard:.*point = 6e040182, host =  0}
 
 	expect 0 0    VCL_Log {^-- re-add some - no 2nd director$}
-	expect 0 0    Error   {^shard vd2: cannot change more than}
 	expect 0 0    Debug   {^shard:.*point =  3d1fe97, host =  3}
 	expect 0 0    Debug   {^shard:.*point =  a25a43b, host =  6}
 	expect 0 0    Debug   {^shard:.*point = 2b20d9a2, host =  1}

bin/varnishtest/tests/d00016.vtc

@@ -110,6 +110,7 @@ varnish v1 -vcl+backend {
 		vd.add_backend(s3, "2");
 		vd.add_backend(s3, "3");
 		vd2.clear();
+		vd2.add_backend(s3, "4");
 		vd.add_backend(s3, "4");
 		vd.add_backend(s3, "5");
 		vd.add_backend(s3, "6");
@@ -169,7 +170,6 @@ logexpect l1 -v v1 -g raw -d 1 {
 	expect 0 1001    VCL_Log {^-- remove s1_2 specifically$}
 	expect 0 1001    VCL_Log {^-- remove all instances of s1$}
 	expect 0 1001    VCL_Log {^-- re-add some - no 2nd director$}
-	expect 0 1001    Error   {^shard vd2: cannot change more than}
 	expect 0 1001    VCL_Log {^-- remove second-last$}
 	expect 0 1001    VCL_Log {^-- remove last$}
 	expect 0 1001    VCL_Log {^-- END$}

doc/changes.rst

@@ -37,6 +37,10 @@ Varnish Cache Next (2021-03-15)
   set headers are now validated to contain only characters allowed by
   RFC7230. A (runtime) VCL failure is triggered if not.
 
+* The shard director now supports reconfiguration (adding/removing
+  backends) of several instances without any special ordering
+  requirement.
+
 ================================
 Varnish Cache 6.5.1 (2020-09-25)
 ================================

lib/libvmod_directors/shard_cfg.c

@@ -80,27 +80,22 @@ struct backend_reconfig {
  *
  * for backend reconfiguration, we create a change list on the VCL workspace in
  * a PRIV_TASK state, which we work in reconfigure.
- *
- * for now, we allow to only reconfigure one shard director at a time.
  */
 
 static struct shard_change *
-shard_change_get(VRT_CTX, struct vmod_priv *priv,
-	const struct sharddir * const shardd)
+shard_change_get(VRT_CTX, const struct sharddir * const shardd)
 {
+	struct vmod_priv *task;
 	struct shard_change *change;
+	const void *id = (const char *)shardd + task_off_cfg;
 
-	if (priv->priv) {
-		CAST_OBJ_NOTNULL(change, priv->priv, SHARD_CHANGE_MAGIC);
-		if (change->shardd == NULL) {
-			change->shardd = shardd;
-			VSTAILQ_INIT(&change->tasks);
-		} else if (change->shardd != shardd) {
-			shard_err0(ctx, shardd,
-			    "cannot change more than one shard director "
-			    "at a time");
-			return (NULL);
-		}
+	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+
+	task = VRT_priv_task(ctx, id);
+
+	if (task->priv != NULL) {
+		CAST_OBJ_NOTNULL(change, task->priv, SHARD_CHANGE_MAGIC);
+		assert (change->shardd == shardd);
 		return (change);
 	}
 
@@ -114,7 +109,7 @@ shard_change_get(VRT_CTX, struct vmod_priv *priv,
 	change->space = NULL;
 	change->shardd = shardd;
 	VSTAILQ_INIT(&change->tasks);
-	priv->priv = change;
+	task->priv = change;
 
 	return (change);
 }
@@ -124,7 +119,6 @@ shard_change_finish(struct shard_change *change)
 {
 	CHECK_OBJ_NOTNULL(change, SHARD_CHANGE_MAGIC);
 
-	change->shardd = NULL;
 	VSTAILQ_INIT(&change->tasks);
 }
 
@@ -150,8 +144,7 @@ shard_change_task_add(VRT_CTX, struct shard_change *change,
 }
 
 static inline struct shard_change_task *
-shard_change_task_backend(VRT_CTX,
-    struct vmod_priv *priv, const struct sharddir *shardd,
+shard_change_task_backend(VRT_CTX, const struct sharddir *shardd,
     enum shard_change_task_e task_e, VCL_BACKEND be, VCL_STRING ident,
     VCL_DURATION rampup)
 {
@@ -161,7 +154,7 @@ shard_change_task_backend(VRT_CTX,
 	CHECK_OBJ_NOTNULL(shardd, SHARDDIR_MAGIC);
 	assert(task_e == ADD_BE || task_e == REMOVE_BE);
 
-	change = shard_change_get(ctx, priv, shardd);
+	change = shard_change_get(ctx, shardd);
 	if (change == NULL)
 		return (NULL);
 
@@ -183,16 +176,15 @@ shard_change_task_backend(VRT_CTX,
  * director reconfiguration tasks
  */
 VCL_BOOL
-shardcfg_add_backend(VRT_CTX, struct vmod_priv *priv,
-    const struct sharddir *shardd, VCL_BACKEND be, VCL_STRING ident,
-    VCL_DURATION rampup, VCL_REAL weight)
+shardcfg_add_backend(VRT_CTX, const struct sharddir *shardd,
+    VCL_BACKEND be, VCL_STRING ident, VCL_DURATION rampup, VCL_REAL weight)
 {
 	struct shard_change_task *task;
 
 	assert (weight >= 1);
 	AN(be);
 
-	task = shard_change_task_backend(ctx, priv, shardd, ADD_BE,
+	task = shard_change_task_backend(ctx, shardd, ADD_BE,
 	    be, ident, rampup);
 
 	if (task == NULL)
@@ -203,21 +195,21 @@ shardcfg_add_backend(VRT_CTX, struct vmod_priv *priv,
 }
 
 VCL_BOOL
-shardcfg_remove_backend(VRT_CTX, struct vmod_priv *priv,
-    const struct sharddir *shardd, VCL_BACKEND be, VCL_STRING ident)
+shardcfg_remove_backend(VRT_CTX, const struct sharddir *shardd,
+    VCL_BACKEND be, VCL_STRING ident)
 {
-	return (shard_change_task_backend(ctx, priv, shardd, REMOVE_BE,
+	return (shard_change_task_backend(ctx, shardd, REMOVE_BE,
 	    be, ident, 0) != NULL);
 }
 
 VCL_BOOL
-shardcfg_clear(VRT_CTX, struct vmod_priv *priv, const struct sharddir *shardd)
+shardcfg_clear(VRT_CTX, const struct sharddir *shardd)
 {
 	struct shard_change *change;
 
 	CHECK_OBJ_NOTNULL(shardd, SHARDDIR_MAGIC);
 
-	change = shard_change_get(ctx, priv, shardd);
+	change = shard_change_get(ctx, shardd);
 	if (change == NULL)
 		return (0);
 
@@ -609,8 +601,7 @@ shardcfg_apply_change(VRT_CTX, struct sharddir *shardd,
  */
 
 VCL_BOOL
-shardcfg_reconfigure(VRT_CTX, struct vmod_priv *priv,
-    struct sharddir *shardd, VCL_INT replicas)
+shardcfg_reconfigure(VRT_CTX, struct sharddir *shardd, VCL_INT replicas)
 {
 	struct shard_change *change;
 
@@ -621,7 +612,7 @@ shardcfg_reconfigure(VRT_CTX, struct vmod_priv *priv,
 		return (0);
 	}
 
-	change = shard_change_get(ctx, priv, shardd);
+	change = shard_change_get(ctx, shardd);
 	if (change == NULL)
 		return (0);
 

lib/libvmod_directors/shard_cfg.h

@@ -28,15 +28,11 @@
  * SUCH DAMAGE.
  */
 
-VCL_BOOL shardcfg_add_backend(VRT_CTX, struct vmod_priv *priv,
-    const struct sharddir *shardd, VCL_BACKEND be, VCL_STRING ident,
-    VCL_DURATION rampup, VCL_REAL weight);
-VCL_BOOL shardcfg_remove_backend(VRT_CTX, struct vmod_priv *priv,
-    const struct sharddir *shardd, VCL_BACKEND be, VCL_STRING ident);
-VCL_BOOL shardcfg_clear(VRT_CTX, struct vmod_priv *priv,
-    const struct sharddir *shardd);
-VCL_BOOL shardcfg_reconfigure(VRT_CTX, struct vmod_priv *priv,
-    struct sharddir *shardd, VCL_INT replicas);
+VCL_BOOL shardcfg_add_backend(VRT_CTX, const struct sharddir *shardd,
+    VCL_BACKEND be, VCL_STRING ident, VCL_DURATION rampup, VCL_REAL weight);
+VCL_BOOL shardcfg_remove_backend(VRT_CTX, const struct sharddir *shardd,
+    VCL_BACKEND be, VCL_STRING ident);
+VCL_BOOL shardcfg_clear(VRT_CTX, const struct sharddir *shardd);
+VCL_BOOL shardcfg_reconfigure(VRT_CTX, struct sharddir *shardd, VCL_INT);
 VCL_VOID shardcfg_set_warmup(struct sharddir *shardd, VCL_REAL ratio);
-VCL_VOID shardcfg_set_rampup(struct sharddir *shardd,
-    VCL_DURATION duration);
+VCL_VOID shardcfg_set_rampup(struct sharddir *shardd, VCL_DURATION duration);

lib/libvmod_directors/shard_dir.h

@@ -73,6 +73,12 @@ struct sharddir {
 	uint32_t				n_points;
 };
 
+/* VRT_priv_task() id offsets */
+enum shard_task_off_e {
+	task_off_param = 0,
+	task_off_cfg = 1
+};
+
 static inline VCL_BACKEND
 sharddir_backend(const struct sharddir *shardd, unsigned id)
 {

lib/libvmod_directors/vmod_directors.vcc

@@ -376,7 +376,7 @@ association.
 The association can be changed per backend request using the *param*
 argument of `xshard.backend()`_.
 
-$Method BOOL .add_backend(PRIV_TASK, BACKEND backend,
+$Method BOOL .add_backend(BACKEND backend,
 	[STRING ident], [DURATION rampup], [REAL weight])
 
 Add a backend *backend* to the director.
@@ -399,28 +399,25 @@ effect of *weight* is also capped such that the total number of
 replicas does not exceed `UINT32_MAX`.
 
 NOTE: Backend changes need to be finalized with
-`xshard.reconfigure()`_ and are only supported on one
-shard director at a time.
+`xshard.reconfigure()`_.
 
-$Method BOOL .remove_backend(PRIV_TASK, [BACKEND backend=0], [STRING ident=0])
+$Method BOOL .remove_backend([BACKEND backend=0], [STRING ident=0])
 
 Remove backend(s) from the director. Either *backend* or *ident* must
 be specified. *ident* removes a specific instance. If *backend* is
 given without *ident*, all instances of this backend are removed.
 
 NOTE: Backend changes need to be finalized with
-`xshard.reconfigure()`_ and are only supported on one
-shard director at a time.
+`xshard.reconfigure()`_.
 
-$Method BOOL .clear(PRIV_TASK)
+$Method BOOL .clear()
 
 Remove all backends from the director.
 
 NOTE: Backend changes need to be finalized with
-`xshard.reconfigure()`_ and are only supported on one
-shard director at a time.
+`xshard.reconfigure()`_.
 
-$Method BOOL .reconfigure(PRIV_TASK, INT replicas=67)
+$Method BOOL .reconfigure(INT replicas=67)
 
 Reconfigure the consistent hashing ring to reflect backend changes.
 

lib/libvmod_directors/vmod_shard.c

@@ -326,11 +326,10 @@ vmod_shard_add_backend(VRT_CTX, struct vmod_directors_shard *vshard,
 			    ".add_backend(weight=%f) ignored", args->weight);
 	}
 
-	return shardcfg_add_backend(ctx, args->arg1,
-	    vshard->shardd, args->backend,
+	return (shardcfg_add_backend(ctx, vshard->shardd, args->backend,
 	    args->valid_ident ? args->ident : NULL,
 	    args->valid_rampup ? args->rampup : nan(""),
-	    weight);
+	    weight));
 }
 
 VCL_BOOL v_matchproto_(td_directors_shard_remove_backend)
@@ -349,23 +348,21 @@ vmod_shard_remove_backend(VRT_CTX, struct vmod_directors_shard *vshard,
 		return (0);
 	}
 
-	return shardcfg_remove_backend(ctx, args->arg1, vshard->shardd,
-	    be, ident);
+	return (shardcfg_remove_backend(ctx, vshard->shardd, be, ident));
 }
 
 VCL_BOOL v_matchproto_(td_directors_shard_clear)
-vmod_shard_clear(VRT_CTX, struct vmod_directors_shard *vshard,
-    struct vmod_priv *priv)
+vmod_shard_clear(VRT_CTX, struct vmod_directors_shard *vshard)
 {
 	CHECK_OBJ_NOTNULL(vshard, VMOD_SHARD_SHARD_MAGIC);
-	return shardcfg_clear(ctx, priv, vshard->shardd);
+	return (shardcfg_clear(ctx, vshard->shardd));
 }
 
 VCL_BOOL v_matchproto_(td_directors_shard_reconfigure)
 vmod_shard_reconfigure(VRT_CTX, struct vmod_directors_shard *vshard,
-    struct vmod_priv *priv, VCL_INT replicas)
+    VCL_INT replicas)
 {
-	return shardcfg_reconfigure(ctx, priv, vshard->shardd, replicas);
+	return (shardcfg_reconfigure(ctx, vshard->shardd, replicas));
 }
 
 static inline uint32_t
@@ -878,12 +875,14 @@ shard_param_task(VRT_CTX, const void *id,
 {
 	struct vmod_directors_shard_param *p;
 	struct vmod_priv *task;
+	const void *task_id;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(pa, VMOD_SHARD_SHARD_PARAM_MAGIC);
 	assert(pa->scope > _SCOPE_INVALID);
 
-	task = VRT_priv_task(ctx, id);
+	task_id = (const char *)id + task_off_param;
+	task = VRT_priv_task(ctx, task_id);
 
 	if (task == NULL) {
 		VRT_fail(ctx, "no priv_task");