[master] 52237ffc6 Improve thread pool parameters error messages

Poul-Henning Kamp phk at FreeBSD.org
Mon Oct 21 11:56:06 UTC 2019


commit 52237ffc68b36de94c56693ffe2d61460b32e4af
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date:   Wed Oct 16 12:03:12 2019 +0200

    Improve thread pool parameters error messages
    
    Inform users when the parameter we try to change depends on another
    parameter.
    
    This is not meant to be merged as-is, we could have a new VSB function
    to trim the end of the string if it matches the one passed as the second
    argument. In this case, we would trim the trailing newline only of the
    CLI response didn't overflow in the first place, adding the extra
    information on the same line. Alternatively we could also simply add the
    explanation for the dynamic bounds on the next line.
    
    Feedback welcome.
    
    Fixes #3098

diff --git a/bin/varnishd/mgt/mgt_pool.c b/bin/varnishd/mgt/mgt_pool.c
index da2eab1ca..5184ea147 100644
--- a/bin/varnishd/mgt/mgt_pool.c
+++ b/bin/varnishd/mgt/mgt_pool.c
@@ -57,26 +57,64 @@ static int
 tweak_thread_pool_min(struct vsb *vsb, const struct parspec *par,
     const char *arg)
 {
+	enum tweak_e tweak;
 
-	if (tweak_generic_uint(vsb, par->priv, arg, par->min, par->max))
-		return (-1);
-	MCF_ParamConf(MCF_MINIMUM, "thread_pool_max",
-	    "%u", mgt_param.wthread_min);
-	MCF_ParamConf(MCF_MAXIMUM, "thread_pool_reserve",
-	    "%u", mgt_param.wthread_min * 950 / 1000);
-	return (0);
+	tweak = tweak_generic_uint(vsb, par->priv, arg, par->min, par->max);
+
+	if (tweak == TWEAK_OK) {
+		MCF_ParamConf(MCF_MINIMUM, "thread_pool_max",
+		    "%u", mgt_param.wthread_min);
+		MCF_ParamConf(MCF_MAXIMUM, "thread_pool_reserve",
+		    "%u", mgt_param.wthread_min * 950 / 1000);
+		return (0);
+	}
+
+	if (arg != JSON_FMT && tweak == TWEAK_ABOVE_MAX) {
+		vsb->s_len--; /* XXX: VSB_trim(vsb, "\n"); instead? */
+		VSB_cat(vsb, " (thread_pool_max)\n");
+	}
+
+	return (-1);
 }
 
 static int
 tweak_thread_pool_max(struct vsb *vsb, const struct parspec *par,
     const char *arg)
 {
+	enum tweak_e tweak;
+
+	tweak = tweak_generic_uint(vsb, par->priv, arg, par->min, par->max);
+
+	if (tweak == TWEAK_OK) {
+		MCF_ParamConf(MCF_MAXIMUM, "thread_pool_min",
+		    "%u", mgt_param.wthread_max);
+		return (0);
+	}
+
+	if (arg != JSON_FMT && tweak == TWEAK_BELOW_MIN) {
+		vsb->s_len--; /* XXX: VSB_trim(vsb, "\n"); instead? */
+		VSB_cat(vsb, " (thread_pool_min)\n");
+	}
+
+	return (-1);
+}
+
+static int
+tweak_thread_pool_reserve(struct vsb *vsb, const struct parspec *par,
+    const char *arg)
+{
+	enum tweak_e tweak;
+
+	tweak = tweak_generic_uint(vsb, par->priv, arg, par->min, par->max);
+	if (tweak == TWEAK_OK)
+		return (0);
+
+	if (arg != JSON_FMT && tweak == TWEAK_ABOVE_MAX) {
+		vsb->s_len--; /* XXX: VSB_trim(vsb, "\n"); instead? */
+		VSB_cat(vsb, " (95% of thread_pool_min)\n");
+	}
 
-	if (tweak_generic_uint(vsb, par->priv, arg, par->min, par->max))
-		return (-1);
-	MCF_ParamConf(MCF_MAXIMUM, "thread_pool_min",
-	    "%u", mgt_param.wthread_max);
-	return (0);
+	return (-1);
 }
 
 /*--------------------------------------------------------------------*/
@@ -120,7 +158,8 @@ struct parspec WRK_parspec[] = {
 		"Minimum is 10 threads.",
 		DELAYED_EFFECT,
 		"100", "threads" },
-	{ "thread_pool_reserve", tweak_uint, &mgt_param.wthread_reserve,
+	{ "thread_pool_reserve", tweak_thread_pool_reserve,
+		&mgt_param.wthread_reserve,
 		NULL, NULL,
 		"The number of worker threads reserved for vital tasks "
 		"in each pool.\n"
diff --git a/bin/varnishtest/tests/r03098.vtc b/bin/varnishtest/tests/r03098.vtc
new file mode 100644
index 000000000..a4d3fee67
--- /dev/null
+++ b/bin/varnishtest/tests/r03098.vtc
@@ -0,0 +1,25 @@
+varnishtest "Explain how param.(re)?set may fail"
+
+# NB: we don't need or want to start the cache process
+
+varnish v1 -cliok "param.set thread_pool_max 10000"
+varnish v1 -cliok "param.set thread_pool_min  8000"
+
+# NB: "varnish v1 -cliexpect" wouldn't work with a non-200 status
+shell -err -expect "Must be at least 8000 (thread_pool_min)" {
+	exec varnishadm -n ${v1_name} param.reset thread_pool_max
+}
+
+varnish v1 -cliok "param.set thread_pool_min  8"
+varnish v1 -cliok "param.set thread_pool_max 10"
+
+shell -err -expect "Must be no more than 10 (thread_pool_max)" {
+	exec varnishadm -n ${v1_name} param.reset thread_pool_min
+}
+
+varnish v1 -cliok "param.reset thread_pool_max"
+varnish v1 -cliok "param.reset thread_pool_min"
+
+shell -err -expect "Must be no more than 95 (95% of thread_pool_min)" {
+	exec varnishadm -n ${v1_name} param.set thread_pool_reserve 96
+}


More information about the varnish-commit mailing list