On (20/01/16 11:09), Pavel Březina wrote:
>On 01/19/2016 02:20 PM, Michal Židek wrote:
>>On 01/19/2016 02:07 PM, Pavel Březina wrote:
>>>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.
>>
>>I'll keep it the way it is.
>>
>>Variables should be declared at the beginning of the block.
>>And this is the inner most block, so I can not make it's
>>scope smaller.
>>
>>>
>>>>
>>>>- /* 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.
>>
>>Done.
>>
>>>
>>>>+ 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) {
>
>Ack.
CI passed
http://sssd-ci.duckdns.org/logs/job/36/61/summary.html
but patches does not have any ticlet links in commot message.
I can add link(s) before pushing if someone provides them :-)