MINOR: server: implement delete server cli command

Implement a new CLI command 'del server'. It can be used to removed a
dynamically added server. Only servers in maintenance mode can be
removed, and without pending/active/idle connection on it.

Add a new reg-test for this feature. The scenario of the reg-test need
to first add a dynamic server. It is then deleted and a client is used
to ensure that the server is non joinable.

The management doc is updated with the new command 'del server'.

doc/management.txt

@@ -1609,6 +1609,12 @@ del ssl crt-list <filename> <certfile[:line]>
   you will need to provide which line you want to delete. To display the line
   numbers, use "show ssl crt-list -n <crtlist>".
 
+del server <backend>/<server>
+  Remove a server attached to the backend <backend>. Only valid on a server
+  added at runtime. The server must be put in maintenance mode prior to its
+  deletion. The operation is cancelled if the serveur still has active
+  or idle connection or its connection queue is not empty.
+
 disable agent <backend>/<server>
   Mark the auxiliary agent check as temporarily stopped.
 

reg-tests/server/cli_delete_server.vtc

@@ -0,0 +1,100 @@
+varnishtest "Delete server via cli"
+
+feature ignore_unknown_macro
+
+#REQUIRE_VERSION=2.4
+
+# static server
+server s1 -repeat 3 {
+	rxreq
+	txresp \
+	  -body "resp from s1"
+} -start
+
+# use as a dynamic server, added then deleted via CLI
+server s2 -repeat 3 {
+	rxreq
+	txresp \
+	  -body "resp from s2"
+} -start
+
+haproxy h1 -conf {
+	defaults
+		mode http
+		${no-htx} option http-use-htx
+		timeout connect 1s
+		timeout client  1s
+		timeout server  1s
+
+	frontend fe
+		bind "fd@${feS}"
+		default_backend test
+
+	backend test
+		server s1 ${s1_addr}:${s1_port}
+} -start
+
+# add a new dynamic server to be able to delete it then
+haproxy h1 -cli {
+	# add a dynamic server and enable it
+	send "experimental-mode on; add server test/s2 ${s2_addr}:${s2_port}"
+	expect ~ "New server registered."
+
+	send "enable server test/s2"
+	expect ~ ".*"
+}
+
+haproxy h1 -cli {
+	# experimental mode disabled
+	send "del server test/s1"
+	expect ~ "This command is restricted to experimental mode only."
+
+	# non existent backend
+	send "experimental-mode on; del server foo/s1"
+	expect ~ "No such backend."
+
+	# non existent server
+	send "experimental-mode on; del server test/other"
+	expect ~ "No such server."
+
+	# static server
+	send "experimental-mode on; del server test/s1"
+	expect ~ "Only servers added at runtime via <add server> CLI cmd can be deleted."
+}
+
+# first check that both servers are active
+client c1 -connect ${h1_feS_sock} {
+	txreq
+	rxresp
+	expect resp.body == "resp from s1"
+
+	txreq
+	rxresp
+	expect resp.body == "resp from s2"
+} -run
+
+# delete the dynamic server
+haproxy h1 -cli {
+	# server not in maintenance mode
+	send "experimental-mode on; del server test/s2"
+	expect ~ "Only servers in maintenance mode can be deleted."
+
+	send "disable server test/s2"
+	expect ~ ".*"
+
+	# valid command
+	send "experimental-mode on; del server test/s2"
+	expect ~ "Server deleted."
+}
+
+# now check that only the first server is used
+client c2 -connect ${h1_feS_sock} {
+	txreq
+	rxresp
+	expect resp.body == "resp from s1"
+
+	txreq
+	rxresp
+	expect resp.body == "resp from s1"
+} -run
+

src/server.c

@@ -4468,6 +4468,131 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 	return 1;
 }
 
+/* Parse a "del server" command
+ * Returns 0 if the server has been successfully initialized, 1 on failure.
+ */
+static int cli_parse_delete_server(char **args, char *payload, struct appctx *appctx, void *private)
+{
+	struct proxy *be;
+	struct server *srv;
+	char *be_name, *sv_name;
+
+	if (!cli_has_level(appctx, ACCESS_LVL_ADMIN))
+		return 1;
+
+	++args;
+
+	sv_name = be_name = args[1];
+	/* split backend/server arg */
+	while (*sv_name && *(++sv_name)) {
+		if (*sv_name == '/') {
+			*sv_name = '\0';
+			++sv_name;
+			break;
+		}
+	}
+
+	if (!*sv_name)
+		return cli_err(appctx, "Require 'backend/server'.");
+
+	/* The proxy servers list is currently not protected by a lock so this
+	 * requires thread isolation.
+	 */
+
+	/* WARNING there is maybe a potential violation of the thread isolation
+	 * mechanism by the pool allocator. The allocator marks the thread as
+	 * harmless before the allocation, but a processing outside of it could
+	 * relies on a particular server triggered at the same time by a
+	 * 'delete server'. Currently, it is unknown if such case is present in
+	 * the current code. If it happens to be, the thread isolation
+	 * mechanism should be improved, maybe with a differentiation between
+	 * read and read+write safe sections.
+	 */
+	thread_isolate();
+
+	get_backend_server(be_name, sv_name, &be, &srv);
+	if (!be) {
+		cli_err(appctx, "No such backend.");
+		goto out;
+	}
+
+	if (!srv) {
+		cli_err(appctx, "No such server.");
+		goto out;
+	}
+
+	if (!(srv->flags & SRV_F_DYNAMIC)) {
+		cli_err(appctx, "Only servers added at runtime via <add server> CLI cmd can be deleted.");
+		goto out;
+	}
+
+	/* Only servers in maintenance can be deleted. This ensures that the
+	 * server is not present anymore in the lb structures (through
+	 * lbprm.set_server_status_down).
+	 */
+	if (!(srv->cur_admin & SRV_ADMF_MAINT)) {
+		cli_err(appctx, "Only servers in maintenance mode can be deleted.");
+		goto out;
+	}
+
+	/* Ensure that there is no active/idle/pending connection on the server.
+	 *
+	 * TODO idle connections should not prevent server deletion. A proper
+	 * cleanup function should be implemented to be used here.
+	 */
+	if (srv->cur_sess || srv->curr_idle_conns ||
+	    !eb_is_empty(&srv->pendconns)) {
+		cli_err(appctx, "Server still has connections attached to it, cannot remove it.");
+		goto out;
+	}
+
+	/* TODO remove server for check list once 'check' will be implemented for
+	 * dynamic servers
+	 */
+
+	/* detach the server from the proxy linked list
+	 * The proxy servers list is currently not protected by a lock, so this
+	 * requires thread_isolate/release.
+	 */
+
+	/* be->srv cannot be empty since we have already found the server with
+	 * get_backend_server */
+	BUG_ON(!be->srv);
+	if (be->srv == srv) {
+		be->srv = srv->next;
+	}
+	else {
+		struct server *next;
+		for (next = be->srv; next && srv != next->next; next = next->next)
+			;
+
+		/* srv cannot be not found since we have already found it
+		 * with get_backend_server */
+		BUG_ON(!next);
+		next->next = srv->next;
+	}
+
+	/* remove srv from addr_node tree */
+	ebpt_delete(&srv->addr_node);
+
+	/* remove srv from idle_node tree for idle conn cleanup */
+	eb32_delete(&srv->idle_node);
+
+	thread_release();
+
+	ha_notice("Server %s/%s deleted.\n", be->id, srv->id);
+	free_server(srv);
+
+	cli_msg(appctx, LOG_INFO, "Server deleted.");
+
+	return 0;
+
+out:
+	thread_release();
+
+	return 1;
+}
+
 /* register cli keywords */
 static struct cli_kw_list cli_kws = {{ },{
 	{ { "disable", "agent",  NULL }, "disable agent  : disable agent checks (use 'set server' instead)", cli_parse_disable_agent, NULL },
@@ -4481,6 +4606,7 @@ static struct cli_kw_list cli_kws = {{ },{
 	{ { "get", "weight", NULL }, "get weight     : report a server's current weight",  cli_parse_get_weight },
 	{ { "set", "weight", NULL }, "set weight     : change a server's weight (deprecated)",  cli_parse_set_weight },
 	{ { "add", "server", NULL }, "add server     : create a new server (EXPERIMENTAL)", cli_parse_add_server, NULL, NULL, NULL, ACCESS_EXPERIMENTAL },
+	{ { "del", "server", NULL }, "del server     : remove a dynamically added server (EXPERIMENTAL)", cli_parse_delete_server, NULL, NULL, NULL, ACCESS_EXPERIMENTAL },
 
 	{{},}
 }};