Sticky sessions implementation #713

vankoven · 2017-03-22T14:09:50Z

Change log:

fix Sticky sessions scheduler #593 implement sticky sessions scheduler
fix setting unlimited sticky cookie life time
improve stability of unit tests

comment #76 (comment) not implemented in the patch, instead i work on it in separate branch since some discussion may be needed.

Default servers keep-alive configuration affect functional tests, set unlimited keep-alive requests count for functional tests.

keshonok · 2017-03-23T09:27:45Z

tempesta_fw/http_sess.c

@@ -78,12 +79,12 @@ typedef struct {
 	spinlock_t		lock;
 } SessHashBucket;

-static TfwCfgSticky tfw_cfg_sticky;
+static TfwCfgSticky		tfw_cfg_sticky;


This is not a structure where members should be aligned. Why TABs are used here?

keshonok · 2017-03-23T09:37:58Z

tempesta_fw/http_sess.c

 			.name = "sess_lifetime",
 			.deflt = "0",
-			.handler = tfw_cfg_set_int,
+			.handler = tfw_http_sticky_sess_lifetime_cfg,
 			.dest = &tfw_cfg_sticky.sess_lifetime,


The way it is now, there's no need for .dest. You'll have to know anyway which variable you need to use in the .handler function.

In tfw_http_sticky_sess_lifetime_cfg i use tfw_cfg_set_int for parsing int value and check it for min-max borders. To use that i need to set .dest either here or in tfw_http_sticky_sess_lifetime_cfg.

keshonok · 2017-03-23T09:56:00Z

tempesta_fw/http_sess.c

 			.dest = &tfw_cfg_sticky.sess_lifetime,
+			.spec_ext = &(TfwCfgSpecInt) {
+				.range = { 0, INT_MAX },
+			},
 			.allow_none = true,


Perhaps, this is not needed right now. The use crypto_free_shash() dictates that there's a certain clean up procedure for it. That clean up is currently done in exit(). That means that start()/stop(), and a possible reconfigure() are not supported (without exiting the kernel module). Perhaps, it's a good idea to introduce a .cleanup() here and unify that with exit.

How to unify? Currently crypto_shash_setkey() is called twice. Once in init(), and the other time in tfw_http_sticky_secret_cfg(). That presents a small bug already as there's no paired crypto_free_shash() before another crypto_shash_setkey() is called. Now, there should be just one place for that, and that place is in tfw_http_sticky_secret_cfg(). The function should, perhaps, process a default zero length string that can be set as a .deflt = "" value in the configuration specs for the directive.

keshonok · 2017-03-23T10:00:29Z

tempesta_fw/http_sess.c

+	int r = tfw_cfg_set_int(cs, ce);
+
+	/* @sess_lifetime value of 0 means unlimited. */
+	if (!r && !tfw_cfg_sticky.sess_lifetime)


Why a check for tfw_cfg_sticky.sess_lifetime? Is there a meaning for multiple sess_lifetime directives in the configuration file? I believe that you need to set .allow_repeat = false in the specs for the directive.

Can other directives in this file be repeated for any meaningful need?

0 in terms of sess_lifetime means unlimited, but setting actually zero to the parameter makes session to be removed right after it was created. As discussed before, UINT_MAX value should represent unlimited value for the code, but for the user it should remain as 0.

I can rewrite condition as follows if you find that more meaningful:

if (!r && !*(cs->dest)) *(cs->dest) = UINT_MAX;

Yes, that would make this function to NOT depend on the specific variable.
That would also make it generic for getting a number value for a directive, and setting is to max value if the number is zero (also a default value).

keshonok · 2017-03-23T10:22:26Z

tempesta_fw/server.h

 * However, not all the schedulers are able to designate target server group.
 * If a scheduler determines server group, then it should register @sched_grp
 * callback. The callback determines the target server group which references
 * a scheduler responsible to distribute messages in the group.
 * For the avoidance of unnecessary calls, any @sched_grp callback must call
- * @sched_srv callback of the target scheduler.
+ * @sched_sg_conn callback or @sched_srv_conn of the target scheduler.


It's unclear when each of the new callbacks is called and for what purpose. Please write a better description.

keshonok · 2017-03-23T10:54:29Z

tempesta_fw/http_sess.h

+	TfwSrvConn		*srv_conn;
+	TfwSrvGroup		*main_sg;
+	TfwSrvGroup		*backup_sg;
+	rwlock_t		conn_lock;


First of all, conn_lock is confusing as the name suggests that is a lock for a connection. But it's not.

Usually, when a lock protects the whole structure its called just 'lock'. When the lock is used, the preceding part of a statement tells what exactly is protected by the lock, i.e. st_conn->lock. If a lock protects only part of a structure, or a specific member of a structure, then a prefix is useful.

krizhanovsky

Can be merged after cleanups pointed out by the review comments. However, comments #713 (comment) and #713 (comment) are serious, but they can be addressed in further PR implementing #76 requirements.

krizhanovsky · 2017-03-26T22:06:20Z

tempesta_fw/http_sess.c

+static SessHashBucket sess_hash[SESS_HASH_SZ] = {
+//	[0 ... (SESS_HASH_SZ - 1)] = {
+//		HLIST_HEAD_INIT,
+//	}


Don't use C++ style comments. Don't leave dead (just commented out) code. Why did you remove the initialization? I know that it's just zeroying, but using the initialization is still a good practice. At least use the list initializer in tfw_http_sess_init() near to spinlock initialization.

Didn't want C++ comments to appear here. I use IDE with integrated clang code analysis and clang gets stuck on this initialization and the whole IDE hangs every time i make changes to this file. I will be twice more careful on sending patches next time.

krizhanovsky · 2017-03-26T22:06:49Z

tempesta_fw/http_sess.c

@@ -565,7 +566,7 @@ tfw_http_sess_obtain(TfwHttpReq *req)
 			goto found;
 	}

-	if (!(sess = kmem_cache_alloc(sess_cache, GFP_ATOMIC))) {
+	if (!(sess = kmem_cache_zalloc(sess_cache, GFP_ATOMIC))) {


Don't use __GFP_ZERO w/o need, that's relatively expensive thing to make the kernel write zeroes to free pages. Instead, you can just write 3 NULLs to the new pointer fields.

krizhanovsky · 2017-03-26T22:07:16Z

tempesta_fw/http_sess.h

+	unsigned long		expires;
+	TfwStickyConn		st_conn;
+};
+


Extra empty line. Why do you define TfwHttpSess ouside of the file?

In last PR i was advised by @keshonok to move all the session related definitions from http.h to separate header. But TfwHttpSess is required for TfwHttpReq in http.h and most functions in this header require definitions from http.h, so circular dependency take place here.

As for me, advice was very valuable: http.h is too big and placing there more and more definitions only breaks it to loosely connected parts.

krizhanovsky · 2017-03-26T22:24:16Z

tempesta_fw/sched.c

+ * scheduling work flow.
+ */
+static inline TfwSrvConn *
+__get_sticky_srv_conn(TfwMsg *msg, TfwHttpSess *sess)


Basically, sched.c isn't an appropriate place for Sticky cookie scheduling. sched.c is the generic scheduling logic. __get_sticky_srv_conn(), __try_conn() and maybe some other functions are good candidates for http_sess.c or a new file sched/tfw_sched_sticky.c. Also keep in mind #685 which is going to extend the logic even more.

krizhanovsky · 2017-03-26T22:26:41Z

tempesta_fw/sched.c

+	TfwStickyConn *st_conn = &sess->st_conn;
+	TfwSrvConn *srv_conn;
+
+	read_lock(&st_conn->lock);


Why do you use read lock for __try_conn() which basically calls lock-free routines only?

If session is under failovering, connection stored in sessions private data is no more valid. Even if original server is suddenly back online.

Isn't it better to use reference counting to make the session private data valid even under failovering?

krizhanovsky · 2017-03-26T22:34:03Z

tempesta_fw/sched.c

+
+	return srv_conn;
+}
+EXPORT_SYMBOL(tfw_sched_get_sg_srv_conn);


Please define the function before the first usage of it, i.e. before __get_sticky_srv_conn().

krizhanovsky · 2017-03-26T22:43:07Z

tempesta_fw/sched/tfw_sched_hash.c

+	TfwServer		*srv;
+	TfwSrvConn		*conn[TFW_SRV_MAX_CONN];
+	unsigned long		hash[TFW_SRV_MAX_CONN];
+} TfwHashSrv;


Comment https://github.com/tempesta-tech/tempesta/pull/666/files#r94275356 is still not addressed. I saw your PR https://github.com/tempesta-tech/tempesta/pull/714/files, but it's somehow already closed.

vankoven added 19 commits March 22, 2017 17:09

don't ignore return code when adding via header

fe192ea

Readme: add description of sticky sessions

ee925ce

sess: fix parsing sess_lifetime

fad63ed

sched: allow to register same scheduler after removing

ebe3545

sched: add new callback for scheduling from server

f9fe3c6

sched: fix probable hash scheduling error

ee9673a

server: remove unused member

12ed42f

sess: add cfg parser for sticky sessions

37bea03

tests: divide functional and stress tests for RR scheduler

1471751

Default servers keep-alive configuration affect functional tests, set unlimited keep-alive requests count for functional tests.

tests: functional test for cookies support

c23758c

tests: add functional tests for sticky sessions

745911d

sched: fix sticky session sheduling order

0611223

sticky: allow setting sticky options per server group

c5c5f67

tests: improve stability

573b035

srv_group: fix error in initialization

5a8dc2c

sched http: don't read flags of default group until it is created

a636d34

tests: sticky sessions stress tests

7a02d91

update docs

9de5f30

remove #593 todo and fix rebase issue

c5c80dd

keshonok reviewed Mar 24, 2017

View reviewed changes

vankoven added 4 commits March 25, 2017 18:13

fix code according code review

a8c491c

unit tests: optimize sched tests for high loads

cb66fa8

unit tests: don't brake kernel on test failure

3b96b46

sess: fix default value for sticky cookie key

2587f8e

krizhanovsky approved these changes Mar 26, 2017

View reviewed changes

fix code according code review

326a21d

vankoven merged commit dfa12c9 into master Mar 27, 2017

vankoven deleted the ik-sticky-sessions-ng branch March 27, 2017 10:53

vankoven mentioned this pull request Mar 27, 2017

Sticky cookies load balancing #685

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sticky sessions implementation #713

Sticky sessions implementation #713

vankoven commented Mar 22, 2017

keshonok Mar 23, 2017

keshonok Mar 23, 2017

vankoven Mar 24, 2017

keshonok Mar 23, 2017

keshonok Mar 23, 2017

vankoven Mar 24, 2017

keshonok Mar 24, 2017

keshonok Mar 23, 2017

keshonok Mar 23, 2017

krizhanovsky left a comment

krizhanovsky Mar 26, 2017

vankoven Mar 27, 2017

krizhanovsky Mar 26, 2017

krizhanovsky Mar 26, 2017

vankoven Mar 27, 2017

krizhanovsky Mar 26, 2017

krizhanovsky Mar 26, 2017

vankoven Mar 27, 2017

krizhanovsky Mar 27, 2017

krizhanovsky Mar 26, 2017

krizhanovsky Mar 26, 2017

Sticky sessions implementation #713

Sticky sessions implementation #713

Conversation

vankoven commented Mar 22, 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

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

Choose a reason for hiding this comment