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
> 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
> 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).