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

Michal Židek mzidek at redhat.com
Wed Jul 30 15:43:53 UTC 2014


On 07/30/2014 04:59 PM, Michal Židek wrote:
> On 07/29/2014 03:29 PM, Jakub Hrozek wrote:
>>
>> 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.
>
> Ok.
>
>>
>>>  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?
>
> Done.
>
>>
>>> +    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.
>
> Ok. I'll go with the NULL check then.
>
>
> New patches are attached.
>
> Michal
>

Jakub found one more coding style violation in the code.
Fixed in the new patches.


Thanks,
Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ptask-Allow-adding-random_offset-to-scheduled-execut.patch
Type: text/x-patch
Size: 6703 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140730/deadb38d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ptask-Add-backoff-feature-to-the-ptask-api.patch
Type: text/x-patch
Size: 7051 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140730/deadb38d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Exit-offline-mode-only-if-server-is-available.patch
Type: text/x-patch
Size: 4808 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140730/deadb38d/attachment-0002.bin>


More information about the sssd-devel mailing list