[SSSD] [PATCHES] Primary server support in SSSD

Jan Zelený jzeleny at redhat.com
Tue Jul 31 08:39:10 UTC 2012


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.

Thanks for the review
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Primary-server-support-introduce-concept-of-reconnec.patch
Type: text/x-patch
Size: 3201 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Primary-server-support-basic-support-in-failover-cod.patch
Type: text/x-patch
Size: 22876 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Primary-server-support-support-for-disconnecting-con.patch
Type: text/x-patch
Size: 4463 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Primary-server-support-IPA-adaptation.patch
Type: text/x-patch
Size: 6718 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Primary-server-support-krb5-adaptation.patch
Type: text/x-patch
Size: 9750 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Primary-server-support-LDAP-adaptation.patch
Type: text/x-patch
Size: 7227 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-Primary-server-support-AD-adaptation.patch
Type: text/x-patch
Size: 7972 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-Primary-server-support-man-page-failover-section.patch
Type: text/x-patch
Size: 1772 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-Primary-server-support-new-option-in-ldap-provider.patch
Type: text/x-patch
Size: 11337 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-Primary-server-support-new-options-in-krb5-provider.patch
Type: text/x-patch
Size: 16326 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-Primary-server-support-new-option-in-IPA-provider.patch
Type: text/x-patch
Size: 4865 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0012-Primary-server-support-new-option-in-AD-provider.patch
Type: text/x-patch
Size: 4510 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/db4aaf44/attachment.sig>


More information about the sssd-devel mailing list