launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04354
[Merge] lp:~jtv/launchpad/bug-815725 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-815725 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #815725 in Launchpad itself: "Long-running transaction in generate-contents-files"
https://bugs.launchpad.net/launchpad/+bug/815725
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-815725/+merge/69056
= Summary =
When the new, python-based generate-contents-files executes apt-ftparchive, that takes a long time and this led to its database connection being reaped in the meantime.
== Proposed fix ==
Make the script run entirely on the slave store (to ensure that we're not forgetting about any database changes the script might otherwise make). Commit just before the long-running part, and to ensure that the transaction manager doesn't quietly start a new transaction, execute the long-running part in a context manager that prevents any database access at all.
== Pre-implementation notes ==
This will also allow the script to run against a slave database. It may not do much good but we don't expect any harm either.
I had to change PublisherConfigSet.getByDistribution to use the policy-dicated store rather than hard-coding the master store. Julian sees no problem with this.
== Implementation details ==
Some methods now take parameters that they could easily take from the script object's state. But note that the parameters are not database-backed values: evaluating something like "distribution.name" might cause the ORM to check the database object underneath "distribution" for changes. We don't want that: it would open a new transaction.
There is no separate set of tests to ensure that the database access really is read-only. However the test does contain a full script run that should just about exercise all cases, and fail if anything touches the master store. In fact it did fail initially because PublisherConfigSet.getByDistribution misguidedly attempted to access the master store.
While I was here I also updated some tests to stop them from creating directories in the source tree as a side effect. Replacing "self.factory.makeDistribution()" with "self.makeDistro()" took care of most cases.
== Tests ==
{{{
./bin/test -vvc lp.archivepublisher.tests.test_generate_contents_files
}}}
== Demo and Q/A ==
Run generate-contents-files.py on a test server. See that for the bulk of its run time, its database connection is "idle" rather than "idle in transaction."
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/archivepublisher/model/publisherconfig.py
lib/lp/archivepublisher/tests/test_generate_contents_files.py
lib/lp/archivepublisher/scripts/generate_contents_files.py
--
https://code.launchpad.net/~jtv/launchpad/bug-815725/+merge/69056
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-815725 into lp:launchpad.
=== modified file 'lib/lp/archivepublisher/model/publisherconfig.py'
--- lib/lp/archivepublisher/model/publisherconfig.py 2011-03-08 12:51:02 +0000
+++ lib/lp/archivepublisher/model/publisherconfig.py 2011-07-25 09:57:00 +0000
@@ -20,6 +20,7 @@
from canonical.launchpad.interfaces.lpstorm import (
IMasterStore,
+ IStore,
)
from lp.archivepublisher.interfaces.publisherconfig import (
IPublisherConfig,
@@ -62,7 +63,6 @@
def getByDistribution(self, distribution):
"""See `IArchiveAuthTokenSet`."""
- store = IMasterStore(PublisherConfig)
- return store.find(
+ return IStore(PublisherConfig).find(
PublisherConfig,
PublisherConfig.distribution_id == distribution.id).one()
=== modified file 'lib/lp/archivepublisher/scripts/generate_contents_files.py'
--- lib/lp/archivepublisher/scripts/generate_contents_files.py 2011-07-12 16:50:53 +0000
+++ lib/lp/archivepublisher/scripts/generate_contents_files.py 2011-07-25 09:57:00 +0000
@@ -10,9 +10,14 @@
from optparse import OptionValueError
import os
+
from zope.component import getUtility
from canonical.config import config
+from canonical.launchpad.webapp.dbpolicy import (
+ DatabaseBlockedPolicy,
+ SlaveOnlyDatabasePolicy,
+ )
from lp.archivepublisher.config import getPubConfig
from lp.registry.interfaces.distribution import IDistributionSet
from lp.registry.interfaces.pocket import pocketsuffix
@@ -218,46 +223,66 @@
self.logger.debug("Creating %s.", path)
os.makedirs(path)
- def copyOverrides(self):
- """Copy overrides into the content archive."""
- if file_exists(self.config.overrideroot):
+ def copyOverrides(self, override_root):
+ """Copy overrides into the content archive.
+
+ This method won't access the database.
+ """
+ if file_exists(override_root):
execute(self.logger, "cp", [
"-a",
- self.config.overrideroot,
+ override_root,
"%s/" % self.content_archive,
])
else:
self.logger.debug("Did not find overrides; not copying.")
- def writeContentsTop(self):
- """Write Contents.top file."""
+ def writeContentsTop(self, distro_name, distro_title):
+ """Write Contents.top file.
+
+ This method won't access the database.
+ """
output_filename = os.path.join(
- self.content_archive, '%s-misc' % self.distribution.name,
- "Contents.top")
+ self.content_archive, '%s-misc' % distro_name, "Contents.top")
parameters = {
- 'distrotitle': self.distribution.title,
+ 'distrotitle': distro_title,
}
output_file = file(output_filename, 'w')
text = file(get_template("Contents.top")).read() % parameters
output_file.write(text)
output_file.close()
- def runAptFTPArchive(self):
- """Run apt-ftparchive to produce the Contents files."""
+ def runAptFTPArchive(self, distro_name):
+ """Run apt-ftparchive to produce the Contents files.
+
+ This method may take a long time to run.
+ This method won't access the database.
+ """
execute(self.logger, "apt-ftparchive", [
"generate",
os.path.join(
- self.content_archive, "%s-misc" % self.distribution.name,
+ self.content_archive, "%s-misc" % distro_name,
"apt-contents.conf"),
])
- def generateContentsFiles(self):
- """Generate Contents files."""
+ def generateContentsFiles(self, override_root, distro_name, distro_title):
+ """Generate Contents files.
+
+ This method may take a long time to run.
+ This method won't access the database.
+
+ :param override_root: Copy of `self.config.overrideroot` that can be
+ evaluated without accessing the database.
+ :param distro_name: Copy of `self.distribution.name` that can be
+ evaluated without accessing the database.
+ :param distro_title: Copy of `self.distribution.title` that can be
+ evaluated without accessing the database.
+ """
self.logger.debug(
"Running apt in private tree to generate new contents.")
- self.copyOverrides()
- self.writeContentsTop()
- self.runAptFTPArchive()
+ self.copyOverrides(override_root)
+ self.writeContentsTop(distro_name, distro_title)
+ self.runAptFTPArchive(distro_name)
def updateContentsFile(self, suite, arch):
"""Update Contents file, if it has changed."""
@@ -340,12 +365,29 @@
self.updateLegacyContentArchiveRoot()
self.setUpContentArchive()
- def main(self):
- """See `LaunchpadScript`."""
+ def process(self):
+ """Do the bulk of the work."""
self.setUp()
suites = self.getPockets()
archs = self.getArchs()
self.writeAptContentsConf(suites, archs)
self.createComponentDirs(suites, archs)
- self.generateContentsFiles()
+
+ overrideroot = self.config.overrideroot
+ distro_name = self.distribution.name
+ distro_title = self.distribution.title
+
+ # This takes a while. Ensure that we do it without keeping a
+ # database transaction open.
+ self.txn.commit()
+ with DatabaseBlockedPolicy():
+ self.generateContentsFiles(
+ overrideroot, distro_name, distro_title)
+
self.updateContentsFiles(suites, archs)
+
+ def main(self):
+ """See `LaunchpadScript`."""
+ # This code has no need to alter the database.
+ with SlaveOnlyDatabasePolicy():
+ self.process()
=== modified file 'lib/lp/archivepublisher/tests/test_generate_contents_files.py'
--- lib/lp/archivepublisher/tests/test_generate_contents_files.py 2011-07-13 16:33:31 +0000
+++ lib/lp/archivepublisher/tests/test_generate_contents_files.py 2011-07-25 09:57:00 +0000
@@ -7,8 +7,9 @@
from optparse import OptionValueError
import os
+from textwrap import dedent
+
from testtools.matchers import StartsWith
-from textwrap import dedent
from canonical.config import config
from canonical.testing.layers import (
@@ -26,6 +27,7 @@
from lp.services.scripts.base import LaunchpadScriptFailure
from lp.services.utils import file_exists
from lp.testing import TestCaseWithFactory
+from lp.testing.faketransaction import FakeTransaction
def write_file(filename, content=""):
@@ -153,6 +155,7 @@
distribution = self.makeDistro()
script = GenerateContentsFiles(test_args=['-d', distribution.name])
script.logger = DevNullLogger()
+ script.txn = FakeTransaction()
if run_setup:
script.setUp()
else:
@@ -206,20 +209,23 @@
def test_looks_up_distro(self):
# The script looks up and keeps the distribution named on the
# command line.
- distro = self.factory.makeDistribution()
+ distro = self.makeDistro()
script = self.makeScript(distro)
self.assertEqual(distro, script.distribution)
def test_queryDistro(self):
# queryDistro is a helper that invokes LpQueryDistro.
- distroseries = self.factory.makeDistroSeries()
- script = self.makeScript(distroseries.distribution)
+ distro = self.makeDistro()
+ distroseries = self.factory.makeDistroSeries(distro)
+ script = self.makeScript(distro)
script.processOptions()
self.assertEqual(distroseries.name, script.queryDistro('supported'))
def test_getArchs(self):
# getArchs returns a list of architectures in the distribution.
- das = self.factory.makeDistroArchSeries()
+ distro = self.makeDistro()
+ distroseries = self.factory.makeDistroSeries(distro)
+ das = self.factory.makeDistroArchSeries(distroseries=distroseries)
script = self.makeScript(das.distroseries.distribution)
self.assertEqual([das.architecturetag], script.getArchs())
@@ -241,7 +247,7 @@
os.makedirs(os.path.join(script.config.distsroot, package.suite))
self.assertEqual([package.suite], script.getPockets())
- def test_getPocket_includes_release_pocket(self):
+ def test_getPockets_includes_release_pocket(self):
# getPockets also includes the release pocket, which is named
# after the distroseries without a suffix.
distro = self.makeDistro()
@@ -290,7 +296,7 @@
distro = self.makeDistro()
script = self.makeScript(distro)
content_archive = script.content_archive
- script.writeContentsTop()
+ script.writeContentsTop(distro.name, distro.title)
contents_top = file(
"%s/%s-misc/Contents.top" % (content_archive, distro.name)).read()
@@ -375,7 +381,7 @@
os.makedirs(os.path.join(script.config.distsroot, package.suite))
self.assertNotEqual([], script.getPockets())
fake_overrides(script, distroseries)
- script.main()
+ script.process()
self.assertTrue(file_exists(os.path.join(
script.config.distsroot, suite,
"Contents-%s.gz" % das.architecturetag)))
