Merge lp:~michael.nelson/launchpad/982938-dont-load-all-tokens-for-all-ppas-into-memory into lp:launchpad

[Michael Nelson]

Don't load all tokens for all affected apps into memory when updating htpasswd.

Summary
=======
This branch aims to reduce the load/memory consumption on germanium while generating htpasswd files.

Proposed fix
============

Use a generator to return a storm result set of tokens for each PPA, rather than
loading all tokens for all PPAs into a dict.

It will result in a larger query count. jml is working on a script that will generate data to be able to verify whether this branch does indeed help.

Pre-implementation notes
========================
Referring to https://bugs.launchpad.net/launchpad/+bug/982938/comments/6
12:27 < noodles> jml: if you think the first of those points (in my comment) is very likely to be causing performance issues, I could start coding that straight away...
12:27 < noodles> (given the numbers achuni mentioned above, it would seem likely, but only the measurements will tell).
12:27 < jml> as you say, only measurements will tell :)
12:28 < jml> noodles: I reckon code it up straight away. :)

Implementation details
======================
Sheesh - this is the quick cover letter? :P

LOC Rationale
=============
Yay - 12 lines of credit.

Tests
=====
./bin/test -vvct TestPPAHtaccessTokenGeneration
./bin/test -vvct TestHtpasswdGeneration

Demo and Q/A
============
If this script runs on staging, we could QA by comparing output before and after running it.

Lint
====
Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py

[Aaron Bentley (abentley)]

review: Approve

[Aaron Bentley (abentley)]

A nice simplification!

review: Approve

=== modified file 'lib/lp/archivepublisher/htaccess.py'
--- lib/lp/archivepublisher/htaccess.py	2011-12-19 23:38:16 +0000
+++ lib/lp/archivepublisher/htaccess.py	2012-05-30 17:10:23 +0000
@@ -23,6 +23,7 @@
 from zope.component import getUtility
 
 from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthTokenSet
+from lp.soyuz.model.archiveauthtoken import ArchiveAuthToken
 
 
 HTACCESS_TEMPLATE = """
@@ -79,13 +80,14 @@
     assert archive.private, "Archive %r must be private" % archive
 
     if tokens is None:
-        tokens = getUtility(IArchiveAuthTokenSet).getByArchive(archive)
+        tokens = getUtility(IArchiveAuthTokenSet).getByArchive(
+            archive).order_by(ArchiveAuthToken.id)
 
     # The first .htpasswd entry is the buildd_secret.
     yield (BUILDD_USER_NAME, archive.buildd_secret, BUILDD_USER_NAME[:2])
 
     # Iterate over tokens and write the appropriate htpasswd
-    # entries for them.  Use a consistent sort order so that the
-    # generated file can be compared to an existing one later.
-    for token in sorted(tokens, key=attrgetter("id")):
+    # entries for them.  For consistent sort order, the tokens
+    # should be ordered by id.
+    for token in tokens:
         yield (token.person.name, token.token, token.person.name[:2])

=== modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2012-05-10 21:44:21 +0000
+++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2012-05-30 17:10:23 +0000
@@ -79,11 +79,10 @@
             write_htaccess(htaccess_filename, pub_config.htaccessroot)
             self.logger.debug("Created .htaccess for %s" % ppa.displayname)
 
-    def generateHtpasswd(self, ppa, tokens):
+    def generateHtpasswd(self, ppa):
         """Generate a htpasswd file for `ppa`s `tokens`.
 
         :param ppa: The context PPA (an `IArchive`).
-        :param tokens: A iterable containing `IArchiveAuthToken`s.
         :return: The filename of the htpasswd file that was generated.
         """
         # Create a temporary file that will be a new .htpasswd.
@@ -94,7 +93,7 @@
         os.close(fd)
 
         write_htpasswd(
-            temp_filename, htpasswd_credentials_for_archive(ppa, tokens))
+            temp_filename, htpasswd_credentials_for_archive(ppa))
 
         return temp_filename
 
@@ -248,23 +247,6 @@
 
         return new_ppa_tokens
 
-    def _getValidTokensForPPAs(self, ppas):
-        """Returns a dict keyed by archive with all the tokens for each."""
-        affected_ppas_with_tokens = dict([
-            (ppa, []) for ppa in ppas])
-
-        store = IStore(ArchiveAuthToken)
-        affected_ppa_tokens = store.find(
-            ArchiveAuthToken,
-            ArchiveAuthToken.date_deactivated == None,
-            ArchiveAuthToken.archive_id.is_in(
-                [ppa.id for ppa in ppas]))
-
-        for token in affected_ppa_tokens:
-            affected_ppas_with_tokens[token.archive].append(token)
-
-        return affected_ppas_with_tokens
-
     def getNewPrivatePPAs(self):
         """Return the recently created private PPAs."""
         # Avoid circular import.
@@ -296,12 +278,7 @@
 
         affected_ppas.update(self.getNewPrivatePPAs())
 
-        affected_ppas_with_tokens = {}
-        if affected_ppas:
-            affected_ppas_with_tokens = self._getValidTokensForPPAs(
-                affected_ppas)
-
-        for ppa, valid_tokens in affected_ppas_with_tokens.iteritems():
+        for ppa in affected_ppas:
             # If this PPA is blacklisted, do not touch it's htaccess/pwd
             # files.
             blacklisted_ppa_names_for_owner = self.blacklist.get(
@@ -322,7 +299,7 @@
                 continue
 
             self.ensureHtaccess(ppa)
-            temp_htpasswd = self.generateHtpasswd(ppa, valid_tokens)
+            temp_htpasswd = self.generateHtpasswd(ppa)
             if not self.replaceUpdatedHtpasswd(ppa, temp_htpasswd):
                 os.remove(temp_htpasswd)
 

=== modified file 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2012-05-10 21:44:21 +0000
+++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2012-05-30 17:10:23 +0000
@@ -126,7 +126,7 @@
 
         # Generate the passwd file.
         script = self.getScript()
-        filename = script.generateHtpasswd(self.ppa, tokens)
+        filename = script.generateHtpasswd(self.ppa)
 
         # It should be a temp file in the same directory as the intended
         # target file when it's renamed, so that os.rename() won't

=== modified file 'lib/lp/archivepublisher/tests/test_htaccess.py'
--- lib/lp/archivepublisher/tests/test_htaccess.py	2012-01-01 02:58:52 +0000
+++ lib/lp/archivepublisher/tests/test_htaccess.py	2012-05-30 17:10:23 +0000
@@ -103,15 +103,13 @@
     def test_credentials_for_archive(self):
         # ArchiveAuthTokens for an archive are returned by
         # credentials_for_archive.
-        # Make some subscriptions and tokens.
         self.ppa.buildd_secret = "geheim"
         name12 = getUtility(IPersonSet).getByName("name12")
         name16 = getUtility(IPersonSet).getByName("name16")
         self.ppa.newSubscription(name12, self.ppa.owner)
         self.ppa.newSubscription(name16, self.ppa.owner)
-        tokens = []
-        tokens.append(self.ppa.newAuthToken(name12))
-        tokens.append(self.ppa.newAuthToken(name16))
+        first_created_token = self.ppa.newAuthToken(name16)
+        tokens = [self.ppa.newAuthToken(name12), first_created_token]
 
         credentials = list(htpasswd_credentials_for_archive(self.ppa, tokens))