On Thu, Jan 21, 2016 at 02:55:51PM +0100, Lukas Slebodnik wrote:
> 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 :-)
Add
https://fedorahosted.org/sssd/ticket/2912
before pushing, please.
master:
* 1b8858b1611db5048592f477059ca5ad66d7ceb1
sssd-1-13:
* 66c6bf86da1241c3253d23aa7e68850d6ec14d15
LS