Merge lp:~jtv/launchpad/katie-and-gina-are-bad-bad-girls into lp:launchpad

[Jeroen T. Vermeulen]

[r=allenap][bug=850926] Katie/Gina cleanup.

= Summary =

Katie and Gina are a bit of a mess. Very old code, basically unmaintained for years, full of sometimes puzzling XXX comments by people who no longer work here.

== Proposed fix ==

Remove some unneeded functions, parameters etc. Oh yes, and this revealed that the katie module was not only unclear and untested, it was unneeded as well.

Gina could do with more cleanups that will reverberate throughout the module. For example, it would help to look up the relevant distroseries once and pass that around, rather than passing distro and series name and expect every callee to look it up for themselves.

== Pre-implementation notes ==

We may be able to ditch the Katie database as well. The Katie celebrity may be slightly harder to get rid of, because Translations uses it as a special person to recognize items produced by a particular internal process.

We'll also want much more fine-grained transaction management in gina. It could run for far too long without committing its changes, and as far as I can see, without good reason. The code will behave fine if restarted after an abortive run that committed only a portion of its changes; it's integral to the design of incremental imports.

== Implementation details ==

I removed the bit in SourcePackageData that handles an Uploaders key in an incoming file and sets self.uploaders. I can do that because nothing references the value, and there is a catch-all (a dubious one though—need to look into that as well) that will naturally do the right thing for this item. It just won't parse the field, which is fine by me.

== Tests ==

{{{
./bin/test -vvc lp.soyuz -t katie -t gina
}}}

== Demo and Q/A ==

We can do this on dogfood; we have some experience running gina now. Run the old gina (“./scripts/gina.py -v sid”). Record resulting SourcePackagePublishingHistory and SourcePackageRelease information for Debian in a scratch table. Upgrade the codebase and run the new gina. Now compare: it should not affect SPPH and SPR statuses and versions.

This is not a very complete test, but since this is not a functional change, the main worry is something blowing up because of, for example, an incorrect argument count.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/scripts/gina/packages.py
  scripts/gina.py
  lib/lp/soyuz/scripts/gina/handlers.py

./scripts/gina.py
      25: '_pythonpath' imported but unused

[Jeroen T. Vermeulen (jtv)]

William just tells me that the Katie celebrity is in fact a completely different Katie. So this cleanup will help clear that up as well.

[Gavin Panella (allenap)]

Looks good. There is also a katie_dbname configuration parameter in schema-lazr.conf that ought to be removed.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks. Removing that config key will have to be a separate bug, since removing a lazr config key is horribly dangerous until it has disappeared from the codebase on all production systems.

[Jeroen T. Vermeulen (jtv)]

Reported that as bug 850921.

=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
--- lib/lp/soyuz/scripts/gina/handlers.py	2011-09-14 06:39:41 +0000
+++ lib/lp/soyuz/scripts/gina/handlers.py	2011-09-15 04:49:09 +0000
@@ -165,8 +165,8 @@
     This class is used to handle the import process.
     """
 
-    def __init__(self, ztm, distro_name, distroseries_name, ktdb,
-                 archive_root, keyrings, pocket, component_override):
+    def __init__(self, ztm, distro_name, distroseries_name, archive_root,
+                 pocket, component_override):
         self.pocket = pocket
         self.component_override = component_override
         self.ztm = ztm
@@ -178,10 +178,10 @@
         self.imported_sources = []
         self.imported_bins = {}
 
-        self.sphandler = SourcePackageHandler(ktdb, archive_root, keyrings,
-                                              pocket, component_override)
-        self.bphandler = BinaryPackageHandler(self.sphandler, archive_root,
-                                              pocket)
+        self.sphandler = SourcePackageHandler(
+            archive_root, pocket, component_override)
+        self.bphandler = BinaryPackageHandler(
+            self.sphandler, archive_root, pocket)
 
         self.sppublisher = SourcePackagePublisher(
             self.distroseries, pocket, self.component_override)
@@ -454,12 +454,9 @@
     on the launchpad db a little easier.
     """
 
-    def __init__(self, KTDB, archive_root, keyrings, pocket,
-                 component_override):
+    def __init__(self, archive_root, pocket, component_override):
         self.distro_handler = DistroHandler()
-        self.ktdb = KTDB
         self.archive_root = archive_root
-        self.keyrings = keyrings
         self.pocket = pocket
         self.component_override = component_override
 
@@ -495,8 +492,8 @@
             return None
 
         # Process the package
-        sp_data.process_package(self.ktdb, self.archive_root, self.keyrings)
-        sp_data.ensure_complete(self.ktdb)
+        sp_data.process_package(self.archive_root)
+        sp_data.ensure_complete()
 
         spr = self.createSourcePackageRelease(sp_data, distroseries)
 

=== removed file 'lib/lp/soyuz/scripts/gina/katie.py'
--- lib/lp/soyuz/scripts/gina/katie.py	2011-09-13 14:48:05 +0000
+++ lib/lp/soyuz/scripts/gina/katie.py	1970-01-01 00:00:00 +0000
@@ -1,136 +0,0 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-__metaclass__ = type
-
-"""Katie database handler.
-
-Class to handle and query the katie db properly.
-"""
-__all__ = ['Katie']
-
-from canonical.config import config
-from canonical.database.sqlbase import connect
-from canonical.launchpad.scripts import log
-
-
-class Katie:
-    def __init__(self, dbname, suite):
-        self.suite = suite
-        self.dbname = dbname
-        log.info("Connecting to %s as %s" % (dbname, config.gina.dbuser))
-        self.db = connect(user=config.gina.dbuser, dbname=dbname)
-
-    #
-    # Database convenience methods
-    #
-
-    def ensure_string_format(self, name):
-        assert isinstance(name, basestring), repr(name)
-        try:
-            # check that this is unicode data
-            name.decode("utf-8").encode("utf-8")
-            return name
-        except UnicodeError:
-            # check that this is latin-1 data
-            s = name.decode("latin-1").encode("utf-8")
-            s.decode("utf-8")
-            return s
-
-    def commit(self):
-        log.debug("Committing")
-        return self.db.commit()
-
-    def close(self):
-        log.info("Closing connection")
-        return self.db.close()
-
-    def _get_dicts(self, cursor):
-        names = [x[0] for x in cursor.description]
-        ret = []
-        for item in cursor.fetchall():
-            res = {}
-            for i in range(len(names)):
-                res[names[i]] = item[i]
-            ret.append(res)
-        return ret
-
-    def _query_to_dict(self, query, args=None):
-        cursor = self._exec(query, args)
-        return self._get_dicts(cursor)
-
-    def _query(self, query, args=None):
-        cursor = self.db.cursor()
-        cursor.execute(query, args or [])
-        results = cursor.fetchall()
-        return results
-
-    def _query_single(self, query, args=None):
-        q = self._query(query, args)
-        if len(q) == 1:
-            return q[0]
-        elif not q:
-            return None
-        else:
-            raise AssertionError(
-                "Expected 0 or 1 result from this query, but got %d: "
-                "'%s' (with arguments: %s)." % (len(q), query, args))
-
-    def _exec(self, query, args=None):
-        cursor = self.db.cursor()
-        cursor.execute(query, args or [])
-        return cursor
-
-    #
-    # Katie domain-specific bits
-    #
-
-    def getSourcePackageRelease(self, name, version):
-        log.debug("Hunting for release %s / %s" % (name, version))
-        ret = self._query_to_dict("""
-            SELECT *
-            FROM source, fingerprint
-            WHERE
-                source = %s AND
-                source.sig_fpr = fingerprint.id AND
-                version = %s""",
-            (name, version))
-        if not ret:
-            log.debug(
-                "that spr didn't turn up. Attempting to find via ubuntu")
-        else:
-            return ret
-
-        # XXX kiko 2005-10-21: what to do when the ubuntu lookup fails?
-        return self._query_to_dict("""SELECT * FROM source, fingerprint
-                                      WHERE  source = %s
-                                      AND    source.sig_fpr = fingerprint.id
-                                      AND    version like '%subuntu%s'""" %
-                                      ("%s", version, "%"), name)
-
-    def getBinaryPackageRelease(self, name, version, arch):
-        return self._query_to_dict("""SELECT * FROM binaries, architecture,
-                                                    fingerprint
-                                      WHERE  package = %s
-                                      AND    version = %s
-                                      AND    binaries.sig_fpr = fingerprint.id
-                                      AND    binaries.architecture =
-                                                architecture.id
-                                      AND    arch_string = %s""",
-                                        (name, version, arch))
-
-    def getSections(self):
-        return self._query("""SELECT section FROM section""")
-
-    def getSourceSection(self, sourcepackage):
-        return self._query_single("""
-        SELECT section.section
-          FROM section,
-               override,
-               suite
-
-         WHERE override.section = section.id
-           AND suite.id = override.suite
-           AND override.package = %s
-           AND suite.suite_name = %s
-        """, (sourcepackage, self.suite))[0]

=== modified file 'lib/lp/soyuz/scripts/gina/packages.py'
--- lib/lp/soyuz/scripts/gina/packages.py	2011-08-22 11:50:51 +0000
+++ lib/lp/soyuz/scripts/gina/packages.py	2011-09-15 04:49:09 +0000
@@ -13,7 +13,6 @@
 
 
 __all__ = [
-    'AbstractPackageData',
     'BinaryPackageData',
     'get_dsc_path',
     'PoolFileNotFound',
@@ -165,40 +164,6 @@
         return v
 
 
-def get_person_by_key(keyrings, key):
-    # XXX kiko 2005-10-23: Untested, should probably be a method.
-    if key and key not in ("NOSIG", "None", "none"):
-        command = ("gpg --no-options --no-default-keyring "
-                   "--with-colons --fingerprint %s %s" % (key, keyrings))
-        h = os.popen(command, "r")
-        for line in h.readlines():
-            if line.startswith("pub"):
-                break
-        else:
-            log.warn("Broke parsing gpg output for %s" % key)
-            return None
-
-        line = line.split(":")
-        algo = int(line[3])
-        id = line[4][-8:]
-        algorithm = algo
-        keysize = line[2]
-        user, rest = line[9].split("<", 1)
-        email = rest.split(">")[0].lower()
-        if line[1] == "-":
-            is_revoked = 0
-        else:
-            is_revoked = 1
-
-        h = os.popen("gpg --export --no-default-keyring %s "
-                     "--armor %s" % (keyrings, key), "r")
-        armor = h.read().strip()
-
-        return (user, email, id, armor, is_revoked, algorithm, keysize)
-    else:
-        return None
-
-
 #
 # Exception classes
 #
@@ -264,7 +229,7 @@
         if missing:
             raise MissingRequiredArguments(missing)
 
-    def process_package(self, kdb, archive_root, keyrings):
+    def process_package(self, archive_root):
         """Process the package using the files located in the archive.
 
         Raises PoolFileNotFound if a file is not found in the pool.
@@ -282,23 +247,16 @@
         # leave it around for forensics.
         shutil.rmtree(tempdir)
 
-        # XXX kiko 2005-10-18: Katie is disabled for the moment;
-        # hardcode the date_uploaded and c'est la vie.
-        # if not self.do_katie(kdb, keyrings):
-        #    return False
         self.date_uploaded = UTC_NOW
-        self.is_processed = True
         return True
 
     def do_package(self, archive_root):
-        raise NotImplementedError
-
-    def do_katie(self, kdb, keyrings):
+        """To be provided by derived class."""
         raise NotImplementedError
 
 
 class SourcePackageData(AbstractPackageData):
-    """This Class holds important data to a given sourcepackagerelease."""
+    """Important data relating to a given `SourcePackageRelease`."""
 
     # Defaults, overwritten by __init__
     directory = None
@@ -308,23 +266,24 @@
     build_depends_indep = ""
     build_conflicts = ""
     build_conflicts_indep = ""
-    # XXX kiko 2005-10-30: This isn't stored at all.
     standards_version = ""
     section = None
     format = None
 
-    # XXX kiko 2005-11-05: Not used anywhere.
-    priority = None
-
-    is_processed = False
-    is_created = False
-
     # These arguments /must/ have been set in the Sources file and
     # supplied to __init__ as keyword arguments. If any are not, a
     # MissingRequiredArguments exception is raised.
     _required = [
-        'package', 'binaries', 'version', 'maintainer', 'section',
-        'architecture', 'directory', 'files', 'component']
+        'package',
+        'binaries',
+        'version',
+        'maintainer',
+        'section',
+        'architecture',
+        'directory',
+        'files',
+        'component',
+        ]
 
     def __init__(self, **args):
         for k, v in args.items():
@@ -345,33 +304,26 @@
             elif k == 'Maintainer':
                 displayname, emailaddress = parse_person(v)
                 try:
-                    self.maintainer = (encoding.guess(displayname),
-                                       emailaddress)
+                    self.maintainer = (
+                        encoding.guess(displayname),
+                        emailaddress,
+                        )
                 except UnicodeDecodeError:
-                    raise DisplayNameDecodingError("Could not decode "
-                                                   "name %s" % displayname)
+                    raise DisplayNameDecodingError(
+                        "Could not decode name %s" % displayname)
             elif k == 'Files':
                 self.files = []
                 files = v.split("\n")
                 for f in files:
                     self.files.append(stripseq(f.split(" ")))
-            elif k == 'Uploaders':
-                # XXX kiko 2005-10-19: We don't do anything with this data,
-                # but I suspect we should.
-                people = stripseq(v.split(","))
-                self.uploaders = [person.split(" ", 1) for person in people]
             else:
                 setattr(self, k.lower().replace("-", "_"), v)
 
         if self.section is None:
-            # XXX kiko 2005-11-05: Untested and disabled.
-            # if kdb:
-            #     log.warn("Source package %s lacks section, "
-            #              "looking it up..." % self.package)
-            #     self.section = kdb.getSourceSection(self.package)
             self.section = 'misc'
-            log.warn("Source package %s lacks section, assumed %r" %
-                     (self.package, self.section))
+            log.warn(
+                "Source package %s lacks section, assumed %r",
+                self.package, self.section)
 
         if '/' in self.section:
             # this apparently happens with packages in universe.
@@ -414,17 +366,18 @@
                 log.warn("Changelog empty for source %s (%s)" %
                          (self.package, self.version))
 
-    def ensure_complete(self, kdb):
+    def ensure_complete(self):
         if self.format is None:
             # XXX kiko 2005-11-05: this is very funny. We care so much about
             # it here, but we don't do anything about this in handlers.py!
-            log.warn("Invalid format in %s, assumed %r" %
-                     (self.package, "1.0"))
             self.format = "1.0"
+            log.warn(
+                "Invalid format in %s, assumed %r", self.package, self.format)
 
         if self.urgency not in ChangesFile.urgency_map:
-            log.warn("Invalid urgency in %s, %r, assumed %r" %
-                     (self.package, self.urgency, "low"))
+            log.warn(
+                "Invalid urgency in %s, %r, assumed %r",
+                self.package, self.urgency, "low")
             self.urgency = "low"
 
 
@@ -435,9 +388,20 @@
     # They are passed in as keyword arguments. If any are not set, a
     # MissingRequiredArguments exception is raised.
     _required = [
-        'package', 'installed_size', 'maintainer', 'section',
-        'architecture', 'version', 'filename', 'component',
-        'size', 'md5sum', 'description', 'summary', 'priority']
+        'package',
+        'installed_size',
+        'maintainer',
+        'section',
+        'architecture',
+        'version',
+        'filename',
+        'component',
+        'size',
+        'md5sum',
+        'description',
+        'summary',
+        'priority',
+        ]
 
     # Set in __init__
     source = None
@@ -463,10 +427,6 @@
     # Overwritten in do_package, optionally
     shlibs = None
 
-    #
-    is_processed = False
-    is_created = False
-    #
     source_version_re = re.compile(r'([^ ]+) +\(([^\)]+)\)')
 
     def __init__(self, **args):
@@ -491,8 +451,6 @@
                         "not a valid integer: %r" % v)
             else:
                 setattr(self, k.lower().replace("-", "_"), v)
-            # XXX kiko 2005-11-03: "enhances" is not used and not stored
-            # anywhere, same for "pre_depends"
 
         if self.source:
             # We need to handle cases like "Source: myspell
@@ -527,13 +485,15 @@
 
         if self.section is None:
             self.section = 'misc'
-            log.warn("Binary package %s lacks a section, assumed %r" %
-                     (self.package, self.section))
+            log.warn(
+                "Binary package %s lacks a section, assumed %r",
+                self.package, self.section)
 
         if self.priority is None:
             self.priority = 'extra'
-            log.warn("Binary package %s lacks valid priority, assumed %r" %
-                     (self.package, self.priority))
+            log.warn(
+                "Binary package %s lacks valid priority, assumed %r",
+                self.package, self.priority)
 
         AbstractPackageData.__init__(self)
 

=== modified file 'scripts/gina.py'
--- scripts/gina.py	2011-09-13 13:56:49 +0000
+++ scripts/gina.py	2011-09-15 04:49:09 +0000
@@ -19,7 +19,6 @@
 
 __metaclass__ = type
 
-import os
 import sys
 import time
 
@@ -44,7 +43,6 @@
     MultiplePackageReleaseError,
     NoSourcePackageError,
     )
-from lp.soyuz.scripts.gina.katie import Katie
 from lp.soyuz.scripts.gina.packages import (
     BinaryPackageData,
     DisplayNameDecodingError,
@@ -58,26 +56,12 @@
 COUNTDOWN = 0
 
 
-def _get_keyring(keyrings_root):
-    # XXX kiko 2005-10-23: untested
-    keyrings = ""
-    for keyring in os.listdir(keyrings_root):
-        path = os.path.join(keyrings_root, keyring)
-        keyrings += " --keyring=%s" % path
-    if not keyrings:
-        raise AttributeError("Keyrings not found in ./keyrings/")
-    return keyrings
-
-
 def run_gina(options, ztm, target_section):
     # Avoid circular imports.
     from lp.registry.interfaces.pocket import PackagePublishingPocket
 
     package_root = target_section.root
-    keyrings_root = target_section.keyrings
     distro = target_section.distro
-    # XXX kiko 2005-10-23: I honestly think having a separate distroseries
-    # bit silly. Can't we construct this based on `distroseries-pocket`?
     pocket_distroseries = target_section.pocketrelease
     distroseries = target_section.distroseries
     components = [c.strip() for c in target_section.components.split(",")]
@@ -87,21 +71,17 @@
     source_only = target_section.source_only
     spnames_only = target_section.sourcepackagenames_only
 
-    KTDB = target_section.katie_dbname
-
     LIBRHOST = config.librarian.upload_host
     LIBRPORT = config.librarian.upload_port
 
     log.info("")
     log.info("=== Processing %s/%s/%s ===", distro, distroseries, pocket)
     log.debug("Packages read from: %s", package_root)
-    log.debug("Keyrings read from: %s", keyrings_root)
     log.info("Components to import: %s", ", ".join(components))
     if component_override is not None:
         log.info("Override components to: %s", component_override)
     log.info("Architectures to import: %s", ", ".join(archs))
     log.debug("Launchpad database: %s", config.database.rw_main_master)
-    log.info("Katie database: %s", KTDB)
     log.info("SourcePackage Only: %s", source_only)
     log.info("SourcePackageName Only: %s", spnames_only)
     log.debug("Librarian: %s:%s", LIBRHOST, LIBRPORT)
@@ -120,12 +100,6 @@
             log.error("Could not find component %s", component_override)
             sys.exit(1)
 
-    kdb = None
-    keyrings = None
-    if KTDB:
-        kdb = Katie(KTDB, distroseries)
-        keyrings = _get_keyring(keyrings_root)
-
     try:
         arch_component_items = ArchiveComponentItems(
             package_root, pocket_distroseries, components, archs,
@@ -137,11 +111,9 @@
 
     packages_map = PackagesMap(arch_component_items)
     importer_handler = ImporterHandler(
-        ztm, distro, distroseries, kdb, package_root, keyrings, pocket,
-        component_override)
+        ztm, distro, distroseries, package_root, pocket, component_override)
 
-    import_sourcepackages(
-        packages_map, kdb, package_root, keyrings, importer_handler)
+    import_sourcepackages(packages_map, package_root, importer_handler)
     importer_handler.commit()
 
     # XXX JeroenVermeulen 2011-09-07 bug=843728: Dominate binaries as well.
@@ -160,27 +132,23 @@
             log.exception("Database setup required for run on %s", archtag)
             sys.exit(1)
 
-    import_binarypackages(
-        packages_map, kdb, package_root, keyrings, importer_handler)
+    import_binarypackages(packages_map, package_root, importer_handler)
     importer_handler.commit()
 
 
-def attempt_source_package_import(source, kdb, package_root, keyrings,
-                                  importer_handler):
+def attempt_source_package_import(source, package_root, importer_handler):
     """Attempt to import a source package, and handle typical errors."""
     package_name = source.get("Package", "unknown")
     try:
         try:
-            do_one_sourcepackage(
-                source, kdb, package_root, keyrings, importer_handler)
+            do_one_sourcepackage(source, package_root, importer_handler)
         except psycopg2.Error:
             log.exception(
                 "Database error: unable to create SourcePackage for %s. "
                 "Retrying once..", package_name)
             importer_handler.abort()
             time.sleep(15)
-            do_one_sourcepackage(
-                source, kdb, package_root, keyrings, importer_handler)
+            do_one_sourcepackage(source, package_root, importer_handler)
     except (
         InvalidVersionError, MissingRequiredArguments,
         DisplayNameDecodingError):
@@ -199,8 +167,7 @@
             "Database duplication processing %s", package_name)
 
 
-def import_sourcepackages(packages_map, kdb, package_root,
-                          keyrings, importer_handler):
+def import_sourcepackages(packages_map, package_root, importer_handler):
     # Goes over src_map importing the sourcepackages packages.
     count = 0
     npacks = len(packages_map.src_map)
@@ -210,27 +177,25 @@
         for source in packages_map.src_map[package]:
             count += 1
             attempt_source_package_import(
-                source, kdb, package_root, keyrings, importer_handler)
+                source, package_root, importer_handler)
             if COUNTDOWN and (count % COUNTDOWN == 0):
                 log.warn('%i/%i sourcepackages processed', count, npacks)
 
 
-def do_one_sourcepackage(source, kdb, package_root, keyrings,
-                         importer_handler):
+def do_one_sourcepackage(source, package_root, importer_handler):
     source_data = SourcePackageData(**source)
     if importer_handler.preimport_sourcecheck(source_data):
         # Don't bother reading package information if the source package
         # already exists in the database
         log.info('%s already exists in the archive', source_data.package)
         return
-    source_data.process_package(kdb, package_root, keyrings)
-    source_data.ensure_complete(kdb)
+    source_data.process_package(package_root)
+    source_data.ensure_complete()
     importer_handler.import_sourcepackage(source_data)
     importer_handler.commit()
 
 
-def import_binarypackages(packages_map, kdb, package_root, keyrings,
-                          importer_handler):
+def import_binarypackages(packages_map, package_root, importer_handler):
     nosource = []
 
     # Run over all the architectures we have
@@ -245,16 +210,16 @@
             count += 1
             try:
                 try:
-                    do_one_binarypackage(binary, archtag, kdb, package_root,
-                                         keyrings, importer_handler)
+                    do_one_binarypackage(
+                        binary, archtag, package_root, importer_handler)
                 except psycopg2.Error:
                     log.exception(
                         "Database errors when importing a BinaryPackage "
                         "for %s. Retrying once..", package_name)
                     importer_handler.abort()
                     time.sleep(15)
-                    do_one_binarypackage(binary, archtag, kdb, package_root,
-                                         keyrings, importer_handler)
+                    do_one_binarypackage(
+                        binary, archtag, package_root, importer_handler)
             except (InvalidVersionError, MissingRequiredArguments):
                 log.exception(
                     "Unable to create BinaryPackageData for %s", package_name)
@@ -291,13 +256,12 @@
                 log.warn(pkg)
 
 
-def do_one_binarypackage(binary, archtag, kdb, package_root, keyrings,
-                         importer_handler):
+def do_one_binarypackage(binary, archtag, package_root, importer_handler):
     binary_data = BinaryPackageData(**binary)
     if importer_handler.preimport_binarycheck(archtag, binary_data):
         log.info('%s already exists in the archive', binary_data.package)
         return
-    binary_data.process_package(kdb, package_root, keyrings)
+    binary_data.process_package(package_root)
     importer_handler.import_binarypackage(archtag, binary_data)
     importer_handler.commit()