documented PackagesSources

10 files changed, 260 insertions, 213 deletions

diff --git a/doc/development/packages.txt b/doc/development/packages.txt
index e52eca496..c310805a7 100644
--- a/doc/development/packages.txt
+++ b/doc/development/packages.txt
@@ -26,7 +26,6 @@ The Collection Object
 =====================
 
 .. automodule:: Bcfg2.Server.Plugins.Packages.Collection
-.. autoclass:: Bcfg2.Server.Plugins.Packages.Collection._Collection
 
 
 The Source Object
diff --git a/src/lib/Bcfg2/Server/Plugin/helpers.py b/src/lib/Bcfg2/Server/Plugin/helpers.py
index 3334dfb27..e6b571eea 100644
--- a/src/lib/Bcfg2/Server/Plugin/helpers.py
+++ b/src/lib/Bcfg2/Server/Plugin/helpers.py
@@ -143,7 +143,7 @@ class FileBacked(object):
 
     def HandleEvent(self, event=None):
         """ HandleEvent is called whenever the FAM registers an event.
-        
+
         :param event: The event object
         :type event: Bcfg2.Server.FileMonitor.Event
         :returns: None
@@ -159,7 +159,7 @@ class FileBacked(object):
 
     def Index(self):
         """ Index() is called by :func:`HandleEvent` every time the
-        data changes, and can parse the data into usable data as
+        data changes, and parses the data into usable data as
         required."""
         pass
 
diff --git a/src/lib/Bcfg2/Server/Plugin/interfaces.py b/src/lib/Bcfg2/Server/Plugin/interfaces.py
index 59f3636fb..87f6ff1bd 100644
--- a/src/lib/Bcfg2/Server/Plugin/interfaces.py
+++ b/src/lib/Bcfg2/Server/Plugin/interfaces.py
@@ -539,7 +539,7 @@ class ClientRunHooks(object):
         :param metadata: The client metadata object
         :type metadata: Bcfg2.Server.Plugins.Metadata.ClientMetadata
         :returns: None
-        """        
+        """
         pass
 
     def end_statistics(self, metadata):
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Apt.py b/src/lib/Bcfg2/Server/Plugins/Packages/Apt.py
index 30be4b089..5e3d86f02 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Apt.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Apt.py
@@ -1,17 +1,18 @@
 import re
 import gzip
-from Bcfg2.Server.Plugins.Packages.Collection import _Collection
+from Bcfg2.Server.Plugins.Packages.Collection import Collection
 from Bcfg2.Server.Plugins.Packages.Source import Source
 
 
-class AptCollection(_Collection):
+class AptCollection(Collection):
     def get_config(self):
         lines = ["# This config was generated automatically by the Bcfg2 " \
                      "Packages plugin", '']
 
         for source in self:
             if source.rawurl:
-                self.logger.info("Packages: Skipping rawurl %s" % source.rawurl)
+                self.logger.info("Packages: Skipping rawurl %s" % 
+                                 source.rawurl)
             else:
                 lines.append("deb %s %s %s" % (source.url, source.version,
                                                " ".join(source.components)))
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Collection.py b/src/lib/Bcfg2/Server/Plugins/Packages/Collection.py
index 0460038c2..f5c035e00 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Collection.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Collection.py
@@ -1,72 +1,73 @@
-""" ``_Collection`` objects represent the set of
-:class:`Bcfg2.Server.Plugins.Packages.Source.Source` objects that apply
-to a given client, and can be used to query all software repositories
-for a client in aggregate.  In some cases this can give faster or more
-accurate results.
+""" ``Collection`` objects represent the set of
+:class:`Bcfg2.Server.Plugins.Packages.Source.Source` objects that
+apply to a given client, and can be used to query all software
+repositories for a client in aggregate.  In some cases this can give
+faster or more accurate results.
 
-In most cases, ``_Collection`` methods have been designed to defer the
-call to the Sources in the ``_Collection`` and aggregate the results
-as appropriate.  The simplest ``_Collection`` implemention is thus
-often a simple subclass that adds no additional functionality.
+In most cases, ``Collection`` methods have been designed to defer the
+call to the Sources in the ``Collection`` and aggregate the results as
+appropriate.  The simplest ``Collection`` implemention is thus often a
+simple subclass that adds no additional functionality.
 
 Overriding Methods
 ------------------
 
-As noted above, the ``_Collection`` object is written expressly so
-that you can subclass it and override no methods or attributes, and it
-will work by deferring all calls to the Source objects it contains.
-There are thus three approaches to writing a ``_Collection`` subclass:
+As noted above, the ``Collection`` object is written expressly so that
+you can subclass it and override no methods or attributes, and it will
+work by deferring all calls to the Source objects it contains.  There
+are thus three approaches to writing a ``Collection`` subclass:
 
 #. Keep the superclass almost entirely intact and defer to the
    ``Source`` objects inside it. For an example of this kind of
-   ``_Collection`` object, see
+   ``Collection`` object, see
    :mod:`Bcfg2.Server.Plugins.Packages.Apt`.
 
-#. Keep :func:`_Collection.complete` intact, and override the methods
-   it calls: :func:`_Collection.is_package`,
-   :func:`_Collection.is_virtual_package`,
-   :func:`_Collection.get_deps`, :func:`_Collection.get_provides`,
-   :func:`_Collection.get_vpkgs`, and :func:`_Collection.setup_data`.
-   There are no examples of this kind of ``_Collection`` subclass yet.
+#. Keep :func:`Collection.complete` intact, and override the methods
+   it calls: :func:`Collection.is_package`,
+   :func:`Collection.is_virtual_package`, :func:`Collection.get_deps`,
+   :func:`Collection.get_provides`, :func:`Collection.get_vpkgs`, and
+   :func:`Collection.setup_data`.  There are no examples of this kind
+   of ``Collection`` subclass yet.
 
-#. Provide your own implementation of :func:`_Collection.complete`, in
+#. Provide your own implementation of :func:`Collection.complete`, in
    which case you do not have to override the above methods.  You may
-   want to override :func:`_Collection.packages_from_entry`,
-   :func:`_Collection.packages_to_entry`, and
-   :func:`_Collection.get_new_packages`.  For an example of this kind
-   of ``_Collection`` object, see
+   want to override :func:`Collection.packages_from_entry`,
+   :func:`Collection.packages_to_entry`, and
+   :func:`Collection.get_new_packages`.  For an example of this kind
+   of ``Collection`` object, see
    :mod:`Bcfg2.Server.Plugins.Packages.yum`.
 
 In either case, you may want to override
-:func:`_Collection.get_groups`, :func:`_Collection.get_group`,
-:func:`_Collection.get_essential`, :func:`_Collection.get_config`,
-:func:`_Collection.filter_unknown`, and
-:func:`_Collection.build_extra_structures`.
+:func:`Collection.get_groups`, :func:`Collection.get_group`,
+:func:`Collection.get_essential`, :func:`Collection.get_config`,
+:func:`Collection.filter_unknown`, and
+:func:`Collection.build_extra_structures`.
 
 .. _pkg-objects:
 
 Conversion Between Package Objects and XML Entries
 --------------------------------------------------
 
-_Collection objects have to translate Bcfg2 entries,
+Collection objects have to translate Bcfg2 entries,
 :class:`lxml.etree._Element` objects, into objects suitable for use by
 the backend for resolving dependencies.  This is handled by two
 functions:
 
-* :func:`_Collection.packages_from_entry` is called to translate an
-  XML entry into a list of packages;
-* :func:`_Collection.packages_to_entry` is called to translate a list
+* :func:`Collection.packages_from_entry` is called to translate an XML
+  entry into a list of packages;
+
+* :func:`Collection.packages_to_entry` is called to translate a list
   of packages back into an XML entry.
 
 Because of this translation layer, the return type of any functions
-below that return packages (e.g., :func:`_Collection.get_group`) is
+below that return packages (e.g., :func:`Collection.get_group`) is
 actually indeterminate; they must return an object suitable for
-passing to :func:`_Collection.packages_to_entry`.  Similarly,
-functions that take a package as an argument (e.g.,
-:func:`_Collection.is_package`) take the appropriate package object.
+passing to :func:`Collection.packages_to_entry`.  Similarly, functions
+that take a package as an argument (e.g.,
+:func:`Collection.is_package`) take the appropriate package object.
 In the documentation below, the actual parameter return type (usually
-.``string``) used in this base implementation is noted, as well as this
-fact.
+.``string``) used in this base implementation is noted, as well as
+this fact.
 
 The Collection Module
 ---------------------
@@ -82,49 +83,19 @@ from Bcfg2.Server.Plugins.Packages.Source import Source
 
 LOGGER = logging.getLogger(__name__)
 
-#: We cache _Collection objects in ``COLLECTIONS`` so that calling
-#: :func:`Bcfg2.Server.Plugins.Packages.Packages.Refresh` or
-#: :func:`Bcfg2.Server.Plugins.Packages.Packages.Reload` can tell the
-#: collection objects to clean up their cache, but we don't actually
-#: use the cache to return a _Collection object when one is requested,
-#: because that prevents new machines from working, since a
-#: _Collection object gets created by
-#: :func:`Bcfg2.Server.Plugins.Packages.Packages.get_additional_data`,
-#: which is called for all clients at server startup.  (It would also
-#: prevent machines that change groups from working properly; e.g., if
-#: you reinstall a machine with a new OS, then returning a cached
-#: _Collection object would give the wrong sources to that client.)
-#: These are keyed by the collection :attr:`_Collection.cachekey`, a
-#: unique key identifying the collection by its *config*, which could
-#: be shared among multiple clients.
-COLLECTIONS = dict()
-
-#: CLIENTS is a cache mapping of hostname ->
-#: :attr:`_Collection.cachekey`.  This _is_ used to return a
-#: _Collection object when one is requested, so each entry is very
-#: short-lived -- it's purged at the end of each client run.
-CLIENTS = dict()
-
-
-class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
-    """ ``_Collection`` objects represent the set of
+
+class Collection(list, Bcfg2.Server.Plugin.Debuggable):
+    """ ``Collection`` objects represent the set of
     :class:`Bcfg2.Server.Plugins.Packages.Source` objects that apply
     to a given client, and can be used to query all software
     repositories for a client in aggregate.  In some cases this can
-    give faster or more accurate results.
-
-    Note that the name of this class starts with an underscore; the
-    factory function :func:`Collection` must be used to instantiate
-    the correct subclass of ``_Collection`` when creating an actual
-    collection object. """
+    give faster or more accurate results. """
 
     #: Whether or not this Packages backend supports package groups
     __package_groups__ = False
 
     def __init__(self, metadata, sources, basepath, debug=False):
-        """ Don't call ``__init__`` directly; use :func:`Collection`
-        to instantiate a new ``_Collection`` object.
-
+        """
         :param metadata: The client metadata for this collection
         :type metadata: Bcfg2.Server.Plugins.Metadata.ClientMetadata
         :param sources: A list of all sources known to the server that
@@ -160,7 +131,7 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
     @property
     def cachekey(self):
         """ A unique identifier for the set of sources contained in
-        this ``_Collection`` object.  This is unique to a set of
+        this ``Collection`` object.  This is unique to a set of
         sources, **not** necessarily to the client, which lets clients
         with identical sources share cache data."""
         return md5(self.sourcelist().encode('UTF-8')).hexdigest()
@@ -628,8 +599,8 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
         return packages, unknown
 
 
-def _get_collection_class(source_type):
-    """ Given a source type, determine the class of _Collection object
+def get_collection_class(source_type):
+    """ Given a source type, determine the class of Collection object
     that should be used to contain these sources.  Note that
     ``source_type`` is *not* a
     :class:`Bcfg2.Server.Plugins.Packages.Source.Source` subclass;
@@ -637,7 +608,7 @@ def _get_collection_class(source_type):
 
     :param source_type: The type of source, e.g., "yum" or "apt"
     :type source_type: string
-    :returns: type - the _Collection subclass that should be used to
+    :returns: type - the Collection subclass that should be used to
               instantiate an object to contain sources of the given type. """
     modname = "Bcfg2.Server.Plugins.Packages.%s" % source_type.title()
     try:
@@ -657,75 +628,3 @@ def _get_collection_class(source_type):
         LOGGER.error(msg)
         raise Bcfg2.Server.Plugin.PluginExecutionError(msg)
     return cclass
-
-
-def clear_cache():
-    """ Clear the caches kept by this
-    module. (:attr:`Bcfg2.Server.Plugins.Packages.Collection.COLLECTIONS`
-    and:attr:`Bcfg2.Server.Plugins.Packages.Collection.CLIENTS`) """
-    global COLLECTIONS, CLIENTS  # pylint: disable=W0603
-    COLLECTIONS = dict()
-    CLIENTS = dict()
-
-
-def Collection(metadata, sources, basepath, debug=False):
-    """ Object factory for subclasses of
-    :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`.
-
-    :param metadata: The client metadata to create a _Collection
-                     object for
-    :type metadata: Bcfg2.Server.Plugins.Metadata.ClientMetadata
-    :param sources: A list of all sources known to the server that
-                    will be used to generate the list of sources that
-                    apply to this client
-    :type sources: list of
-                   :class:`Bcfg2.Server.Plugins.Packages.Source.Source`
-                   objects
-    :param basepath: The base filesystem path where cache and other
-                     temporary data will be stored
-    :type basepath: string
-    :param debug: Enable debugging output
-    :type debug: bool
-    :return: An instance of the appropriate subclass of
-             :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
-             that contains all relevant sources that apply to the
-             given client
-    """
-    global COLLECTIONS  # pylint: disable=W0602
-
-    if not sources.loaded:
-        # if sources.xml has not received a FAM event yet, defer;
-        # instantiate a dummy _Collection object
-        return _Collection(metadata, [], basepath)
-
-    if metadata.hostname in CLIENTS:
-        return COLLECTIONS[CLIENTS[metadata.hostname]]
-
-    sclasses = set()
-    relevant = list()
-
-    for source in sources:
-        if source.applies(metadata):
-            relevant.append(source)
-            sclasses.update([source.__class__])
-
-    if len(sclasses) > 1:
-        LOGGER.warning("Packages: Multiple source types found for %s: %s" %
-                       ",".join([s.__name__ for s in sclasses]))
-        cclass = _Collection
-    elif len(sclasses) == 0:
-        LOGGER.error("Packages: No sources found for %s" % metadata.hostname)
-        cclass = _Collection
-    else:
-        cclass = _get_collection_class(sclasses.pop().__name__.replace("Source",
-                                                                       ""))
-
-    if debug:
-        LOGGER.error("Packages: Using %s for Collection of sources for %s" %
-                     (cclass.__name__, metadata.hostname))
-
-    collection = cclass(metadata, relevant, basepath, debug=debug)
-    ckey = collection.cachekey
-    CLIENTS[metadata.hostname] = ckey
-    COLLECTIONS[ckey] = collection
-    return collection
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Pac.py b/src/lib/Bcfg2/Server/Plugins/Packages/Pac.py
index 15fb62431..13090cd9f 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Pac.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Pac.py
@@ -1,9 +1,9 @@
 import tarfile
-from Bcfg2.Server.Plugins.Packages.Collection import _Collection
+from Bcfg2.Server.Plugins.Packages.Collection import Collection
 from Bcfg2.Server.Plugins.Packages.Source import Source
 
 
-class PacCollection(_Collection):
+class PacCollection(Collection):
     pass
 
 
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/PackagesSources.py b/src/lib/Bcfg2/Server/Plugins/Packages/PackagesSources.py
index 0d565be31..329dfc394 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/PackagesSources.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/PackagesSources.py
@@ -1,14 +1,40 @@
 import os
 import sys
-import lxml.etree
 import Bcfg2.Server.Plugin
 from Bcfg2.Server.Plugins.Packages.Source import SourceInitError
 
+
 class PackagesSources(Bcfg2.Server.Plugin.StructFile,
                       Bcfg2.Server.Plugin.Debuggable):
+    """ PackagesSources handles parsing of the
+    :mod:`Bcfg2.Server.Plugins.Packages` ``sources.xml`` file, and the
+    creation of the appropriate
+    :class:`Bcfg2.Server.Plugins.Packages.Source.Source` object for
+    each ``Source`` tag. """
+
     __identifier__ = None
 
     def __init__(self, filename, cachepath, fam, packages, setup):
+        """
+        :param filename: The full path to ``sources.xml``
+        :type filename: string
+        :param cachepath: The full path to the directory where
+                          :class:`Bcfg2.Server.Plugins.Packages.Source.Source`
+                          data will be cached
+        :type cachepath: string
+        :param fam: The file access monitor to use to create watches
+                    on ``sources.xml`` and any XIncluded files.
+        :type fam: Bcfg2.Server.FileMonitor.FileMonitor
+        :param packages: The Packages plugin object ``sources.xml`` is
+                         being parsed on behalf of (i.e., the calling
+                         object)
+        :type packages: Bcfg2.Server.Plugins.Packages.Packages
+        :param setup: A Bcfg2 options dict
+        :type setup: dict
+
+        :raises: :class:`Bcfg2.Server.Plugin.exceptions.PluginInitError` -
+                 If ``sources.xml`` cannot be read
+        """
         Bcfg2.Server.Plugin.Debuggable.__init__(self)
         try:
             Bcfg2.Server.Plugin.StructFile.__init__(self, filename, fam=fam,
@@ -16,12 +42,14 @@ class PackagesSources(Bcfg2.Server.Plugin.StructFile,
         except OSError:
             err = sys.exc_info()[1]
             msg = "Packages: Failed to read configuration file: %s" % err
-            if not os.path.exists(self.name):
-                msg += " Have you created it?"
             self.logger.error(msg)
             raise Bcfg2.Server.Plugin.PluginInitError(msg)
+
+        #: The full path to the directory where
+        #: :class:`Bcfg2.Server.Plugins.Packages.Source.Source` data
+        #: will be cached
         self.cachepath = cachepath
-        self.setup = setup
+
         if not os.path.exists(self.cachepath):
             # create cache directory if needed
             try:
@@ -30,16 +58,38 @@ class PackagesSources(Bcfg2.Server.Plugin.StructFile,
                 err = sys.exc_info()[1]
                 self.logger.error("Could not create Packages cache at %s: %s" %
                                   (self.cachepath, err))
+        #: The Bcfg2 options dict
+        self.setup = setup
+
+        #: The :class:`Bcfg2.Server.Plugins.Packages.Packages` that
+        #: instantiated this ``PackagesSources`` object
         self.pkg_obj = packages
+
+        #: The set of all XML files that have been successfully
+        #: parsed.  This is used by :attr:`loaded` to determine if the
+        #: sources have been fully parsed and the
+        #: :class:`Bcfg2.Server.Plugins.Packages.Packages` plugin
+        #: should be told to reload its data.
         self.parsed = set()
 
     def toggle_debug(self):
         Bcfg2.Server.Plugin.Debuggable.toggle_debug(self)
         for source in self.entries:
             source.toggle_debug()
+    toggle_debug.__doc__ = Bcfg2.Server.Plugin.Plugin.toggle_debug.__doc__
 
     def HandleEvent(self, event=None):
-        Bcfg2.Server.Plugin.XMLFileBacked.HandleEvent(self, event=event)
+        """ HandleEvent is called whenever the FAM registers an event.
+
+        When :attr:`loaded` becomes True,
+        :func:`Bcfg2.Server.Plugins.Packages.Packages.Reload` is
+        called to reload all plugin data from the configured sources.
+
+        :param event: The event object
+        :type event: Bcfg2.Server.FileMonitor.Event
+        :returns: None
+        """
+        Bcfg2.Server.Plugin.StructFile.HandleEvent(self, event=event)
         if event and event.filename != self.name:
             for fpath in self.extras:
                 if fpath == os.path.abspath(event.filename):
@@ -52,23 +102,42 @@ class PackagesSources(Bcfg2.Server.Plugin.StructFile,
 
     @property
     def loaded(self):
+        """ Whether or not all XML files (``sources.xml`` and
+        everything XIncluded in it) have been parsed. This flag is
+        used to determine if the Packages plugin should be told to
+        load its data. """
         return sorted(list(self.parsed)) == sorted(self.extras)
 
     def Index(self):
-        Bcfg2.Server.Plugin.XMLFileBacked.Index(self)
+        Bcfg2.Server.Plugin.StructFile.Index(self)
         self.entries = []
         for xsource in self.xdata.findall('.//Source'):
             source = self.source_from_xml(xsource)
             if source is not None:
                 self.entries.append(source)
+    Index.__doc__ = Bcfg2.Server.Plugin.StructFile.Index.__doc__ + """
+
+``Index`` is responsible for calling :func:`source_from_xml` for each
+``Source`` tag in each file. """
 
     def source_from_xml(self, xsource):
-        """ create a *Source object from its XML representation in
-        sources.xml """
+        """ Create a
+        :class:`Bcfg2.Server.Plugins.Packages.Source.Source` subclass
+        object from XML representation of a source in ``sources.xml``.
+        ``source_from-xml`` determines the appropriate subclass of
+        ``Source`` to instantiate according to the ``type`` attribute
+        of the ``Source`` tag.
+
+        :param xsource: The XML tag representing the source
+        :type xsource: lxml.etree._Element
+        :returns: :class:`Bcfg2.Server.Plugins.Packages.Source.Source`
+                  subclass, or None on error
+        """
         stype = xsource.get("type")
         if stype is None:
-            self.logger.error("Packages: No type specified for source, "
-                              "skipping")
+            self.logger.error("Packages: No type specified for source at %s, "
+                              "skipping" % (xsource.get("rawurl",
+                                                        xsource.get("url"))))
             return None
 
         try:
@@ -77,8 +146,9 @@ class PackagesSources(Bcfg2.Server.Plugin.StructFile,
                              stype.title())
             cls = getattr(module, "%sSource" % stype.title())
         except (ImportError, AttributeError):
-            ex = sys.exc_info()[1]
-            self.logger.error("Packages: Unknown source type %s (%s)" % (stype, ex))
+            err = sys.exc_info()[1]
+            self.logger.error("Packages: Unknown source type %s (%s)" % (stype,
+                                                                         err))
             return None
 
         try:
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Source.py b/src/lib/Bcfg2/Server/Plugins/Packages/Source.py
index 273f6cbb4..3269f8c08 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Source.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Source.py
@@ -3,11 +3,11 @@
 multiple repositories (if it uses the "url" attribute instead of
 "rawurl"), and so can the ``Source`` object.  This can be the source
 (har har) of some confusion.  See
-:func:`Bcfg2.Server.Plugins.Packages.Collection._Collection.sourcelist`
+:func:`Bcfg2.Server.Plugins.Packages.Collection.Collection.sourcelist`
 for the proper way to get all repos from a ``Source`` object.
 
 Source objects are aggregated into
-:class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
+:class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
 objects, which are actually called by
 :class:`Bcfg2.Server.Plugins.Packages.Packages`.  This way a more
 advanced subclass can query repositories in aggregate rather than
@@ -16,12 +16,12 @@ individually, which may give faster or more accurate results.
 The base ``Source`` object must be subclassed to handle each
 repository type.  How you subclass ``Source`` will depend on how you
 subclassed
-:class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`; see
+:class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`; see
 :mod:`Bcfg2.Server.Plugins.Packages.Collection` for more details on
 different methods for doing that.
 
 If you are using the stock (or a near-stock)
-:class:`Bcfg2.Server.Plugins.Packages.Collection._Collection` object,
+:class:`Bcfg2.Server.Plugins.Packages.Collection.Collection` object,
 then you will need to implement the following methods and attributes
 in your ``Source`` subclass:
 
@@ -41,7 +41,7 @@ methods and attributes:
 For an example of this kind of ``Source`` object, see
 :mod:`Bcfg2.Server.Plugins.Packages.Apt`.
 
-If you are overriding the ``_Collection`` object in more depth, then
+If you are overriding the ``Collection`` object in more depth, then
 you have more leeway in what you might want to override or implement
 in your ``Source`` subclass.  For an example of this kind of
 ``Source`` object, see :mod:`Bcfg2.Server.Plugins.Packages.Yum`.
@@ -113,7 +113,7 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
 
     #: A predicate that is used by :func:`filter_unknown` to filter
     #: packages from the results of
-    #: :func:`Bcfg2.Server.Plugins.Packages.Collection._Collection.complete`
+    #: :func:`Bcfg2.Server.Plugins.Packages.Collection.Collection.complete`
     #: that should not be shown to the end user (i.e., that are not
     #: truly unknown, but are rather packaging system artifacts).  By
     #: default, excludes any package whose name starts with "choice"
@@ -240,19 +240,19 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
         #: A set of all package names in this source.  This will not
         #: necessarily be populated, particularly by backends that
         #: reimplement large portions of
-        #: :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
+        #: :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
         self.pkgnames = set()
 
         #: A dict of ``<package name>`` -> ``<list of dependencies>``.
         #: This will not necessarily be populated, particularly by
         #: backends that reimplement large portions of
-        #: :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
+        #: :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
         self.deps = dict()
 
         #: A dict of ``<package name>`` -> ``<list of provided
         #: symbols>``.  This will not necessarily be populated,
         #: particularly by backends that reimplement large portions of
-        #: :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
+        #: :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
         self.provides = dict()
 
         #: The file (or directory) used for this source's cache data
@@ -570,7 +570,7 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
 
     def filter_unknown(self, unknown):
         """ After
-        :func:`Bcfg2.Server.Plugins.Packages.Collection._Collection.complete`,
+        :func:`Bcfg2.Server.Plugins.Packages.Collection.Collection.complete`,
         filter out packages that appear in the list of unknown
         packages but should not be presented to the user.
         :attr:`unknown_filter` is called to assess whether or not a
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Yum.py b/src/lib/Bcfg2/Server/Plugins/Packages/Yum.py
index 7de8d1fb3..c1dd80689 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Yum.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Yum.py
@@ -9,7 +9,7 @@ from subprocess import Popen, PIPE
 import Bcfg2.Server.Plugin
 from Bcfg2.Compat import StringIO, cPickle, HTTPError, URLError, \
     ConfigParser, json
-from Bcfg2.Server.Plugins.Packages.Collection import _Collection
+from Bcfg2.Server.Plugins.Packages.Collection import Collection
 from Bcfg2.Server.Plugins.Packages.Source import SourceInitError, Source, \
      fetch_url
 
@@ -73,7 +73,7 @@ def _setup_pulp(setup):
     return PULPSERVER
 
 
-class YumCollection(_Collection):
+class YumCollection(Collection):
     #: YumCollections support package groups
     __package_groups__ = True
 
@@ -83,7 +83,7 @@ class YumCollection(_Collection):
     option_blacklist = ["use_yum_libraries", "helper"]
 
     def __init__(self, metadata, sources, basepath, debug=False):
-        _Collection.__init__(self, metadata, sources, basepath, debug=debug)
+        Collection.__init__(self, metadata, sources, basepath, debug=debug)
         self.keypath = os.path.join(self.basepath, "keys")
 
         if self.use_yum:
@@ -333,7 +333,7 @@ class YumCollection(_Collection):
 
     def is_package(self, package):
         if not self.use_yum:
-            return _Collection.is_package(self, package)
+            return Collection.is_package(self, package)
         elif isinstance(package, tuple):
             if package[1] is None and package[2] == (None, None, None):
                 package = package[0]
@@ -346,7 +346,7 @@ class YumCollection(_Collection):
 
     def is_virtual_package(self, package):
         if not self.use_yum:
-            return _Collection.is_virtual_package(self, package)
+            return Collection.is_virtual_package(self, package)
         else:
             # this should really never get called; it's just provided
             # for API completeness
@@ -354,7 +354,7 @@ class YumCollection(_Collection):
 
     def get_deps(self, package):
         if not self.use_yum:
-            return _Collection.get_deps(self, package)
+            return Collection.get_deps(self, package)
         else:
             # this should really never get called; it's just provided
             # for API completeness
@@ -362,7 +362,7 @@ class YumCollection(_Collection):
 
     def get_provides(self, required, all=False, silent=False):
         if not self.use_yum:
-            return _Collection.get_provides(self, required)
+            return Collection.get_provides(self, required)
         else:
             # this should really never get called; it's just provided
             # for API completeness
@@ -478,7 +478,7 @@ class YumCollection(_Collection):
 
     def complete(self, packagelist):
         if not self.use_yum:
-            return _Collection.complete(self, packagelist)
+            return Collection.complete(self, packagelist)
 
         if packagelist:
             result = \
@@ -541,7 +541,7 @@ class YumCollection(_Collection):
 
     def setup_data(self, force_update=False):
         if not self.use_yum:
-            return _Collection.setup_data(self, force_update)
+            return Collection.setup_data(self, force_update)
 
         if force_update:
             # we call this twice: one to clean up data from the old
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/__init__.py b/src/lib/Bcfg2/Server/Plugins/Packages/__init__.py
index 083bcd5e8..f7932dd75 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/__init__.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/__init__.py
@@ -6,7 +6,8 @@ import lxml.etree
 import Bcfg2.Logger
 import Bcfg2.Server.Plugin
 from Bcfg2.Compat import ConfigParser, urlopen
-from Bcfg2.Server.Plugins.Packages import Collection
+from Bcfg2.Server.Plugins.Packages.Collection import Collection, \
+    get_collection_class
 from Bcfg2.Server.Plugins.Packages.PackagesSources import PackagesSources
 
 #: The default path for generated yum configs
@@ -46,10 +47,14 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
         Bcfg2.Server.Plugin.Connector.__init__(self)
         Bcfg2.Server.Plugin.ClientRunHooks.__init__(self)
 
-        self.sentinels = set()
+        #: Packages does a potentially tremendous amount of on-disk
+        #: caching.  ``cachepath`` holds the base directory to where
+        #: data should be cached.
         self.cachepath = \
             self.core.setup.cfp.get("packages", "cache",
                                     default=os.path.join(self.data, 'cache'))
+
+        #: Where Packages should store downloaded GPG key files
         self.keypath = \
             self.core.setup.cfp.get("packages", "keycache",
                                     default=os.path.join(self.data, 'keys'))
@@ -57,9 +62,43 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
             # create key directory if needed
             os.makedirs(self.keypath)
 
+        #: The
+        #: :class:`Bcfg2.Server.Plugins.Packages.PackagesSources.PackagesSources`
+        #: object used to generate
+        #: :class:`Bcfg2.Server.Plugins.Packages.Source.Source` objects for
+        #: this plugin.
         self.sources = PackagesSources(os.path.join(self.data, "sources.xml"),
                                        self.cachepath, core.fam, self,
                                        self.core.setup)
+
+        #: We cache
+        #: :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
+        #: objects in ``collections`` so that calling :func:`Refresh`
+        #: or :func:`Reload` can tell the collection objects to clean
+        #: up their cache, but we don't actually use the cache to
+        #: return a ``Collection`` object when one is requested,
+        #: because that prevents new machines from working, since a
+        #: ``Collection`` object gets created by
+        #: :func:`get_additional_data`, which is called for all
+        #: clients at server startup and various other times.  (It
+        #: would also prevent machines that change groups from working
+        #: properly; e.g., if you reinstall a machine with a new OS,
+        #: then returning a cached ``Collection`` object would give
+        #: the wrong sources to that client.)  These are keyed by the
+        #: collection
+        #: :attr:`Bcfg2.Server.Plugins.Packages.Collection.Collection.cachekey`,
+        #: a unique key identifying the collection by its *config*,
+        #: which could be shared among multiple clients.
+        self.collections = dict()
+
+        #: clients is a cache mapping of hostname ->
+        #: :attr:`Bcfg2.Server.Plugins.Packages.Collection.Collection.cachekey`.
+        #: Unlike :attr:`collections`, this _is_ used to return a
+        #: :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
+        #: object when one is requested, so each entry is very
+        #: short-lived -- it's purged at the end of each client run.
+        self.clients = dict()
+
     __init__.__doc__ = Bcfg2.Server.Plugin.Plugin.__init__.__doc__
 
     def toggle_debug(self):
@@ -137,7 +176,7 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
 
         * All ``Package`` entries have their ``version`` and ``type``
           attributes set according to the appropriate
-          :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
+          :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
           object for this client.
         * ``Path`` entries are delegated to :func:`create_config`
 
@@ -208,7 +247,7 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
            complete package list.
 
         #. Calls
-           :func:`Bcfg2.Server.Plugins.Packages.Collection._Collection.build_extra_structures`
+           :func:`Bcfg2.Server.Plugins.Packages.Collection.Collection.build_extra_structures`
            to add any other extra data required by the backend (e.g.,
            GPG keys)
 
@@ -251,7 +290,7 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
         :param collection: The collection of sources for this client.
                            If none is given, one will be created with
                            :func:`_get_collection`
-        :type collection: Bcfg2.Server.Plugins.Packages.Collection._Collection
+        :type collection: Bcfg2.Server.Plugins.Packages.Collection.Collection
         """
         if self.disableResolver:
             # Config requests no resolver
@@ -335,16 +374,16 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
                              upstream repository.
         :type force_update: bool
         """
-        self.sentinels = set()
         cachefiles = set()
 
-        for collection in list(Collection.COLLECTIONS.values()):
+        for collection in list(self.collections.values()):
             cachefiles.update(collection.cachefiles)
             if not self.disableMetaData:
                 collection.setup_data(force_update)
-            self.sentinels.update(collection.basegroups)
 
-        Collection.clear_cache()
+        # clear Collection caches
+        self.clients = dict()
+        self.collections = dict()
 
         for source in self.sources:
             cachefiles.add(source.cachefile)
@@ -393,21 +432,61 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
 
     def _get_collection(self, metadata):
         """ Get a
-        :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
+        :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
         object for this client.
 
         :param metadata: The client metadata to get a Collection for
         :type metadata: Bcfg2.Server.Plugins.Metadata.ClientMetadata
-        :returns: Bcfg2.Server.Plugins.Packages.Collection._Collection
+        :returns: An instance of the appropriate subclass of
+                :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
+                that contains all relevant sources that apply to the
+                given client
         """
-        return Collection.Collection(metadata, self.sources, self.data,
-                                     debug=self.debug_flag)
+
+        if not self.sources.loaded:
+            # if sources.xml has not received a FAM event yet, defer;
+            # instantiate a dummy Collection object
+            return Collection(metadata, [], self.data)
+
+        if metadata.hostname in self.clients:
+            return self.collections[self.clients[metadata.hostname]]
+
+        sclasses = set()
+        relevant = list()
+
+        for source in self.sources:
+            if source.applies(metadata):
+                relevant.append(source)
+                sclasses.update([source.__class__])
+
+        if len(sclasses) > 1:
+            self.logger.warning("Packages: Multiple source types found for %s: "
+                                "%s" % ",".join([s.__name__ for s in sclasses]))
+            cclass = Collection
+        elif len(sclasses) == 0:
+            self.logger.error("Packages: No sources found for %s" %
+                              metadata.hostname)
+            cclass = Collection
+        else:
+            cclass = get_collection_class(
+                sclasses.pop().__name__.replace("Source", ""))
+
+        if self.debug_flag:
+            self.logger.error("Packages: Using %s for Collection of sources "
+                              "for %s" % (cclass.__name__, metadata.hostname))
+
+        collection = cclass(metadata, relevant, self.data,
+                            debug=self.debug_flag)
+        ckey = collection.cachekey
+        self.clients[metadata.hostname] = ckey
+        self.collections[ckey] = collection
+        return collection
 
     def get_additional_data(self, metadata):
         """ Return additional data for the given client.  This will be
         a dict containing a single key, ``sources``, whose value is a
         list of data returned from
-        :func:`Bcfg2.Server.Plugins.Packages.Collection._Collection.get_additional_data`,
+        :func:`Bcfg2.Server.Plugins.Packages.Collection.Collection.get_additional_data`,
         namely, a list of
         :attr:`Bcfg2.Server.Plugins.Packages.Source.Source.url_map`
         data.
@@ -420,21 +499,20 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
         return dict(sources=collection.get_additional_data())
 
     def end_client_run(self, metadata):
-        """ Hook to clear the cache for this client at
-        :attr:`Bcfg2.Server.Plugins.Packages.Collection.CLIENTS`,
-        which must persist only the duration of a client run.
+        """ Hook to clear the cache for this client in
+        :attr:`clients`, which must persist only the duration of a
+        client run.
 
         :param metadata: The client metadata
         :type metadata: Bcfg2.Server.Plugins.Metadata.ClientMetadata
         """
-        if metadata.hostname in Collection.CLIENTS:
-            del Collection.CLIENTS[metadata.hostname]
+        if metadata.hostname in self.clients:
+            del self.clients[metadata.hostname]
 
     def end_statistics(self, metadata):
-        """ Hook to clear the cache for this client at
-        :attr:`Bcfg2.Server.Plugins.Packages.Collection.CLIENTS` once
-        statistics are processed to ensure that a stray cached
-        :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
+        """ Hook to clear the cache for this client in :attr:`clients`
+        once statistics are processed to ensure that a stray cached
+        :class:`Bcfg2.Server.Plugins.Packages.Collection.Collection`
         object is not built during statistics and preserved until a
         subsequent client run.