If you do a generic connection pool system please allow for a max uses before forcing a reconnection. My desire to see this is primarily that after a failure of say an LDAP server it can take some unnecessary work to force a more balanced usage of servers behind load balancers. On 23 Jun 2011, at 17:28, Alan DeKok <aland@deployingradius.com> wrote:

Phil Mayers wrote:

...

So, some discussion on the JANET-ROAMING list leads me to believe that, during an "ldap server down" condition, rlm_ldap will incur "net_timeout" on every (or many) passes through the module.

It's better for the module to track when connections are down, and return quickly if all are down.

...

I don't really understand the MAX_FAILED_* logic at the start of perform_search, but it seems to conflict with the comments at the top of the file:

* If conn->failed_conns > MAX_FAILED_CONNS_START then we don't * try to do anything and we just do conn->failed_conns++ and * return RLM_MODULE_FAIL

Yeah...

...

...perform_search has no such logic; in any event, it seems like it would be better to do an optional time-based per-server "fast fail" so that:

redundant { ldap1 ldap2 }

...fails quickly if ldap1 is down.

Sure. That should be easy to do.

...

In some ways it's a shame we can't use a worker thread to manage the LDAP connection(s); that way, the module could be marked "fast fail" unless and until a live connection exists. Is there any scope for that?

I'd really like 3.0 to have generic connection pools. That would solve this problem by having common code, instead of stuff in rlm_sql, rlm_ldap, etc.

Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

On 23/06/11 17:28, Alan DeKok wrote:

I'd really like 3.0 to have generic connection pools. That would solve this problem by having common code, instead of stuff in rlm_sql, rlm_ldap, etc.

Do you have any pointers how to get started on this? Off the top of my head it seems we'd need something like the code below; a struct to hold module-supplied connection create/keepalive/delete functions, some code in the server core to set and re-set "last used" times and call a keepalive function, and delete typedef struct { int alive; void *conn; /* mutex stuff */ } fr_pool_conn; typedef struct { int pool_min; int pool_max; int pool_idle; fr_pool_conn *conns; void* (*conn_create)(void *arg); int (*conn_keepalive)(void *conn, void *arg); int (*conn_delete)(void *conn, void *arg); void *arg; } fr_conn_pool; ...and then something like: void* ldap_conn_create(void *arg) { ldap_instance *instance = arg; LDAP *blah = ldap_init(...) ldap_bind(); if (fail) return NULL; return blah; } int ldap_conn_keepalive(void *conn, void *arg) { LDAP *blah = conn; ldap_instance *inst = arg; /* send some noop search * or e.g. "select 1" in rlm_sql */ return 0 } int ldap_conn_delete(void *conn, void *arg) { /* connection unused, close it */ } int ldap_init(...) { inst->ldap_pool = fr_pool_create( 5, /* min connection */, 15, /* max connection */, 60, /* idle time - if num > min, reap connections */, ldap_conn_create, ldap_conn_keepalive, ldap_conn_delete, inst /* argument to conn_{create,keepalive,delete} ) } int perform_search() { ldap_instance *inst = ...; /* timeout argument - 0 == pool - seconds or microseconds? */ LDAP *conn = fr_pool_get(&inst->ldap_pool, 0); /* stuff fr_pool_release(&inst->ldap_pool, conn); }

On 06/28/2011 08:01 PM, Alexander Clouter wrote:

Phil Mayers<p.mayers@imperial.ac.uk> wrote:

...

...

I'd really like 3.0 to have generic connection pools. That would solve this problem by having common code, instead of stuff in rlm_sql, rlm_ldap, etc.

Do you have any pointers how to get started on this? Off the top of my head it seems we'd need something like the code below; a struct to hold module-supplied connection create/keepalive/delete functions, some code in the server core to set and re-set "last used" times and call a keepalive function, and delete

I probably would not bother with keep alive (or 'last used'). I would

The idea of the keepalive was not to "hold the connection open" at the TCP layer. It's to detect dead server(s) in a timely fashion i.e. hopefully _before_ someone tries to run a radius packet through the module. But now that I think about it, it won't work as I envisaged. The "open" and "keepalive" call on a connection may block, so can't be run in the main event loop - it needs to be in a thread. You don't want to just connect on-demand, since the whole point is that a pool with connections==0 will fast-fail and let "redundant {}" do its work. It needs a worker thread, or a proper async LDAP API, which libldap doesn't have sadly :o(

imagine in practice your 'idle' time should be shorter that any NAT or server daemon concept of idleness? What I'm trying to say is the cost of an open idle link is low, tearing it down and rebuilding it is...if the connection has been idle for a long time the server (or NAT/firewall) would have probably killed it.

If you really want keep alives, it probably would be better to go for SO_KEEPALIVE (as NOOP as you can get)? No doubt this would have to be done in the driver rather than the layer you are constructing?

Maybe. Depends if the driver layer offers you any ability to tweak underlying TCP parameters. But as I say, I'm not concerned about TCP connections; I'm concerned about detecting dead servers. And SO_KEEPALIVE is way, way too slow to detect dead servers.

As a passing not, I susepect you do not care for async LDAP queries?

On the contrary, I really like async LDAP queries, and async/event driven architectures in general. My preferred network programming framework is Python Twisted, which is completely non-blocking event/callback driven Problem is, async LDAP queries are a little (!) more work to implement in a threadpool-based server like FreeRADIUS: * put LDAP query params into struct * call query function; this allocates a semaphore, locks a queue, puts the query & semaphore into the queue, unlocks then waits on the semaphore * a separate worker thread/threadpool continually locks/pulls requests off/unlocks the queue, issues them & stores the LDAP msgid, then polls in a loop over ldap_result(); as each message comes back, it finds the corresponding query, copies a pointer to the result and flags the semaphore Basically you have run another thread/pool AFACIT. And I was hoping to avoid that, as it seems to me likely to risk stomping all over FreeRADIUS carefully crafted and testing internals. It's a real shame that libldap doesn't offer a better way to integrate the open LDAP TCP socket into a select()-based loop. Some projects go the whole hog and fork() a child process to do their LDAP queries e.g. sssd, but this is just tedious; you have to marshal the LDAP query and results across process boundaries.

It's probably the only database FreeRADIUS supports that supports this anyway so probably not worth thinking about.

Postgres, at least, can be used in async mode. And it can cooperate with select()

On 06/23/2011 05:28 PM, Alan DeKok wrote:

Phil Mayers wrote:

...

So, some discussion on the JANET-ROAMING list leads me to believe that, during an "ldap server down" condition, rlm_ldap will incur "net_timeout" on every (or many) passes through the module.

It's better for the module to track when connections are down, and return quickly if all are down.

So: https://github.com/philmayers/freeradius-server/commit/58e545bd183029da9cdb1... ...this is *not* a connection pool, but an example of one way to solve the problem; spawn a child thread to create connections. I'm aware the code as-is has big problems but it might inspire something more useful; off the top of my head: * the new "failure" flag to ldap_release_conn is used too aggressively, meaning rlm_ldap will drop a connection in some cases it doesn't need to * it doesn't touch the eDir code - I don't have a way to test it * there's no way to terminate and re-start the connection manager thread * the connection-manager thread does not obey the "-s" command line argument * it uses a dumb sleep() rather than semaphore to wake and commence re-connects ...and probably lots more. Related to this, connection re-binding in the non-async case should probably live inside ldap_get_conn and move out of perform_search() and siblings. But the diff as-is is hopefully easier to read. This patch also doesn't solve "LDAP-Group == X" pointing at one and only one module. One possible way to solve that is as per Alex suggestion, to manage the TCP connections ourselves (which we could do inside the worker thread) and when people pass in >1 hostname to the module, do some kind of round-robin / fastest-wins connection algorithm. Comments welcome.

On 29/06/11 18:48, John Dennis wrote:

an observation). I kinda think it might be worthwhile to start with a clean slate, write down the requirements for the module and write it cleanly from scratch to match the requirements.

Maybe; but then you've got the problem of a) throwing away a lot of code and having to rewrite it, and having the time to rewrite it and b) ensuring you understand all the use-cases that people put it to, and either accounting for them or providing methods of doing the same. I guess you could rename rlm_ldap to rlm_ldapold and create a new rlm_ldap with some "helpers" defined in policy.conf, but it seems like a lot of work. Additionally, given the confusion that reigns on the main list sometimes from old FAQs, I suspect it would cause havoc in terms of support.

...

* it doesn't touch the eDir code - I don't have a way to test it

Perhaps a bit off topic for this discussion, but I always thought it was dubious to have special code for a specific LDAP server in FreeRADIUS. I

Examining the code, it seems that eDir needs specific LDAP controls to extract the cleartext password; grumble. So I suspect it needs to stay.

Side comment on server models:

Sorry, forgot who said this in the last couple of days, but they endorsed the event loop driven asynchronous model. After working for

I think that was me ;o)

many years on a variety of servers I too have come to believe event loop driven architectures are superior in contrast to forking children, spawning threads, etc. Anything we've written recently follows the event loop model. It's not perfect by any means but it gets rid of a lot of nasty problems and IMHO the resulting code simplier and easier to

Obviously I agree, but a lot of people don't. Some people hate the way the callback model breaks up the flow control for anything involving composite operations. It also means you need protocol implementations that will let you take over the networking - drive the socket receive/transmit loop yourself, and feed data into/out of the protocol, generating protocol PDU callbacks back into your app. Sadly (from my point of view), those are very rare. TBH I don't think it matters that much to anyone except the developers what the underlying processing model is - users interact with the module processing, string-expansion and unlang features, not the select() loop. On that latter topic: I do wonder if we might start running into problems with fd_set size limitations now that "master" supports TCP sockets. Eduroam sites (e.g. us) using radius-over-TLS and DNS-based autodiscovery could, conceivably, have many tens of TCP connections open at any given time. But an epoll event core is probably pretty trivial, given that Alan has factored the existing loop cleanly.

On 30/06/11 16:45, Alexander Clouter wrote:

Phil Mayers<p.mayers@imperial.ac.uk> wrote:

...

On that latter topic: I do wonder if we might start running into problems with fd_set size limitations now that "master" supports TCP sockets. Eduroam sites (e.g. us) using radius-over-TLS and DNS-based autodiscovery could, conceivably, have many tens of TCP connections open at any given time. But an epoll event core is probably pretty trivial, given that Alan has factored the existing loop cleanly.

epoll is for when you have 1000's of FD's...not less than 100. epoll is

I'll let JP reply for me: http://stackoverflow.com/questions/2032598/caveats-of-select-poll-vs-epoll-r... tl;dr - select has a microscopic advantage over epoll for very tiny numbers of FDs, but it's negligible in modern systems. epoll scales better and has far, far better worst-case performance, as well as handling many idle connections better. There's absolutely no discernible issue with using epoll over select in any app except a micro-benchmark designed to test syscall latency in the case with small numbers of FDs. So, I disagree completely ;o)

On 30/06/11 20:28, Alexander Clouter wrote:

Phil Mayers<p.mayers@imperial.ac.uk> wrote:

...

There's absolutely no discernible issue with using epoll over select in any app

...so you will be showing me example epoll() code for Windows, Solaris and *BSD eh? ;)

Sure: http://monkey.org/~provos/libevent/ Why invent when you can steal? Or even "use"?

...

except a micro-benchmark designed to test syscall latency in the case with small numbers of FDs.

I never mentioned benchmarks :P

I was more muttering on the portability front, the path of #ifdef madness is a wicked one...udpfromto.c anyone (free beer to the person who finds the worst example of addition to #ifdef)?

Well, I did prefix my remarks with "if": """I do wonder if we might start running into problems with ...""" So the whole following sentence was predicated on the assumption that problems would arise; if they did "#ifdef is hard let's go shopping" is hardly a suitable response. Maybe it will never come up; maybe the usage patterns will mean select is fine for the foreseeable future. But: * If Josh and his ABFAB lot get their way, copies of FreeRADIUS will be serving as super-Shibboleth authenticators for all your users distributed web auth needs; 1000 open FDs seems like a pretty conservative number in that event, and; * If a windows native port is to work, there will probably need to be #ifdef code around a different event loop core for win32 anyway, since select is rubbish on that platform (63 FDs, IIRC?) Anyway, offtopic as you say. My last post on the subject.

John Dennis <jdennis@redhat.com> wrote:

Sorry, forgot who said this in the last couple of days, but they endorsed the event loop driven asynchronous model. After working for many years on a variety of servers I too have come to believe event loop driven architectures are superior in contrast to forking children, spawning threads, etc. Anything we've written recently follows the event loop model. It's not perfect by any means but it gets rid of a lot of nasty problems and IMHO the resulting code simplier and easier to understand, which means less bugs. It's too big a change for FreeRADIUS but I thought I would at least endorse the previous comment.

I generally agree, it's what I use too, but there are lots of cases where the spawn thread/fork model is a better idea. Software such as syslog-ng is currently limited to just a single CPU, apache would be pretty horrible without, etc. Really boils down to if the task can benefit from parallelism and is CPU bound rather than IO. I suspect though you were not making a sweeping statement for all applications though :) Cheers -- Alexander Clouter .sigmonster says: Taxes are not levied for the benefit of the taxed.

On 06/29/2011 05:59 PM, Phil Mayers wrote:

On 06/23/2011 05:28 PM, Alan DeKok wrote:

...

Phil Mayers wrote:

...

So, some discussion on the JANET-ROAMING list leads me to believe that, during an "ldap server down" condition, rlm_ldap will incur "net_timeout" on every (or many) passes through the module.

It's better for the module to track when connections are down, and return quickly if all are down.

So:

https://github.com/philmayers/freeradius-server/commit/58e545bd183029da9cdb1...

...and for completion, more or less orthogonal, an implementation of Alex's idea - needs some autoconf love to detect the ldap_init_fd private function in libldap (the local prototype and #define seem to be required - both curl and samba do that, as the function is private) https://github.com/philmayers/freeradius-server/commit/f17db6f5e8e71a71d96b7... Dinner time I think!