[SSSD] [PATCHES] Primary server support in SSSD

Jakub Hrozek jhrozek at redhat.com
Wed Aug 1 14:32:42 UTC 2012


On Wed, Aug 01, 2012 at 02:34:05PM +0200, Pavel Březina wrote:
> On 08/01/2012 01:13 PM, Jan Zelený wrote:
> >Dne úterý 31 července 2012 15:14:16, Pavel Březina napsal(a):
> >>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().
> >
> >Fixed
> >
> >>When you provide unresolvable hostnames, it does not try next server
> >>(log and sssd.conf attached).
> 
> ACK!
> 
> Good job.

All patches pushed to master.



More information about the sssd-devel mailing list