Hi,
this patch solves the ticket https://fedorahosted.org/sssd/ticket/2355
I was able to reproduce the slowdowns that the reported experienced by pausing a virtual machine with the server. With this patch the slowdowns no longer appear.
I use the already implemented function check_if_online in the new periodic task. See patch description for more details.
Patches for master and sssd-1-11 are in the attachment.
Thanks, Michal
On Thu, Jul 03, 2014 at 01:56:09PM +0200, Michal Židek wrote:
Hi,
this patch solves the ticket https://fedorahosted.org/sssd/ticket/2355
I was able to reproduce the slowdowns that the reported experienced by pausing a virtual machine with the server. With this patch the slowdowns no longer appear.
I use the already implemented function check_if_online in the new periodic task. See patch description for more details.
I have one minor concern with the approach. On busy systems there is no difference after a minute SSSD tries to go online. But if the system is idle SSSD waited for some activity before tying to go online. With your patch it will continue to try to go online every minute. For a system running idle during the night this might be a lot of useless request.
I wonder if the current online-check can just be made a background task and the backend returns immediately with the offline status? For busy systems there would be basically no difference as well. For the running idle over night use-case there would be a little drawback. With your scheme if SSSD goes online during the night the first request in the morning already it attempted online. While with my suggestion it will still be offline. But since the first would be a lookup request in most case and an authentication following this has a fair chance to already to online I think it is only a minor drawback.
Please note that this is just a suggestion, your patch is fine and it is a big improvement of the current behaviour. If you or others prefer to keep it this way I'm fine and I'll happily would start to review the patch.
bye, Sumit
Patches for master and sssd-1-11 are in the attachment.
Thanks, Michal
On Fri, Jul 04, 2014 at 10:16:21AM +0200, Sumit Bose wrote:
On Thu, Jul 03, 2014 at 01:56:09PM +0200, Michal Židek wrote:
Hi,
this patch solves the ticket https://fedorahosted.org/sssd/ticket/2355
I was able to reproduce the slowdowns that the reported experienced by pausing a virtual machine with the server. With this patch the slowdowns no longer appear.
I use the already implemented function check_if_online in the new periodic task. See patch description for more details.
I have one minor concern with the approach. On busy systems there is no difference after a minute SSSD tries to go online. But if the system is idle SSSD waited for some activity before tying to go online. With your patch it will continue to try to go online every minute. For a system running idle during the night this might be a lot of useless request.
I wonder if the current online-check can just be made a background task and the backend returns immediately with the offline status? For busy systems there would be basically no difference as well. For the running idle over night use-case there would be a little drawback. With your scheme if SSSD goes online during the night the first request in the morning already it attempted online. While with my suggestion it will still be offline. But since the first would be a lookup request in most case and an authentication following this has a fair chance to already to online I think it is only a minor drawback.
I think this is a good suggestion. Yes there is a drawback, but I think it can be dismissed because: 1) if the system was running offline, checks that usually involve going to the server like the possibility of a password change or group membership change wouldn't still propagate. So we are really talking about a short window of 'going online'. Moreover, the online/offline status is currently a bit hidden to the user so if there is a system that typically goes offline,
2) In many cases (like the disconnected laptop use-case) SSSD would be notified asynchronously that it's time to go online with the resolv.conf inotify watch or libnl integration.
I'm just wondering if we risk regression for systems that might go offline (maybe not laptops, but servers that might lose connectivity to authentication servers) but don't use cache_credentials. But still, we're only debating the first authentication after going offline, the authentication requests during the offline state would still fail, so this is a moot point probably..
Please note that this is just a suggestion, your patch is fine and it is a big improvement of the current behaviour. If you or others prefer to keep it this way I'm fine and I'll happily would start to review the patch.
bye, Sumit
Patches for master and sssd-1-11 are in the attachment.
Thanks, Michal
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 07/04/2014 11:05 AM, Jakub Hrozek wrote:
On Fri, Jul 04, 2014 at 10:16:21AM +0200, Sumit Bose wrote:
On Thu, Jul 03, 2014 at 01:56:09PM +0200, Michal Židek wrote:
Hi,
this patch solves the ticket https://fedorahosted.org/sssd/ticket/2355
I was able to reproduce the slowdowns that the reported experienced by pausing a virtual machine with the server. With this patch the slowdowns no longer appear.
I use the already implemented function check_if_online in the new periodic task. See patch description for more details.
I have one minor concern with the approach. On busy systems there is no difference after a minute SSSD tries to go online. But if the system is idle SSSD waited for some activity before tying to go online. With your patch it will continue to try to go online every minute. For a system running idle during the night this might be a lot of useless request.
I wonder if the current online-check can just be made a background task and the backend returns immediately with the offline status? For busy systems there would be basically no difference as well. For the running idle over night use-case there would be a little drawback. With your scheme if SSSD goes online during the night the first request in the morning already it attempted online. While with my suggestion it will still be offline. But since the first would be a lookup request in most case and an authentication following this has a fair chance to already to online I think it is only a minor drawback.
I think this is a good suggestion. Yes there is a drawback, but I think it can be dismissed because: 1) if the system was running offline, checks that usually involve going to the server like the possibility of a password change or group membership change wouldn't still propagate. So we are really talking about a short window of 'going online'. Moreover, the online/offline status is currently a bit hidden to the user so if there is a system that typically goes offline,
2) In many cases (like the disconnected laptop use-case) SSSD would be notified asynchronously that it's time to go online with the resolv.conf inotify watch or libnl integration.I'm just wondering if we risk regression for systems that might go offline (maybe not laptops, but servers that might lose connectivity to authentication servers) but don't use cache_credentials. But still, we're only debating the first authentication after going offline, the authentication requests during the offline state would still fail, so this is a moot point probably..
Please note that this is just a suggestion, your patch is fine and it is a big improvement of the current behaviour. If you or others prefer to keep it this way I'm fine and I'll happily would start to review the patch.
bye, Sumit
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.
Thanks, Michal
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 ...
Simo.
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 ...
It would also probably be a good idea to randomize a little the delay, otherwise we risk thundering herd situations.
Simo.
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?
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? 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
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..
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
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
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
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
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
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@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@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@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.
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@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@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@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
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@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@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@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
On Thu, Jul 31, 2014 at 11:49:00AM +0200, Jakub Hrozek wrote:
On Wed, Jul 30, 2014 at 05:43:53PM +0200, Michal Židek wrote:
Jakub found one more coding style violation in the code. Fixed in the new patches.
Thanks, Michal
These patches work for me!
ACK
Pushed to master so far: 45eb92f4564c76d04535b5c8e85e769db3ea93a3 b39098de5bbb7513d5509fb30a3096a3bd9d43f2 ab0ab5a30379b84d6e05e1f2dc457bd1dd97401f
Can you prepare a 1.11 version?
On 07/31/2014 11:54 AM, Jakub Hrozek wrote:
On Thu, Jul 31, 2014 at 11:49:00AM +0200, Jakub Hrozek wrote:
On Wed, Jul 30, 2014 at 05:43:53PM +0200, Michal Židek wrote:
Jakub found one more coding style violation in the code. Fixed in the new patches.
Thanks, Michal
These patches work for me!
ACK
Pushed to master so far: 45eb92f4564c76d04535b5c8e85e769db3ea93a3 b39098de5bbb7513d5509fb30a3096a3bd9d43f2 ab0ab5a30379b84d6e05e1f2dc457bd1dd97401f
Can you prepare a 1.11 version?
Sure.
1.11 patches in attachment.
Thanks for the review.
Michal
On Thu, Jul 31, 2014 at 12:52:46PM +0200, Michal Židek wrote:
On 07/31/2014 11:54 AM, Jakub Hrozek wrote:
On Thu, Jul 31, 2014 at 11:49:00AM +0200, Jakub Hrozek wrote:
On Wed, Jul 30, 2014 at 05:43:53PM +0200, Michal Židek wrote:
Jakub found one more coding style violation in the code. Fixed in the new patches.
Thanks, Michal
These patches work for me!
ACK
Pushed to master so far: 45eb92f4564c76d04535b5c8e85e769db3ea93a3 b39098de5bbb7513d5509fb30a3096a3bd9d43f2 ab0ab5a30379b84d6e05e1f2dc457bd1dd97401f
Can you prepare a 1.11 version?
Sure.
1.11 patches in attachment.
Thanks for the review.
Michal
ACK for sssd-1-11
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Michal Židek -
Simo Sorce -
Sumit Bose