[SSSD] LDAP connection tracking, sharing and fail-over retry framework

Eugene Indenbom eindenbom at gmail.com
Thu Jun 17 12:32:20 UTC 2010


On 06/16/2010 06:20 PM, Stephen Gallagher wrote:
> Comments inline below. This is a purely code-inspection review at the
> moment. Functional testing will be performed once we get the code in shape.
>
> On 06/16/2010 02:43 AM, Eugene Indenbom wrote:
>    
>> The patches attached provide a framework for LDAP connection tracking,
>> sharing and fail-over retries in LDAP ID backend, which is designed to
>> address the tickets #468, 464, 465 and 466.
>>
>> The patches are structured as follows:
>>
>> 0001-GSSAPI-ticket-expiry-time-is-returned-from-ldap_chil.patch
>> A small utility patch saving GSSAPI ticket expiry time in sdap_handle
>> for future use.
>>
>>      
> You need to pass&expire_time into ldap_child_get_tgt_sync(). Right now
> you're passing a time_t instead of a time_t *
>    
Oops, thanks for catching it. I have missed a compiler warning as I am 
used to compiling with -Werror option and forgot to do a full rebuild 
before sending patches out. Also as I have mentioned before, I haven't 
tested the patches after merging from 1.2 code base to master. :(

> Minor nitpicks: We prefer that pointers be initialized to NULL, rather
> than 0 explicitly. In parse_child_response you initialize to zero. Also,
> talloc_free() is NULL-aware, so it's unnecessary to wrap it in an if
> (ccn != 0) {} block. Finally, I don't see the need for the goto cleanup
> here. There is no other way to jump here, so it makes more sense to me
> to just call talloc_free() inside the error handling block and return
> from there.
>
>    
Done.
>> 0002-Added-an-interface-to-query-number-of-configured-and.patch
>> Another utility patch allowing to dynamically determine number of
>> fail-over servers (including resolved through SRV records)
>>
>>      
> Looks straightforward. Ack.
>
>    
>> 0003-LDAP-connection-usage-tracking-sharing-and-failover-.patch
>> The main patch -- LDAP connection tracking, sharing and fail-over retry
>> framework
>>      
> sdap_cli_connect_recv_new() is unnecessary. It should replace
> sdap_cli_connect_recv() (and retain the old name).
I have added a new function to minimize changes. I have planned to 
remove old sdap_cli_connect_recv (and rename sdap_cli_connect_recv_new) 
when all code will be converted to new framework.

> Also, change the
> sdap_cli_connect_recv_new() function to check for NULL before assigning
> to can_retry (making it an optional return). Then this function is just
> a drop-in replacement for anywhere it's currently used, just adding a
> NULL argument.
>    
Done.
> If sdap_id_release_conn_data() is called by
> sdap_id_conn_cache_be_offline_cb() and returns without releasing the
> connection (because of active operations), we probably need to set a
> flag on the conn_data and continue trying to release it at the end of
> each operation (until the last operation finally can do so).
>    
That's already done. See sdap_id_op_hook_conn_data. Whenever connection 
is detached from operation (in sdap_id_op_done, destructor or failed 
connection), sdap_id_release_conn_data is called to garbage collect 
no-longer-needed connection.
> Could you please explain the comment "we clean out list of ops to make
> sure that order of destruction does not matter" in
> sdap_id_conn_data_destroy()? I'm not sure what effect you think this is
> having on the destruction.
>    
When sdap_id_ctx is being destroyed and there are outstanding LDAP 
queries, the order of destruction is unclear (or probably even 
undefined): it is possible that BE request (and consequently 
corresponding sdap_id_op) will be destroyed before sdap_id_conn_cache. 
As both of them have common ancestor, but none of them is child of the 
other.

Therefore when sdap_id_conn_data is being destroyed and there is active 
sdap_id_op we break the link between them by removing sdap_id_op from 
list, but do not call completion callbacks (for example for connection 
establishment) as we assume that sdap_id_op is being destroyed anyway.

> Also, sdap_id_conn_data_destroy() is not a properly constructed talloc
> destructor (I'm not sure why it's compiling without a warning). The
> argument needs to be of type 'TALLOC_CTX *ctx' and then you should do
> struct sdap_id_op *op = talloc_get_type(ctx, struct sdap_id_op). You
> will also need to cast to (TALLOC_CTX *) in the destructor
> initialization. Same for sdap_id_op_destroy().
>    
TALLOC library has changed meanwhile. TALLOC version 2 uses typed 
destructors. Just take a look into talloc.h and see how type safety is 
implemented using simple macros and modern gcc features.
> Please add more parentheses in the comparison in
> sdap_is_connection_expired(). I can tell what the intended order of
> operations is, but I'm not 100% sure that it will end up being the
> actual order :)
>
>    
Done.
> sdap_id_conn_data_set_expire_timer() does not properly set the timeval.
> Use tevent_timeval_current_ofs() to set the timeval instead. Tevent uses
> explicit times, not offsets, so what you're doing here is guaranteeing
> that the event will always be fired immediately (unless your system
> clock happens to be running close to the epoch), not expire_time seconds
> in the future.
>    
What I have done looks OK for me. I am setting timeval not to offset, 
but first to connection expiration time (absolute time in seconds from 
EPOCH):
     tv.tv_sec = conn_data->sh->expire_time;
Then I subtract LDAP query timeout from it:
     tv.tv_sec -= timeout;
I assume that connection should be dropped from cache if it will expire 
before query time-out.

Actually this code was tested on 1.2 code base and works, at least for me.

After looking in tevent.h I wanted to change my code to use 
tevent_timeval_set and tevent_timeval_add, but tevent_timeval_set uses 
32-bit seconds, which is long obsolete as time_t is 64-bit and there is 
no tevent_timeval_sub. :(
> The DLIST_REMOVE() in sdap_id_op_hook_conn_data() has a NULL dereference
> error if op is NULL. You need to check for valid input to the function
> before this step. (Preferably with a DEBUG message if it is not valid)
>
>    
Done.
> sdap_id_op_can_reconnect() Rather than special-case two retries here, we
> should probably just mark the server as unusable when the connection is
> broken and move to the next failover server. The slim chance that the
> server will be available after a reconnect is not worth the overhead of
> waiting for a connection timeout.
>    
I agree that it's better to move to next server after current one 
failed, but it is not acceptable to mark current server as not working 
just because of broken connection as there is a good change that it was 
a recoverable problem like expired GSSAPI ticket. Unfortunately 
fail-over API currently does not have such functionality.

So I would propose to make a separate patch which will add such a 
functionality to fail-over and then use it in sdap_id_op_done when 
broken connection is detected.
> sdap_id_opt_connect() is not following proper tevent_req behavior at
> all. First of all, it should be named sdap_id_opt_connect_send() and
> must have a return type of tevent_req *. On failure, this function must
> return NULL (and may also return an error code variable by reference).
> The _send function itself MUST create the connection request.
> Pre-creating it to be used when this function is called is unsafe at any
> speed. Also, the tevent_req MUST be returned on a success condition,
> even if reusing an existing operation connection. For this purpose,
> tevent_req_post() should be used to guarantee that the results fire
> immediately after the calling function returns control to the mainloop
> (in the case where we are reusing the connection. Never use
> tevent_req_post() except in a _send() function). Any processing based on
> results from this request should be handled in the corresponding _done()
> function. Short-circuiting it in the original calling function (as is
> done in users_get_retry in Patch 0004) is unsafe (as it duplicates code
> and makes it harder to keep track of).
>    
I do not agree that short-circuiting in this case is not worthwhile. 
 From my point of view, overhead of allocating request and 
tevent_request_post should be avoided.

But anyway, changed it to full tevent style compliance.
> sdap_id_op_connect_retry() should be converted to
> sdap_id_opt_connect_step() and should be a tevent_req *subreq function.
> Once it returns a valid connection (or has determined that no such
> connection CAN be made) it should call tevent_req_done() or
> tevent_req_error() and return control to sdap_id_opt_connect_done().
> Follow the style set out by sdap_cli_connect_send() and _done() and
> _recv() here.
>    
Done.
> If you have questions on how to approach this, please ask. I know I've
> given only a high-level reply here.
>
> Minor nitpicks:
> The change to the prototype for users_get_recv() belongs in patch 0004.
>    
Oops. Fixed.
> Also, our coding style is to use 'int *var' not 'int* var' (which I
> notice you have been following elsewhere, so I assume this is just a
> typo) You did the same in sdap_id_conn_cache_create() for the conn_cache
> variable.
>    
Fixed. Code style in my company is int*. So it's more matter of custom, 
but I am trying hard to follow SSSD code style. :)
> While it's not strictly required by C, we prefer for function prototypes
> to list the same variable names as the function implementation. static
> int sdap_id_op_destroy(struct sdap_id_op*); does not do this.
>
>    
Done.
>> 0004-Use-new-LDAP-connection-framework-to-get-user-accoun.patch
>> Sample usage of the new LDAP connection framework for user account
>> information request
>>
>>      
> See comments above regarding the proper tevent_req style for
> sdap_id_opt_connect_send() and sdap_id_opt_connect_recv().
>
>
>    
Done.

The modified patches are attached. As last time the changes were not 
tested, just compiled. If they look OK, the next step for me would be to 
convert all the remaining code in series of patches and test the result.

Eugene
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-GSSAPI-ticket-expiry-time-is-returned-from-ldap_chil.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100617/a3a97d69/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-Added-an-interface-to-query-number-of-configured-and.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100617/a3a97d69/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0003-LDAP-connection-usage-tracking-sharing-and-failover-.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100617/a3a97d69/attachment-0002.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0004-Use-new-LDAP-connection-framework-to-get-user-accoun.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100617/a3a97d69/attachment-0003.ksh>


More information about the sssd-devel mailing list