[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