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(a)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(a)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(a)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.