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