[PATCH] selftest: close connections after tests in samba4.ldap.acl.python

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
Hi all,

I'm taking a look at the memory usage of the testsuite.
samba4.ldap.acl.python is currently all that puts peak usage over the
8GB line -- A patch to fix is attached (with more likely to follow for
other tests).

Review appreciated.

- Jamie McClymont

samba-close-connections-ldap-acl-python.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
On Mon, 2018-01-08 at 13:38 +1300, Jamie McClymont via samba-technical
wrote:
> Hi all,
>
> I'm taking a look at the memory usage of the testsuite.
> samba4.ldap.acl.python is currently all that puts peak usage over the
> 8GB line -- A patch to fix is attached (with more likely to follow for
> other tests).

This looks great.  If you could see if we can skip the gc.collect()
stuff that would be great, and pick up the next few so we can get the
build back under 8GB.  

(In the long term we really need to get this under 2GB to fit into free
runners on gitlab, but 8GB will still be much less expensive).

Thanks!

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
Patch with gc removed (confirmed not to impact), plus fix for secdesc
attached.

Thanks

On 08/01/18 14:00, Andrew Bartlett wrote:

> On Mon, 2018-01-08 at 13:38 +1300, Jamie McClymont via samba-technical
> wrote:
>> Hi all,
>>
>> I'm taking a look at the memory usage of the testsuite.
>> samba4.ldap.acl.python is currently all that puts peak usage over the
>> 8GB line -- A patch to fix is attached (with more likely to follow for
>> other tests).
>
> This looks great.  If you could see if we can skip the gc.collect()
> stuff that would be great, and pick up the next few so we can get the
> build back under 8GB.  
>
> (In the long term we really need to get this under 2GB to fit into free
> runners on gitlab, but 8GB will still be much less expensive).
>
> Thanks!
>
> Andrew Bartlett
>

samba-close-connections-ldap-acl-and-secdesc-python.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
(now with missing signoff!)

On 08/01/18 14:08, Jamie McClymont wrote:

> Patch with gc removed (confirmed not to impact), plus fix for secdesc
> attached.
>
> Thanks
>
> On 08/01/18 14:00, Andrew Bartlett wrote:
>> On Mon, 2018-01-08 at 13:38 +1300, Jamie McClymont via samba-technical
>> wrote:
>>> Hi all,
>>>
>>> I'm taking a look at the memory usage of the testsuite.
>>> samba4.ldap.acl.python is currently all that puts peak usage over the
>>> 8GB line -- A patch to fix is attached (with more likely to follow for
>>> other tests).
>>
>> This looks great.  If you could see if we can skip the gc.collect()
>> stuff that would be great, and pick up the next few so we can get the
>> build back under 8GB.  
>>
>> (In the long term we really need to get this under 2GB to fit into free
>> runners on gitlab, but 8GB will still be much less expensive).
>>
>> Thanks!
>>
>> Andrew Bartlett
>>

samba-close-connections-ldap-acl-and-secdesc-python-signed-off.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
On Mon, 2018-01-08 at 14:13 +1300, Jamie McClymont wrote:
> (now with missing signoff!)

Thanks.  I've pushed that to autobuild.  Reducing our memory use can
make a massive difference to the size VM we need to do our testing on,
and the amount of memory used on sn-devel, so this is really helpful.

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Monday, 8 January 2018 02:13:18 CET Jamie McClymont via samba-technical
wrote:
> (now with missing signoff!)

Thanks for those patches. But normally you have setup and teardown functions
which should be responsible for allocating and cleaning up memory, creating
and deleting users etc. Those setup functions can then be shared in different
tests.

A lot of python tests don't seem to follow basic unit testing rules. So I
think just correctly working with setup and teardown would fix a lot more bugs
we have in those tests.

Just my 2 cents.


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
On Mon, 2018-01-08 at 09:47 +0100, Andreas Schneider wrote:

> On Monday, 8 January 2018 02:13:18 CET Jamie McClymont via samba-technical
> wrote:
> > (now with missing signoff!)
>
> Thanks for those patches. But normally you have setup and teardown functions
> which should be responsible for allocating and cleaning up memory, creating
> and deleting users etc. Those setup functions can then be shared in different
> tests.
>
> A lot of python tests don't seem to follow basic unit testing rules. So I
> think just correctly working with setup and teardown would fix a lot more bugs
> we have in those tests.
>
> Just my 2 cents.

Jamie can write more in the morning, but to clarify without waiting a
day, here are the docs for addCleanup (which we implement even on
python 2.6):

https://docs.python.org/2/library/unittest.html#unittest.TestCase.addCleanup

That is, even if this wasn't a complex test using a tricky balance of
class inheritance on top of the password_lockout test addCleanup would
still be a good way to incrementally clean up things created during
setUp.

We should use it more, in my view, it would save a lot of the 'delete
the user in case the last run faulted' code.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 08/01/18 21:47, Andreas Schneider via samba-technical wrote:
> normally you have setup and teardown functions which should be
responsible for allocating and cleaning up memory

Hi Andreas

On top of what Andrew said, the particular reasoning here was that
super's teardown needs access to the admin user to delete things -- the
way in which cleanups are run in reverse order is very convenient here,
and allows cleanups to be added alongside where the resource is created
(so that someone adding more connections would remember to add the cleanup).

Thanks
- Jamie

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] selftest: close connections after tests in samba4.ldap.acl.python

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Something like this:

--- a/python/samba/tests/__init__.py
+++ b/python/samba/tests/__init__.py
@@ -52,8 +52,14 @@ HEXDUMP_FILTER=''.join([(len(repr(chr(x)))==3) and chr(x) or '.' for x in range(
 class TestCase(unittest.TestCase):
     """A Samba test case."""
 
+    def clean_up_dangling_ldbs(self):
+        for k, v in self.__dict__.items():
+            if isinstance(v, samba.Ldb):
+                del self.__dict__[k]
+
     def setUp(self):
         super(TestCase, self).setUp()
+        self.addCleanup(self.clean_up_dangling_ldbs)
         test_debug_level = os.getenv("TEST_DEBUG_LEVEL")
         if test_debug_level is not None:
             test_debug_level = int(test_debug_level)


would ensure that all ldbs get tidied up just after the last moment
they could be of any possible use (rather than at subprocess death).

While it doesn't ensure the ldbs get deleted in a particular order
*within* the test (as Jamie's patch does), it does mean they are
deleted in a deterministic order *across* tests, rather than in the
higgledy-piggledy hash order they are now.

I don't know if this different determinism is useful or how much
memory is saved. Unfortunately the only evidence I have after two runs
is that it seems to make the whole thing slightly slower.

Douglas