On 31.7.2012 10:39, Jan Zelený wrote:
> Dne pondělí 30 července 2012 13:53:50, Pavel Březina napsal(a):
>> On 07/20/2012 02:04 PM, Jan Zelený wrote:
>>> Dne čtvrtek 19 července 2012 16:10:08, Stephen Gallagher napsal(a):
>>>> On Tue, 2012-07-17 at 11:21 +0200, Jan Zelený wrote:
>>>>> Dne středa 11 července 2012 13:34:33, Stephen Gallagher napsal(a):
>>>>>> On Thu, 2012-06-21 at 12:15 +0200, Jan Zelený wrote:
>>>>>>>> On Fri, 2012-06-08 at 12:56 +0200, Jan Zelený wrote:
>>>>>>>>> Hi everybody,
>>>>>>>>> I'm sending some patches which implement the
primary server
>>>>>>>>> support as
>>>>>>>>> I
>>>>>>>>> see it. It it based on comment #6 in the related
ticket
>>>>>>>>>
https://fedorahosted.org/sssd/ticket/1128
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Patch #0001 basically adds the necessary support in
failover code
>>>>>>>>> Patches #0002 - #0004 extend this support in each
provider
>>>>>>>>> Patch #0005 documents the new concept in failover
section in man
>>>>>>>>> pages
>>>>>>>>> Patches #0006 - #0008 add new options for each
provider which
>>>>>>>>> utilize
>>>>>>>>> this
>>>>>>>>> concept.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Just briefly about the approach. When adding a new
server to the
>>>>>>>>> list
>>>>>>>>> of
>>>>>>>>> servers related to a service, each server can be
marked either as
>>>>>>>>> primary
>>>>>>>>> or secondary.
>>>>>>>>>
>>>>>>>>> When selecting new server from failover list, the
algorithm
>>>>>>>>> iterates
>>>>>>>>> over
>>>>>>>>> the list twice - first it tries to look for primary
server and if
>>>>>>>>> none
>>>>>>>>> is
>>>>>>>>> found, it tries also secondary server.
>>>>>>>>>
>>>>>>>>> If a server is returned from failover, and it is not
primary
>>>>>>>>> server, a
>>>>>>>>> timeout is set (currently hard-coded for 30 seconds)
for primary
>>>>>>>>> server
>>>>>>>>> lookup. This timeout is rescheduled until a primary
server is
>>>>>>>>> found.
>>>>>>>>> If a
>>>>>>>>> primary server (either working or neutral) is found
after this
>>>>>>>>> timeout,
>>>>>>>>> status of the backend is reset, i.e. first all
offline and then
>>>>>>>>> all
>>>>>>>>> online callbacks are called. This is done to
interrupt connection
>>>>>>>>> to
>>>>>>>>> the
>>>>>>>>> secondary server in favor of new connection to a
primary server.
>>>>>>>>
>>>>>>>> I think this is dangerous. I don't want us to force
offline
>>>>>>>> operation
>>>>>>>> even momentarily.
>>>>>>>
>>>>>>> I changed the concept to re-connection, which is done
instantly,
>>>>>>> without
>>>>>>> scheduling any events. Combined with the possibility to mark
LDAP
>>>>>>> connections as "disconnecting", it is addressing
the original issue
>>>>>>> of
>>>>>>> going offline and back online.
>>>>>>>
>>>>>>>>> I have just couple concerns about things which I
have yet to
>>>>>>>>> inspect.
>>>>>>>>> First of all when connection to the old server is
interrupted,
>>>>>>>>> what if
>>>>>>>>> an
>>>>>>>>> operation is currently in progress on this
connection? I know
>>>>>>>>> ticket
>>>>>>>>> #1027 induces similar scenario. Maybe to kill two
birds with one
>>>>>>>>> stone
>>>>>>>>> I
>>>>>>>>> could design an extension to immediately invoke
callbacks of all
>>>>>>>>> operations running on existing connection. What do
you think about
>>>>>>>>> that?
>>>>>>>>
>>>>>>>> Better behavior would be to allow existing
communications to
>>>>>>>> complete,
>>>>>>>> and only direct new requests to the primary server. I
think
>>>>>>>> interrupting
>>>>>>>> in-progress requests would be dangerous and
unpredictable.
>>>>>>>
>>>>>>> Done, see the proposed concept of reconnection.
>>>>>>>
>>>>>>>>> My second concern is about port status timeout.
After some time a
>>>>>>>>> port
>>>>>>>>> is
>>>>>>>>> marked as neutral if originally marked as
not-working. That
>>>>>>>>> effectively
>>>>>>>>> leads to second attempt to primary server
reconnection being
>>>>>>>>> successful
>>>>>>>>> even though the server is still not running. As a
result, an
>>>>>>>>> unnecessary
>>>>>>>>> reconnection to secondary server is performed once
some data are
>>>>>>>>> needed
>>>>>>>>> from the server.
>>>>>>>>
>>>>>>>> This is intentional behavior. It's done so that if
we eventually
>>>>>>>> lose
>>>>>>>> connection to the current server, we'll be able to
retry that one
>>>>>>>> again
>>>>>>>> while looping through the failover code. I'd suggest
just resetting
>>>>>>>> the
>>>>>>>> port-neutralizer timeout when you make the primary
reconnection
>>>>>>>> attempt
>>>>>>>> and it fails.
>>>>>>>
>>>>>>> I left it as it was. It is impossible for failover code to
check if
>>>>>>> the
>>>>>>> port is online. Therefore this solution is probably the best
we can
>>>>>>> do.>
>>>>>>>
>>>>>>>>> Thank you very much for your opinions on this
>>>>>>>>> Jan
>>>>>>>>
>>>>>>>> General comment: I don't think I like the term
"secondary" here. I
>>>>>>>> think
>>>>>>>> we might want to use something more descriptive. Perhaps
"backup"?
>>>>>>>> I'm
>>>>>>>> soliciting suggestions :)
>>>>>>>
>>>>>>> Backup seems as good as any. Should I rename all those
config
>>>>>>> options
>>>>>>> or
>>>>>>> do you have any other ideas?
>>>>>>>
>>>>>>>> Patch 0001: Nack
>>>>>>>> See above concerns.
>>>>>>>
>>>>>>> Hopefully addressed.
>>>>>>>
>>>>>>>> Patch 0002: Nack
>>>>>>>>
>>>>>>>> Please add a comment explaining why secondary_urls gets
promoted to
>>>>>>>> primary.
>>>>>>>
>>>>>>> Added DEBUG message, that's more useful I guess
>>>>>>>
>>>>>>>> Patch 0003: Ack
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Patch 0004: Nack
>>>>>>>>
>>>>>>>> Please add a comment explaining why secondary_urls gets
promoted to
>>>>>>>> primary.
>>>>>>>
>>>>>>> Added DEBUG message, that's more useful I guess
>>>>>>>
>>>>>>>> Patch 0005: Nack
>>>>>>>>
>>>>>>>> Typos in manpage:
>>>>>>>> "For each failover-enabled config option two
variants exists:"
>>>>>>>> should be
>>>>>>>> "For each failover-enabled config option, two
variants exist:"
>>>>>>>> and
>>>>>>>> "it will replace current" should be "it
will replace the current"
>>>>>>>
>>>>>>> Fixed
>>>>>>>
>>>>>>>> Patch 0006: Nack
>>>>>>>>
>>>>>>>> Typo in the manpage:
>>>>>>>> "If neither options is specified," should be
"If neither option is
>>>>>>>> specified,"
>>>>>>>>
>>>>>>>> You removed the warning about missing ldap_uri. You
should put it
>>>>>>>> back
>>>>>>>> in if both urls and secondary_urls are NULL.
>>>>>>>>
>>>>>>>> Please add the same config debug information that you
used in the
>>>>>>>> IPA
>>>>>>>> provider (about promoting secondary servers to primary)
>>>>>>>
>>>>>>> Done
>>>>>>>
>>>>>>>> Patch 0007: Nack
>>>>>>>>
>>>>>>>> Same comment about the warning when the servers are
unspecified.
>>>>>>>
>>>>>>> Done
>>>>>>
>>>>>> These patches look good by inspection, but due to divergence in
the
>>>>>> codebase, they no longer apply atop the master. Please rebase
them so
>>>>>> I
>>>>>> can run some tests.
>>>>>>
>>>>>> Also, the AD provider probably needs modification as well to
take
>>>>>> advantage of this new feature (sorry, it landed first).
>>>>>
>>>>> Rebased on top of current master. Those AD provider changes are
>>>>> included
>>>>> as
>>>>> well, I didn't have a chance to test them though.
>>>>
>>>> These patches don't apply cleanly. Patch 0010 fails with missing
blobs.
>>>> Please submit rebased patches (or identify which other patches are
>>>> required).
>>>
>>> Here it is, rebased on top of current master (strange that many times
>>> rebase works while applying the patch doesn't). No additional patches
>>> required.
>>>
>>> Thanks
>>> Jan
>>
>> Nack.
>>
>> IPA and LDAP works. I didn't test AD because I don't have any AD server
>> available.
>
> I'll send you details about our testing AD environment.
>
>> Kerberos backup servers don't seem to work. I think it is because you
>> are loading wrong option in krb5_init.c:103.
>
> You are right, fixed.
>
>> Manpage failover section:
>> - says that the primary server timeout is 60 seconds but you use 30
>> seconds in the code.
>> - "After this timeout SSSD will periodically try to reconnect to one of
>> *the* primary servers."?
>
> Both corrected.
>
>> data_provider_fo.c:452 You have two subsequent spaces in the debug
>> message.
>
> Deleted
>
>> data_provider_fo.c:477 Can you put the DEBUG statement just on two lines
>> please? I found this little confusing.
>
> Done
>
>> data_provider_fo.c:538 You should check if new_subreq is not null. I
>> know it is not possible in the current code flow, but what if.
>
> That is too defensive for no gain I think. If someone will modify this
> code? I'm sure the modification will be better suited there.
>
>> fail_over.c:836 The commentary says: "Also cancel the primary server
>> reactivation event until the lookup is complete". Are you cancelling the
>> timer somewhere?
>
> No, the comment was left there from older version of the patch set.
> Removed
Nack.
>> sdap_service_init(), ad_servers_init(): You're allocating tmp_ctx on
>> mem_ctx instead of NULL.
>
> That was issue in the original code that was in place. Fixed
You left it in ad_servers_init().
Ooops, I fixed ipa_servers_init() instead. Fixed now.
>> krb5_common.c:623 Invalid debug message. At this point
backup servers
>> aren't present.
>
> Good catch. Corrected.
>
>
> Sending corrected patches.
Works for IPA, LDAP, KRB5 and AD.
Backup KDC for GSSAPI is not functional. You need to add backup servers
in sdap_gssapi_init().
When you provide unresolvable hostnames, it does not try next server
(log and sssd.conf attached).