On 07/07/2010 04:02 PM, Stephen Gallagher wrote:
>> Using hbac_ctx->sdap_op == NULL to mean "operating in offline mode"
is
>> very hard to read. Please just set a boolean state like
>> hbac_ctx->offline instead. When I read "if (!hbac_ctx->sdap_op)",
my
>> natural expectation is for the following lines to set up the sdap_op.
>>
...
> The problem here is that we get 2 interrelated fields in hbac_ctx and
> will have to add asserts testing in online mode that hbac_ctx->sdap_op
> is not NULL.
> The same applies to the prototypes of HBAC stages:
> hbac_get_host_info_send()
> hbac_get_rules_send()
> hbac_get_service_data_send()
>
> We will have to pass both hbac_ctx->offline and hbac_ctx->sdap_op as
> parameters with the same problem as above.
>
> From my point of view, the solution could be to pass the above methods
> hbac_ctx itself and define hbac_ctx_is_offline() test. This way the code
> will be readable and no duplicate info will be kept in data structures.
>
Yes, I think hbac_ctx_is_offline() would be a better idea. That way it
reads clearly.
Sounds good. I just want a confirmation that it is in line with tevent
coding style to pass pointer to higher level state (hbac_ctx) into
sub-operations (hbac_get_host_info_send() and others).
>
>
>>
>>
>>> 0011-Use-new-LDAP-connection-framework-in-IPA-dynamic-DNS.patch
>>>
>>>
>> Nack.
>>
>> There are several problems with this patch:
>>
>> 1) When the dynamic DNS update code was written, it expected that when
>> we are online, we would always have an active LDAP connection. This is
>> no longer guaranteed to be true. As we have discussed previously, it
>> will be possible with this new interface (eventually) to be able to
>> configure it so that connections to LDAP are not maintained after the
>> currently-running set of operations completes. So it's possible that it
>> will not be available to sdap_id_get_cached_connection() here. This
>> function needs to be able to create a connection when necessary (so
>> we'll need to convert part of the dynamic DNS update code to expect an
>> async operation at this point.
>>
>>
> Well, actually I have modelled the implementation after current approach
> that can currently even segfault if connection gets dropped during 30ms
> delay. Another reason is that ldap/ipa backend ONLINE callbacks are only
> called when LDAP connection is established, so it does make sense to
> assume that connection is established.
>
Yeah, I knew when I wrote it that this was a possible risk. I was hoping
that we could eliminate it with your new connection wrapper.
> Please, let me know your opinion and I will add async establishment of
> LDAP connection if needed.
>
I think this would be the better way to go. It will avoid the potential
race condition.
I will do it today.
>> 2) sdap_id_op_connect_cached() is never used anywhere. As
mentioned
>> above, we should ask the connection cache if there is an available
>> connection and using it if so, creating a new connection if not.
>>
>>
>>
> I have added this method for the sake of completeness. It can be safely
> removed. Currently I do not see any use for it.
>
Yeah, if it's not used, let's drop it. Dead code is a distraction.
OK
I had a discussion about these patches with Simo yesterday, and he
raised an interesting question. His thought was that perhaps we are
wrapping the LDAP code at too high of a level. His thoughts were that,
instead of creating an sdap_op at the top level of the LDAP request, we
could instead wrap individual calls to sdap_generic_send(), so that if
we reconnect in the middle of a multi-step operation, we do not have to
restart and rerun all of the earlier steps.
This is an interesting idea, but it also occurred to me that there is a
potential risk as well: if we fail over to another LDAP server, there is
no guarantee that it is perfectly in sync with the primary server in the
failover chain. The result could be that we would get some data from one
server or the other that doesn't align.
With this in mind, I think our current approach is a bit safer (even if
it might be slower in a failover situation). With this approach, we at
least guarantee consistency within a single LDAP server.
What are your thoughts, Eugene?
We have already had this discussion with Simo. My opinion is very
similar to yours. For multi-stage operations we should use the same LDAP
server for all stages. I know just too well what tricks multi-master
replication could play in some setups. The risk to get inconsistent data
from different servers is substantial. That's why I think it's better to
have a separate tear for multi-stage operation.
From my point of view we should go even further and not to store
intermediate stage results to sysdb until all data is received from LDAP
server. Otherwise we could use inconsistent data in OFFLINE mode.
Another reason to have sdap_id_op as separate tear is caching strategy:
it would ineffective if LDAP connection was dropped in the middle of
multi-stage operation, that could be the case if connection usage
tracking is done within sdap_generic_send. Using higher level tear
ensures that connection is available through complete operation, not
just within single stage.