Merge lp:~adeuring/charmworld/1180118-check-if-charm-is-deleted into lp:~juju-jitsu/charmworld/trunk

[Abel Deuring]

jobs.lp: queue also charms that do no longer have a Launchpad branch.

This branch fixes the "deleted branches" part of bug 1180118:
Charmworld does not know about deleted/merged charms

It adds a new attribute "branch_deleted" to the charm data; those parts
of the ingest job are skipped that require access to a charm's files.

The queue/lp job now also uses the set of all charms stored in
the Mongo DB as a second source of charms that will be checked
by the ingest job.

This second source implemented in the new function jobs.lp.db_charms()

The sequences db_charms() and available_charms() are merged by a
new function job.utils.merge_sequences(a, b). This function compares two
elements from each source; if they are considered to be equal,
the value from sequence a is returned, otherwise the "smaller" value
is returned.

The sequences returned by db_charms() and available_charms() are ordered
by branch_spec, hence cmp(x['branch_spec'], y['branch_spec']) is used
as the comparison function in merge_sequences.

Since the data from db_charms() is only used when available_charms()
does not have a corresponding element, db_charms() can already set
the values charm['promulgated'], charm['branch_deleted'] and
charm['distro_series'].

Implementation note for merge_sequences(): I played at first with
try: ... except StopIteration: ... in this function but I found it
hard to find a half way readable implementation that works correctly
in the "elif compared == 0" part when one or both of the "source
sequences" run out of elements. Appending a "marker element" to these
sequences made the implementaion mch easier to read.

[Aaron Bentley (abentley)]

This code needs further work. The merge_sequences work is supposed to prevent memory exhaustion, but the possibility of memory exhaustion is vanishingly remote. The size of the charm dicts is measured in kilobytes, but the memory size of current machines is measured in gigabytes. We would need hundreds of millions of charms to run the risk of memory exhaustion. And even if this code prevented memory exhaustion from the db lookups, it doesn't address the memory impact of the LP lookup in get_branch_tips. So even with merge_sequences, the memory use would be half, which is the same order of magnitude.

This is premature optimization. If we actually start to encounter memory issues, we should solve it then. I expect the branch lookups are one of the last places that will give us issues. And we will likely have changed that code before we experience such issues, anyhow.

We don't have a LOC limit in this project, and I don't think we should, but I don't think the 90+ lines required by merge_sequences are worth the maintenance burden.

Here is a patch that provides the obvious implementation: http://pastebin.ubuntu.com/5742373/

Please apply the patch or take a similar approach.

Note that the patch uses basic data structures like dict and list. That kind of code is always easier to maintain than code that uses project-specific functions.

There are also bugs in the handling of the all_charms limit and default parameters. The import_filter is ignored entirely, and import_filter=default is supplied to available_charms.

The limit is applied only to the LP data, not the db data. This means that too many results will be returned (assuming the number of charms in the db exceeds limit), and that any results excluded from the available_charms output because of the limit that are present in the DB will incorrectly be assigned branch_deleted = True.

Simply applying a limit to both sets is not going to work, because discrepancies between the db and LP's branch listings will cause a branch to be excluded from the LP data based on the limit, while include in the DB data based on the limit. For example:

LP: ['a', 'b', 'c']
DB: ['a', 'c']

With a limit of 2, the LP set will be ['a', 'b'] and DB set will be ['a', 'c'], falsely implying that 'c' was deleted from the DB.

I think the simplest option is to apply the limit after merging LP and DB data.

It's arguable that ProofIngestJob should not abort if the branch is deleted, since we would want the proof data to be updated as our proofer is updated. Of course, it would need to abort if the branch_dir does not exist. (Yes, once the number of worker nodes increases, relying on the branch cache means that you may randomly get different results depending on the worker.)

review: Needs Fixing

[Aaron Bentley (abentley)]

Also, please put docstrings on all public functions.

[Abel Deuring (adeuring)]

The two sequences available_charms() and db_charms() are now merged via dict.update(); CHARM_IMPORT_FILTER and limit are now used in all_charms().

But I am not sure if we should try to proof charm without LP branches: As you noted, this can lead to random behaviour when we have more than one worker.

Alternatively, we could pull the charm data from the main store when no LP data is available. But I am not sure if we care enough about this sort of "half-dead"/abandoned charms. (We should discuss though if it makes sense to pull the charm data from the store for _all_ charms and use that instead of the LP branches as our main source. This might avoid a few other possible inconsistencies.)

[Aaron Bentley (abentley)]

This looks good, except that I believe _merge_iter is unused and untested code. Please consider removing it.

I am find with disabling proofing for now-- I just thought it was worth mentioning.

review: Approve

[Charmworld Lander (charmworld-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Abel Deuring (adeuring)]

review: Approve

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-06-05 18:33:46 +0000
+++ charmworld/jobs/ingest.py	2013-06-10 14:27:33 +0000
@@ -156,6 +156,8 @@
 
     def run(self, charm_data, retry=True):
         """Fetch a branch from bzr, and augment charm data."""
+        if charm_data['branch_deleted']:
+            return
         branch_dir = os.path.abspath(
             str(os.path.join(self.root_dir,
                              charm_data["series"],
@@ -287,6 +289,8 @@
         return d
 
     def run(self, charm_data):
+        if charm_data['branch_deleted']:
+            return
         branch_dir = charm_data["branch_dir"]
         charm_data.update(self.get_changes(branch_dir))
 
@@ -482,6 +486,8 @@
         if not self.proofer:
             self.log.exception("proof aborted.")
             raise Exception("No proofer")
+        if charm['branch_deleted']:
+            return
         proof = {}
         lint, exit_code = self.proofer(charm['branch_dir'])
         for line in lint:

=== modified file 'charmworld/jobs/lp.py'
--- charmworld/jobs/lp.py	2013-06-05 15:12:51 +0000
+++ charmworld/jobs/lp.py	2013-06-10 14:27:33 +0000
@@ -4,6 +4,7 @@
 
 import argparse
 import logging
+import pymongo
 
 from charmworld.lp import get_branch_tips
 from charmworld.models import getconnection
@@ -14,24 +15,21 @@
 )
 from config import CHARM_IMPORT_FILTER
 from config import CHARM_QUEUE
+from utils import get_queue
 from utils import lock
 from utils import LockHeld
-from utils import get_queue
 from utils import parse_branch
 
 
 default = object()
 
 
-def available_charms(charm_data=None, limit=None, import_filter=default):
-    if import_filter is default:
-        import_filter = CHARM_IMPORT_FILTER
+def available_charms(charm_data=None):
+    # Return data needed for the ingest job for charm branches on Launchpad.
     log = logging.getLogger("charm.launchpad")
     if not charm_data:
         charm_data = get_branch_tips()
 
-    count = 0
-
     for repo, commit, series in charm_data:
         _, branch_name = repo.rsplit("/", 1)
 
@@ -41,32 +39,60 @@
             log.warning("Unable to parse repo %s", repo)
             continue
 
-        if import_filter and not data['branch_spec'].startswith(
-                import_filter):
-            continue
-
         if data["bname"] not in ("trunk", "trunk-1"):
             log.debug("Skipped branch %s", repo)
             continue
 
-        # XXX Abel Deuring 2013-05-24 bug=1180118
-        # This breaks if a promulgated branch is unpromulgated and
-        # at the same time deleted from Launchpad.
         data['promulgated'] = data['series'] in data['distro_series']
+        data['branch_deleted'] = False
 
-        log.info("Queueing %s", data['branch_spec'])
         yield data
 
+
+def db_charms(db):
+    # Return data needed for the ingest job for charms stored in
+    # charmworld's database.
+    for charm in db.charms.find(
+            fields=('owner', 'series', 'name', 'bname', 'branch_spec'),
+            sort=[('branch_spec', pymongo.ASCENDING)]):
+        # Override the promulgation related settings: Data from this
+        # iterator is used by all_charms() only when the charm has no
+        # longer a related Launchpad branch. A charm is promulgated if
+        # its LP branch is linked to a sourcepackage, hence a charm
+        # without a branch cannot be promulgated.
+        charm['promulgated'] = False
+        charm['distro_series'] = []
+        charm['branch_deleted'] = True
+        yield charm
+
+
+def all_charms(db, charm_data=None, limit=None, import_filter=default):
+    # Return a sequence of charms that have a Launchpad branch or that are
+    # already known in charmworld's database.
+    log = logging.getLogger("charm.launchpad")
+    if import_filter is default:
+        import_filter = CHARM_IMPORT_FILTER
+    all = dict((charm['branch_spec'], charm) for charm in db_charms(db))
+    available = available_charms(charm_data)
+    all.update((charm['branch_spec'], charm) for charm in available)
+    all = sorted(all.values(), key=lambda charm: charm['branch_spec'])
+
+    count = 0
+    for charm in all:
+        if import_filter and not charm['branch_spec'].startswith(
+                import_filter):
+            continue
+        log.info("Queueing %s", charm['branch_spec'])
+        yield charm
         count += 1
-
         if limit and count >= limit:
             log.info("Import limit reached (%d)... stopping", limit)
             break
 
 
-def queue_charms(out_queue, limit=None, import_filter=default):
+def queue_charms(db, out_queue, limit=None, import_filter=default):
     log = logging.getLogger("charm.launchpad")
-    for charm in available_charms(limit=limit, import_filter=import_filter):
+    for charm in all_charms(db, limit=limit, import_filter=import_filter):
         added = out_queue.put(charm)
         if added and 0:
             log.info("Queued %s", charm)

=== modified file 'charmworld/jobs/tests/test_bzr.py'
--- charmworld/jobs/tests/test_bzr.py	2013-06-05 18:30:47 +0000
+++ charmworld/jobs/tests/test_bzr.py	2013-06-10 14:27:33 +0000
@@ -104,6 +104,30 @@
         self.assertIn('files', charm_data)
         self.assertNotIn('icon', charm_data)
 
+    def test_deleted_branch(self):
+        # If the Launchpad branch of a charm is deleted, BzrJob does
+        # not do anything.
+        job = BzrIngestJob()
+        job.setup(root_dir=self.test_dir, db=self.db)
+        ignore, charm = factory.makeCharm(
+            self.db, branch_root=self.test_dir, branch_deleted=True)
+        charm_path = os.path.abspath(os.path.join(
+            self.test_dir,
+            charm['series'],
+            charm['owner'],
+            charm['name'],
+            charm['bname']))
+        with patch.object(job, 'checkout_charm') as checkout_mock:
+            with patch.object(job, 'charm_is_current') as is_current_mock:
+                with patch.object(job, 'add_files') as add_files_mock:
+                    with patch.object(job, 'update_charm') as update_mock:
+                        job.run(charm)
+        self.assertFalse(os.path.exists(charm_path))
+        self.assertFalse(checkout_mock.called)
+        self.assertFalse(is_current_mock.called)
+        self.assertFalse(add_files_mock.called)
+        self.assertFalse(update_mock.called)
+
 
 @contextmanager
 def bzr_isolation():
@@ -247,3 +271,12 @@
         self.assertEqual([], charm_data['changes'])
         self.assertIs(None, charm_data['first_change'])
         self.assertIs(None, charm_data['last_change'])
+
+    def test_branch_deleted(self):
+        # ChangelogIngestJob does not do anything if the Launchpad branch
+        # of a charm is deleted.
+        job = ChangelogIngestJob()
+        charm = factory.get_charm_json(branch_deleted=True)
+        with patch.object(job, 'get_changes') as mock:
+            job.run(charm)
+        self.assertFalse(mock.called)

=== modified file 'charmworld/jobs/tests/test_ingest.py'
--- charmworld/jobs/tests/test_ingest.py	2013-05-29 20:43:01 +0000
+++ charmworld/jobs/tests/test_ingest.py	2013-06-10 14:27:33 +0000
@@ -108,6 +108,7 @@
             'name': charm_data['name'],
             'bname': charm_data['bname'],
             'branch_spec': charm_data['branch_spec'],
+            'branch_deleted': charm_data['branch_deleted'],
             'promulgated': charm_data['promulgated'],
         }
 

=== modified file 'charmworld/jobs/tests/test_lp.py'
--- charmworld/jobs/tests/test_lp.py	2013-06-05 18:33:46 +0000
+++ charmworld/jobs/tests/test_lp.py	2013-06-10 14:27:33 +0000
@@ -7,10 +7,13 @@
 from mock import patch
 
 from charmworld.testing import (
+    factory,
     JobTestBase,
 )
 
 from charmworld.jobs.lp import (
+    all_charms,
+    db_charms,
     available_charms,
     queue_charms,
 )
@@ -36,6 +39,7 @@
 
         self.assertEqual([{
             'branch_spec': u'~charmers/charms/precise/someproject/trunk',
+            'branch_deleted': False,
             'owner': u'charmers',
             'series': u'precise',
             'name': u'someproject',
@@ -57,6 +61,7 @@
 
         self.assertEqual([{
             'branch_spec': u'~charmers/charms/precise/someproject/trunk',
+            'branch_deleted': False,
             'owner': u'charmers',
             'series': u'precise',
             'name': u'someproject',
@@ -78,30 +83,37 @@
         self.assertIn(err_msg, log_messages)
         self.assertEqual([], charms)
 
-    def test_available_charms_filter(self):
+    def test_all_charms_filter(self):
         # If the branch doesn't start with the defined filter (i.e.
         # ~charmers/charms/precise) it is not returned
         charm_data = [[u'~not-charmers/charms/precise/someproject/trunk',
                        u'[e-mail]-20120329093714-s2m9e28dwotmijqc',
                        []]]
+        factory.makeCharm(self.db, owner='not-charmers')
         with patch('charmworld.jobs.lp.CHARM_IMPORT_FILTER',
                    '~charmers/charms/precise'):
-            charms = [charm for charm in available_charms(charm_data)]
+            charms = [charm for charm in all_charms(self.db, charm_data)]
         self.assertEqual([], charms)
 
-    def test_available_charms_explicit_filter(self):
+    def test_all_charms_explicit_filter(self):
         # If the branch doesn't start with the defined filter (i.e.
         # ~charmers/charms/precise) it is not returned
-        charm_data = [[u'~not-charmers/charms/precise/someproject/trunk',
-                       u'[e-mail]-20120329093714-s2m9e28dwotmijqc',
-                       []],
-                      [u'~charmers/charms/precise/someproject/trunk',
-                       u'[e-mail]-20120329093714-s2m9e28dwotmijqc',
-                       []],
-                      ]
-        charms = available_charms(
-            charm_data, import_filter='~charmers/charms/precise')
-        self.assertEqual(['~charmers/charms/precise/someproject/trunk'],
+        lp_data = [
+            [u'~not-charmers/charms/precise/someproject/trunk',
+             u'[e-mail]-20120329093714-s2m9e28dwotmijqc',
+             []],
+            [u'~charmers/charms/precise/someproject/trunk',
+             u'[e-mail]-20120329093714-s2m9e28dwotmijqc',
+             []],
+        ]
+        factory.makeCharm(
+            self.db, owner='charmers', name='only-in-db')
+        factory.makeCharm(
+            self.db, owner='not-charmers', name='only-in-db')
+        charms = all_charms(
+            self.db, lp_data, import_filter='~charmers/charms/precise')
+        self.assertEqual(['~charmers/charms/precise/only-in-db/trunk',
+                          '~charmers/charms/precise/someproject/trunk'],
                          [charm['branch_spec'] for charm in charms])
 
     def test_available_charms_not_trunk(self):
@@ -117,22 +129,105 @@
         self.assertIn(err_msg, log_messages)
         self.assertEqual([], charms)
 
-    def test_available_charm_limit(self):
+    def test_all_charms_limit(self):
         handler = self.get_handler("charm.launchpad")
-        charm_data = [[u'~charmers/charms/precise/someproject/trunk',
-                       u'[e-mail]-20120329093714-s2m9e28dwotmijqc',
-                       []]] * 200
-        charms = [charm for charm in available_charms(charm_data, limit=110)]
+        charm_data = [
+            [u'~charmers/charms/precise/someproject-%s/trunk' % count,
+             u'[e-mail]-20120329093714-s2m9e28dwotmijqc', []]
+            for count in xrange(100)]
+        for count in range(20):
+            factory.makeCharm(self.db)
+        charms = [charm for charm in all_charms(
+            self.db, charm_data, limit=110)]
         log_messages = [record.getMessage() for record in handler.buffer]
         err_msg = ("Import limit reached (110)... stopping")
         self.assertIn(err_msg, log_messages)
         self.assertEqual(110, len(charms))
 
+    def test_db_charms(self):
+        charm_1 = factory.makeCharm(
+            self.db, owner='o2', series='s1', name='c1')[1]
+        charm_2 = factory.makeCharm(
+            self.db, owner='o1', series='s2', name='c1')[1]
+        charm_3 = factory.makeCharm(
+            self.db, owner='o1', series='s1', name='c2')[1]
+        charm_4 = factory.makeCharm(
+            self.db, owner='o1', series='s1', name='c1')[1]
+        result = [c['branch_spec'] for c in db_charms(self.db)]
+        expected = [
+            c['branch_spec'] for c in (charm_4, charm_3, charm_2, charm_1)]
+        self.assertEqual(expected, result)
+
+    def makeLPData(self, owner, series, name, promulgated=False):
+        if promulgated:
+            linked_series = [series]
+        else:
+            linked_series = []
+        return ['~%s/charms/%s/%s/trunk' % (owner, series, name),
+                '[e-mail]-%s-%s' % (owner, series, name), linked_series]
+
+    def makeCharmAndLPData(self, owner, series, name, promulgated=False):
+        charm = factory.makeCharm(
+            self.db, owner=owner, series=series, name=name,
+            promulgated=promulgated)[1]
+        return charm, self.makeLPData(owner, series, name, promulgated)
+
+    def test_all_charms(self):
+        lp_data = []
+        charm_1, lp_1 = self.makeCharmAndLPData(
+            'owner', 'series', 'c1', promulgated=True)
+        lp_data.append(lp_1)
+        charm_2, lp_2 = self.makeCharmAndLPData(
+            'owner', 'series', 'c2', promulgated=True)
+        lp_3 = self.makeLPData('owner', 'series', 'c3')
+        lp_data.append(lp_3)
+        result = list(all_charms(self.db, lp_data))
+
+        # Note that data for the promulgated charm_2 does not appear
+        # in lp_data; db_charms() changed its promulgated status to
+        # false.
+        expected = [
+            {
+                'branch_spec': '~owner/charms/series/c1/trunk',
+                'branch_deleted': False,
+                'bname': 'trunk',
+                'commit': '[e-mail]-series-c1',
+                'distro_series': ['series'],
+                'name': 'c1',
+                'owner': 'owner',
+                'promulgated': True,
+                'series': 'series',
+            },
+            {
+                'branch_spec': '~owner/charms/series/c2/trunk',
+                'branch_deleted': True,
+                'bname': 'trunk',
+                'distro_series': [],
+                'name': 'c2',
+                'owner': 'owner',
+                'series': 'series',
+                'promulgated': False,
+                '_id': '~owner/charms/series/c2/trunk',
+            },
+            {
+                'branch_spec': '~owner/charms/series/c3/trunk',
+                'branch_deleted': False,
+                'bname': 'trunk',
+                'commit': '[e-mail]-series-c3',
+                'distro_series': [],
+                'name': 'c3',
+                'owner': 'owner',
+                'promulgated': False,
+                'series': 'series',
+            },
+        ]
+        self.assertEqual(expected, result)
+
     @patch('charmworld.jobs.lp.available_charms', available_charms_mock)
     def test_queue_charms(self):
         out_queue = get_queue('test_queue')
         self.addCleanup(out_queue.clear)
-        queue_charms(out_queue=out_queue)
+        queue_charms(self.db, out_queue=out_queue)
         item = out_queue.next()
         self.assertEqual(
             {'branch_dir': 'foo', 'branch_spec': 'bar'}, item.payload)

=== modified file 'charmworld/jobs/tests/test_proof.py'
--- charmworld/jobs/tests/test_proof.py	2013-04-23 15:27:26 +0000
+++ charmworld/jobs/tests/test_proof.py	2013-06-10 14:27:33 +0000
@@ -1,7 +1,10 @@
 # Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from mock import patch
+
 from charmworld.jobs.ingest import ProofIngestJob
+from charmworld.testing import factory
 from charmworld.testing import JobTestBase
 
 
@@ -19,3 +22,13 @@
                "could not find charm proof path.")
         self.assertIn(err, log_messages)
         self.assertIn('CHARM_PROOF_PATH: %s' % nonexistant_path, log_messages)
+
+    def test_no_proof_for_deleted_branch(self):
+        # If the branch of a charm is deleted from Launchpad, the proof
+        # job does nothing.
+        job = ProofIngestJob()
+        job.setup()
+        charm = factory.get_charm_json(branch_deleted=True)
+        with patch.object(job, 'proofer') as mock:
+            job.run(charm)
+        self.assertFalse(mock.called)

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-06-05 22:00:41 +0000
+++ charmworld/models.py	2013-06-10 14:27:33 +0000
@@ -191,6 +191,7 @@
         # Provided by Lp charms queue.
         'branch_spec': '',
         'bzr_branch': '',
+        'branch_deleted': False,
         'bname': '',
         'commit': '',
         'distro_series': [],
@@ -268,6 +269,12 @@
         return "lp:%s" % self.branch_spec
 
     @property
+    def branch_deleted(self):
+        """True if the Launchpad branch of this charm is deleted, else False.
+        """
+        return self._representation['branch_deleted']
+
+    @property
     def owner(self):
         """The owner of the charm's branch.'
 

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-05-29 19:19:19 +0000
+++ charmworld/testing/factory.py	2013-06-10 14:27:33 +0000
@@ -120,7 +120,8 @@
                    commit_message='maintainer', provides=None, requires=None,
                    options=None, files=None, charm_error=False,
                    date_created=None, downloads_in_past_30_days=0,
-                   is_featured=False, promulgated=False, categories=None):
+                   is_featured=False, promulgated=False, categories=None,
+                   branch_deleted=False):
     """Return the json of a charm."""
     if not description:
         description = """byobu-class provides remote terminal access through
@@ -188,6 +189,7 @@
     charm = {
         "bname": bname,
         "branch_dir": os.path.join(branch_root, name),
+        "branch_deleted": branch_deleted,
         "branch_spec": "~%s/charms/%s/%s/trunk" % (owner, series, name),
         "categories": categories,
         "changes": changes,