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 :-)
LS