[RFC] [PATCH] Retry system bus connection once messagebus is up

[Jakub Hrozek]

Hi,

the attached (unpolished, see my question below) patches fix
https://fedorahosted.org/sssd/ticket/2360
also known as:
https://bugzilla.redhat.com/show_bug.cgi?id=1110369

Let me explain the problem first -- if SSSD starts before messagebus is
up, the InfoPipe responder fails to start and doesn't retry, so the
system bus service is simply not there.

A simple solution would be to start messagebus before SSSD. But I don't
think that is a robust solution, because the messagebus configuration
can reference user names, which the SSSD provides. So at the time
messagebus is up, the identities should be resolvable -- which means the
NSS responder and the back ends must be up.

The attached patches take advantage of bus activation messagebus
provides. If the interface InfoPipe provides is not registered on the
bus when requested, messagebus signals the sssd, which tells the IFP
responder to retry the system bus connection.

Currently, the WIP patches use sss_debuglevel which sends HUP to the
sssd process, but I think USR2 (aka "go online") would be better. So the
final patch version would include a helper binary that would do nothing
but singal the monitor..

I have one question to discuss though.. is it OK to use signals for the
IPC? An alternative might be to let IFP spawn a client socket and implement
only a single 'command' to retry the connection. But that seems like an
overkill to me. The disadvantage of the signal is that it's also used to
reset the online status so in theory there might be some timeouts in the
offline case, though.

In the 1.13 timeframe, we will be implementing socket-based activation.
I think it would be nice to make the IFP responder bus-activated as part
of that effort. But in the traditional schema where monitor manages all
the processes, the changes to make InfoPipe bus-activated instead of
managed by the monitor would be too invasive (I've tried to do that
yesterday..)

[Jan Pazdziora]

On Wed, Jun 25, 2014 at 01:32:58PM +0200, Jakub Hrozek wrote:
> the attached (unpolished, see my question below) patches fix
> https://fedorahosted.org/sssd/ticket/2360
> also known as:
> https://bugzilla.redhat.com/show_bug.cgi?id=1110369
>
> Let me explain the problem first -- if SSSD starts before messagebus is
> up, the InfoPipe responder fails to start and doesn't retry, so the
> system bus service is simply not there.
>
> A simple solution would be to start messagebus before SSSD. But I don't
> think that is a robust solution, because the messagebus configuration
> can reference user names, which the SSSD provides. So at the time
> messagebus is up, the identities should be resolvable -- which means the
> NSS responder and the back ends must be up.
>
> The attached patches take advantage of bus activation messagebus
> provides. If the interface InfoPipe provides is not registered on the
> bus when requested, messagebus signals the sssd, which tells the IFP
> responder to retry the system bus connection.
>
> Currently, the WIP patches use sss_debuglevel which sends HUP to the
> sssd process, but I think USR2 (aka "go online") would be better. So the
> final patch version would include a helper binary that would do nothing
> but singal the monitor..
>
> I have one question to discuss though.. is it OK to use signals for the
> IPC? An alternative might be to let IFP spawn a client socket and implement
> only a single 'command' to retry the connection. But that seems like an
> overkill to me. The disadvantage of the signal is that it's also used to
> reset the online status so in theory there might be some timeouts in the
> offline case, though.

My uninformed view is that signals are a good start. Something else
than HUP might be useful.

[Pavel Březina]

On 06/25/2014 01:32 PM, Jakub Hrozek wrote:
> Hi,
>
> the attached (unpolished, see my question below) patches fix
> https://fedorahosted.org/sssd/ticket/2360
> also known as:
> https://bugzilla.redhat.com/show_bug.cgi?id=1110369
>
> Let me explain the problem first -- if SSSD starts before messagebus is
> up, the InfoPipe responder fails to start and doesn't retry, so the
> system bus service is simply not there.
>
> A simple solution would be to start messagebus before SSSD. But I don't
> think that is a robust solution, because the messagebus configuration
> can reference user names, which the SSSD provides. So at the time
> messagebus is up, the identities should be resolvable -- which means the
> NSS responder and the back ends must be up.
>
> The attached patches take advantage of bus activation messagebus
> provides. If the interface InfoPipe provides is not registered on the
> bus when requested, messagebus signals the sssd, which tells the IFP
> responder to retry the system bus connection.

The patches looks good to me. Do you also plan error counter so we can 
fail if we can't establish the connection for a longer time?
> Currently, the WIP patches use sss_debuglevel which sends HUP to the
> sssd process, but I think USR2 (aka "go online") would be better. So the
> final patch version would include a helper binary that would do nothing
> but singal the monitor..
> I have one question to discuss though.. is it OK to use signals for the
> IPC? An alternative might be to let IFP spawn a client socket and implement
> only a single 'command' to retry the connection. But that seems like an
> overkill to me. The disadvantage of the signal is that it's also used to
> reset the online status so in theory there might be some timeouts in the
> offline case, though.

I think semantics of "go online" signal is exactly what we want here. We 
can also send USR2 directly to the ifp responder to avoid possible timeouts.
> In the 1.13 timeframe, we will be implementing socket-based activation.
> I think it would be nice to make the IFP responder bus-activated as part
> of that effort. But in the traditional schema where monitor manages all
> the processes, the changes to make InfoPipe bus-activated instead of
> managed by the monitor would be too invasive (I've tried to do that
> yesterday..)

I agree.

[PATCH] Retry system bus connection once messagebus is up

[Jakub Hrozek]

On Tue, Jul 01, 2014 at 10:34:11AM +0200, Pavel Březina wrote:
> On 06/25/2014 01:32 PM, Jakub Hrozek wrote:
> > Hi,
> >
> > the attached (unpolished, see my question below) patches fix
> > https://fedorahosted.org/sssd/ticket/2360
> > also known as:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1110369
> >
> > Let me explain the problem first -- if SSSD starts before messagebus is
> > up, the InfoPipe responder fails to start and doesn't retry, so the
> > system bus service is simply not there.
> >
> > A simple solution would be to start messagebus before SSSD. But I don't
> > think that is a robust solution, because the messagebus configuration
> > can reference user names, which the SSSD provides. So at the time
> > messagebus is up, the identities should be resolvable -- which means the
> > NSS responder and the back ends must be up.
> >
> > The attached patches take advantage of bus activation messagebus
> > provides. If the interface InfoPipe provides is not registered on the
> > bus when requested, messagebus signals the sssd, which tells the IFP
> > responder to retry the system bus connection.
>
> The patches looks good to me. Do you also plan error counter so we can fail
> if we can't establish the connection for a longer time?

No, currently the IFP responder only tries once and if it fails, it
doesn't retry until the bus activation.
> > Currently, the WIP patches use sss_debuglevel which sends HUP to the
> > sssd process, but I think USR2 (aka "go online") would be better. So the
> > final patch version would include a helper binary that would do nothing
> > but singal the monitor..
> > I have one question to discuss though.. is it OK to use signals for the
> > IPC? An alternative might be to let IFP spawn a client socket and implement
> > only a single 'command' to retry the connection. But that seems like an
> > overkill to me. The disadvantage of the signal is that it's also used to
> > reset the online status so in theory there might be some timeouts in the
> > offline case, though.
>
> I think semantics of "go online" signal is exactly what we want here. We can
> also send USR2 directly to the ifp responder to avoid possible timeouts.

I think so too, but I was thinking about the following scenario:
    1) sssd_nss, sssd_pam and sssd_be start up early. Network is not yet
      up
    2) some service started after sssd resolves a sssd user. sssd tries
      to connect, which might take some time depending on the conditions
      and eventually sssd goes offline
    3) messagebus starts up, resets the offline status
    4) another service does 2) all over again
    ....
    N) network is finally up

But I think this won't be a problem at least in the general case because
most services user local UNIX users anyway and even if sssd was queried,
the timeouts during boot would be quite fast because /etc/resolv.conf
would be empty before NM/dhcpd starts.
> > In the 1.13 timeframe, we will be implementing socket-based activation.
> > I think it would be nice to make the IFP responder bus-activated as part
> > of that effort. But in the traditional schema where monitor manages all
> > the processes, the changes to make InfoPipe bus-activated instead of
> > managed by the monitor would be too invasive (I've tried to do that
> > yesterday..)
>
> I agree.

I will start a separate thread also with #2351 in mind.

New patches are attached, this time I consider them ready for review, so
I'll remove the RFC tag.

[PATCH] Retry system bus connection once messagebus is up

[Pavel Březina]

On 07/02/2014 10:36 AM, Jakub Hrozek wrote:
> On Tue, Jul 01, 2014 at 10:34:11AM +0200, Pavel Březina wrote:
> > On 06/25/2014 01:32 PM, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > the attached (unpolished, see my question below) patches fix
> > > https://fedorahosted.org/sssd/ticket/2360
> > > also known as:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1110369
> > >
> > > Let me explain the problem first -- if SSSD starts before messagebus is
> > > up, the InfoPipe responder fails to start and doesn't retry, so the
> > > system bus service is simply not there.
> > >
> > > A simple solution would be to start messagebus before SSSD. But I don't
> > > think that is a robust solution, because the messagebus configuration
> > > can reference user names, which the SSSD provides. So at the time
> > > messagebus is up, the identities should be resolvable -- which means the
> > > NSS responder and the back ends must be up.
> > >
> > > The attached patches take advantage of bus activation messagebus
> > > provides. If the interface InfoPipe provides is not registered on the
> > > bus when requested, messagebus signals the sssd, which tells the IFP
> > > responder to retry the system bus connection.
> >
> > The patches looks good to me. Do you also plan error counter so we can fail
> > if we can't establish the connection for a longer time?
>
> No, currently the IFP responder only tries once and if it fails, it
> doesn't retry until the bus activation.
> > > Currently, the WIP patches use sss_debuglevel which sends HUP to the
> > > sssd process, but I think USR2 (aka "go online") would be better. So the
> > > final patch version would include a helper binary that would do nothing
> > > but singal the monitor..
> > > I have one question to discuss though.. is it OK to use signals for the
> > > IPC? An alternative might be to let IFP spawn a client socket and implement
> > > only a single 'command' to retry the connection. But that seems like an
> > > overkill to me. The disadvantage of the signal is that it's also used to
> > > reset the online status so in theory there might be some timeouts in the
> > > offline case, though.
> >
> > I think semantics of "go online" signal is exactly what we want here. We can
> > also send USR2 directly to the ifp responder to avoid possible timeouts.
>
> I think so too, but I was thinking about the following scenario:
>      1) sssd_nss, sssd_pam and sssd_be start up early. Network is not yet
>        up
>      2) some service started after sssd resolves a sssd user. sssd tries
>        to connect, which might take some time depending on the conditions
>        and eventually sssd goes offline
>      3) messagebus starts up, resets the offline status
>      4) another service does 2) all over again
>      ....
>      N) network is finally up
>
> But I think this won't be a problem at least in the general case because
> most services user local UNIX users anyway and even if sssd was queried,
> the timeouts during boot would be quite fast because /etc/resolv.conf
> would be empty before NM/dhcpd starts.
> > > In the 1.13 timeframe, we will be implementing socket-based activation.
> > > I think it would be nice to make the IFP responder bus-activated as part
> > > of that effort. But in the traditional schema where monitor manages all
> > > the processes, the changes to make InfoPipe bus-activated instead of
> > > managed by the monitor would be too invasive (I've tried to do that
> > > yesterday..)
> >
> > I agree.
>
> I will start a separate thread also with #2351 in mind.
>
> New patches are attached, this time I consider them ready for review, so
> I'll remove the RFC tag.

LGTM
Can you just add a debug message that will say that ifp_sysbus_reconnect 
is executed?

[PATCH] Retry system bus connection once messagebus is up

[Jakub Hrozek]

On Mon, Jul 07, 2014 at 11:47:28AM +0200, Pavel Březina wrote:
> > I will start a separate thread also with #2351 in mind.
> >
> > New patches are attached, this time I consider them ready for review, so
> > I'll remove the RFC tag.
>
> LGTM

btw Jan did some testing already on RHEL6:
    https://bugzilla.redhat.com/show_bug.cgi?id=1110369
> Can you just add a debug message that will say that ifp_sysbus_reconnect is
> executed?

Sure, do the attached patches look any better?

[PATCH] Retry system bus connection once messagebus is up

[Pavel Reichl]

On Mon, 2014-07-07 at 11:57 +0200, Jakub Hrozek wrote:
> On Mon, Jul 07, 2014 at 11:47:28AM +0200, Pavel Březina wrote:
> > > I will start a separate thread also with #2351 in mind.
> > >
> > > New patches are attached, this time I consider them ready for review, so
> > > I'll remove the RFC tag.
> >
> > LGTM
>
> btw Jan did some testing already on RHEL6:
>     https://bugzilla.redhat.com/show_bug.cgi?id=1110369
> > Can you just add a debug message that will say that ifp_sysbus_reconnect is
> > executed?
>
> Sure, do the attached patches look any better?
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Hi

patches LGTM, I just noted one little nitpick, but I really don't think
its important so ACK to patches as they are now.
> static struct data_provider_iface ifp_dp_methods = {
> @@ -257,6 +260,39 @@ fail:
>      return ret;
>  }
>
> +static int ifp_sysbus_reconnect(struct sbus_request *dbus_req, void *data)
> +{
> struct resp_ctx *rctx = talloc_get_type(data, struct resp_ctx);
> struct ifp_ctx *ifp_ctx = (struct ifp_ctx*) rctx->pvt_ctx;
> errno_t ret;
> DEBUG(SSSDBG_TRACE_FUNC, "Attempting to reconnect to the system bus\n");
> if (ifp_ctx->sysbus) {
>    DEBUG(SSSDBG_TRACE_LIBS, "Already connected to sysbus\n");
>    goto done;
> }
> /* Connect to the D-BUS system bus and set up methods */
> ret = sysbus_init(ifp_ctx, ifp_ctx->rctx->ev,
>                  INFOPIPE_IFACE,
>                  ifp_ifaces,
>                  ifp_ctx, &ifp_ctx->sysbus);
> if (ret == ERR_NO_SYSBUS) {
>    DEBUG(SSSDBG_MINOR_FAILURE,
>          "The system bus is not available..\n");
>    goto done;
> } else if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "Failed to connect to the system message bus\n");
>    return ret;
> }
> DEBUG(SSSDBG_TRACE_LIBS, "Reconnected to the system bus!\n");

I think it's a common practice to have a blank line before goto label,
but I don't think it's in our code guide.
> +done:
> return sbus_request_return_and_finish(dbus_req, DBUS_TYPE_INVALID);
>
> +}

[PATCH] Retry system bus connection once messagebus is up

[Jakub Hrozek]

On Tue, Jul 08, 2014 at 03:50:01PM +0200, Pavel Reichl wrote:
> On Mon, 2014-07-07 at 11:57 +0200, Jakub Hrozek wrote:
> > On Mon, Jul 07, 2014 at 11:47:28AM +0200, Pavel Březina wrote:
> > > > I will start a separate thread also with #2351 in mind.
> > > >
> > > > New patches are attached, this time I consider them ready for review, so
> > > > I'll remove the RFC tag.
> > >
> > > LGTM
> >
> > btw Jan did some testing already on RHEL6:
> >     https://bugzilla.redhat.com/show_bug.cgi?id=1110369
> > > Can you just add a debug message that will say that ifp_sysbus_reconnect is
> > > executed?
> >
> > Sure, do the attached patches look any better?
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Hi
>
> patches LGTM, I just noted one little nitpick, but I really don't think
> its important so ACK to patches as they are now.
> > static struct data_provider_iface ifp_dp_methods = {
> > @@ -257,6 +260,39 @@ fail:
> >      return ret;
> >  }
> >
> > +static int ifp_sysbus_reconnect(struct sbus_request *dbus_req, void *data)
> > +{
> > struct resp_ctx *rctx = talloc_get_type(data, struct resp_ctx);
> > struct ifp_ctx *ifp_ctx = (struct ifp_ctx*) rctx->pvt_ctx;
> > errno_t ret;
> > DEBUG(SSSDBG_TRACE_FUNC, "Attempting to reconnect to the system bus\n");
> > if (ifp_ctx->sysbus) {
> >    DEBUG(SSSDBG_TRACE_LIBS, "Already connected to sysbus\n");
> >    goto done;
> > }
> > /* Connect to the D-BUS system bus and set up methods */
> > ret = sysbus_init(ifp_ctx, ifp_ctx->rctx->ev,
> >                  INFOPIPE_IFACE,
> >                  ifp_ifaces,
> >                  ifp_ctx, &ifp_ctx->sysbus);
> > if (ret == ERR_NO_SYSBUS) {
> >    DEBUG(SSSDBG_MINOR_FAILURE,
> >          "The system bus is not available..\n");
> >    goto done;
> > } else if (ret != EOK) {
> >    DEBUG(SSSDBG_CRIT_FAILURE,
> >          "Failed to connect to the system message bus\n");
> >    return ret;
> > }
> > DEBUG(SSSDBG_TRACE_LIBS, "Reconnected to the system bus!\n");
>
> I think it's a common practice to have a blank line before goto label,
> but I don't think it's in our code guide.

It's not in the code guide, but I took the liberty to add the blank line
w/o re-sending the patches for another review, the code now looks
better.
> > +done:
> > return sbus_request_return_and_finish(dbus_req, DBUS_TYPE_INVALID);
> >
> > +}

Thank you for the review, I pushed the patches to master:
    1a59af8245f183f22d87d067a90197d8e2ea958d
    1746e8b8399da2a7a8da4aace186f66055ccfec1
    149f40dc2d4ead57811c70b5028648ac83f6a1a7
    b76419cf8830440b46c20a15585562343c7b1924
    0c1d65998907930678da2d091789446f2c344d5d
    1f2507e1fd089f2bf3458cfb4faeaa9669d72f98
    4df1a6a977df74420867d9b1daddcca0eea4b2e1

The 1.11 version will be sent out separately.

[PATCH] Retry system bus connection once messagebus is up

[Jakub Hrozek]

On Tue, Jul 08, 2014 at 08:42:43PM +0200, Jakub Hrozek wrote:
> The 1.11 version will be sent out separately.

Here are the 1.11 patches. There are no functional differences, just
some minor rebasing.

[PATCH] Retry system bus connection once messagebus is up

[Pavel Březina]

On 07/14/2014 11:54 AM, Jakub Hrozek wrote:
> On Tue, Jul 08, 2014 at 08:42:43PM +0200, Jakub Hrozek wrote:
> > The 1.11 version will be sent out separately.
>
> Here are the 1.11 patches. There are no functional differences, just
> some minor rebasing.

Ack.

[PATCH] Retry system bus connection once messagebus is up

[Jakub Hrozek]

On Tue, Jul 15, 2014 at 02:47:21PM +0200, Pavel Březina wrote:
> On 07/14/2014 11:54 AM, Jakub Hrozek wrote:
> > On Tue, Jul 08, 2014 at 08:42:43PM +0200, Jakub Hrozek wrote:
> > > The 1.11 version will be sent out separately.
> >
> > Here are the 1.11 patches. There are no functional differences, just
> > some minor rebasing.
>
> Ack.

sssd-1-11:
    * 80af7e9daed52b283af037864bcdd86d96695618
    * 42b0c3442815c0374735377c7f5ced4fe1a00e97
    * 87d3c7d23885b0b6dca3d7cf0494c7b93225429c
    * fbc3f000ca0672bb18797201768bd13e5611eaad
    * 3e57c78c8163f6ee395bdf34b1e2c550cd8467f1
    * 727f4bf4829f2405c978a4c9b960bef3ad86b002
    * 906177a2666bf360a3d85fec55fc942cf9b33163