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

Michal Židek mzidek at redhat.com
Wed Jul 30 14:59:44 UTC 2014


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
-------------- 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/89d12980/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/89d12980/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: 4806 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140730/89d12980/attachment-0002.bin>


More information about the sssd-devel mailing list