MEDIUM: server: allow to remove servers at runtime except non purgeable

Relax the condition on "delete server" CLI handler to be able to remove
all servers, even non dynamic, except if they are flagged as non
purgeable.

This change is necessary to extend the use cases for dynamic servers
with reload. It's expected that each dynamic server created via the CLI
is manually commited in the haproxy configuration by the user. Dynamic
servers will be present on reload only if they are present in the
configuration file. This means that non-dynamic servers must be allowed
to be removable at runtime.

The dynamic servers removal reg-test has been updated and renamed to
reflect its purpose. A new test is present to check that non-purgeable
servers cannot be removed.

5 files changed, 155 insertions, 56 deletions

diff --git a/doc/management.txt b/doc/management.txt
index a6baf8e2d..58d06e48c 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1470,6 +1470,11 @@ add server <backend>/<server> [args]*
   init-addr method. This means that no resolution will be undertaken if a FQDN
   is specified as an address, even if the server creation will be validated.
 
+  To support the reload operations, it is expected that the server created via
+  the CLI is also manually inserted in the relevant haproxy configuration file.
+  A dynamic server not present in the configuration won't be restored after a
+  reload operation.
+
   A dynamic server may use the "track" keyword to follow the check status of
   another server from the configuration. However, it is not possible to track
   another dynamic server. This is to ensure that the tracking chain is kept
@@ -1786,10 +1791,11 @@ del ssl crt-list <filename> <certfile[: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.
+  Remove a server attached to the backend <backend>. All servers are eligible,
+  except servers which are referenced by other configuration elements. 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.
diff --git a/reg-tests/server/cli_delete_dynamic_server.vtc b/reg-tests/server/cli_delete_dynamic_server.vtc
new file mode 100644
index 000000000..b1ddc4f1d
--- /dev/null
+++ b/reg-tests/server/cli_delete_dynamic_server.vtc
@@ -0,0 +1,98 @@
+# This script is to test the proper behavior with dynamic servers insertion and
+# deletion, in particular with the load-balancing of requests.
+#
+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
+		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."
+}
+
+# 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
+
diff --git a/reg-tests/server/cli_delete_server.vtc b/reg-tests/server/cli_delete_server.vtc
index 7b92d7146..d752d6f8c 100644
--- a/reg-tests/server/cli_delete_server.vtc
+++ b/reg-tests/server/cli_delete_server.vtc
@@ -1,24 +1,21 @@
+# This script is to test the ability to remove servers, unless they are
+# referenced by some elements from the configuration.
+#
 varnishtest "Delete server via cli"
 
 feature ignore_unknown_macro
 
 #REQUIRE_VERSION=2.4
 
-# static server
-server s1 -repeat 3 {
+server s1 {
 	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"
+	txresp
 } -start
 
 haproxy h1 -conf {
+	global
+		lua-load ${testdir}/get_srv_stats.lua
+
 	defaults
 		mode http
 		timeout connect 1s
@@ -27,22 +24,19 @@ haproxy h1 -conf {
 
 	frontend fe
 		bind "fd@${feS}"
+		acl s1_full srv_sess_rate(test/s1) gt 50
 		default_backend test
 
 	backend test
-		server s1 ${s1_addr}:${s1_port}
+		http-request add-header s4-stats %[lua.get_srv_stats(s4)]
+		use-server s3 unless { always_false }
+		server s1 ${s1_addr}:${s1_port}          # referenced in ACL
+		server s2 ${s1_addr}:${s1_port} check    # referenced in track
+		server s3 ${s1_addr}:${s1_port} track s2 # referenced in use-server
+		server s4 ${s1_addr}:${s1_port}          # referenced in lua script
+		server s5 ${s1_addr}:${s1_port}          # removable server
 } -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"
@@ -56,44 +50,37 @@ haproxy h1 -cli {
 	send "experimental-mode on; del server test/other"
 	expect ~ "No such server."
 
-	# static server
+	# server referenced in ACL
 	send "experimental-mode on; del server test/s1"
-	expect ~ "Only servers added at runtime via <add server> CLI cmd can be deleted."
+	expect ~ "This server cannot be removed at runtime due to other configuration elements pointing to it."
+
+	# tracked server
+	send "experimental-mode on; del server test/s2"
+	expect ~ "This server cannot be removed at runtime due to other configuration elements pointing to it."
+
+	# tracked server
+	send "experimental-mode on; del server test/s3"
+	expect ~ "This server cannot be removed at runtime due to other configuration elements pointing to it."
 }
 
-# first check that both servers are active
+# make a request to force the execution of the lua script which references a
+# server
 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"
+	send "experimental-mode on; del server test/s4"
+	expect ~ "This server cannot be removed at runtime due to other configuration elements pointing to it."
+
+	send "experimental-mode on; del server test/s5"
 	expect ~ "Only servers in maintenance mode can be deleted."
 
-	send "disable server test/s2"
+	send "disable server test/s5"
 	expect ~ ".*"
 
 	# valid command
-	send "experimental-mode on; del server test/s2"
+	send "experimental-mode on; del server test/s5"
 	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
-
diff --git a/reg-tests/server/get_srv_stats.lua b/reg-tests/server/get_srv_stats.lua
new file mode 100644
index 000000000..105b9546e
--- /dev/null
+++ b/reg-tests/server/get_srv_stats.lua
@@ -0,0 +1,11 @@
+local function lua_get_srv_stats(txn, name)
+	for _, backend in pairs(core.backends) do
+		for _, server in pairs(backend.servers) do
+			if server.name == name then
+				return server:get_stats()
+			end
+		end
+	end
+end
+
+core.register_fetches('get_srv_stats', lua_get_srv_stats)
diff --git a/src/server.c b/src/server.c
index e055fc2c1..f0228be81 100644
--- a/src/server.c
+++ b/src/server.c
@@ -4741,14 +4741,11 @@ static int cli_parse_delete_server(char **args, char *payload, struct appctx *ap
 		goto out;
 	}
 
-	if (!(srv->flags & SRV_F_DYNAMIC)) {
-		cli_err(appctx, "Only servers added at runtime via <add server> CLI cmd can be deleted.");
+	if (srv->flags & SRV_F_NON_PURGEABLE) {
+		cli_err(appctx, "This server cannot be removed at runtime due to other configuration elements pointing to it.");
 		goto out;
 	}
 
-	/* A dynamic server cannot be tracked. */
-	BUG_ON(srv->trackers);
-
 	/* 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).