Hi HAproxy community,
Let's assume that HAproxy starts with non-zero values for close-spread-time
and hard-stop-after, and soft-stop is used to initiate the shutdown during
deployments.
There are times where we need to modify these values, eg, in case the
operator needs to restart HAproxy faster, but still use the soft-stop
workflow.
So, it will be nice to have the ability to modify these values via CLI.
RFC:-
- Does this seem like a fair use-case?
- If this is good, I can also work on the patch for hard-stop-after.
Patch questions:-
- I've added reg-tests under a new folder "cli" because I was unable to
find a better match. Let me know if that should be moved.
- If that's a concern, there is code duplication b/w proxy.c [1] and this
cli.c. I am unable to find a file where we can create a utility function.
Mostly, the concern is to modify "global.tune.options" whenever
"global.close_spread_time" changes.
- I noticed global struct is accessed without any locks, is that like a
"known" race condition for these simple operations? I don't primarily
develop C code, and this was confusing to me.
Please find the patch attached with this email.
1.
https://github.com/haproxy/haproxy/blob/70251a2aeb5930f3fc25aadf979d5ce5007d0f9d/src/proxy.c#L2072
--
Cheers,
Abhijeet (https://abhi.host)
From 896d3a100faa8369928c080d5aa67f9cb44204c4 Mon Sep 17 00:00:00 2001
From: Abhijeet Rastogi <abhijeet.1989@gmail.com>
Date: Mon, 8 Apr 2024 15:30:44 -0700
Subject: [PATCH] MINOR: cli: add option to modify close-spread-time
close-spread-time is only set during config parse stage, and then there
is no API available today to modify it.
If there is a requirement to speed-up the soft-stop for HAproxy, it is
not possible to dynamically lower this value before initiating
soft-stop. This CLI feature now provides ability to modify the
close-spread-time value dynamically, after HAProxy process has already
started.
A new `notice` message is also added to showcase the current value of
close-spread-time when it is non-zero.
New reg-tests dir `cli` is created as there is currently no appropriate
director where this test falls in.
---
reg-tests/cli/cli_set_close_spread_time.vtc | 55 ++++++++++++++++++++
src/cli.c | 56 +++++++++++++++++++++
src/proxy.c | 2 +
3 files changed, 113 insertions(+)
create mode 100644 reg-tests/cli/cli_set_close_spread_time.vtc
diff --git a/reg-tests/cli/cli_set_close_spread_time.vtc b/reg-tests/cli/cli_set_close_spread_time.vtc
new file mode 100644
index 000000000..33db4915b
--- /dev/null
+++ b/reg-tests/cli/cli_set_close_spread_time.vtc
@@ -0,0 +1,55 @@
+varnishtest "Set close-spread-time via CLI"
+
+feature ignore_unknown_macro
+
+# for "set close-spread-time <time>"
+# for "get close-spread-time"
+#REGTEST_TYPE=devel
+
+# Do nothing. Is there only to create s1_* macros
+server s1 {
+} -start
+
+haproxy h1 -conf {
+ global
+ close-spread-time 10s
+
+ defaults
+ mode http
+ timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout client "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout server "${HAPROXY_TEST_TIMEOUT-5s}"
+
+ frontend myfrontend
+ bind "fd@${my_fe}"
+ default_backend test
+
+ backend test
+ server www1 ${s1_addr}:${s1_port}
+} -start
+
+haproxy h1 -cli {
+ # Starts with 10s
+ send "get close-spread-time"
+ expect ~ "close-spread-time=10000ms"
+
+ send "set close-spread-time 1s"
+ expect ~ ""
+ send "get close-spread-time"
+ expect ~ "close-spread-time=1000ms"
+
+ # Disabling close-spread-time is possible
+ send "set close-spread-time 0"
+ expect ~ ""
+ send "get close-spread-time"
+ expect ~ "close-spread-time=0"
+
+ # Negative value is error
+ send "set close-spread-time -1"
+ expect ~ "Invalid"
+ send "set close-spread-time 25d"
+ expect ~ "Timer overflow"
+ send "get close-spread-time"
+ expect ~ "close-spread-time=0"
+} -wait
+
diff --git a/src/cli.c b/src/cli.c
index 02cb06843..4939efe94 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -1716,6 +1716,60 @@ static int cli_parse_show_fd(char **args, char *payload, struct appctx *appctx,
return 0;
}
+/* parse a "get close_spread_time" CLI request. It always returns 1. */
+static int cli_parse_get_close_spread_time(char **args, char *payload, struct appctx *appctx, void *private)
+{
+ char *output = NULL;
+
+ if (!cli_has_level(appctx, ACCESS_LVL_ADMIN))
+ return 1;
+
+ memprintf(&output, "close-spread-time=%dms\n", global.close_spread_time);
+
+ cli_dynmsg(appctx, LOG_INFO, output);
+
+ return 1;
+}
+
+
+/* parse a "set close_spread_time" CLI request. It always returns 1. */
+static int cli_parse_set_close_spread_time(char **args, char *payload, struct appctx *appctx, void *private)
+{
+ const char *res;
+
+ if (!cli_has_level(appctx, ACCESS_LVL_ADMIN))
+ return 1;
+
+ if (!*args[2])
+ return cli_err(appctx, "Expects an integer value or infinite.\n");
+
+ if (strcmp(args[2], "infinite") == 0) {
+ global.tune.options |= GTUNE_DISABLE_ACTIVE_CLOSE;
+ global.close_spread_time = TICK_ETERNITY;
+ return 1;
+ } else if (strcmp(args[2], "0") == 0) {
+ global.tune.options &= ~GTUNE_DISABLE_ACTIVE_CLOSE;
+ global.close_spread_time = 0;
+ return 1;
+ }
+
+ res = parse_time_err(args[2], &global.close_spread_time, TIME_UNIT_MS);
+ if (res == PARSE_TIME_OVER) {
+ return cli_err(appctx, "Timer overflow, maximum possible value 2147483647");
+ }
+ else if (res == PARSE_TIME_UNDER) {
+ return cli_err(appctx, "Timer underflow, min is too small");
+ }
+ else if (res) {
+ return cli_err(appctx, "Invalid close-spread-time value.\n");
+ }
+
+ global.tune.options &= ~GTUNE_DISABLE_ACTIVE_CLOSE;
+
+ return 1;
+}
+
+
/* parse a "set timeout" CLI request. It always returns 1. */
static int cli_parse_set_timeout(char **args, char *payload, struct appctx *appctx, void *private)
{
@@ -3576,6 +3630,8 @@ static struct cli_kw_list cli_kws = {{ },{
{ { "set", "anon", "on" }, "set anon on [value] : activate the anonymized mode", cli_parse_set_anon, NULL, NULL },
{ { "set", "anon", "off" }, "set anon off : deactivate the anonymized mode", cli_parse_set_anon, NULL, NULL },
{ { "set", "anon", "global-key", NULL }, "set anon global-key <value> : change the global anonymizing key", cli_parse_set_global_key, NULL, NULL },
+ { { "set", "close-spread-time", NULL }, "set close-spread-time <time> : change the close-spread-time setting", cli_parse_set_close_spread_time, NULL, NULL },
+ { { "get", "close-spread-time", NULL }, "get close-spread-time : get the close-spread-time setting", cli_parse_get_close_spread_time, NULL, NULL },
{ { "set", "maxconn", "global", NULL }, "set maxconn global <value> : change the per-process maxconn setting", cli_parse_set_maxconn_global, NULL },
{ { "set", "rate-limit", NULL }, "set rate-limit <setting> <value> : change a rate limiting value", cli_parse_set_ratelimit, NULL },
{ { "set", "severity-output", NULL }, "set severity-output [none|number|string]: set presence of severity level in feedback information", cli_parse_set_severity_output, NULL, NULL },
diff --git a/src/proxy.c b/src/proxy.c
index 92c5df143..2fe320ae9 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -2151,6 +2151,8 @@ static void do_soft_stop_now()
if (tick_isset(global.close_spread_time)) {
global.close_spread_end = tick_add(now_ms, global.close_spread_time);
+ ha_notice("close-spread-time is currently set to %dms\n",
+ global.close_spread_time);
}
/* schedule a hard-stop after a delay if needed */
--
2.34.1
