[PATCH] MONITOR: Poll for resolv.conf if not available during boot

[Michal Židek]

Hi,

this patch partially solves the ticket
https://fedorahosted.org/sssd/ticket/2590

It does not subscribe to the systemd dbus interface and does only
the polling (this would have to be done anyway for the case
the interface is not available).

I will investigate how to add the systemd/dbus check as well,
but the attached patch should decrease priority of the ticket.

Thanks,
Michal

[Jakub Hrozek]

On Wed, Jun 17, 2015 at 07:13:17PM +0200, Michal Židek wrote:
> Hi,
>
> this patch partially solves the ticket
> https://fedorahosted.org/sssd/ticket/2590
>
> It does not subscribe to the systemd dbus interface and does only
> the polling (this would have to be done anyway for the case
> the interface is not available).
>
> I will investigate how to add the systemd/dbus check as well,
> but the attached patch should decrease priority of the ticket.
>
> Thanks,
> Michal
> From bb57b2bcba891cd395d795a7706cf2ea77f8bca2 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= 
> Date: Wed, 17 Jun 2015 18:54:14 +0200
> Subject: [PATCH] MONITOR: Poll for resolv.conf if not available during boot
>
> If resolv.conf is not available when SSSD is starting,
> check for its existence later.
>
> Ticket:
> https://fedorahosted.org/sssd/ticket/2590
>
> src/monitor/monitor.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index 01c882a..24fca28 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -2259,11 +2259,39 @@ static errno_t monitor_config_file_fallback(TALLOC_CTX *mem_ctx,
>      return EOK;
>  }
>
> +#define MISSING_RESOLV_CONF_POLL_TIME 10
>
> +static void missing_resolv_conf(struct tevent_context *ev,
>                            struct tevent_timer *te,
>                            struct timeval tv, void *data)
>
> +{
> int ret;
> struct mt_ctx *ctx = talloc_get_type(data, struct mt_ctx);
> ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH,
>                          monitor_update_resolv, false);
> if (ret == ENOENT) {
>    tv = tevent_timeval_current_ofs(MISSING_RESOLV_CONF_POLL_TIME, 0);
>    te = tevent_add_timer(ctx->ev, ctx, tv, missing_resolv_conf, ctx);
>    if (te == NULL) {
>        DEBUG(SSSDBG_FATAL_FAILURE,
>              "tevent_add_timer failed. resolv.conf will be ignored.\n");
>    }
> } else if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Monitor_config_file failed. resolv.conf will be ignored.\n");
> }

I think we should add signal_res_init() here, because in my testing the
inotify watch is established if I mv a resolv.conf file into place, but
the backends remain offline.

We should also (in a different patch) change the DEBUG levels in
monitor_config_file, because currently we print a 0x0010-level message.
> return;
>
> +}

[Michal Židek]

On 06/23/2015 01:48 PM, Jakub Hrozek wrote:
> On Wed, Jun 17, 2015 at 07:13:17PM +0200, Michal Židek wrote:
> > Hi,
> >
> > this patch partially solves the ticket
> > https://fedorahosted.org/sssd/ticket/2590
> >
> > It does not subscribe to the systemd dbus interface and does only
> > the polling (this would have to be done anyway for the case
> > the interface is not available).
> >
> > I will investigate how to add the systemd/dbus check as well,
> > but the attached patch should decrease priority of the ticket.
> >
> > Thanks,
> > Michal
> > From bb57b2bcba891cd395d795a7706cf2ea77f8bca2 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Michal=20=C5=BDidek?= 
> > Date: Wed, 17 Jun 2015 18:54:14 +0200
> > Subject: [PATCH] MONITOR: Poll for resolv.conf if not available during boot
> >
> > If resolv.conf is not available when SSSD is starting,
> > check for its existence later.
> >
> > Ticket:
> > https://fedorahosted.org/sssd/ticket/2590
> >
> > src/monitor/monitor.c | 38 ++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > index 01c882a..24fca28 100644
> > --- a/src/monitor/monitor.c
> > +++ b/src/monitor/monitor.c
> > @@ -2259,11 +2259,39 @@ static errno_t monitor_config_file_fallback(TALLOC_CTX *mem_ctx,
> >       return EOK;
> >   }
> >
> > +#define MISSING_RESOLV_CONF_POLL_TIME 10
> >
> > +static void missing_resolv_conf(struct tevent_context *ev,
> >                            struct tevent_timer *te,
> >                            struct timeval tv, void *data)
> >
> > +{
> > int ret;
> > struct mt_ctx *ctx = talloc_get_type(data, struct mt_ctx);
> > ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH,
> >                          monitor_update_resolv, false);
> > if (ret == ENOENT) {
> >    tv = tevent_timeval_current_ofs(MISSING_RESOLV_CONF_POLL_TIME, 0);
> >    te = tevent_add_timer(ctx->ev, ctx, tv, missing_resolv_conf, ctx);
> >    if (te == NULL) {
> >        DEBUG(SSSDBG_FATAL_FAILURE,
> >              "tevent_add_timer failed. resolv.conf will be ignored.\n");
> >    }
> > } else if (ret != EOK) {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Monitor_config_file failed. resolv.conf will be ignored.\n");
> > }
>
> I think we should add signal_res_init() here, because in my testing the
> inotify watch is established if I mv a resolv.conf file into place, but
> the backends remain offline.

Yes, sorry for that. It worked during my testing because I had something
in /etc/hosts, so the missing resolv.conf was shadowed.
> We should also (in a different patch) change the DEBUG levels in
> monitor_config_file, because currently we print a 0x0010-level message.

Ok, but I think we should special case resolv.conf here. Otherwise
the missing (and not ignored) config file should always be reported.
> > return;
> >
> > +}

New patches attached.

Michal

[Jakub Hrozek]

On Tue, Jun 23, 2015 at 06:28:08PM +0200, Michal Židek wrote:
> > We should also (in a different patch) change the DEBUG levels in
> > monitor_config_file, because currently we print a 0x0010-level message.
>
> Ok, but I think we should special case resolv.conf here. Otherwise
> the missing (and not ignored) config file should always be reported.

I don't think we need to, the knowledge of /what/ the config file is
should remain in the caller. As long as we return a proper error code,
the caller can do the special casing.

[Michal Židek]

On 06/23/2015 07:28 PM, Jakub Hrozek wrote:
> On Tue, Jun 23, 2015 at 06:28:08PM +0200, Michal Židek wrote:
> > > We should also (in a different patch) change the DEBUG levels in
> > > monitor_config_file, because currently we print a 0x0010-level message.
> >
> > Ok, but I think we should special case resolv.conf here. Otherwise
> > the missing (and not ignored) config file should always be reported.
>
> I don't think we need to, the knowledge of /what/ the config file is
> should remain in the caller. As long as we return a proper error code,
> the caller can do the special casing.

Ok... would SSSDBG_MINOR_FAILURE level be OK, or is it too verbose?

Michal

[Michal Židek]

On 06/23/2015 07:55 PM, Michal Židek wrote:
> On 06/23/2015 07:28 PM, Jakub Hrozek wrote:
> > On Tue, Jun 23, 2015 at 06:28:08PM +0200, Michal Židek wrote:
> > > > We should also (in a different patch) change the DEBUG levels in
> > > > monitor_config_file, because currently we print a 0x0010-level message.
> > >
> > > Ok, but I think we should special case resolv.conf here. Otherwise
> > > the missing (and not ignored) config file should always be reported.
> >
> > I don't think we need to, the knowledge of /what/ the config file is
> > should remain in the caller. As long as we return a proper error code,
> > the caller can do the special casing.
>
> Ok... would SSSDBG_MINOR_FAILURE level be OK, or is it too verbose?
>
> Michal

New patches attached.

Michal

[Pavel Reichl]

Hello Michal, I have just a few nitpicks/comment to your patch.

On 06/23/2015 08:08 PM, Michal Židek wrote:
> +#define MISSING_RESOLV_CONF_POLL_TIME 10
>
> +static void missing_resolv_conf(struct tevent_context *ev,
>                            struct tevent_timer *te,
>                            struct timeval tv, void *data)
>
> +{
> int ret;
> struct mt_ctx *ctx = talloc_get_type(data, struct mt_ctx);
> ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH,
>                          monitor_update_resolv, false);
> if (ret == EOK) {
>    signal_res_init(ctx);
> } if (ret == ENOENT) {

Did you want to write 'else if' ?  (If 'if' is used on purpose then 
please add new line.)
>    tv = tevent_timeval_current_ofs(MISSING_RESOLV_CONF_POLL_TIME, 0);
>    te = tevent_add_timer(ctx->ev, ctx, tv, missing_resolv_conf, ctx);
>    if (te == NULL) {
>        DEBUG(SSSDBG_FATAL_FAILURE,
>              "tevent_add_timer failed. resolv.conf will be ignored.\n");
>    }
> } else if (ret != EOK) {

If my prev. comment is valid you should use else here, right?
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Monitor_config_file failed. resolv.conf will be ignored.\n");
> }
> return;

Explicit return is in my opinion needless.
> +}

Thanks!

[Michal Židek]

On 06/24/2015 01:38 PM, Pavel Reichl wrote:
> Hello Michal, I have just a few nitpicks/comment to your patch.
>
> On 06/23/2015 08:08 PM, Michal Židek wrote:
> > +#define MISSING_RESOLV_CONF_POLL_TIME 10
> >
> > +static void missing_resolv_conf(struct tevent_context *ev,
> >                            struct tevent_timer *te,
> >                            struct timeval tv, void *data)
> >
> > +{
> > int ret;
> > struct mt_ctx *ctx = talloc_get_type(data, struct mt_ctx);
> > ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH,
> >                          monitor_update_resolv, false);
> > if (ret == EOK) {
> >    signal_res_init(ctx);
> > } if (ret == ENOENT) {
>
> Did you want to write 'else if' ?  (If 'if' is used on purpose then
> please add new line.)

It should have been 'else if'.
> >    tv =
> >
> > tevent_timeval_current_ofs(MISSING_RESOLV_CONF_POLL_TIME, 0);
> >    te = tevent_add_timer(ctx->ev, ctx, tv, missing_resolv_conf,
> >
> > ctx);
> >    if (te == NULL) {
> >        DEBUG(SSSDBG_FATAL_FAILURE,
> >              "tevent_add_timer failed. resolv.conf will be
> >
> > ignored.\n");
> >    }
> > } else if (ret != EOK) {
>
> If my prev. comment is valid you should use else here, right?

Yes.
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Monitor_config_file failed. resolv.conf will be
> >
> > ignored.\n");
> > }
> > return;
>
> Explicit return is in my opinion needless.

Ok.
> > +}
>
> Thanks!

New patches attached.

Michal

[Pavel Reichl]

On 06/24/2015 03:35 PM, Michal Židek wrote:
> From 145f6b193f5ff9e382af441299864aac55c31dd4 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?=
> Date: Wed, 17 Jun 2015 18:54:14 +0200
> Subject: [PATCH 1/2] MONITOR: Poll for resolv.conf if not available during
>   boot
>
> If resolv.conf is not available when SSSD is starting,
> check for its existence later.
>
> Ticket:
> https://fedorahosted.org/sssd/ticket/2590
>
> src/monitor/monitor.c | 38 ++++++++++++++++++++++++++++++++++++--
>   1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index 01c882a..1804ad1 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -2259,11 +2259,39 @@ static errno_t monitor_config_file_fallback(TALLOC_CTX *mem_ctx,
>       return EOK;
>   }
>
> +#define MISSING_RESOLV_CONF_POLL_TIME 10
>
> +static void missing_resolv_conf(struct tevent_context *ev,
>                            struct tevent_timer *te,
>                            struct timeval tv, void *data)
>
> +{
> int ret;
> struct mt_ctx *ctx = talloc_get_type(data, struct mt_ctx);
> ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH,
>                          monitor_update_resolv, false);
> if (ret == EOK) {
>    signal_res_init(ctx);
> } else if (ret == ENOENT) {
>    tv = tevent_timeval_current_ofs(MISSING_RESOLV_CONF_POLL_TIME, 0);
>    te = tevent_add_timer(ctx->ev, ctx, tv, missing_resolv_conf, ctx);
>    if (te == NULL) {
>        DEBUG(SSSDBG_FATAL_FAILURE,
>              "tevent_add_timer failed. resolv.conf will be ignored.\n");
>    }
> } else {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Monitor_config_file failed. resolv.conf will be ignored.\n");
> }
>
> +}
> static int monitor_process_init(struct mt_ctx *ctx,
>                                 const char *config_file)
> {
>     TALLOC_CTX *tmp_ctx;
>     struct tevent_signal *tes;
> struct timeval tv;
> struct tevent_timer *te;
>  struct sss_domain_info *dom;
>  char *rcachedir;
>  int num_providers;
>
> @@ -2351,8 +2379,14 @@ static int monitor_process_init(struct mt_ctx *ctx,
>   #endif
>       /* Watch for changes to the DNS resolv.conf */

Michal would you consider renaming 'missing_resolv_conf' to 
'check_resolv_conf' or something like that and  then replace following 
code segment by simple call of it? I think we could save a few lines of 
code and reuse the function.  Do you see any problems about that? Thanks!
>   ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH,
>                         monitor_update_resolv,true);
> if (ret != EOK) {
>                          monitor_update_resolv, false);
> if (ret == ENOENT) {
>    tv = tevent_timeval_current_ofs(MISSING_RESOLV_CONF_POLL_TIME, 0);
>    te = tevent_add_timer(ctx->ev, ctx, tv, missing_resolv_conf, ctx);
>    if (te == NULL) {
>        DEBUG(SSSDBG_FATAL_FAILURE, "resolv.conf will be ignored\n");
>    }
> } else if (ret != EOK) {
>      return ret;
>  }
>
> -- 2.1.0

[Michal Židek]

On 06/25/2015 01:30 PM, Pavel Reichl wrote:
> On 06/24/2015 03:35 PM, Michal Židek wrote:
> > From 145f6b193f5ff9e382af441299864aac55c31dd4 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Michal=20=C5=BDidek?=
> > Date: Wed, 17 Jun 2015 18:54:14 +0200
> > Subject: [PATCH 1/2] MONITOR: Poll for resolv.conf if not available during
> >   boot
> >
> > If resolv.conf is not available when SSSD is starting,
> > check for its existence later.
> >
> > Ticket:
> > https://fedorahosted.org/sssd/ticket/2590
> >
> > src/monitor/monitor.c | 38 ++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > index 01c882a..1804ad1 100644
> > --- a/src/monitor/monitor.c
> > +++ b/src/monitor/monitor.c
> > @@ -2259,11 +2259,39 @@ static errno_t monitor_config_file_fallback(TALLOC_CTX *mem_ctx,
> >       return EOK;
> >   }
> >
> > +#define MISSING_RESOLV_CONF_POLL_TIME 10
> >
> > +static void missing_resolv_conf(struct tevent_context *ev,
> >                            struct tevent_timer *te,
> >                            struct timeval tv, void *data)
> >
> > +{
> > int ret;
> > struct mt_ctx *ctx = talloc_get_type(data, struct mt_ctx);
> > ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH,
> >                          monitor_update_resolv, false);
> > if (ret == EOK) {
> >    signal_res_init(ctx);
> > } else if (ret == ENOENT) {
> >    tv = tevent_timeval_current_ofs(MISSING_RESOLV_CONF_POLL_TIME, 0);
> >    te = tevent_add_timer(ctx->ev, ctx, tv, missing_resolv_conf, ctx);
> >    if (te == NULL) {
> >        DEBUG(SSSDBG_FATAL_FAILURE,
> >              "tevent_add_timer failed. resolv.conf will be ignored.\n");
> >    }
> > } else {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Monitor_config_file failed. resolv.conf will be ignored.\n");
> > }
> >
> > +}
> > static int monitor_process_init(struct mt_ctx *ctx,
> >                                 const char *config_file)
> > {
> >     TALLOC_CTX *tmp_ctx;
> >     struct tevent_signal *tes;
> > struct timeval tv;
> > struct tevent_timer *te;
> >  struct sss_domain_info *dom;
> >  char *rcachedir;
> >  int num_providers;
> >
> > @@ -2351,8 +2379,14 @@ static int monitor_process_init(struct mt_ctx *ctx,
> >   #endif
> >       /* Watch for changes to the DNS resolv.conf */
>
> Michal would you consider renaming 'missing_resolv_conf' to
> 'check_resolv_conf' or something like that and  then replace following
> code segment by simple call of it? I think we could save a few lines of
> code and reuse the function.  Do you see any problems about that? Thanks!

You are right that it would save us few lines, but It would also require
directly calling a function that was made as tevent callback. I tried it
and it looks IMO uglier (just filling the arguments to the function
looks weird). So I would not go that way if you do not insist on it.
> >   ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH,
> >                         monitor_update_resolv,true);
> > if (ret != EOK) {
> >                          monitor_update_resolv, false);
> > if (ret == ENOENT) {
> >    tv = tevent_timeval_current_ofs(MISSING_RESOLV_CONF_POLL_TIME, 0);
> >    te = tevent_add_timer(ctx->ev, ctx, tv, missing_resolv_conf, ctx);
> >    if (te == NULL) {
> >        DEBUG(SSSDBG_FATAL_FAILURE, "resolv.conf will be ignored\n");
> >    }
> > } else if (ret != EOK) {
> >      return ret;
> >  }

[Pavel Reichl]

On 06/25/2015 04:31 PM, Michal Židek wrote:
> On 06/25/2015 01:30 PM, Pavel Reichl wrote:
> > On 06/24/2015 03:35 PM, Michal Židek wrote:
> > > [snip]
> >
> > Michal would you consider renaming 'missing_resolv_conf' to
> > 'check_resolv_conf' or something like that and  then replace following
> > code segment by simple call of it? I think we could save a few lines of
> > code and reuse the function.  Do you see any problems about that? 
> > Thanks!
>
> You are right that it would save us few lines, but It would also require
> directly calling a function that was made as tevent callback. I tried it
> and it looks IMO uglier (just filling the arguments to the function
> looks weird). So I would not go that way if you do not insist on it.

OK, thank you for investigating that proposal.  Code looks good to me. 
CI passed: http://sssd-ci.duckdns.org/logs/job/18/26/summary.html

I tried to remove resolv.conf and restart SSSD, then I saw your debug 
messages in log file and could not resolve a user. After adding 
resolf.conf back debug message stopped to show in log and the user could 
be resolved.

Unless somebody else has some ideas about further testing or code 
comments I think we can ACK the patch.

Thanks for patience!

[Jakub Hrozek]

On Mon, Jun 29, 2015 at 11:39:30AM +0200, Pavel Reichl wrote:
> On 06/25/2015 04:31 PM, Michal Židek wrote:
> > On 06/25/2015 01:30 PM, Pavel Reichl wrote:
> > > On 06/24/2015 03:35 PM, Michal Židek wrote:
> > > > [snip]
> > >
> > > Michal would you consider renaming 'missing_resolv_conf' to
> > > 'check_resolv_conf' or something like that and  then replace following
> > > code segment by simple call of it? I think we could save a few lines of
> > > code and reuse the function.  Do you see any problems about that?
> > > Thanks!
> >
> > You are right that it would save us few lines, but It would also require
> > directly calling a function that was made as tevent callback. I tried it
> > and it looks IMO uglier (just filling the arguments to the function
> > looks weird). So I would not go that way if you do not insist on it.
>
> OK, thank you for investigating that proposal.  Code looks good to me. CI
> passed: http://sssd-ci.duckdns.org/logs/job/18/26/summary.html
>
> I tried to remove resolv.conf and restart SSSD, then I saw your debug
> messages in log file and could not resolve a user. After adding resolf.conf
> back debug message stopped to show in log and the user could be resolved.
>
> Unless somebody else has some ideas about further testing or code comments I
> think we can ACK the patch.

This is the testing I did as well.
    * 0469c14cae927298838e92d5827c803ca694e7e0
    * 66615eee77792c1437e309d53b19cfb642eca502