NSS: disable midpoint refresh for netgroups if ptask refresh is enabled

[Pavel Reichl]

Hello, 

please see attached patches.

patch #1 - disable midpoint refresh for netgroups if ptask refresh is
enabled

The rest of patches solves some minor problems that occurred while I
was working on 1st patch:

patch #2 - fixes sysdb_getnetgr to return ENOENT as is as is expected in
code
patch #3 - first check return value then access output parameter
patch #4 - some minor code style improvements, some lines over 80
columns, IMO strange indentation of string constants - feel free to
NACK.

Bye,

Pavel Reichl

[Pavel Březina]

On 02/26/2014 06:35 PM, Pavel Reichl wrote:
>  Hello,
>
>  please see attached patches.
>
>  patch #1 - disable midpoint refresh for netgroups if ptask refresh is
>  enabled 
Nack.
It is still possible having a netgroup expired if 
refresh_expired_interval is misconfigured (> entry_cache_timeout). Thus 
you still need to check if the netgroup is valid, maybe throw a visible 
warning if it is expired and periodic refresh is enabled.

>  The rest of patches solves some minor problems that occurred while I
>  was working on 1st patch:
>
>  patch #2 - fixes sysdb_getnetgr to return ENOENT as is as is expected in
>  code 
This is not necessary. ENOENT is returned from ldb_search(). Or did you 
manage to get EOK and count==0 somehow?

>  patch #3 - first check return value then access output parameter
Tentatively ack. The change looks correct and it will not change any 
behaviour, but I couldn't understand code flow in this function in a 
short time - hence tentatively.

Also there are many ways how to leak memory of 'name'. Can you fix it as 
well?

>  patch #4 - some minor code style improvements, some lines over 80
>  columns, IMO strange indentation of string constants - feel free to
>  NACK. 
There will be lots of places in the code like this one:
          DEBUG(SSSDBG_TRACE_LIBS, "netgroup [%s] was already removed\n",
-                                  netgr->name);
+              netgr->name);

It is due to the recent change where we removed parentheses around 
message and its arguments. Aligning format parameters with message was 
more natural when it was closed in parentheses, but IMHO it is still 
better to keep it this way.

-            DEBUG(SSSDBG_OP_FAILURE, "No results for netgroup %s 
(domain %s)\n",
-                      name, dom->name);
+            DEBUG(SSSDBG_OP_FAILURE,
+                  "No results for netgroup %s (domain %s)\n",
+                  name, dom->name);

I personally prefer to keep debug statements on as fewer lines as 
possible, like this:

+            DEBUG(SSSDBG_OP_FAILURE, "No results for netgroup "
+                  "%s (domain %s)\n", name, dom->name);

Many of us uses different approach how to wrap debug statements. I think 
we should agree on one style and make it part of our coding style.

I'm leaving this patch open for now -- it is not nack nor ack.

>  Bye,
>
>  Pavel Reichl

[Pavel Reichl]

On Thu, 2014-02-27 at 11:55 +0100, Pavel Březina wrote:

Hello Pavel,
[snip]
>  > patch #2 - fixes sysdb_getnetgr to return ENOENT as is as is
> expected in
>  > code
>  
>  This is not necessary. ENOENT is returned from ldb_search(). Or did you 
>  manage to get EOK and count==0 somehow?
>   
I actually did manage as is it not hard at all. Give it a try:-). 
I don't know if ldb_search ever returns ENOENT but I dare to say that it
always returns EOK and count == 0 if no record is found. 

That is reason for this ticket
https://fedorahosted.org/sssd/ticket/1991

[Pavel Reichl]

On Thu, 2014-02-27 at 11:55 +0100, Pavel Březina wrote:
[snip]
>  Nack.
>  It is still possible having a netgroup expired if 
>  refresh_expired_interval is misconfigured (> entry_cache_timeout). Thus 
>  you still need to check if the netgroup is valid, maybe throw a visible 
>  warning if it is expired and periodic refresh is enabled.
>   
[snip]

So how about checking that: 

   refresh_expired_intervat < entry_cache_timeout

when configuration is loaded? 

I thing these values are never changed although they are not const.

[Jakub Hrozek]

On Thu, Feb 27, 2014 at 05:15:00PM +0100, Pavel Reichl wrote:
>  On Thu, 2014-02-27 at 11:55 +0100, Pavel Březina wrote:
>  [snip]
>  > Nack.
>  > It is still possible having a netgroup expired if 
>  > refresh_expired_interval is misconfigured (> entry_cache_timeout). Thus 
>  > you still need to check if the netgroup is valid, maybe throw a visible 
>  > warning if it is expired and periodic refresh is enabled.
>  > 
>  [snip]
>   
Reviving another stalled thread :-)

>  So how about checking that: 
>  
>     refresh_expired_intervat < entry_cache_timeout 
Are you proposing to abort startup in this case? That sounds like a bit
too heavy solution..

>  
>  when configuration is loaded? 
>  
>  I thing these values are never changed although they are not const.  
Yeah, I'm not worried about these values changing at runtime. 

In general I see the point of your patch in that the netgroups are only
ever refreshed by the periodical task and no refreshes are scheduled by
the nss frontend..

btw can we get into this situation where the refresh is on but a netgroup
is expired even without misconfiguration? For instance if a netgroup was
saved to the cache right before the backend was about to schedule the
periodic task, then before the next task the netgroup was requested and
already expired? Is it OK in this case to just return stale data and let
the next periodic update refresh the netgroup? I think so, because the
cache would eventually converge..

[Pavel Reichl]

On Mon, 2014-04-07 at 19:07 +0200, Jakub Hrozek wrote:
>  On Thu, Feb 27, 2014 at 05:15:00PM +0100, Pavel Reichl wrote:
>  > On Thu, 2014-02-27 at 11:55 +0100, Pavel Březina wrote:
>  > [snip]
>  > > Nack.
>  > > It is still possible having a netgroup expired if 
>  > > refresh_expired_interval is misconfigured (> entry_cache_timeout). Thus 
>  > > you still need to check if the netgroup is valid, maybe throw a visible 
>  > > warning if it is expired and periodic refresh is enabled.
>  > > 
>  > [snip]
>  > 
>  
>  Reviving another stalled thread :-)
>  
>  > So how about checking that: 
>  > 
>  >    refresh_expired_intervat < entry_cache_timeout
>  
>  Are you proposing to abort startup in this case? That sounds like a bit
>  too heavy solution.. 
No, I would rather log loudly about this problem and then I would set 
refresh_expired_interval to entry_cache_timeout * 3/4 as is advised in
man pages. And again I would log this action loudly.

>  
>  > 
>  > when configuration is loaded? 
>  > 
>  > I thing these values are never changed although they are not const. 
>  
>  Yeah, I'm not worried about these values changing at runtime. 
>  
>  In general I see the point of your patch in that the netgroups are only
>  ever refreshed by the periodical task and no refreshes are scheduled by
>  the nss frontend.. 
I'm sorry, I'm not sure I fully understand - by 'point of my patch' you
actually mean 'problem of my patch'?

>  
>  btw can we get into this situation where the refresh is on but a netgroup
>  is expired even without misconfiguration? For instance if a netgroup was
>  saved to the cache right before the backend was about to schedule the
>  periodic task, then before the next task the netgroup was requested and
>  already expired? Is it OK in this case to just return stale data and let
>  the next periodic update refresh the netgroup? I think so, because the
>  cache would eventually converge.. 
I'm not sure about this, I'll run some
tests tomorrow and I'll let you
know.

>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Wed, Apr 09, 2014 at 08:07:25PM +0200, Pavel Reichl wrote:
>  On Mon, 2014-04-07 at 19:07 +0200, Jakub Hrozek wrote:
>  > On Thu, Feb 27, 2014 at 05:15:00PM +0100, Pavel Reichl wrote:
>  > > On Thu, 2014-02-27 at 11:55 +0100, Pavel Březina wrote:
>  > > [snip]
>  > > > Nack.
>  > > > It is still possible having a netgroup expired if 
>  > > > refresh_expired_interval is misconfigured (> entry_cache_timeout). Thus
>
>  > > > you still need to check if the netgroup is valid, maybe throw a visible 
>  > > > warning if it is expired and periodic refresh is enabled.
>  > > > 
>  > > [snip]
>  > > 
>  > 
>  > Reviving another stalled thread :-)
>  > 
>  > > So how about checking that: 
>  > > 
>  > >    refresh_expired_intervat < entry_cache_timeout
>  > 
>  > Are you proposing to abort startup in this case? That sounds like a bit
>  > too heavy solution..
>  
>  No, I would rather log loudly about this problem and then I would set 
>  refresh_expired_interval to entry_cache_timeout * 3/4 as is advised in
>  man pages. And again I would log this action loudly. 
That would work for me. I don't think we should over-engineer the
solution, after all, the goal was to make it clear to the admin that
they need to tune the config themselves..

>  
>  > 
>  > > 
>  > > when configuration is loaded? 
>  > > 
>  > > I thing these values are never changed although they are not const. 
>  > 
>  > Yeah, I'm not worried about these values changing at runtime. 
>  > 
>  > In general I see the point of your patch in that the netgroups are only
>  > ever refreshed by the periodical task and no refreshes are scheduled by
>  > the nss frontend..
>  
>  I'm sorry, I'm not sure I fully understand - by 'point of my patch' you
>  actually mean 'problem of my patch'? 
What I meant is that this addition:
+        if (req_type == SSS_DP_NETGR &&
+            dctx->domain->refresh_expired_interval != 0) {
+            ret = EOK;

Makes sense as well to me, because this way neither midpoint refresh nor 
expired record request would be scheduled. What I'm not sure about is --
would we expand this condition in the future when we add more background
refresh types?

>  
>  > 
>  > btw can we get into this situation where the refresh is on but a netgroup
>  > is expired even without misconfiguration? For instance if a netgroup was
>  > saved to the cache right before the backend was about to schedule the
>  > periodic task, then before the next task the netgroup was requested and
>  > already expired? Is it OK in this case to just return stale data and let
>  > the next periodic update refresh the netgroup? I think so, because the
>  > cache would eventually converge..
>  I'm not sure about this, I'll run some tests tomorrow and I'll let you
>  know.

[Pavel Reichl]

On Mon, 2014-04-07 at 19:07 +0200, Jakub Hrozek wrote:
[snip]
>  btw can we get into this situation where the refresh is on but a
> netgroup
>  is expired even without misconfiguration? For instance if a netgroup was
>  saved to the cache right before the backend was about to schedule the
>  periodic task, then before the next task the netgroup was requested and
>  already expired?  
I see your concern now. But I think that the most critical moment would
be if netgroup was saved to the cache right *after* the periodic task
being scheduled. That could lead to netgroup not being refreshed for
almost 2*refresh_expired_interval and thus being stale for 1/2
entry_cache_timeout (~2700 seconds) if default values were used*.

>  Is it OK in this case to just return stale data and let
>  the next periodic update refresh the netgroup? 
>  I think so, because the
>  cache would eventually converge.. 
I think so.

--
*
entry_cache_timeout = 5400
refresh_expired_interval = 3/4 * entry_cache_timeout
entry_cache_netgroup_timeout = entry_cache_timeout

[Pavel Reichl]

On Mon, 2014-04-14 at 16:32 +0200, Jakub Hrozek wrote:
>  On Wed, Apr 09, 2014 at 08:07:25PM +0200, Pavel Reichl wrote:
>  > On Mon, 2014-04-07 at 19:07 +0200, Jakub Hrozek wrote:
>  > > On Thu, Feb 27, 2014 at 05:15:00PM +0100, Pavel Reichl wrote:
>  > > > On Thu, 2014-02-27 at 11:55 +0100, Pavel Březina wrote:
>  > > > [snip]
>  > > > > Nack.
>  > > > > It is still possible having a netgroup expired if 
>  > > > > refresh_expired_interval is misconfigured (> entry_cache_timeout).
> Thus 
>  > > > > you still need to check if the netgroup is valid, maybe throw a
> visible 
>  > > > > warning if it is expired and periodic refresh is enabled.
>  > > > > 
>  > > > [snip]
>  > > > 
>  > > 
>  > > Reviving another stalled thread :-)
>  > > 
>  > > > So how about checking that: 
>  > > > 
>  > > >    refresh_expired_intervat < entry_cache_timeout
>  > > 
>  > > Are you proposing to abort startup in this case? That sounds like a bit
>  > > too heavy solution..
>  > 
>  > No, I would rather log loudly about this problem and then I would set 
>  > refresh_expired_interval to entry_cache_timeout * 3/4 as is advised in
>  > man pages. And again I would log this action loudly.
>  
>  That would work for me. I don't think we should over-engineer the
>  solution, after all, the goal was to make it clear to the admin that
>  they need to tune the config themselves..
>  
>  > 
>  > > 
>  > > > 
>  > > > when configuration is loaded? 
>  > > > 
>  > > > I thing these values are never changed although they are not const. 
>  > > 
>  > > Yeah, I'm not worried about these values changing at runtime. 
>  > > 
>  > > In general I see the point of your patch in that the netgroups are only
>  > > ever refreshed by the periodical task and no refreshes are scheduled by
>  > > the nss frontend..
>  > 
>  > I'm sorry, I'm not sure I fully understand - by 'point of my patch'
> you
>  > actually mean 'problem of my patch'?
>  
>  What I meant is that this addition:
>  +        if (req_type == SSS_DP_NETGR &&
>  +            dctx->domain->refresh_expired_interval != 0) {
>  +            ret = EOK;
>  
>  Makes sense as well to me, because this way neither midpoint refresh nor 
>  expired record request would be scheduled. What I'm not sure about is --
>  would we expand this condition in the future when we add more background
>  refresh types? 
I can't see why we couldn't :-).

I just think that in the case we add some more refresh types we should
update man page regarding advise on how to set safe value for
refresh_expired_interval. 

Something like:
- You can consider setting this value to 3/4 * entry_cache_timeout
+ You can consider setting this value to 3/4 *
+ min(entry_cache_netgroup_timeout, entry_cache_xxx_timeout,...)

And customize computation of safe value in case of misconfiguration
accordingly. 

>  
>  > 
>  > > 
>  > > btw can we get into this situation where the refresh is on but a netgroup
>  > > is expired even without misconfiguration? For instance if a netgroup was
>  > > saved to the cache right before the backend was about to schedule the
>  > > periodic task, then before the next task the netgroup was requested and
>  > > already expired? Is it OK in this case to just return stale data and let
>  > > the next periodic update refresh the netgroup? I think so, because the
>  > > cache would eventually converge..
>  > I'm not sure about this, I'll run some tests tomorrow and I'll let you
>  > know.
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Wed, Apr 16, 2014 at 03:04:16PM +0200, Pavel Reichl wrote:
>  On Mon, 2014-04-07 at 19:07 +0200, Jakub Hrozek wrote:
>  [snip]
>  > btw can we get into this situation where the refresh is on but a netgroup
>  > is expired even without misconfiguration? For instance if a netgroup was
>  > saved to the cache right before the backend was about to schedule the
>  > periodic task, then before the next task the netgroup was requested and
>  > already expired? 
>  
>  I see your concern now. But I think that the most critical moment would
>  be if netgroup was saved to the cache right *after* the periodic task
>  being scheduled. That could lead to netgroup not being refreshed for
>  almost 2*refresh_expired_interval and thus being stale for 1/2
>  entry_cache_timeout (~2700 seconds) if default values were used*. 
Stale records, if they are in the range of minutes are OK in this case,
I think. The caching comes at a cost.

What would not be so ideal is if the next getnetgrent() would cause the
whole hierarchy to be downloaded again.

[Jakub Hrozek]

On Wed, Apr 16, 2014 at 03:20:41PM +0200, Pavel Reichl wrote:
>  On Mon, 2014-04-14 at 16:32 +0200, Jakub Hrozek wrote:
>  > On Wed, Apr 09, 2014 at 08:07:25PM +0200, Pavel Reichl wrote:
>  > > On Mon, 2014-04-07 at 19:07 +0200, Jakub Hrozek wrote:
>  > > > On Thu, Feb 27, 2014 at 05:15:00PM +0100, Pavel Reichl wrote:
>  > > > > On Thu, 2014-02-27 at 11:55 +0100, Pavel Březina wrote:
>  > > > > [snip]
>  > > > > > Nack.
>  > > > > > It is still possible having a netgroup expired if 
>  > > > > > refresh_expired_interval is misconfigured (>
> entry_cache_timeout). Thus 
>  > > > > > you still need to check if the netgroup is valid, maybe throw a
> visible 
>  > > > > > warning if it is expired and periodic refresh is enabled.
>  > > > > > 
>  > > > > [snip]
>  > > > > 
>  > > > 
>  > > > Reviving another stalled thread :-)
>  > > > 
>  > > > > So how about checking that: 
>  > > > > 
>  > > > >    refresh_expired_intervat < entry_cache_timeout
>  > > > 
>  > > > Are you proposing to abort startup in this case? That sounds like a bit
>  > > > too heavy solution..
>  > > 
>  > > No, I would rather log loudly about this problem and then I would set 
>  > > refresh_expired_interval to entry_cache_timeout * 3/4 as is advised in
>  > > man pages. And again I would log this action loudly.
>  > 
>  > That would work for me. I don't think we should over-engineer the
>  > solution, after all, the goal was to make it clear to the admin that
>  > they need to tune the config themselves..
>  > 
>  > > 
>  > > > 
>  > > > > 
>  > > > > when configuration is loaded? 
>  > > > > 
>  > > > > I thing these values are never changed although they are not const. 
>  > > > 
>  > > > Yeah, I'm not worried about these values changing at runtime. 
>  > > > 
>  > > > In general I see the point of your patch in that the netgroups are only
>  > > > ever refreshed by the periodical task and no refreshes are scheduled by
>  > > > the nss frontend..
>  > > 
>  > > I'm sorry, I'm not sure I fully understand - by 'point of my
> patch' you
>  > > actually mean 'problem of my patch'?
>  > 
>  > What I meant is that this addition:
>  > +        if (req_type == SSS_DP_NETGR &&
>  > +            dctx->domain->refresh_expired_interval != 0) {
>  > +            ret = EOK;
>  > 
>  > Makes sense as well to me, because this way neither midpoint refresh nor 
>  > expired record request would be scheduled. What I'm not sure about is --
>  > would we expand this condition in the future when we add more background
>  > refresh types?
>  
>  I can't see why we couldn't :-). 
Mostly because the code would be messy with ifs dependent on particular
lookup type.

>  
>  I just think that in the case we add some more refresh types we should
>  update man page regarding advise on how to set safe value for
>  refresh_expired_interval. 
>  
>  Something like:
>  - You can consider setting this value to 3/4 * entry_cache_timeout
>  + You can consider setting this value to 3/4 *
>  + min(entry_cache_netgroup_timeout, entry_cache_xxx_timeout,...)
>  
>  And customize computation of safe value in case of misconfiguration
>  accordingly.  
Hm, probably. In general I prefer caching to work out-of-the-box where
possible, but in this case, some tuning might be neccessary.

[Lukas Slebodnik]

On (26/02/14 18:35), Pavel Reichl wrote:
> Hello, 
>
> please see attached patches.
>
> patch #1 - disable midpoint refresh for netgroups if ptask refresh is
> enabled
>
> The rest of patches solves some minor problems that occurred while I
> was working on 1st patch:
>
> patch #2 - fixes sysdb_getnetgr to return ENOENT as is as is expected in
> code
> patch #3 - first check return value then access output parameter
> patch #4 - some minor code style improvements, some lines over 80
> columns, IMO strange indentation of string constants - feel free to
> NACK.
>
> Bye,
>
> Pavel Reichl 
It seems that tese patches are superseeded by your the latest patches (Apr 17)

Can you confirm?
because I want to change state for this thread in patchwork
https://patchwork.acksyn.org/patch/5197/

LS

[Pavel Reichl]

yes, they are

----- Original Message -----
From: "Lukas Slebodnik" <lslebodn(a)redhat.com&gt;
To: "Development of the System Security Services Daemon"
<sssd-devel(a)lists.fedorahosted.org&gt;
Sent: Thursday, April 17, 2014 7:43:00 PM
Subject: Re: [SSSD] NSS: disable midpoint refresh for netgroups if ptask refresh is
enabled

On (26/02/14 18:35), Pavel Reichl wrote:
> Hello, 
>
> please see attached patches.
>
> patch #1 - disable midpoint refresh for netgroups if ptask refresh is
> enabled
>
> The rest of patches solves some minor problems that occurred while I
> was working on 1st patch:
>
> patch #2 - fixes sysdb_getnetgr to return ENOENT as is as is expected in
> code
> patch #3 - first check return value then access output parameter
> patch #4 - some minor code style improvements, some lines over 80
> columns, IMO strange indentation of string constants - feel free to
> NACK.
>
> Bye,
>
> Pavel Reichl 
It seems that tese patches are superseeded by your the latest patches (Apr 17)

Can you confirm?
because I want to change state for this thread in patchwork
https://patchwork.acksyn.org/patch/5197/

LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel