On 01/19/2016 01:13 PM, Michal Židek wrote:
On 01/19/2016 12:28 PM, Pavel Březina wrote:
> On 01/19/2016 12:24 PM, Michal Židek wrote:
>> On 01/19/2016 11:56 AM, Pavel Březina wrote:
>>> On 01/19/2016 02:08 AM, Michal Židek wrote:
>>>> Hello,
>>>>
>>>> this patch applies on top of the
>>>> patch from thred:
>>>> [SSSD] [PATCH] NSS: Refresh also netgroup cache if needed
>>>>
>>>> It (re-)fixes the ticket:
>>>>
https://fedorahosted.org/sssd/ticket/2102
>>>>
>>>> I separated them to give this one special attention :)
>>>> and also because I am not sure if it is a good solution or
>>>> not.
>>>>
>>>> I would like pbrezina or preichl to review
>>>> this one, because they were the original
>>>> reviewer/author for the fix, so maybe they
>>>> still remember why it was done that way.
>>>>
>>>> Michal
>>>
>>> Nack.
>>>
>>>> if (ret == EOK) {
>>>> DEBUG(SSSDBG_TRACE_FUNC, "Cached entry is valid,
>>>> returning..\n");
>>>> return EOK;
>>>> } else if (ret == EAGAIN &&
>>>> dctx->domain->refresh_expired_interval
>>>> && req_type == SSS_DP_NETGR) {
>>>> /* Skip midpoint refresh if background refresh is enabled
>>>> * (for netgroups only)
>>>> */
>>>> return EOK;
>>>> } else if (ret != EAGAIN && ret != ENOENT) {
>>>> DEBUG(SSSDBG_CRIT_FAILURE, "Error checking cache:
%d\n",
>>>> ret);
>>>> goto error;
>>>> }
>>>
>>> Please, return is_refresh_on_bg function and extend it to return true
>>> also for users and groups and merge the condition with the first one so
>>> debug message is visible, like so:
>>>
>>> if (ret == EOK || (ret == EAGAIN && is_refreshed_on_bg(...)) {
>>> DEBUG(SSSDBG_TRACE_FUNC, "Cached entry is valid,
returning..\n");
>>> return EOK;
>>> }
>>>
>>> Maybe you can merge those two patches together..
>>
>> I wanted this one only in master. Do you think it should be
>> backported as well? I wanted only necessary changes in downstream.
>>
>> Michal
>
> I think it should be backported since "Tests showed that having midpoint
> refresh enabled may actually slow down netgroup request occasionally."
> See:
>
https://fedorahosted.org/sssd/ticket/2102
>
> I don't remember thought on which tests is this statement based on...
Ok, I merged the two then. Attached is the merged patch for master and
1.13.
Michal
0001-NSS-do-not-skip-cache-check-for-netgoups.patch
From c4209629af439f3ead805b7c89ee9ac36c6264a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?=<mzidek(a)redhat.com>
Date: Mon, 18 Jan 2016 22:02:55 +0100
Subject: [PATCH] NSS: do not skip cache check for netgoups
When refresh_expired_interval was not zero,
the NSS responder only refreshed netgroup cache
using background periodic task and ignored
SYSDB_CACHE_EXPIRE attribute.
With this behaviour it was impossible to
get new netgroup from remote server even
after sss_cache tool was used to expire
existing entry in the cache.
---
src/responder/nss/nsssrv_cmd.c | 48 +++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index d6ac9dc..1f24d1b 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -579,10 +579,9 @@ static int nss_cmd_getpw_send_reply(struct nss_dom_ctx *dctx, bool
filter)
return EOK;
}
-/* Currently only refreshing expired netgroups is supported. */
static bool
is_refreshed_on_bg(enum sss_dp_acct_type req_type,
- enum sss_dp_acct_type refresh_expired_interval)
+ uint32_t refresh_expired_interval)
{
if (refresh_expired_interval == 0) {
return false;
@@ -590,6 +589,8 @@ is_refreshed_on_bg(enum sss_dp_acct_type req_type,
switch (req_type) {
case SSS_DP_NETGR:
+ case SSS_DP_USER:
+ case SSS_DP_GROUP:
return true;
default:
return false;
@@ -753,31 +754,30 @@ errno_t check_cache(struct nss_dom_ctx *dctx,
get_dp_name_and_id(dctx->cmdctx, dctx->domain, req_type, opt_name, opt_id,
&name, &id);
- /* if we have any reply let's check cache validity, but ignore netgroups
- * if refresh_expired_interval is set (which implies that another method
- * is used to refresh netgroups)
- */
+ /* if we have any reply let's check cache validity */
if (res->count > 0) {
- if (is_refreshed_on_bg(req_type,
- dctx->domain->refresh_expired_interval)) {
- ret = EOK;
- } else {
- if (req_type == SSS_DP_INITGROUPS) {
- cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
- SYSDB_INITGR_EXPIRE,
- 0);
- } else {
- cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
- SYSDB_CACHE_EXPIRE,
- 0);
- }
+ bool refreshed_on_bg;
+ uint32_t bg_refresh_interval = dctx->domain->refresh_expired_interval;
If you want to declare variables inside the block you can also move it
so it is next to its assignment to reduce the scope even further since
we use C99 anyway... but I'll leave it up to you.
- /* if we have any reply let's check cache validity */
- ret = sss_cmd_check_cache(res->msgs[0],
- nctx->cache_refresh_percent,
- cacheExpire);
+ if (req_type == SSS_DP_INITGROUPS) {
+ cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
+ SYSDB_INITGR_EXPIRE,
+ 0);
+ } else {
+ cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
+ SYSDB_CACHE_EXPIRE,
+ 0);
}
- if (ret == EOK) {
+
+ /* Check if background refresh is enabled for this entry */
You can put this on one line.
+ refreshed_on_bg = is_refreshed_on_bg(req_type,
+ bg_refresh_interval);
+
+ /* if we have any reply let's check cache validity */
+ ret = sss_cmd_check_cache(res->msgs[0],
+ nctx->cache_refresh_percent,
+ cacheExpire);
+ if (ret == EOK || (ret == EAGAIN && refreshed_on_bg)) {
DEBUG(SSSDBG_TRACE_FUNC, "Cached entry is valid,
returning..\n");
return EOK;
} else if (ret != EAGAIN && ret != ENOENT) {