ehlo,
If id provider is {ipa, ad} periodic task will be stared in sssm_{ipa,ad}_init If you enable enumeration and use different providers for id and sudo(autofs) then another periodic task will be scheduled. This can cause weird behaviour (e.g. missing members of group)
I provided test package to reporter of bug #2153 with attached patch (actually it was patch for 1.9 branch). I was not able to reproduce problem with missing groups. Thus I was wainting for response from customer. But it will be better to do a (pre-)review of patch.
I am also attaching part of log file. You can notice Two enumerations are started. There is difference only few milliseconds.
LS
On Sat, Feb 22, 2014 at 12:18:01AM +0100, Lukas Slebodnik wrote:
ehlo,
If id provider is {ipa, ad} periodic task will be stared in sssm_{ipa,ad}_init If you enable enumeration and use different providers for id and sudo(autofs) then another periodic task will be scheduled. This can cause weird behaviour (e.g. missing members of group)
I provided test package to reporter of bug #2153 with attached patch (actually it was patch for 1.9 branch). I was not able to reproduce problem with missing groups. Thus I was wainting for response from customer. But it will be better to do a (pre-)review of patch.
I am also attaching part of log file. You can notice Two enumerations are started. There is difference only few milliseconds.
LS
I admit I would have preferred an idempotent solution that would just result in a noop if called twice (maybe checking if either cleanup or enum is loaded), but after some time thinking about the solution I don't think it would be so easy.
Could you just add a comment to ldap_id_init_internal() explaining what the main_init does and another comment to sssm_ldap_id_init() reminding the developer that changes should go directly to ldap_id_init_internal() ?
On Mon, Feb 24, 2014 at 08:34:35PM +0100, Jakub Hrozek wrote:
On Sat, Feb 22, 2014 at 12:18:01AM +0100, Lukas Slebodnik wrote:
ehlo,
If id provider is {ipa, ad} periodic task will be stared in sssm_{ipa,ad}_init If you enable enumeration and use different providers for id and sudo(autofs) then another periodic task will be scheduled. This can cause weird behaviour (e.g. missing members of group)
I provided test package to reporter of bug #2153 with attached patch (actually it was patch for 1.9 branch). I was not able to reproduce problem with missing groups. Thus I was wainting for response from customer. But it will be better to do a (pre-)review of patch.
I am also attaching part of log file. You can notice Two enumerations are started. There is difference only few milliseconds.
LS
I admit I would have preferred an idempotent solution that would just result in a noop if called twice (maybe checking if either cleanup or enum is loaded), but after some time thinking about the solution I don't think it would be so easy.
Could you just add a comment to ldap_id_init_internal() explaining what the main_init does and another comment to sssm_ldap_id_init() reminding the developer that changes should go directly to ldap_id_init_internal() ?
Or what about moving the periodic task setup from ldap_id_init_internal() to sssm_ldap_id_init() after the internal init is finished? That way we'd be able to drop the boolean parameter as well.
On (24/02/14 20:45), Jakub Hrozek wrote:
On Mon, Feb 24, 2014 at 08:34:35PM +0100, Jakub Hrozek wrote:
On Sat, Feb 22, 2014 at 12:18:01AM +0100, Lukas Slebodnik wrote:
ehlo,
If id provider is {ipa, ad} periodic task will be stared in sssm_{ipa,ad}_init If you enable enumeration and use different providers for id and sudo(autofs) then another periodic task will be scheduled. This can cause weird behaviour (e.g. missing members of group)
I provided test package to reporter of bug #2153 with attached patch (actually it was patch for 1.9 branch). I was not able to reproduce problem with missing groups. Thus I was wainting for response from customer. But it will be better to do a (pre-)review of patch.
I am also attaching part of log file. You can notice Two enumerations are started. There is difference only few milliseconds.
LS
I admit I would have preferred an idempotent solution that would just result in a noop if called twice (maybe checking if either cleanup or enum is loaded), but after some time thinking about the solution I don't think it would be so easy.
Could you just add a comment to ldap_id_init_internal() explaining what the main_init does and another comment to sssm_ldap_id_init() reminding the developer that changes should go directly to ldap_id_init_internal() ?
Or what about moving the periodic task setup from ldap_id_init_internal() to sssm_ldap_id_init() after the internal init is finished? That way we'd be able to drop the boolean parameter as well.
As you wish master.
updated patch is attached.
LS
On Tue, Feb 25, 2014 at 10:03:23PM +0100, Lukas Slebodnik wrote:
On (25/02/14 21:05), Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 08:05:45PM +0100, Lukas Slebodnik wrote:
updated patch is attached.
LS
I think you sent the original patch by accident.
Once again.
LS
I tested the combination the user described (id_provider=ipa && sudo_provider=ldap) and at least verified other providers (autofs) do start.
The code looks good to me as well.
ACK
On Wed, Feb 26, 2014 at 10:22:53AM +0100, Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 10:03:23PM +0100, Lukas Slebodnik wrote:
On (25/02/14 21:05), Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 08:05:45PM +0100, Lukas Slebodnik wrote:
updated patch is attached.
LS
I think you sent the original patch by accident.
Once again.
LS
I tested the combination the user described (id_provider=ipa && sudo_provider=ldap) and at least verified other providers (autofs) do start.
The code looks good to me as well.
ACK
Pushed to master and sssd-1-11
On Wed, Feb 26, 2014 at 10:41:04AM +0100, Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 10:22:53AM +0100, Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 10:03:23PM +0100, Lukas Slebodnik wrote:
On (25/02/14 21:05), Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 08:05:45PM +0100, Lukas Slebodnik wrote:
updated patch is attached.
LS
I think you sent the original patch by accident.
Once again.
LS
I tested the combination the user described (id_provider=ipa && sudo_provider=ldap) and at least verified other providers (autofs) do start.
The code looks good to me as well.
ACK
Pushed to master and sssd-1-11
Hi Lukas, can you also send a version of this patch that applies cleanly on sssd-1-9? It was requested by downstream.
On (31/03/14 13:49), Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 10:41:04AM +0100, Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 10:22:53AM +0100, Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 10:03:23PM +0100, Lukas Slebodnik wrote:
On (25/02/14 21:05), Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 08:05:45PM +0100, Lukas Slebodnik wrote:
updated patch is attached.
LS
I think you sent the original patch by accident.
Once again.
LS
I tested the combination the user described (id_provider=ipa && sudo_provider=ldap) and at least verified other providers (autofs) do start.
The code looks good to me as well.
ACK
Pushed to master and sssd-1-11
Hi Lukas, can you also send a version of this patch that applies cleanly on sssd-1-9? It was requested by downstream.
Sure, attached patch is for sssd-1-9 branch.
LS
On Mon, Mar 31, 2014 at 04:21:38PM +0200, Lukas Slebodnik wrote:
On (31/03/14 13:49), Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 10:41:04AM +0100, Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 10:22:53AM +0100, Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 10:03:23PM +0100, Lukas Slebodnik wrote:
On (25/02/14 21:05), Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 08:05:45PM +0100, Lukas Slebodnik wrote: > updated patch is attached. > > LS
I think you sent the original patch by accident.
Once again.
LS
I tested the combination the user described (id_provider=ipa && sudo_provider=ldap) and at least verified other providers (autofs) do start.
The code looks good to me as well.
ACK
Pushed to master and sssd-1-11
Hi Lukas, can you also send a version of this patch that applies cleanly on sssd-1-9? It was requested by downstream.
Sure, attached patch is for sssd-1-9 branch.
LS
Thank you, ACK
On Mon, Mar 31, 2014 at 05:14:24PM +0200, Jakub Hrozek wrote:
On Mon, Mar 31, 2014 at 04:21:38PM +0200, Lukas Slebodnik wrote:
On (31/03/14 13:49), Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 10:41:04AM +0100, Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 10:22:53AM +0100, Jakub Hrozek wrote:
On Tue, Feb 25, 2014 at 10:03:23PM +0100, Lukas Slebodnik wrote:
On (25/02/14 21:05), Jakub Hrozek wrote: >On Tue, Feb 25, 2014 at 08:05:45PM +0100, Lukas Slebodnik wrote: >> updated patch is attached. >> >> LS > >I think you sent the original patch by accident.
Once again.
LS
I tested the combination the user described (id_provider=ipa && sudo_provider=ldap) and at least verified other providers (autofs) do start.
The code looks good to me as well.
ACK
Pushed to master and sssd-1-11
Hi Lukas, can you also send a version of this patch that applies cleanly on sssd-1-9? It was requested by downstream.
Sure, attached patch is for sssd-1-9 branch.
LS
Thank you, ACK
Pushed to sssd-1-9: f52d80dccb56409c178aed4fb43c1ad0195d2a0a
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik