Merge lp:~allenap/maas/no-requests-for-simplestreams into lp:~maas-committers/maas/trunk

[Gavin Panella]

Experimentally force simplestreams to use `urllib2` instead of `requests`.

The latter appears to be allowing simplestreams to leak file-descriptors; streaming requests that are not explicitly closed remain referenced by a connection pool and thus not garbage-collected.

[Raphaël Badin (rvb)]

Looks good, but let's get proof that this solves the problem we're seeing before landing this…

[Ricardo Bánffy (rbanffy)]

Seems OK.

[Gavin Panella (allenap)]

This is meant first as a cowboy-patch for OIL to see if it helps. If it does help I'll probably pursue a proper fix in simplestreams.

Thanks for the reviews :)

[Blake Rouse (blake-rouse)]

So I was able to test this branch. I setup the cluster to sync from the region every 10 seconds. With it syncing every 10 seconds with this change there where no CLOSE_WAIT sockets.

sudo lsof | grep maas | grep CLOSE_WAIT | wc -l
0

With this change disabled, so simplestreams using python-requests I got many CLOSE_WAITS

sudo lsof | grep maas | grep CLOSE_WAIT | wc -l
88
sudo lsof | grep maas | grep CLOSE_WAIT | wc -l
104

And it keeps growing. This fixes the problem lets land this now. So 1.8 is stable, without the cluster will need to keep being restarted. At some point people will reach a limit no matter the ulimit.

review: Approve

[Scott Moser (smoser)]

python-requests uses urllib3 which does connection pooling.
it'd seem easy enough to reproduce this outside of maas.
just look at sstream-query . you could run that in a loop, and we should see similar leakage.

[Ricardo Bánffy (rbanffy)]

I was left wondering if we should adopt a convention for naming "cowboy patches" (if that's the term we'll use) so that we don't confuse them with long-term solutions.

I've been using "bug-NNNNN-something-meaningful" as my personal sanity-keeping memory aid, but the "bug-" part could be morphed into something more interesting.

[Gavin Panella (allenap)]

> python-requests uses urllib3 which does connection pooling. it'd seem
> easy enough to reproduce this outside of maas. just look at
> sstream-query . you could run that in a loop, and we should see
> similar leakage.

Thanks Scott. I've tried this:

  $ python -i /usr/bin/sstream-query \
  > --keyring=/usr/share/keyrings/ubuntu-cloudimage-keyring.gpg \
  > http://maas.ubuntu.com/images/ephemeral-v2/daily/
  ...
  >>> import time
  >>> while True:
  ... main()
  ... time.sleep(0.1)

but I wasn't able to accumulate connections in CLOSE_WAIT. Could there
be a code path taken (or not) that might mean we're missing something?
I've found five code paths through contentsource.py that /might/ leak (I
have put together a patch to fix them, but it's not tested:
http://paste.ubuntu.com/11648573/).

[Gavin Panella (allenap)]

> I was left wondering if we should adopt a convention for naming
> "cowboy patches" (if that's the term we'll use) so that we don't
> confuse them with long-term solutions.
>
> I've been using "bug-NNNNN-something-meaningful" as my personal
> sanity-keeping memory aid, but the "bug-" part could be morphed into
> something more interesting.

Raphaël moved this into a module called provisioningserver.monkey today.

=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py	2015-05-18 20:17:28 +0000
+++ src/provisioningserver/plugin.py	2015-06-05 15:01:24 +0000
@@ -20,6 +20,7 @@
 import os
 import socket
 from socket import error as socket_error
+import sys
 
 from provisioningserver.utils.debug import (
     register_sigusr2_thread_dump_handler,
@@ -45,6 +46,27 @@
 from zope.interface import implementer
 
 
+def force_simplestreams_to_use_urllib2():
+    """Monkey-patch `simplestreams` to use `urllib2`.
+
+    This prevents the use of `requests` which /may/ be helping simplestreams
+    to lose file-descriptors.
+    """
+    import simplestreams.contentsource
+
+    if sys.version_info > (3, 0):
+        import urllib.request as urllib_request
+        import urllib.error as urllib_error
+    else:
+        import urllib2 as urllib_request
+        urllib_error = urllib_request
+
+    vars(simplestreams.contentsource).update(
+        URL_READER=simplestreams.contentsource.Urllib2UrlReader,
+        URL_READER_CLASSNAME="Urllib2UrlReader", urllib_error=urllib_error,
+        urllib_request=urllib_request)
+
+
 @implementer(ICredentialsChecker)
 class SingleUsernamePasswordChecker:
     """An `ICredentialsChecker` for a single username and password."""
@@ -231,6 +253,7 @@
     def makeService(self, options):
         """Construct the MAAS Cluster service."""
         register_sigusr2_thread_dump_handler()
+        force_simplestreams_to_use_urllib2()
 
         from provisioningserver import services
         from provisioningserver.config import Config