Live reconfiguration #851

vankoven · 2017-10-28T09:29:53Z

Live reconfiguration, only core features are supported in reconfiguration:

Adding and removing server groups
adding and removing servers in server groups
adding more connections to each server (removing of connection are not supported)
adding and removing http match rules
changing any server group options
changing scheduler assigned to a server group

Known limitations/issues:

can't reduce number of server connections

To start live reconfiguration simply run ./scripts/tempesta.sh --reload or write sysctl variable sysctl -w net.tempesta.state=start while Tempesta FW is running.

A server group was added to tempesta's internal lists before it was fully filled with data and before the data was verified. The patch separates these steps. A server group is filled with data first, the data is verified, and only then the group is added to Tempesta.

'call_counter' served two functions. One was to mark specs of directives that were found in configuration file, and attempt to apply default values to directives whose specs were not marked. The other was to indicate specs of directives that required a cleanup as memory may had been allocated. However, `call_counter` was cleared in some cases (read: nested specs), which broke the function of a cleanup. Two different flags are created __called_now and __called_ever, each serving its specific function.

That makes it so much easier to navigate, analyze, and modify.

The patch makes it possible to "start" Tempesta when it's running already. With that, a new configuration file may be supplied, provided that Tempesta can handle a live reconfiguration.

Tempesta can now handle multiple consecutive starts. In case of errors it cleans up correctly. The actual live reconfiguration of any directive is not there yet.

The function is called after complete configuration file is parsed and pushed to all modules. It gives the opportunity to cross-check configuration data, or set up other data based on configuration data before the system is started.

krizhanovsky · 2017-10-28T17:23:32Z

scripts/tempesta.sh

@@ -35,7 +35,7 @@ tfw_mod=tempesta_fw
 tfw_sched_mod=tfw_sched_$sched


Please update copyright year at the above

krizhanovsky · 2017-10-28T17:46:10Z

scripts/tempesta.sh

+		--reload)
+			reload
+			exit
+			;;


Please move Build, Run & Stop and Configuration sections from README.md to the Wiki (leaving reference to the Wiki section(s) in README.md) and add description of the new command.

krizhanovsky · 2017-10-28T18:21:44Z

tempesta_fw/cfg.c

 			return -EINVAL;
 		}

 		ret = spec_handle_entry(matching_spec, &ps->e);
-		entry_reset(&ps->e);
 		if (ret)
 			return ret;


If we return here or at the above, who calls entry_reset(ps), i.e. free()'s all the entry items? I didn't get the sense of 71545e6 (how the change is linked with 'show errors'), but it seems the change introduces memory leak.

entry_reset() is called here:

tempesta/tempesta_fw/cfg.c

Line 1513 in cb39ace

entry_reset(&ps.e);

The 71545e6 commit is for showing erroneous line number and line content for nested directives in print_parse_error(). E.g. for the config:

srv_group A { server 127.0.0.1:8000 error_string; }

Current master will produce the next error message:

[tempesta] ERROR: Invalid number of arguments: 2 [tempesta] ERROR: configuration parsing error

While the current branch gives more helpful error message:

[tempesta] ERROR: Invalid number of arguments: 2 [tempesta] ERROR: configuration parsing error: 2: server 127.0.0.1:8000 error_string; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

krizhanovsky · 2017-10-28T18:46:39Z

tempesta_fw/server.c

+		__tfw_sg_for_each(&sg_list_reconfig, list_reconfig);
+	}
+	else {
+		__tfw_sg_for_each(&sg_list, list);


In all use cases tfw_sg_for_each_srv() is called with true or false as reconfig value, so actually there are two functions aggregated together with the boolean argument. It's better to write 2 normal functions with only one argument.

I'm not sure whether it has sense to move 2-lines loop to the macro, but the macro makes the function bit more complex: the first impression is that cb is unused, moreover since the macro is global it seems like it's used also in some other place but actually it isn't. Please move the macro definition into the funcrion with undef at the end of the function or just replace the macro calls by the loop - not a big deal to copy & paste 2 lines of code.

krizhanovsky · 2017-10-28T18:59:08Z

tempesta_fw/server.c

 */
 TfwSrvGroup *
-tfw_sg_lookup(const char *name)
+__tfw_sg_lookup(const char *name, bool reconfig)


The function is always called with explicit true or false as a value of reconfig, so please split the function to two real functions.

krizhanovsky · 2017-10-30T00:01:01Z

tempesta_fw/server.h

@@ -79,10 +81,11 @@ typedef struct {
 */
 struct tfw_srv_group_t {
 	struct list_head	list;
+	struct list_head	list_reconfig;


Please comment the item above. Need to answer why list isn't enough, i.e. when a group can be referenced from sg_list and be in sg_list_reconfig at the same time.

krizhanovsky · 2017-10-30T00:20:29Z

tempesta_fw/sock_srv.c

+
+	/* Some servers was removed, so scheduler update is required */
+	if (sg_cfg->orig_sg &&
+	    (sg_cfg->orig_sg->srv_n != sg_cfg->parsed_sg->srv_n)) {


Please place opening curly braces on next line for complex if conditions

krizhanovsky · 2017-10-30T00:23:17Z

tempesta_fw/sock_srv.c

+	list_for_each_entry(srv, &sg_cfg->parsed_sg->srv_list, list) {
+		if (tfw_addr_eq(&srv->addr, &orig_srv->addr))
+			break;
+	}


There is no need curly braces

krizhanovsky · 2017-10-30T00:24:40Z

tempesta_fw/sock_srv.c

+		/*
+		 * TODO: shrink connections: disconnect connection and remove
+		 * it from server's conn_list
+		 */


It's a bad thing to leave server connection open - it could lead to serious memory leakage. Why don't you use tfw_sock_srv_disconnect()?

The function was called later in one loop for all removed servers. I've reworked this to call disconnect() as soon as possible.

I didn't understand your comment: if you reworked the code, then why the condition and the comment are still here?

krizhanovsky · 2017-10-30T00:30:17Z

tempesta_fw/sock_srv.c

+	int r;
+
+	INIT_LIST_HEAD(&sg_cfg_list);
+	if ((r = tfw_cfgop_new_sg_cfg_default()))


It seems it fixes #748. If so, please mark 1ab95a2 as the fix for the issue, so it will be closed on merge.

Actually no. Default server group is created to store various server group options as global defaults. Implicit default server group is not registered as server group available after reconfig until cfgend().

…ions

…m group

Sticky sessions may became invalid after live reconfiguration, since pinned server or group can be removed from the configuration.

Don't remove back reference from server to server group since it's required in server connection callbacks.

@keshonok

BUG_ON is not needed here. This was found by @keshonok, see #692 (comment) for details.

aleksostapenko · 2017-11-17T14:03:41Z

tempesta_fw/sock_srv.c

+ * invalid.
+ *
+ * On configuration parsing stage binary representation of a server group is
+ * saved into @parsed_sg, while @orig_sg poins to the existing server group


poins -> points

aleksostapenko · 2017-11-17T14:32:44Z

tempesta_fw/sched/tfw_sched_http.c

+	tfw_http_match_list_free(tfw_rules_reconfig);
+	tfw_rules_reconfig = NULL;
+
+	if (tfw_runstate_is_reconfig())


If I correctly understand - this code will be executed after failed reconfiguration (on parse stage). And in this case Tempesta should continue working with old configuration, but we release old tfw_rules here. Maybe condition should be inverted...

Yes, you're absolutely right, i'll fix that

…ing of sg default opts

…is used

aleksostapenko · 2017-12-17T07:29:22Z

tempesta_fw/server.c

+{
+	TfwServer *srv;
+
+	write_lock(&sg->lock);


It seems that read_lock/unlock is not used anywhere with sg->lock - maybe using spin_lock with sg->lock will be simpler.

Hm, frankly speaking I don't see write operation here - this is just a list scan which is supposed to be a read operation. It seems read lock should be used here.

aleksostapenko · 2017-12-17T08:02:35Z

tempesta_fw/server.c

+}
+
+/**
+ * Iterate over all server groups of given server grop @sg and call @cb for


server groups -> servers
grop - > group

aleksostapenko · 2017-12-17T08:04:46Z

tempesta_fw/server.c

@@ -202,7 +395,26 @@ __tfw_sg_for_each_srv(TfwSrvGroup *sg, int (*cb)(TfwServer *srv))
 }

 /**
- * Iterate over all server groups and call @cb for each server.
+ * Iterate over all the acive server groups and call @cb for each server group.


acive -> active

aleksostapenko · 2017-12-17T08:22:37Z

tempesta_fw/sock_srv.c

+		.name = "sticky_sessions",
+		.deflt = TFW_CFG_DFLT_VAL,
+		.handler = tfw_cfgop_in_sticky_sess,
+		.cleanup = tfw_cfgop_cleanup_srv_groups,


As I understand cleanup callbacks is not called for nested srv_group options specs. Does it make sense to define these callbacks?

aleksostapenko · 2017-12-17T08:25:48Z

tempesta_fw/sock_srv.c

+ * stage since the new configuration is provided by a user and may contain
+ * errors.
+ * - applying stage: tfw_sock_srv_start(). Errors are unrecoverable and mean
+ * that only a part of changes was appied, so the resulting configuration is


appied -> applied

aleksostapenko · 2017-12-17T08:45:22Z

tempesta_fw/sock_srv.c

 		return -EINVAL;
-	tfw_cfg_out_slstsz++;
+	}


If in config file a server outside any group is defined at first and then default group is defined explicitly - is this an acceptable case?

No, it's not acceptable. Once a server is defined outside of any group, default group will be registered as available after reconfiguration. If the explicit default group is defined after that, tfw_cfgop_begin_srv_group() will throw a Group '...' already exists error.

Yes - got it. Thanks.

aleksostapenko · 2017-12-17T08:58:42Z

tempesta_fw/sock_srv.c

+		setup_timer(&srv->gs_timer, tfw_sock_srv_grace_shutdown_cb,
+			    (unsigned long)srv);
+		mod_timer(&srv->gs_timer, jiffies + tfw_cfg_grace_time * HZ);
+		tfw_sock_srv_grace_list_add(srv);


It seems safer to place tfw_sock_srv_grace_list_add(srv) before mod_timer(&srv->gs_timer, ...).

aleksostapenko · 2017-12-17T09:01:00Z

tempesta_fw/server.h

@@ -41,20 +41,30 @@ typedef struct tfw_scheduler_t TfwScheduler;
 * Server descriptor, a TfwPeer successor.
 *
 * @list	- member pointer in the list of servers of a server group;
+ * @gs_timmer	- grace shutdown timer;


gs_timmer -> gs_timer

krizhanovsky

Good to merge after small fixes and cleanups (Yahoo!)

krizhanovsky · 2017-12-28T20:32:08Z

tempesta_fw/http.c

+
+	tfw_http_conn_evict_timeout(srv_conn, &equeue);
+	if (unlikely(resched))
+		tfw_http_conn_collect(srv_conn, &sch_queue, &equeue);


This isn't about this particular patch set, but is about the whole recent changes: just too many list traversals and many of them are under lock. Such these two functions both traverse forward list: one to evict timed out requests and the second one to collect the rest of them. Why not to do this in one shot? Let's leave it for #687 .

krizhanovsky · 2017-12-28T20:55:39Z

tempesta_fw/sock_srv.c

+	else if (orig_srv->conn_n > srv->conn_n) {
+		/*
+		 * TODO: shrink number of connections. Disconnects are performed
+		 * asynchronously, can't destroy connection here and now.


I still didn't get why for example we can't schedule the connections for eviction, i.e. it's still unclear why we can't implement the logic now and have to leave with the TODO. Please add a requirement to #687 - let's put the TODOs from the PR there.

When we need to shrink number of connections to a given server, we must do two things: disconnect and destroy some connections; and update scheduler data for that server. The scheduler data is normally common for the whole server group.

Disconnecting of connections is an asynchronous operation. So we have to keep connections in srv->conn_list until disconnect is completed. In the same time right after scheduling for disconnects we use all connections from srv->conn_list to build scheduler data. Dead connections poison scheduler, since number of scheduling attempts is limited.

UPD: Somehow i missed a possible solution: we can check TFW_CONN_B_DEL flag when building scheduler data to skip connections scheduled to disconnect. Any way that can be done as a separate task.

krizhanovsky · 2017-12-28T21:39:22Z

tempesta_fw/sock_srv.c

+ * @tfw_cfg_sg_def since options of explicitly defined 'default' group must not
+ * be changed when user updates default server group options.
+ */
+static TfwCfgSrvGroup *tfw_cfg_sg_opts = NULL;


I'm wondering if there is a functional test for #748? The issue has clear configuration example, but I couldn't quickly find the example in the tests.

The problem with the current test system is that all the tested scenarios are expressed in Python and it's quite difficult to find a particular case. Meantime, e.g. MariaDB uses SQL-like (the language of the pruduct) templates to express the test cases, for example https://github.com/tempesta-tech/mariadb/blob/trunk/mysql-test/suite/versioning/t/select.test . If you just familiar with SQL you can quickly get an idea which case is covered by the test.

Can we do the same for out test system? It'd be great to have some configuration templates with some high level commands like start Nginx with some configuration, start N instances of ab and so on. @intelfx @vladtcvs opinions?

test functional/reconf/test_stress_sched_http.py covers #748 and reconfiguration of http scheduler in the same time.

krizhanovsky · 2017-12-28T21:50:33Z

tempesta_fw/server.c

+{
+	TfwServer *srv;
+
+	write_lock(&sg->lock);


Hm, frankly speaking I don't see write operation here - this is just a list scan which is supposed to be a read operation. It seems read lock should be used here.

krizhanovsky · 2017-12-29T15:53:09Z

tempesta_fw/server.c

+tfw_sg_destroy(TfwSrvGroup *sg)
+{
+	TFW_DBG2("release group: '%s'\n", sg->name);
+	BUG_ON(!list_empty(&sg->srv_list));


Do we definitely crash on this condition? We saw many crashes due to such checks and some of them don't actually mean inevitable crashes. It's better to use WARN() to notify us (developers) or a user about some problem. Also note https://github.com/tempesta-tech/linux-4.9.35-tfw/blob/master/include/asm-generic/bug.h#L37 . @aleksostapenko also please note this.

krizhanovsky · 2017-12-29T16:10:34Z

tempesta_fw/sock_srv.c

+		/* Server was not found in new configuration. */
+		if (!(srv->flags & TFW_CFG_M_ACTION)) {
+			if ((r = tfw_sock_srv_grace_shutdown_srv(sg, srv)))
+				return r;


Need to release sg->lock

krizhanovsky · 2017-12-29T16:17:27Z

tempesta_fw/sock_srv.c

 		}
+		else if (srv->flags & TFW_CFG_F_MOD)
+			if ((r = tfw_cfgop_update_srv(srv, sg_cfg)))
+				goto err;


Need to release sg->lock

krizhanovsky · 2017-12-29T16:48:22Z

tempesta_fw/sock_srv.c

+	while (1)
+	{
+		TfwServer *srv;
+		int act;


Please place open curly brace on a next line only in case of complex multi-line conditions and place it on the same line in all other cases.

Do you really need act variable? Doesn't

if (del_timer_sync(&srv->gs_timer)) tfw_sock_srv_grace_stop(srv);

look simpler?

keshonok and others added 17 commits October 17, 2017 17:27

Split module management from configuration processing.

8e68713

Bring all configuration specs to a single style.

a983acd

That makes it so much easier to navigate, analyze, and modify.

Make tempesta.sh work gracefully when Tempesta is started already.

03ffa4d

The patch makes it possible to "start" Tempesta when it's running already. With that, a new configuration file may be supplied, provided that Tempesta can handle a live reconfiguration.

Let the parsing code handle default values for 'apm_stats' directive.

fbcf0de

Handle multiple consecutive Tempesta starts (without stops).

b1b5fd9

Tempesta can now handle multiple consecutive starts. In case of errors it cleans up correctly. The actual live reconfiguration of any directive is not there yet.

cfg: show errors on processing nested specs

71545e6

cfg: rename cfgfin to cfgend

e36efce

sched: rcu sched_data

294e3cf

sock_srv: allow reconfiguration for all options

1ab95a2

sched_http: allow reconfiguration

c65af40

sock_clnt: run checks on reconfig

6aea925

minor fixups in according to live reconfig

f9f1bd7

scripts: add live reconfiguration command

a5de8f1

unit tests: update tests to support modules with live reconfiguration

20d519c

krizhanovsky requested changes Oct 30, 2017

View reviewed changes

vankoven added 12 commits October 30, 2017 16:11

Merge branch 'master' into ik-51

1527c4f

sg management: split reconfig-aware functions into two separate funct…

e637527

…ions

sock_srv: update cleanup and remove duplicated code and add doc

4d5ab48

procfs: export server statistics by server groups directories

73db1ee

sched: assign scheg_data only when the full data is ready

783df9d

sched: fix missinng rcu_dereference

00786ad

fix code review comments

6c58a0c

sock_srv: disconnect server connections as soon as server removed fro…

bd29178

…m group

rework sticky session scheduling

9b3c0ce

Sticky sessions may became invalid after live reconfiguration, since pinned server or group can be removed from the configuration.

sock_srv: add garbage collector for orphaned servers

5755578

fix server removal on the fly

094bf46

Don't remove back reference from server to server group since it's required in server connection callbacks.

ss_skb: fix crash on GRO skb processing

800e03c

BUG_ON is not needed here. This was found by @keshonok, see #692 (comment) for details.

aleksostapenko reviewed Nov 17, 2017

View reviewed changes

vankoven added 8 commits November 21, 2017 14:49

fix code review comments by @aleksostapenko

21f57e7

fix #748. register default server group at first sight, correct handl…

f1e86d6

…ing of sg default opts

control lifetime of TfwServer and TfwSrvGroup by refcounters

dc7548f

move cleanup of closed connection to separate function

c616c1b

grace shutdown for servers

c58e825

Merge branch 'master' into ik-51

8ec3603

update reload option in systemd script

526cfe1

immediately stop servers on tempesta shutdown even if grace shutdown …

3179566

…is used

aleksostapenko force-pushed the ik-51 branch from 3179566 to cc61a75 Compare December 3, 2017 17:10

aleksostapenko reviewed Dec 17, 2017

View reviewed changes

vankoven added 2 commits December 18, 2017 12:04

fix code review comments

f2bb7a9

sock_srv: remove unused cleanup callbacks

9acf4af

aleksostapenko approved these changes Dec 29, 2017

View reviewed changes

krizhanovsky approved these changes Dec 29, 2017

View reviewed changes

krizhanovsky mentioned this pull request Dec 29, 2017

Improve the architecture that supports the correct order of HTTP responses #687

Open

fix code review comments by @krizhanovsky

f668db6

vankoven merged commit 3f49ada into master Dec 29, 2017

vankoven deleted the ik-51 branch December 29, 2017 18:53

krizhanovsky mentioned this pull request Dec 29, 2017

DSL for test scenarios #878

Closed

krizhanovsky added a commit that referenced this pull request Feb 15, 2018

#687: fix #851 (comment) - do less list traversals under fwd_qlock

1cb0cb4

i-rinat mentioned this pull request Jun 6, 2019

[cfg] Make sure Tempesta doesn't specify itself as a backend #774

Closed

		@@ -35,7 +35,7 @@ tfw_mod=tempesta_fw
		tfw_sched_mod=tfw_sched_$sched

Live reconfiguration #851

Live reconfiguration #851

Conversation

vankoven commented Oct 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Nov 17, 2017 •

edited

Loading

aleksostapenko Dec 17, 2017 •

edited

Loading