[SSSD] [PATCH] Exit offline mode only if server is available.

Jakub Hrozek jhrozek at redhat.com
Tue Jul 29 13:29:48 UTC 2014


On Tue, Jul 22, 2014 at 07:16:53PM +0200, Michal Židek wrote:
> On 07/09/2014 03:35 PM, Michal Židek wrote:
> >On 07/07/2014 07:33 PM, Michal Židek wrote:
> >>On 07/07/2014 03:09 PM, Michal Židek wrote:
> >>>On 07/07/2014 03:01 PM, Jakub Hrozek wrote:
> >>>>On Mon, Jul 07, 2014 at 02:11:52PM +0200, Michal Židek wrote:
> >>>>>On 07/06/2014 06:44 PM, Simo Sorce wrote:
> >>>>>>On Sun, 2014-07-06 at 12:42 -0400, Simo Sorce wrote:
> >>>>>>>On Fri, 2014-07-04 at 12:34 +0200, Michal Židek wrote:
> >>>>>>>>Hi Sumit,
> >>>>>>>>
> >>>>>>>>actually your suggestion was one of my first ideas, but
> >>>>>>>>It looked like strange behaviour to me if the first
> >>>>>>>>getent (or any request for data not present int the
> >>>>>>>>cache) after offline_timeout period returned nothing
> >>>>>>>>even if the server was already available and the second
> >>>>>>>>(1 or 2 seconds after the first) returned proper data. I
> >>>>>>>>thought this may be considered a bug for some people (even
> >>>>>>>>if it is not a major regression :) ). The patch I sent
> >>>>>>>>was then an improvement over this initial idea (from my POV).
> >>>>>>>>
> >>>>>>>>I understand your suggestion and concern, but I would
> >>>>>>>>prefer to stick with the approach in the patch I sent.
> >>>>>>>
> >>>>>>>Can we at least use a backoff scheme where we increase timeout
> >>>>>>>periods
> >>>>>>>every time we try and find ourselves offline ?
> >>>>>>>
> >>>>>>>Try 1 minute, then 2, then 4, then 8, 16, 32, 64, 128 ...
> >>>>>
> >>>>>Good idea, but this might result in SSSD being too long
> >>>>>offline for no good reason. Would it make sense
> >>>>>to introduce a new config option for upper bound of
> >>>>>this delay (like offline_timeout_max)? Without this option
> >>>>>it would be difficult for me to accept this change. But
> >>>>>I wonder what the default value should be... maybe 64?
> >>>>
> >>>>I would actually prefer to have a sane default..one hour with some
> >>>>randomization to avoid "8AM flood" maybe.
> >>>>
> >>>>>
> >>>>>>
> >>>>>>It would also probably be a good idea to randomize a little the
> >>>>>>delay,
> >>>>>>otherwise we risk thundering herd situations.
> >>>>>>
> >>>>>>Simo.
> >>>>>>
> >>>>>
> >>>>>Should the maximal value of the random offset be configurable?
> >>>>
> >>>>I would prefer to have neither the random offset nor the maximum cap
> >>>>configurable, or have a undocumented option unless we know it makes
> >>>>sense to let the admin configure the option.
> >>>>
> >>>>I'm afraid we have too many timeout options already and it's difficult
> >>>>for admins to know them even without adding onother one..
> >>>>
> >>>
> >>>That is true. I will hardcode the values then.
> >>>
> >>>>>Or is it not needed (I personally think it is not, but would like
> >>>>>to hear other opinions as well)? I am also not sure what the
> >>>>>default value should be (I was thinking 30 seconds).
> >>>>>
> >>>>>Will send new patches soon, thanks for suggestions.
> >>>>>
> >>>>>Michal
> >>>>
> >>
> >>Here are the new patches. The hardcoded values are
> >>30 seconds for the maximum random offset
> >>3600 seconds for the maximum backoff delay
> >>
> >>Michal
> >>
> >
> >Btw. I intentionally did not send the patches for 1.11.
> >I will send them after the patches for master are
> >accepted.
> >
> >Thanks,
> >Michal
> >
> 
> I had a typo in one of the comments. Sending new patches.
> 
> Michal
> 

So far I only read the patches, no testing yet, see some comments
inline:

> From 0724c54b3e4ff73b091afed86122cdf8dbc746ea Mon Sep 17 00:00:00 2001
> From: Michal Zidek <mzidek at redhat.com>
> Date: Mon, 7 Jul 2014 16:46:18 +0200
> Subject: [PATCH 1/3] ptask: Allow adding random_offset to scheduled execution
>  time

ACK

> From bf9d6aebda21397278b5a3f782efaa1a538992cb Mon Sep 17 00:00:00 2001
> From: Michal Zidek <mzidek at redhat.com>
> Date: Mon, 7 Jul 2014 17:45:45 +0200
> Subject: [PATCH 2/3] ptask: Add backoff feature to the ptask api.

I would prefer if the backoff was a parameter to the create function,
creating would better be atomic.

> From fd85d23e9721293adaaaba9927947d31ab68da8a Mon Sep 17 00:00:00 2001
> From: Michal Zidek <mzidek at redhat.com>
> Date: Thu, 3 Jul 2014 12:49:20 +0200
> Subject: [PATCH 3/3] Exit offline mode only if server is available.
> 
> This patch adds periodic check to test if
> backend can exit offline mode and only
> marks backend as not offline if server
> for the service is available.
> 
> Prior to this patch we marked backend as not
> offline if the offline_timeout was reached
> without checking for the server availability
> and when the next request failed again
> we switched back to the offline mode.
> This caused significant slowdowns in some
> edge cases.
> 
> Fixes:
> https://fedorahosted.org/sssd/ticket/2355
> ---
>  src/providers/data_provider_be.c | 68 ++++++++++++++++++++++++++++++----------
>  src/providers/dp_backend.h       |  5 +++
>  2 files changed, 56 insertions(+), 17 deletions(-)
> 
> diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
> index 4c3eaa6..76b7f82 100644
> --- a/src/providers/data_provider_be.c
> +++ b/src/providers/data_provider_be.c
> @@ -450,35 +450,68 @@ static void be_queue_next_request(struct be_req *be_req, enum bet_type type)
>  
>  bool be_is_offline(struct be_ctx *ctx)
>  {
> -    time_t now = time(NULL);
> -    int offline_timeout;
> -    int ret;
> +    return ctx->offstat.offline;
> +}
>  
> -    /* check if we are past the offline blackout timeout */
> -    ret = confdb_get_int(ctx->cdb, ctx->conf_path,
> -                         CONFDB_DOMAIN_OFFLINE_TIMEOUT, 60,
> -                         &offline_timeout);
> -    if (ret != EOK) {
> -        DEBUG(SSSDBG_MINOR_FAILURE,
> -              "Failed to get offline_timeout from confdb. "
> -               "Using default value (60 seconds)\n");
> -        offline_timeout = 60;
> -    }
> +static void check_if_online(struct be_ctx *ctx);
>  
> -    if (ctx->offstat.went_offline + offline_timeout < now) {
> -        ctx->offstat.offline = false;
> -    }
> +errno_t
> +try_to_go_online(TALLOC_CTX *mem_ctx,
> +                 struct tevent_context *ev,
> +                 struct be_ctx *be_ctx,
> +                 struct be_ptask *be_ptask,
> +                 void *be_ctx_void)
> +{

Can you make this function static?

> +    struct be_ctx *ctx = (struct be_ctx*) be_ctx_void;
>  
> -    return ctx->offstat.offline;
> +    check_if_online(ctx);
> +    return EOK;
>  }
>  
>  void be_mark_offline(struct be_ctx *ctx)
>  {
> +    int offline_timeout;
>      DEBUG(SSSDBG_TRACE_INTERNAL, "Going offline!\n");
>  
>      ctx->offstat.went_offline = time(NULL);
>      ctx->offstat.offline = true;
>      ctx->run_online_cb = true;
> +    errno_t ret;
> +
> +    if (ctx->online_check_ptask_initialized == false) {

Can you use just:
    if (ctx->check_if_online_ptask == NULL) 

?

If not, then instead of another structure member, isn't it more readable
to add a new function be_ptask_is_enabled() and then you could call:
    if (ctx->check_if_online_ptask && 
            be_ptask_is_enabled(ctx->check_if_online_ptask)) {
        }

The andvantage is that you don't have to keep track of anything
separately and all the state info is kept in the ptask itself.



More information about the sssd-devel mailing list