[SSSD] [PATCH v2] intg: Add more LDAP tests

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Thu Nov 5 14:27:19 UTC 2015


On 11/04/2015 05:01 PM, Nikolai Kondrashov wrote:
> On 11/04/2015 04:25 PM, Michal Židek wrote:
>> On 11/03/2015 06:30 PM, Nikolai Kondrashov wrote:
>>> Now, I think I got confused here. I'm not actually testing negative
>>> cache, but
>>> only the fact that the changes to LDAP don't appear before the caches
>>> expire.
>>> Since the negative cache timeout equals the timeout of positive caches,
>>> there
>>> are really no negative cache-specific tests.
>>>
>>> That said, I can still remove the tests you mention in your patch, if you
>>> think we should remove them. However, just for the record, I didn't see any
>>> issues with them.
>>>
>>> Another thing is, I couldn't make the tests work with those sleep(4)'s
>>> removed
>>> as you did in your patch up-thread. In fact all *add_remove* tests failed
>>> except the add_remove_user_memcache_mix (delay results are not checked
>>> there).
>>>
>>> The problem being, the assertions following these delays check
>>> enumeration and
>>> enumeration timeout is still 4 seconds, so we need to wait for it. For that
>>> matter removing tests you mention doesn't have an impact on execution time.
>>
>> I see where the confusion comes from now. But the enumeration should not
>> play a role here. You can think about enumeration as forced sysdb fill
>> operation, but it is not necessary in order to grab user from LDAP.
>> If the user is not found in the sysdb (and also not found in the
>> negcache) SSSD grabs the user from LDAP even before the next
>> enumeration.
>
> Sure. However, ent.assert_passwd does pwd.getpwall, not only pwd.getpwnam and
> pwd.getpwuid. Pwd.getpwall result check is the one that fails.
> Ent.assert_group behaves similarly.
>
> I think I remember Jakub asking to have this test without enumeration as well,
> and I can do that, but I think enumeration test is also valuable.
>
>>> I'm not quite sure how you made it work. Was some other change omitted from
>>> your patch accidentally? I'm talking about changes like this:
>>
>> I do not think I omitted something. Did you really set
>> the negative_cache to 0 in sssd.conf?
>
> Yes. I tried the exact patch you attached.
>
>>>
>>>   def create_sssd_cleanup(request):
>>> @@ -462,12 +464,9 @@ def user_and_groups_rfc2307_bis(request, ldap_conn):
>>>   def test_add_remove_user(ldap_conn, blank_rfc2307):
>>>       """Test user addition and removal are reflected by SSSD"""
>>>       e = ldap_ent.user(ldap_conn.ds_inst.base_dn, "user", 1001, 2000)
>>> -    time.sleep(2)
>>>       # Add the user
>>>       ent.assert_passwd(ent.contains_only())
>>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Here you fill the negative cache.
>>
>>>       ldap_conn.add_s(*e)
>>> -    ent.assert_passwd(ent.contains_only())
>>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Here you are testing the negative cache. If there was no negcache
>> hit, SSSD would grab the user from LDAP.
>
> Ah, yes, it seems this should fetch from the negative cache.
>
>>> -    time.sleep(4)
>>         ^^^^^^^^^^^^^
>> So this sleep should not be necessary if the negcache is disabled.
>>
>>>       ent.assert_passwd(ent.contains_only(dict(name="user", uid=1001)))
>>>       # Remove the user
>>>       ldap_conn.delete_s(e[0])
>>>
>>> All in all, setting memcache_timeout to zero and commenting-out memcache
>>> file
>>> removal was enough to make *all* tests work fine.
>>>
>>
>> That is good! If the patches do not work for you without the sleep()s
>> you can send them with the sleep()s for now and I will take a look.

Please find attached patches with what I have working right now. The remaining
work includes:

* Put the 4s timeout into a global variable and use it instead of all the
   literal timeouts throughout the code. Instead of 2s timeouts use that
   variable divided by 2 - that's its meaning.

* Attempt refactoring configuration generation inside loops within tests into
   separate tests with separate fixtures. That concerns the following
   functions:

     test_filter_users
     test_filter_groups_rfc2307
     test_filter_groups_rfc2307_bis

   Michal's comment was:

     Instead of generating the conf files in nested for loops, it would be
     better to split the test to multiple smaller tests where each has
     different config file fixture (instad of void_conf and void_sssd).
     This may seem like code duplication, but I believe it will result in
     better readable code.

   My response was:

     I would really like to do that, but the problem is we will either need to
     copy-paste a ton of tests and fixtures, or use py.test's convoluted means
     of sharing parameters between fixtures and tests which will produce much
     more complicated code that this. At least this is my impression after
     researching that.

   We can try doing that and seeing how it works.

* Consider avoiding any assertions hitting negative cache, and disabling
   negative cache to possibly increase reliability - Michal's suggestion.
   Michal, please correct me if I cite it wrong or misunderstood it.

* Check the patch with pep8 and resolve any found issues.

* After the tests are merged, rebase the "intg: Add a memcache invalidation
   failure test" patch on top of it and attach it to ticket #2726 as the
   possible regression tracker.

Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-intg-Add-more-LDAP-tests.patch
Type: text/x-patch
Size: 22479 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/caea9aab/attachment-0001.bin>


More information about the sssd-devel mailing list