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

Michal Židek mzidek at redhat.com
Thu Oct 29 14:52:06 UTC 2015


On 10/27/2015 04:10 PM, Nikolai Kondrashov wrote:
> Hi Michal,
>
> Thanks a lot for the detailed review and testing!
> Please see my comments below.
>
> On 10/23/2015 02:54 PM, Michal Židek wrote:
>> Hi!
>>
>> There is one new pep8 error in the code:
>> ../src/tests/intg/ldap_test.py:819:37: E126 continuation line
>> over-indented for hanging indent
>
> Sure, will fix it.
>
>> For the memcache workaroud please do this change in the
>> code:
>> diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
>> index 8263d88..eb466ab 100644
>> --- a/src/tests/intg/ldap_test.py
>> +++ b/src/tests/intg/ldap_test.py
>> @@ -194,8 +194,10 @@ def cleanup_sssd_process():
>>       subprocess.call(["sss_cache", "-E"])
>>       for path in os.listdir(config.DB_PATH):
>>           os.unlink(config.DB_PATH + "/" + path)
>> -    for path in os.listdir(config.MCACHE_PATH):
>> -        os.unlink(config.MCACHE_PATH + "/" + path)
>> +    # FIXME: Uncomment this when ticket #2726 is solved
>> +    # https://fedorahosted.org/sssd/ticket/2726
>> +    # for path in os.listdir(config.MCACHE_PATH):
>> +    #    os.unlink(config.MCACHE_PATH + "/" + path)
>
>
>>> +def format_interactive_conf(ldap_conn, schema):
>>> +    """Format an SSSD configuration with all caches refreshing in 4
>>> seconds"""
>>> +    return \
>>> +        format_basic_conf(ldap_conn, schema, enum=True) + \
>>> +        unindent("""
>>> +            [nss]
>>> +            memcache_timeout                    = 4
>>
>> It is better to set memcache timeout to zero outside tests
>> that are not dedicated to memcache. This will also probably
>> solve the membership tests failure that you saw when using the
>> workaround for the memcache tests failure.
>
> Hmm, perhaps. However, could you please explain why the membership tests
> fail?
>
> I noticed that they also fail if I simply don't run the group
> addition/removal
> tests before them, in addition to enabling the workaround. Also, they
> work if
> I put "run_shell" before each "assert" and immediately exit the spawned
> shells
> even without doing anything in them.

I really do not know why the run_shell helped. Maybe it
forced pytest to initialize new client memory cache context.
I tried to do some tests with run_shell, but they did not
work for me.

Anyway, the failures were connected to memory cache
and memory cache currently is not reliable in these
test, so I did not investigated further, why they
failed.

>
>>> +            enum_cache_timeout                  = 4
>>> +            entry_negative_timeout              = 4
>>
>> I would set negative cache timeout to zero as well,
>> see comments below.
>
> Replying below.
>
>>> +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)
>>
>> What is the purpose of this timeout? It does not seem to be
>> necessary. You use it in all the other tests as well, so
>> I guess it had some meaning (but I tried to remove them and
>> tests passed for me without problems).
>
> This puts test actions and tests in the middle of the 4 second cache
> timeouts,
> so they are more reliable and time drift doesn't affect them that much.
>
> E.g.:
>
>      0 - sssd start
>      1 -
>      2 - add user, check it's not yet present
>      3 -
>      4 - cache expiry/purging
>      5 -
>      6 - check user added
>      7 -
>      8 - cache expiry/purging
>
> etc.
>
> IIRC, I got 400+ perfect runs before the first failure occurred with this -
> better than other schemes.

Hmmm...maybe this was because SSSD was not initialized (fully
started) when the test was run. In which case it is problem
of the fixture that starts SSSD and should be solved inside
the fixture (the sleep() should be added there).

>
>>> +    # Add the user
>>> +    ent.assert_passwd(ent.contains_only())
>>> +    ldap_conn.add_s(*e)
>>> +    ent.assert_passwd(ent.contains_only())
>>
>> I would avoid testing the negative cache outside tests
>> that are dedicated to negative cache. We had this same
>> pattern in our C code tests and it timed out when CI
>> was under heavy load. We can add dedicated tests
>> for negative cache in CI later with big enough timeout
>> to pass even under heavy load CI.
>>
>> So please, remove the negative cache testing from everywhere
>> in these tests.
>
> I'm OK with minimizing the tests and not exercising some caching
> mechanisms. I understand it will make tests more reliable.
>
> However, my intention was to test the full end-user functionality, what
> actually matters to users. Users don't really care about caches, they just
> want everything work fast and reliably. They just want their LDAP changes
> propagated. For that matter I only touch cache timeouts to make tests
> run in
> reasonable time. I don't disable them, because users will probably have
> them
> enabled as well.
>
> We can test all the cache mechanisms separately, but we will still have to
> test them working together. Can we do that? Can we have these particular
> tests
> do that? Or is it too hard/impossible?
>
> If we can do that, how would you like to see them?
>
> If not, I'll just disable memory cache and remove negative cache testing as
> you request.
>
> Or do I misunderstand the idea behind this?

I understand your point, but negative cache IMO just slows
down the tests. If we disable it, we still actually use it
under the hood, it just timeouts immediately (so we trigger
the same codepath, that most users will trigger in their
environment). Users will usually not add and delete users
in such short time, so using negative cache does not get
us closer to real cases and it really is just test scenario.

Main reason why I would like to have separate tests for
negative cache (which I do not want you to add now)
is that in order to test it in CI, we need higher timeout
value for pattern:
test_if_user_exists (does not)
add_user
test_if_user_exists (does not - negative cache hit)

This is the pattern that you use and we had problems
with this pattern in our C unit test (which we solved
by increasing the timeout). In python we may need
even higher values than in C.

>
>>> +    time.sleep(4)
>>
>> Instead of the number 4, could you use some global constant. You
>> could use the constant to generate the "interactive" config
>> file as well. If we later see some timeout issues related to
>> this, we can change it on one place.
>
> Yes, good idea, thank you.
>
>> Also I think that this
>> particular timeout will not be necessary if we stop testing the
>> negative cache, which will speed up the tests.
>
> OK, if we decide to disable negative tests,
> then I'll see if I can speed it up.
>
>>> +            conf = \
>>> +                format_basic_conf(ldap_conn, SCHEMA_RFC2307,
>>> enum=True) + \
>>> +                unindent("""
>>> +                    [nss]
>>> +                    filter_users            = {filter_users_str}
>>> +                    filter_users_in_groups  = {filter_users_in_groups}
>>
>> 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.
>
> 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.
>
> Do you have any particular design for this in mind?

No, I dont. I think copy-pasting is Ok in this case. The
copy-pasted functions will not be big and all on one
place.

>
>>> +        conf = \
>>> +            format_basic_conf(ldap_conn, SCHEMA_RFC2307, enum=True) + \
>>> +            unindent("""
>>> +                [nss]
>>> +                filter_groups   = {filter_groups_str}
>>
>> Same as above, please do not generate the conf file inside the test, but
>> split it to multiple tests.
>
> Please see my comment right above.
>
>>> +        conf = \
>>> +            format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS,
>>> enum=True) + \
>>> +            unindent("""
>>> +                [nss]
>>> +                filter_groups   = {filter_groups_str}
>>
>> The same thing as above.
>>
>
> Please see my comment above.
>
>>> Add a memcache invalidation failure integration test. It fails as is,
>>> and passes if ldap_enumeration_refresh_timeout in the first half is set
>>> to e.g. 30. The sss_nss_check_header function was observed to always
>>> return 0 for the second half of the test, when it was failing.
>>
>> This test passes for me with the memcache workaround. I guess we do
>> not need it then. The reproducer for the failure can be achieved
>> by removing the workaround (which we will do once we fix the
>> root cause).
>
> Do you mean we should not merge it, remove it from the patchset? Or add it
> along with a workaround, so after we fix the failure and remove the
> workaround
> it stays on the lookout?

I wanted to remove the test completely, but maybe it can
serve as regression test. Can you add the patch as attachment
to the ticket #2726? We can decide later if we want to
use it or not, still I would like to push it out of this
thread.

>
>> Just for info, this is git diff with
>> changes I made when testing your patches:
>
> I see, thank you!
>
> Sincerely,
> Nick



More information about the sssd-devel mailing list