Merge lp:~psivaa/jenkins-launchpad-plugin/stop-tmp-leak into lp:jenkins-launchpad-plugin

[Para Siva]

Fix to wrap the launchpadlib leaking launchpadlib_cache directory.

To fix launchpadlib leaking launchpadlib_cachedir on failures and exceptions.
The proposed one afaik appears to be the less intrusive fix to overcome the leak.
These have been patched and tested in [1]

Note:
These directories are actually created by launchpadlib [2](in the absence of the calling functions not creating directories and passing the reference). So removing them inside jlp is not so intuitive but I guess that's all we could do to fix this issue with less changes. We may actually create the directories and pass the reference to launchpadlib to use, but I think this would be outside the scope for this story.

[1]: https://ci-jenkaas.staging.ubuntu.com/
[2]: http://bazaar.launchpad.net/~lazr-developers/launchpadlib/trunk/view/head:/src/launchpadlib/launchpad.py#L603
--

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:127
http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-ci/40/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-trusty-amd64-ci/9/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-trusty-i386-ci/5/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/jenkins-launchpad-plugin-ci/40/rebuild

review: Needs Fixing (continuous-integration)

[Francis Ginther (fginther)]

You can extend the try block up within autoland.py all the way up to the line:

    mp = launchpadutils.get_mp_handle_from_url(lp_handle,
                                               args['merge_proposal'])

The finally clause will still kick in even if the code execution leaves through the return statement [1], so you just need the single rmtree() inside the 'finally'. The jenkins ci runs are failing due to pep8 issues (line too long). Otherwise this looks good. Thanks for identifying and solving the issue.

An alternative to the try/finally approach is to use an atexit handler [2] to register the rmtree as a cleanup function.

[1] - https://docs.python.org/2/tutorial/errors.html#defining-clean-up-actions
[2] - https://docs.python.org/2/library/atexit.html

review: Needs Fixing

[lp:~psivaa/jenkins-launchpad-plugin/stop-tmp-leak]

128. By Para Siva on 2015-09-24

Use atexit to handle cleanups

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:128
http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-ci/41/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-trusty-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-trusty-i386-ci/6

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/jenkins-launchpad-plugin-ci/41/rebuild

review: Approve (continuous-integration)

[Para Siva (psivaa)]

Thanks for the atexit suggestion Francis. That proved a lot easier and lesser intrusive.

Would appreciate a re-review. Thanks

[lp:~psivaa/jenkins-launchpad-plugin/stop-tmp-leak]

129. By Para Siva on 2015-09-24

Add comments to why cleanup function

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:129
http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-ci/43/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-trusty-amd64-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-trusty-i386-ci/8

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/jenkins-launchpad-plugin-ci/43/rebuild

review: Approve (continuous-integration)

[Joe Talbott (joetalbott)]

LGTM. I wonder if we can avoid duplicating the block of code in all three scripts by adding the atexit method in __init__.py, but we don't need to block on that unless it's super easy to take care of.

review: Approve

[Francis Ginther (fginther)]

I agree that the atexit approach saves a lot or indentation changes and looks cleaner.

Just a suggestion:
I prefer to see the atexit handler right next to the code that adds it. For example:

    launchpad_cachedir = os.path.join('/tmp',
                                      str(os.getpid()),
                                      '.launchpadlib')
    atexit.register(rmtree, os.path.join('/tmp', str(os.getpid())), ignore_errors=True)

The advantage of this is that if the code is modified in the future to change the location of the launchpad_cachedir, it's a lot easier to see that there is an exit handler that needs to be updated as well.

Another approach would be to refactor launchpad_cachedir a bit and use a global variable that both the atexit handler and the original code could share. Something like:

cachedir_base = os.path.join('/tmp', str(os.getpid()))
@atexit.register
def clean_launchpadlib_cachedir():
    rmtree(cachedir_base, ignore_errors=True)

...

    launchpad_cachedir = os.path.join(cachedir_base, '.launchpadlib')

These are just style suggestoins, you may choose to use them or not.

review: Approve

[lp:~psivaa/jenkins-launchpad-plugin/stop-tmp-leak]

130. By Para Siva on 2015-09-24

Move atexit registeration closer to where it requires attention to

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:130
http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-ci/44/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-trusty-amd64-ci/13
    SUCCESS: http://jenkins.qa.ubuntu.com/job/jenkins-launchpad-plugin-trusty-i386-ci/9

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/jenkins-launchpad-plugin-ci/44/rebuild

review: Approve (continuous-integration)

[Francis Ginther (fginther)]

Looks great, thanks for all the work on this one.

review: Approve

=== modified file 'jlp/commands/autoland.py'
--- jlp/commands/autoland.py	2013-06-14 19:02:11 +0000
+++ jlp/commands/autoland.py	2015-09-24 16:09:34 +0000
@@ -1,6 +1,7 @@
 import re
 from argparse import ArgumentParser
 import argparse
+import atexit
 from jlp.launchpadutils import build_state, LaunchpadVote, get_vote_subject
 from jlp import (launchpadutils, Branch,
                  DputRunner, jenkinsutils, get_launchpad,
@@ -222,6 +223,12 @@
     launchpad_cachedir = os.path.join('/tmp',
                                       str(os.getpid()),
                                       '.launchpadlib')
+
+    # `launchpad_cachedir` is leaked upon unexpected exits
+    # adding this cleanup to stop directories filling up `/tmp/`
+    atexit.register(rmtree, os.path.join('/tmp',
+                                         str(os.getpid())),
+                    ignore_errors=True)
     lp_handle = get_launchpad(launchpadlib_dir=launchpad_cachedir)
     mp = launchpadutils.get_mp_handle_from_url(lp_handle,
                                                args['merge_proposal'])
@@ -253,5 +260,4 @@
 
     # merging starts here
     ret = merge_and_commit(mp, args)
-    rmtree(launchpad_cachedir, ignore_errors=True)
     return ret

=== modified file 'jlp/commands/getMergeProposals.py'
--- jlp/commands/getMergeProposals.py	2013-05-10 14:10:52 +0000
+++ jlp/commands/getMergeProposals.py	2015-09-24 16:09:34 +0000
@@ -1,4 +1,5 @@
 from argparse import ArgumentParser
+import atexit
 from jlp import get_launchpad, launchpadutils, logger
 import os
 from shutil import rmtree
@@ -14,13 +15,18 @@
     launchpad_cachedir = os.path.join('/tmp',
                                       str(os.getpid()),
                                       '.launchpadlib')
+
+    # `launchpad_cachedir` is leaked upon unexpected exits
+    # adding this cleanup to stop directories filling up `/tmp/`
+    atexit.register(rmtree, os.path.join('/tmp',
+                                         str(os.getpid())),
+                    ignore_errors=True)
     lp_handle = get_launchpad(launchpadlib_dir=launchpad_cachedir)
     for b in branch:
         mps = launchpadutils.get_merge_proposals(lp_handle, b, [merge_type])
         logger.debug("[%s] merge proposals: %s" % (b, mps))
         logger.debug("count: %d" % (len(mps)))
         count += len(mps)
-    rmtree(launchpad_cachedir, ignore_errors=True)
     return count
 
 

=== modified file 'jlp/commands/voteOnMergeProposal.py'
--- jlp/commands/voteOnMergeProposal.py	2013-05-10 14:10:52 +0000
+++ jlp/commands/voteOnMergeProposal.py	2015-09-24 16:09:34 +0000
@@ -1,4 +1,5 @@
 from argparse import ArgumentParser
+import atexit
 from jlp import launchpadutils, get_launchpad, logger
 import re
 import os
@@ -24,6 +25,12 @@
     launchpad_cachedir = os.path.join('/tmp',
                                       str(os.getpid()),
                                       '.launchpadlib')
+
+    # `launchpad_cachedir` is leaked upon unexpected exits
+    # adding this cleanup to stop directories filling up `/tmp/`
+    atexit.register(rmtree, os.path.join('/tmp',
+                                         str(os.getpid())),
+                    ignore_errors=True)
     lp_handle = get_launchpad(launchpadlib_dir=launchpad_cachedir)
     mp = launchpadutils.get_mp_handle_from_url(lp_handle,
                                                args['merge_proposal'])
@@ -61,5 +68,4 @@
                                      args['build_url'],
                                      reason)
 
-    rmtree(launchpad_cachedir, ignore_errors=True)
     return 0