Fix #672: Add health monitoring for backend servers. #877

aleksostapenko · 2017-12-28T21:51:09Z

No description provided.

krizhanovsky

Good to merge after several clenups. Don't forget to close #683 since it isn't mentioned in the patch title.

krizhanovsky · 2018-01-04T16:36:47Z

tempesta_fw/apm.c

 	},
-	{}
+	{ 0 }


I just noticed that APM configuration isn't documented on our Wiki at all, so please update https://github.com/tempesta-tech/tempesta/wiki/Configuration and https://github.com/tempesta-tech/tempesta/wiki/Performance-monitoring chapters. The last one also requires description on the logic from tasks #672 and #683, however probably the chapter for backend servers also must be updated.

Also please create a test issues for #672 and #683. The test must at least test following:

all the new configuration options

a server returning error response must not be scheduled, but only for configured error responses

after some time, when server stops return error responses, it receives requests again

procfs statistic must be verified for enabled and disabled server states

@krizhanovsky Done. Test created. Wiki:

https://github.com/tempesta-tech/tempesta/wiki/Health-monitor created.

https://github.com/tempesta-tech/tempesta/wiki/Servers:-Tempesta's-side updated.

https://github.com/tempesta-tech/tempesta/wiki/Servers-statistics updated.

https://github.com/tempesta-tech/tempesta/wiki/APM created.

krizhanovsky · 2018-01-04T16:43:06Z

tempesta_fw/apm.c

+	char			*req;
+	unsigned long		reqsz;
+	u32			crc32;
+	unsigned short		tmt;


Please place the members to minimize alignment holes

krizhanovsky · 2018-01-04T16:45:52Z

tempesta_fw/apm.c

+	atomic_t		rearm;
+} TfwApmHMCtl;
+
+/* Entry for configureation of separate health monitors. */


"configureation' -> "configuration"

krizhanovsky · 2018-01-04T16:54:40Z

tempesta_fw/http.c

+	INIT_LIST_HEAD(&hm->msg.seq_list);
+	INIT_LIST_HEAD(&((TfwHttpReq *)hm)->fwd_list);
+	INIT_LIST_HEAD(&((TfwHttpReq *)hm)->nip_list);
+	hm->destructor = tfw_http_req_destruct;


The function is partial copy & paste from tfw_http_msg_alloc(). Moreover, it's body is generic enough, but it's named with "hwmonitor" suffix. Please unify it with tfw_http_msg_alloc().

krizhanovsky · 2018-01-04T18:14:30Z

tempesta_fw/apm.c

+	BUG_ON(!srv->apmref);
+	hmctl = &((TfwApmData *)srv->apmref)->hmctl;
+	list_for_each_entry(hm, &tfw_hm_list, list) {
+		if(!strcasecmp(name, hm->name)) {


Please place a space between if and opening brace. Also it's better to write such loops like

if (strcasecmp(name, hm->name)) continue; // the if body

krizhanovsky · 2018-01-04T18:33:22Z

tempesta_fw/http.c

+	if (!req->conn) {
+		tfw_http_msg_free((TfwHttpMsg *)req);
+		return;
+	}


Later it'll be unclear what requests without a connection are, so please write a comment, or, better since there are a few such places, write a small function for the requests deletion.

Corrected. Replaced these pointer checks with TFW_HTTP_HMONITOR request flag verification, according to #877 (comment).

krizhanovsky · 2018-01-04T18:40:01Z

tempesta_fw/str.c

+
+	return 0;
+}
+EXPORT_SYMBOL(tfw_str_crc32_calc);


Please add an appropriate unit test to t/unit/test_tfw_str.c

Removed this function. tfw_str_to_cstr() used instead.

vankoven

Mostly the patch is good, but i don't really like new hmonitor parameter in scheduling functions and frequent checks for requests without origin connections. This should be discussed in comments before merging

vankoven · 2018-01-10T14:14:11Z

tempesta_fw/apm.c

@@ -372,6 +373,103 @@ tfw_stats_update(TfwPcntRanges *rng, unsigned int r_time)
 	return;
 }

+/* Time granularity for HTTP codes accounting during health monitoring. */
+#define HM_FREQ		10
+


Extra empty line is not needed here

vankoven · 2018-01-11T12:37:41Z

tempesta_fw/apm.c


 	BUG_ON(srv->apmref);

-	if (!(data = tfw_apm_create()))
+	if (!(data = tfw_apm_create(hm)))
 		return -ENOMEM;


Not a real issue, just a notice. tfw_apm_create() may fail due to improper input values (hm, tfw_hm_codes_cnt) in addition to lack of free memory. You can pass actual error code in pointer by using ERR_PTR() macro and check it using IS_ERR() macro. It's widely used across kernel and TempestaFW code.

Removed hm argument from tfw_apm_create(), due to reconfiguration changes.

vankoven · 2018-01-11T12:55:21Z

tempesta_fw/apm.c

+	BUG_ON(!hmstats);
+	for (i = 0; i < tfw_hm_codes_cnt; ++i) {
+		if (hmstats[i].hmcfg->code == status ||
+		    hmstats[i].hmcfg->code == (int)(status / 100))


It's not clear for the first sight why we must take into account the first digit in status (status / 100), comment is required here. Also documentation of code field of the TfwApmHMCfg should be extended to cover all possible values: 4* and 5* masks in addition to exact status codes.

Explicit casting to int is not requited since status is already int.

vankoven · 2018-01-11T13:12:06Z

tempesta_fw/apm.c

+
+	if (tfw_http_parse_status(ce->vals[0], &code)) {
+		TFW_ERR_NL("Unable to parse http code value: '%s'\n",
+			   ce->vals[0] ? ce->vals[0] : "No value specified");


ce->vals[0|1|2] is non NULL if tfw_cfg_check_val_n(ce, 3) conditions has met.

vankoven · 2018-01-11T13:46:01Z

tempesta_fw/http.c

@@ -1179,6 +1235,13 @@ tfw_http_conn_resched(struct list_head *sch_queue, struct list_head *equeue)
 					     " an available back end server");
 			continue;
 		}
+
+		if (!req->conn && sch_conn->peer != prev_conn->peer) {


Checking for req->conn here and in many other places seems a little bit hacky. Isn't it better to introduce some flag for TfwHttpReq to indicate that the request was issued by TempestaFW it self and not by the end user.
With that flag you can also remove newly added hmonitor operand in sched_srv_conn() functions.

TempestaFW will generate more requests form it's own in the future, e.g. conditional requests to backend servers, so it's likely that we will need some way to distinguish requests from Tempesta and end clients.

Corrected. TFW_HTTP_HMONITOR request flag introduced.

vankoven · 2018-01-11T13:58:41Z

tempesta_fw/http.c

@@ -1179,6 +1235,13 @@ tfw_http_conn_resched(struct list_head *sch_queue, struct list_head *equeue)
 					     " an available back end server");
 			continue;
 		}
+
+		if (!req->conn && sch_conn->peer != prev_conn->peer) {


It's better to find out first is the request from client or from hmonitor and then chose the right scheduling function: generic scheduling (can be slow) or scheduling to the same server (the fastest way).

vankoven · 2018-01-11T14:03:06Z

tempesta_fw/str.c

+	unsigned long len = 0;
+
+	BUG_ON(str->len && !str->ptr);
+	data = p = kmalloc(str->len, GFP_ATOMIC);


Why not use tfw_str_to_cstr() here?

Yes, missed that handy function. Thanks.
Corrected.

keshonok · 2018-01-19T10:54:31Z

tempesta_fw/apm.c

+	BUG_ON(list_empty(&tfw_hm_list));
+	if (!tfw_hm_entry->codes && !tfw_hm_entry->crc32) {
+		TFW_ERR_NL("Response codes and crc32 values are not"
+			   " configured for '%s'\n", cs->name);


The error message is confusing and doesn't report the actual reason for the error. Both of these configuration options are marked as allow_none which means that they can be skipped. So the actual error here, I believe, is that at least one of these (or both, for that matter) must be configured.

@keshonok Thank you for review.

keshonok · 2018-01-19T11:10:31Z

tempesta_fw/apm.c

+	},
+	{
+		.name		= "request_url",
+		.deflt		= NULL,


I believe, that If the request option has a default value, then it's logical that this option should have a default value as well.

keshonok · 2018-01-19T11:11:30Z

tempesta_fw/apm.c

+	tfw_hm_entry->req = (char *)__get_free_pages(GFP_KERNEL,
+						     get_order(size));
+	if (!tfw_hm_entry->req) {
+		TFW_ERR_NL("Can't allocate memory for helth"


helth -> health

keshonok · 2018-01-19T11:19:29Z

tempesta_fw/apm.c

+
+	if (tfw_cfg_check_val_n(ce, 1))
+		return -EINVAL;
+


My understanding is that these two checks are congenerical - about a very similar thing, simple, and logically in the same realm. Usually there's no need for an empty line between checks like these as they are not logically separated. There's lot of examples in the code for this.

Corrected (and two cases below too).

keshonok · 2018-01-19T11:20:59Z

tempesta_fw/apm.c

+
+	if (tfw_cfg_check_val_n(ce, 3))
+		return -EINVAL;
+


My understanding is that these two checks are congenerical - about a very similar thing, simple, and logically in the same realm. Usually there's no need for an empty line between checks like these as they are not logically separated. There's lot of examples in the code for this.

keshonok · 2018-01-19T11:32:35Z

tempesta_fw/apm.c

+
+	if (tfw_cfg_check_single_val(ce))
+		return -EINVAL;
+


My understanding is that these two checks are congenerical - about a very similar thing, simple, and logically in the same realm. Usually there's no need for an empty line between checks like these as they are not logically separated. There's lot of examples in the code for this.

keshonok · 2018-01-19T12:03:10Z

tempesta_fw/http_msg.c

+		}
+		hm->h_tbl->size = __HHTBL_SZ(1);
+		hm->h_tbl->off = TFW_HTTP_HDR_RAW;
+		memset(hm->h_tbl->tbl, 0, __HHTBL_SZ(1) * sizeof(TfwStr));


These changes leave a strange feeling of being half-done. On one hand, the new argument allows for creating of special messages without the header table that is required for the parser. On the other hand, the parser is still initialized regardless of that in the code that follows. Then, there's tfw_http_msg_alloc_err_resp() that basically does the same thing but it's still there and not removed.

Now the new argument is required for all allocations, and it looks somewhat ugly.

I believe this (and related) code should be refactored. The original top-level interface should stay the same in the form of tfw_http_msg_alloc(int type). A different interface should be introduced for messages that are not going through the parser AND are not modified - which are the messages that are fully prepared from scratch by Tempesta. Perhaps, internally, these two different sets of interfaces may use the same common code - either by calling some inline functions or with macros. tfw_http_msg_alloc_err_resp() would be superseded with the new interface.

Perhaps, you could call it tfw_http_msg_alloc_light() - nothing better comes to mind right now.

keshonok · 2018-01-19T12:12:10Z

tempesta_fw/procfs.c

@@ -206,6 +208,24 @@ tfw_srvstats_seq_show(struct seq_file *seq, void *off)
 			atomic64_read(&srv->sess_n));
 	seq_printf(seq, "Total schedulable connections\t: %zd\n",
 			srv->conn_n - rc);
+
+	seq_printf(seq, "HTTP health monitor is enabled\t: %d\n", hm);
+	if (hm) {


Perhaps, hm_stats should be declared within this block as it's not used elsewhere.

keshonok · 2018-01-19T12:39:06Z

tempesta_fw/server.h

@@ -64,9 +65,15 @@ typedef struct {
 	atomic64_t		refcnt;
 	unsigned int		weight;
 	unsigned int		flags;
+	unsigned int		hm_flags;


Correct me if I am wrong, but my understanding is that both atomic and non-atomic ops can be used on the same variable. There's only a handful of flags, and it's a 32-bit variable. So, perhaps, a separate hm_flags is unnecessary?

Also, I am not sure I understand what's going on here. For atomic access you need an unsigned long variable, but this is an unsigned int. Everywhere in the code it's cast to unsigned long, but I believe that it's incorrect as it would extend to memory occupied by other unrelated data.

We can not mix atomic and non-atomic access to the same data: if one operation doesn't lock the system bus, then it just writes back it's outdated view to the memory location. I agree with wrong type conversions. Also it seems the atomicity on the flags adjustment is broken, see comment for tfw_srv_mark_alive().

In current implementation of set_bit/clear_bit functions for x86, in cases of constant bit index argument - per-byte instructions orb and andb are used. So if for bit index we use immediate value less then 32 - we will not go beyond the int boarders. But yes, this is not very safe approach.
Corrected.

keshonok · 2018-01-19T14:57:56Z

tempesta_fw/http.c

+		}
+		else
+			tfw_http_req_error(srv_conn, req, equeue, 500,
+					   "request dropped: forwarding error");


Curly braces are required here according to the coding standard (because the other branch has them).

keshonok · 2018-01-19T15:02:36Z

tempesta_fw/http.c

+		tfw_http_msg_free((TfwHttpMsg *)req);
+		return;
+	}
+
 	if (reply) {
 		TfwCliConn *cli_conn = (TfwCliConn *)req->conn;
 		tfw_connection_unlink_msg(req->conn);


Not visible here, but curly braces are required here in the else branch.

keshonok · 2018-01-19T15:08:09Z

tempesta_fw/http.c

+	tfw_connection_unlink_msg(resp->conn);
+	tfw_apm_update(((TfwServer *)resp->conn->peer)->apmref,
+		       resp->jrxtstamp,
+		       resp->jrxtstamp - req->jtxtstamp);


This line fits perfectly in the preceding line of code.

keshonok · 2018-01-19T15:19:40Z

tempesta_fw/http.c

-				   "request dropped: forwarding error");
+		if (req->flags & TFW_HTTP_HMONITOR) {
+			tfw_http_req_delist(srv_conn, req);
+			tfw_http_msg_free((TfwHttpMsg *)req);


So, if we're unable to send a health monitoring request, then it's simply dropped without any trace or action upon that? Is that the intended behaviour?

Yeah, maybe having a warning here would be good.

Warning added.

keshonok · 2018-01-19T16:56:44Z

tempesta_fw/sock_srv.c

+		memcpy(health->name, hname, size);
+		srv = orig_srv ? : new_srv;
+	} else {
+		srv = orig_srv;


It's difficult to understand the variety of combinations of function's arguments without comments. orig_srv may be NULL. The code suggest it's definitely NOT NULL when !hname. Why is that? A proper comment would be helpful.

This code has been redesigned.

keshonok · 2018-01-19T17:11:11Z

tempesta_fw/sock_srv.c

+static int
+tfw_cfg_srv_set_health(void)
+{
+	TfwSrvHealth *hth;


There're some common shortcuts that are understood in a specific way. For instance, nth usually mean n-th consecutive number, or something like that. Of course you can name variables any way you like in a small function, but it's best if the name resemble an unambiguous meaning.

There's a number of "rules" for creating short names, one of which is removing all vowels: default -> dflt, etc. With that, it would be health -> hlth. Or just leave it health, it's sufficiently short in this particular case. Or even just h - as long as it can't be confused with h-th number or something. :-)

This code has been redesigned.

keshonok · 2018-01-19T21:46:22Z

tempesta_fw/http.c

+		return false;
+	}
+	return true;
+}


Perhaps, this can be made a simple static const array to avoid unnecessary run-time branching?

keshonok · 2018-01-19T22:03:44Z

tempesta_fw/http.c

-			   ce->vals[0]
-			   ? ce->vals[0]
-			   : "No value specified");
+			   ce->vals[0]);


Can't this message be made clearer? What's 'value'? Perhaps, 'HTTP status code'?

keshonok · 2018-01-19T22:35:15Z

tempesta_fw/http.c

+			return;
+		}
+		tfw_str_to_cstr(&resp->body, body_cstr, resp->body.len + 1);
+		crc32 = crc32(0, body_cstr, resp->body.len);


Frankly, this looks like extra unnecessary work. The whole body of a response is copied to memory allocated from pool just to calculate CRC32 on it.

This is a response to a health monitoring request, so I guess it's relatively small, and because it comes from a server it probably comes in minimal number of chunks (i.e. it's not sent one byte at a time). As far as I can see crc32() function is perfectly suited for calculating the checksum over data that is located in different chunks (the seed argument), so it should be straightforward to write a function that goes over string's chunks and calculates CRC32.

Yes, definitely: with #76 in mind we can have ~1M sites with enabled health monitoring and the copyings can be harmful. We also shouldn't make an assumption about size of health checking response - this is 100% depends on a user preferences. In general, we put a lot of effort for zero-copying - there are plenty of TfwStr and skb routines processing data chunks.

…PR#877).

…#877).

…(PR#877).

krizhanovsky

There are still several issues. hm_flags atomicity is still unclear. Also please write a wiki doc for the next review - it'd be good to review it as well.

krizhanovsky · 2018-02-07T16:38:51Z

tempesta_fw/str.c

+	BUG_ON(len != str->len);
+	return crc;
+}
+EXPORT_SYMBOL(tfw_str_crc32_calc);


The function must be tested in t/unit/test_tfw_str.c, at least that CRC32 calculated for the same data represented in single block and TfwStr's chunks have the same values. Also please calculate a checksum for some data using Linxu crc32 command (as a sysadmin is supposed to use it for calculating HTML file checksums) and use the checkusm in the test - our implementation and the utility must return the same values. Or, the better, to use the utility in the functional thest for HM.

Also please mention the Linux utility crc32 in the Wiki in an example for health monitoring settings.

krizhanovsky · 2018-02-07T16:43:20Z

tempesta_fw/str.c

+	BUG_ON(str->len && !str->ptr);
+	TFW_STR_FOR_EACH_CHUNK(c, str, end) {
+		crc = crc32(crc, c->ptr, c->len);
+		len += c->len;//!!!


I think there is no need to introduce the overhead in the function. There'd be too expensive to check the length consistency in all chunks traversing logic, so only strings creation functions should care about the consistency.

Yes, forgot to remove this debug code. Thanks!
Corrected.

krizhanovsky · 2018-02-07T16:49:37Z

tempesta_fw/t/functional/regression/test_health_monitor.py

+            'request "GET / HTTP/1.1\r\nHost: example.com\r\n\r\n";\n'
+            'request_url "/page.html";\n'
+            'resp_code 200;\n'
+            'resp_crc32 3456;\n'


Please use more realistic HTML response and properly calcucalted, using crc32 Linux tool, CRC32 checksum. The test must verify that CRC32 is calculated by Tempesta FW correctly and that Tempesta FW catches bad responses with wrong CRC32 and marks the server as dead.

@ikoveshnikov and @vladtcvs should we enumerate somewhere all the test with stress options to run them in long running mode on the CI system?

Done. New functional test TestHealthMonitorCRCOnly (for separate CRC32 only verification) is added.

krizhanovsky · 2018-02-07T18:08:31Z

tempesta_fw/http.c

@@ -2879,8 +3044,10 @@ tfw_http_req_key_calc(TfwHttpReq *req)

 	req->hash = tfw_hash_str(&req->uri_path);



There is no sense to fall down for !(req->flags & TFW_HTTP_HMONITOR) and calculate CRC32 over empty data, so just return here.

krizhanovsky · 2018-02-07T18:24:16Z

tempesta_fw/apm.c

+
+	BUG_ON(!srv->apmref);
+	BUG_ON(!hm);
+	BUG_ON(test_bit(TFW_SRV_B_HMONITOR, &srv->hm_flags));


It seems the check should be WARN_ON_ONCE() since failing it desn't lead to the whole system crash, just to misbehaving health monitoring.

krizhanovsky · 2018-02-07T19:36:54Z

tempesta_fw/apm.c

+
+	BUG_ON(!hm);
+	if (hm->codes && test_bit(HTTP_CODE_BIT_NUM(status), hm->codes))
+		return true;


So if a server return us a 'good' response code, then we consider it as alive and don't check the response body. However, missbehaving server can return us 200 responce code with messy response body. Only both the correct response code and the body checksum must be treated as success.

But in case of default auto monitor - it seems that we haven't any acceptable CRC32 value to examine.

Please use crc32 of the first response, but in auto mode only.

However, for applications with dynamic content, it's good to be able to explicitly disable crc32 check. Probably, the easiest way to do this is just explicitly configure auto HM without crc32, e.g.

health_check auto { request "GET / HTTTP/1.0\r\n\r\n"; request_url "/"; resp_code 200; timeout 10; }

Other possibility is to explicitly set auto value for crc32 field if a user want's auto calculated crc32, e.g.:

health_check auto { request "GET / HTTTP/1.0\r\n\r\n"; request_url "/"; resp_code 200; resp_crc32 auto; timeout 10; }

Please update the Wiki about explicit auto configuration and automatic crc32 calculation.

Done (the last approach).

krizhanovsky · 2018-02-07T19:43:23Z

tempesta_fw/server.h

+tfw_srv_suspended(TfwServer *srv)
+{
+	return test_bit(TFW_SRV_B_HMONITOR, &srv->hm_flags)
+		&& test_bit(TFW_SRV_B_SUSPEND, &srv->hm_flags);


TFW_SRV_B_SUSPEND is set in tfw_http_hm_control() after check for TFW_SRV_B_HMONITOR, so TFW_SRV_B_SUSPEND can be set for HM-enabled servers only. test_bit(TFW_SRV_B_HMONITOR, &srv->hm_flags) isn't needed here and can be replaced by WARN_ON_ONCE().

...also see comment for tfw_http_hm_control(). Should the function look like?

unsigned long tmp = READ_ONCE(srv->hm_flags) & (TFW_SRV_B_HMONITOR | TFW_SRV_B_SUSPEND); return tmp == (TFW_SRV_B_HMONITOR | TFW_SRV_B_SUSPEND));

It this way we can read and check both the flags at onece.

Corrected: #877 (comment).

krizhanovsky · 2018-02-07T19:55:47Z

tempesta_fw/server.h

@@ -64,9 +65,15 @@ typedef struct {
 	atomic64_t		refcnt;
 	unsigned int		weight;
 	unsigned int		flags;
+	unsigned long		hm_flags;


I had a look at flags and it's needed for live reconfiguration only, so why not to treat the TFW_CFG_F_* the same way through atomic bit operations? It's really confusing to see two different flags. There is nothing about performance or massive concurrent updates, the only difference is that hm_flags can be ptentially updated concurrently.

Also TFW_SRV_B_HMONITOR is cleared in tfw_server_destroy() -> tfw_apm_del_srv() called from tfw_sock_srv_grace_shutdown_srv(), which sets srv->flags |= TFW_CFG_F_DEL, so probably there is a direct link between TFW_SRV_B_HMONITOR and TFW_CFG_F_DEL flags. @ikoveshnikov could you please also review this?

Done: flags have been merged.

krizhanovsky · 2018-02-07T20:16:23Z

tempesta_fw/http.c

+
+	if (!test_bit(TFW_SRV_B_HMONITOR, &srv->hm_flags))
+		return;
+


...I still don't understand the function intention: we check TFW_SRV_B_HMONITOR at the above and check it again in tfw_srv_suspended() at the calls at the below - is it some kind of protection against concurrent clearing from tfw_apm_hm_disable_srv()? If so then why tfw_apm_hm_disable_srv() can't be called just between test_bit(TFW_SRV_B_HMONITOR, &srv->hm_flags) and test_bit(TFW_SRV_B_SUSPEND, &srv->hm_flags)? Also see comment for tfw_srv_suspended().

What's wrong if at this point of code the server is destroying due to reconfiguration? TFW_SRV_B_SUSPEND seems much easier: it seems the worse can happen is just unnecessary calculation of response body calculation or wrong mark the server as dead (the last one will be quickly fixed by the next HM message). @ikoveshnikov also please review the point.

Functions tfw_http_hm_control() and tfw_srv_mark_suspended() have been redesigned in accordance with discussion in chat.

krizhanovsky · 2018-02-07T22:06:07Z

tempesta_fw/apm.c

+}
+
+bool
+tfw_apm_get_hm(const char *name, void **res_hm)


Why not to return hm? Why do we need the pointer and a boolean return value?

krizhanovsky

Minor fixes are still required. Please if you have some objections about my comments - ping me in the chat.

krizhanovsky · 2018-02-17T21:54:46Z

tempesta_fw/sock_srv.c

@@ -681,7 +694,7 @@ tfw_sock_srv_grace_shutdown_srv(TfwSrvGroup *sg, TfwServer *srv)

 	tfw_server_get(srv);
 	__tfw_sg_del_srv(sg, srv, false);
-	srv->flags |= TFW_CFG_F_DEL;
+	set_bit(TFW_CFG_B_DEL, &srv->flags);


There are several other places which are good to update for the code consistency:

./sock_srv.c:1993: if (!(srv->flags & TFW_CFG_M_ACTION)) { ./sock_srv.c:2000: else if (srv->flags & TFW_CFG_F_MOD) ./sock_srv.c:2012: if (!(srv->flags & TFW_CFG_F_ADD)) ./sock_srv.c:2066: if (!(srv->flags & TFW_CFG_M_ACTION)) ./http_sess.c:757: if (unlikely(srv->flags & TFW_CFG_F_DEL)) {

While technically it's OK to read the flags on x86-64 in this manner, it's better to have to code consistent in using the same API to access the flags.

Corrected, but with TFW_CFG_M_ACTION - left the same as didn't find acceptable interface for bit mask checking.

krizhanovsky · 2018-02-17T22:40:04Z

tempesta_fw/http.c

+				    flags | TFW_SRV_F_SUSPEND);
+		if (likely(old_flags == flags)) {
+			TFW_WARN_ADDR("server has been suspended: limit"
+				      " for bad responses is exceeded",


Please add resp->status to the message to provide more information to a system administrator.

krizhanovsky · 2018-02-17T22:48:38Z

tempesta_fw/apm.c

+
+	BUG_ON(!hm);
+	if (hm->codes && test_bit(HTTP_CODE_BIT_NUM(status), hm->codes))
+		return true;


Please use crc32 of the first response, but in auto mode only.

However, for applications with dynamic content, it's good to be able to explicitly disable crc32 check. Probably, the easiest way to do this is just explicitly configure auto HM without crc32, e.g.

health_check auto { request "GET / HTTTP/1.0\r\n\r\n"; request_url "/"; resp_code 200; timeout 10; }

Other possibility is to explicitly set auto value for crc32 field if a user want's auto calculated crc32, e.g.:

health_check auto { request "GET / HTTTP/1.0\r\n\r\n"; request_url "/"; resp_code 200; resp_crc32 auto; timeout 10; }

Please update the Wiki about explicit auto configuration and automatic crc32 calculation.

krizhanovsky · 2018-02-17T22:53:57Z

tempesta_fw/apm.c

+	if (hm->crc32 && body->len &&
+	    tfw_str_crc32_calc(body) == hm->crc32)
+		return true;
+


Please print here a warning with expected crc32 (needed for auto mode), the response crc32 and response status code.

krizhanovsky

Thanks for the corrections! Good to merge.

vankoven · 2018-02-26T09:34:10Z

tempesta_fw/sched/tfw_sched_hash.c

 {
 	return !tfw_srv_conn_restricted(conn)
 		&& !tfw_srv_conn_queue_full(conn)
-		&& tfw_srv_conn_get_if_live(conn);
+		&& tfw_srv_conn_get_if_live(conn)
+		&& (hmonitor || !tfw_srv_suspended((TfwServer *)conn->peer));


Need to make tfw_srv_suspended() check before any others:

That will reduce number of atomic operations.

An extra reference might be taken in tfw_srv_conn_get_if_live(). The reference won't be released if tfw_srv_suspended failed.

vankoven · 2018-02-26T09:41:40Z

tempesta_fw/sched/tfw_sched_ratio.c

+	 * helth monitoring of backend server.
+	 */
+	if (!(((TfwHttpReq *)msg)->flags & TFW_HTTP_HMONITOR)
+	    && tfw_srv_suspended(srv))


The check can be done before rcu operations: no need trying to schedule request if server is suspended.

vankoven · 2018-02-26T09:50:31Z

tempesta_fw/server.h

@@ -255,6 +272,24 @@ tfw_srv_conn_need_resched(TfwSrvConn *srv_conn)
 	return ((ACCESS_ONCE(srv_conn->recns) >= sg->max_recns));
 }

+/*
+ * Put server into alive or suspended (excluded from processing) state.


Comment is misleading: function can't put server into suspended state.

vankoven · 2018-02-26T10:18:52Z

tempesta_fw/apm.c

+	TfwApmHMStats		*hmstats;
+	atomic64_t		rcount;
+	unsigned long		jtmstamp;
+	struct timer_list	timer;


Sending requests should be done in separate kernel not in timer function. Must be done as a part of #736

vankoven · 2018-02-26T10:33:26Z

tempesta_fw/sock_srv.c

+		if (!(srv->flags & TFW_CFG_M_ACTION))
+			continue;
+
+		tfw_cfgop_update_srv_health(srv, sg_cfg->hm_name, hm);


It was admitted that the main disadvantage of merged ik-51 branch (runtime backend server reconfiguration) was too many traversal across lists. In this patch you traverse the same sg->srv_list twice: here and in tfw_cfgop_update_sg_srv_list(). Can't we do the same job with only one traversal?

vankoven · 2018-02-26T10:56:23Z

tempesta_fw/sock_srv.c

-	TfwSrvGroup *sg = sg_cfg->parsed_sg;
+	void *hm = NULL;
+
+	if (sg_cfg->hm_name && !(hm = tfw_apm_get_hm(sg_cfg->hm_name)))


Nobody checks if user specified available health monitor in tfw_cfgop_health_monitor() or later during cfgend(). In that case we may get an error here, tfw_cfgop_start_sg_cfg() will return -EINVAL and so does tfw_sock_srv_start() . This behaviour is not user-friendly since errors on start are non-recoverable and TempestaFW will be stopped. Too hard penalty for possible misprints as for me.

Instead we must check if the health monitor named sg_cfg->hm_name is available on cfgend stage. Error will be found, a new configuration will be dropped, and the TempestaFW will continue working with old good configuration.

vankoven · 2018-02-26T11:19:57Z

tempesta_fw/apm.c

+ * @rearm	- flag for gracefull stopping of @timer;
+ */
+typedef struct {
+	TfwApmHM		*hm;


Seems at least hmstats, hm are server group-wide options and should be stored there. We can also use one timer per server group: send health check message to all server in group. What do you think? not the comment below.

We cannot make hmstats group-wide - since this will break the essence of the task (server-wide HTTP health statistics and server-wide suspend/alive decisions).
As for hm - yes, theoretically we can use it group-wide, but for now I'd prefer leave it server-wide for three reasons:

Changing current implementation (originally grounded on a server-wide approach) will require significant code changes and re-testing.

In current approach all health monitoring functionality is concentrated in APM module and the there is no need to spreading it between other modules (e.g sock_srv etc.)

Current solution is more flexible and give more granularity in HM management if it will be needed in future, and I do not see significant advantages in group-wide approach except of saving memory on hm pointers for each server.

vankoven

Good to merge! I've commented a couple of moments where performance optimisation is possible.

vankoven · 2018-03-02T15:19:35Z

tempesta_fw/sock_srv.c

+ * and still remain in new configuration too.
+ */
+static void
+tfw_cfgop_update_sg_health(TfwCfgSrvGroup *sg_cfg, void *hm)


It's more like a note to all places where tfw_cfgop_update_srv_health() is called. All servers in the group has the same hmonitor. That means you can check that hmonitor has changed only once, like it done here for scheduler:

tempesta/tempesta_fw/sock_srv.c

Lines 1474 to 1475 in 9381703

if (tfw_cfgop_sched_changed(sg_cfg))

sg_cfg->reconf_flags |= TFW_CFG_MDF_SG_SCHED;

We can add a new flag TFW_CFG_MDF_SG_HMON and set it in tfw_cfgop_setup_srv_group().

Then when we enter tfw_cfgop_update_sg_health() function, we can return immediately if the flag is not set.

Then we update server list in tfw_cfgop_update_sg_srv_list() we need to call tfw_cfgop_update_srv_health() only if the flag set.

the tfw_apm_hm_srv_eq() check in tfw_cfgop_update_sg_health() won't be needed.

This suggestion is only performance optimisation. The only effect is reducing of tfw_apm_hm_srv_eq() calls: once per group instead of once per server.

vankoven · 2018-03-02T15:23:14Z

tempesta_fw/sock_srv.c

+		 * later (in 'tfw_cfgop_update_sg_srv_list()') it
+		 * will be stopped and removed.
+		 */
+		if (!(srv->flags & TFW_CFG_M_ACTION))


The check is obsolete: the function is called now only if tfw_cfgop_update_sg_srv_list() won't be called. Thus srv->flags & TFW_CFG_M_ACTION == TFW_CFG_F_KEEP for all server in group.

vankoven · 2018-03-02T15:33:05Z

tempesta_fw/sock_srv.c

+	if (tfw_cfg_check_single_val(ce))
+		return -EINVAL;
+	if (!tfw_cfgop_sg_set_hm_name(sg_cfg, ce->vals[0])) {
+		TFW_ERR_NL("Unable to add group's health"


The string can be fit in single line.

vankoven · 2018-03-02T15:46:38Z

tempesta_fw/t/functional/regression/test_health_monitor.py

+    )
+    return response
+
+def make_502_expected():


The function is not specific to this test, it's rather generic one. Please move the function to chains.py. Almost the same was done in another PR: https://github.com/tempesta-tech/tempesta/pull/897/files#diff-2395c33b49b27bf69b94b134a3625992R19

vankoven · 2018-03-02T15:59:58Z

tempesta_fw/t/functional/regression/test_health_monitor.py

+        )
+        path = self.tester.get_server_path()
+        stats, _ = self.tempesta.get_server_stats(path)
+        s = r'HTTP availability\s+: (\d+)'


Same here: too generic steps in the function for the test. I'd create a new helper class in tempesta.py, which can help to pass server statistics. Init it with pointer to Tempesta instance, server group and the server name and use like helpers.tempesta.Stats class. No need to parse everything in server stats in this PR, just only needed stats.

@vladtcvs Do you have any suggestions here?

vankoven · 2018-03-02T16:10:16Z

tempesta_fw/t/functional/regression/test_health_monitor.py

+
+
+class TestHealthMonitor(functional.FunctionalTest):
+    """ Test for health monitor functionality with stress option.


I wouldn't say it's 'regression' test. It's definitely test for a independent new feature - health monitoring. Please move the test to it's own directory. We'll need more tests about HTTP availability in future.

Fix #672: Add health monitoring for backend servers.

dd9936d

aleksostapenko requested review from krizhanovsky and vankoven December 28, 2017 21:51

krizhanovsky approved these changes Jan 4, 2018

View reviewed changes

vankoven suggested changes Jan 11, 2018

View reviewed changes

aleksostapenko added 7 commits January 12, 2018 21:33

Merge branch 'master' into ao-672

af183dd

Corrections according review comments (PR#877).

d4a0b12

Correct reschedule procedure for hmonitor requests (PR#877).

fa3a229

Clean temporary debug comments (PR#877).

a509719

Add functional test for health monitor (PR#877).

ac99b7f

Fix minor bug in health monitor tests (PR#877).

3e945cd

Indentation corrections (PR#877).

ac0eadc

keshonok reviewed Jan 19, 2018

View reviewed changes

aleksostapenko added 7 commits February 6, 2018 19:36

Corrections in processing of health monitor atomic flags (PR#877).

3df0ff0

Merge branch 'master' into ao-672

283be0c

Removal of default 'auto' health monitor during configuration error (…

d380608

…PR#877).

Correct processing of health monitoring request in hash scheduler (PR…

cc00f36

…#877).

Change 'test_health_monitor' to support group-wide health monitoring …

98f4072

…(PR#877).

Make separate 'health' directive for server group (PR#877).

9ab7b1d

Add documentation for health monitoring directives (PR#877).

5017d88

aleksostapenko added the ready for review label Feb 7, 2018

krizhanovsky requested changes Feb 7, 2018

View reviewed changes

Corrections according review comments (PR#877).

2e613c3

krizhanovsky requested changes Feb 17, 2018

View reviewed changes

Change crc32 processing and minor corrections (PR#877).

b1cc532

krizhanovsky approved these changes Feb 19, 2018

View reviewed changes

Replace codes array of HM with dynamic allocated one (PR#877).

7ae00c9

vankoven suggested changes Feb 26, 2018

View reviewed changes

aleksostapenko mentioned this pull request Feb 27, 2018

Early bind of response and request #884

Merged

aleksostapenko removed the ready for review label Feb 27, 2018

aleksostapenko added 3 commits February 28, 2018 20:37

Corrections according review comments (PR#877).

86c48aa

Merge branch 'master' into ao-672

662a5b7

Additional functional tests created (PR#877).

8b3fafa

aleksostapenko added the ready for review label Feb 28, 2018

Avoid repeating list traversals during configuration (PR#877).

aa29321

vankoven approved these changes Mar 2, 2018

View reviewed changes

aleksostapenko added 3 commits March 4, 2018 15:31

Merge branch 'master' into ao-672

6e7ddc8

Corrections according review comments (PR#877).

e973ddb

Corrections in health monitor functional tests (PR#877).

0f31190

aleksostapenko merged commit 93263ee into master Mar 4, 2018

aleksostapenko deleted the ao-672 branch March 4, 2018 16:33

aleksostapenko removed the ready for review label Mar 4, 2018

aleksostapenko mentioned this pull request Mar 4, 2018

Upstream servers performance and health checking #683

Closed

		@@ -2879,8 +3044,10 @@ tfw_http_req_key_calc(TfwHttpReq *req)

		req->hash = tfw_hash_str(&req->uri_path);

	if (tfw_cfgop_sched_changed(sg_cfg))
	sg_cfg->reconf_flags \|= TFW_CFG_MDF_SG_SCHED;



		class TestHealthMonitor(functional.FunctionalTest):
		""" Test for health monitor functionality with stress option.

Fix #672: Add health monitoring for backend servers. #877

Fix #672: Add health monitoring for backend servers. #877

Conversation

aleksostapenko commented Dec 28, 2017

krizhanovsky left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Feb 12, 2018 • 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

Choose a reason for hiding this comment

aleksostapenko Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

vankoven 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

aleksostapenko Jan 18, 2018 • 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

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

keshonok Jan 19, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Feb 7, 2018 • 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

keshonok Jan 19, 2018 • 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

krizhanovsky left a comment

krizhanovsky left a comment •

edited

Loading

aleksostapenko Feb 12, 2018 •

edited

Loading

aleksostapenko Jan 18, 2018 •

edited

Loading

aleksostapenko Jan 18, 2018 •

edited

Loading

keshonok Jan 19, 2018 •

edited

Loading

aleksostapenko Feb 7, 2018 •

edited

Loading

keshonok Jan 19, 2018 •

edited

Loading