launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05116
[Merge] lp:~jtv/launchpad/pre-717969 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/pre-717969 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #717969 in Launchpad itself: "storeBuildInfo is sometimes ineffective"
https://bugs.launchpad.net/launchpad/+bug/717969
For more details, see:
https://code.launchpad.net/~jtv/launchpad/pre-717969/+merge/77299
= Summary =
The twisted code in buildd-manager updates a build's state in one callback, but commits its changes in another. We believe this is probably a bug.
Before we change that, we want to make sure that we can limit and control the regions where the code makes changes to the model and the database. This will make it easier to reason about and maintain the code.
== Proposed fix ==
In this branch you'll find a context manager that brackets the region of code they control, and says whether database updates are allowed or not in that region.
== Pre-implementation notes ==
Discussed with Stuart, among others. There is a similar helper called read_transaction, but that serves a different purpose: it decorates a function that is designed to _be_ a transaction, and provides support for transaction retry (a very scary thing to throw into code that wasn't designed for it). It doesn't actually enforce or verify that the transaction is read-only, which is the essence of what we want here, and doesn't isolate the new transaction (although for supporting retries, it really ought to). Nor does it appear to support nesting or multiple transactions, which could be important for buildd-manager.
== Implementation details ==
Hopefully the code documentation will be self-explanatory. The plan is to wrap the entire buildd scanning loop in a single read-only policy, and exempt a few isolated places that need to change the model using nested read-write policy objects. At that point we can be reasonably sure that we understand all changes that the module makes to the model (even if hidden further down the call stack) and that all changes are committed before returning to the twisted reactor.
== Tests ==
{{{
./bin/test -vvc lp.services.database.tests.test_transaction_policy
}}}
== Demo and Q/A ==
Validate this by incorporating it into the buildd manager and running it in that form. The problem of incomplete commits under concurrent loads should go away. We have yet to work out the details of how to reproduce those.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/services/database/tests/test_transaction_policy.py
lib/lp/services/database/transaction_policy.py
--
https://code.launchpad.net/~jtv/launchpad/pre-717969/+merge/77299
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/pre-717969 into lp:launchpad.
=== added file 'lib/lp/services/database/tests/test_transaction_policy.py'
--- lib/lp/services/database/tests/test_transaction_policy.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/database/tests/test_transaction_policy.py 2011-09-28 10:20:33 +0000
@@ -0,0 +1,170 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test `TransactionPolicy`."""
+
+__metaclass__ = type
+
+from psycopg2 import InternalError
+import transaction
+
+from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.registry.model.person import Person
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
+from lp.testing import TestCaseWithFactory
+
+
+class TestTransactionPolicy(TestCaseWithFactory):
+ layer = ZopelessDatabaseLayer
+
+ def writeToDatabase(self):
+ """Write an object to the database.
+
+ :return: A token that `hasDatabaseBeenWrittenTo` can look for.
+ """
+ name = self.factory.getUniqueString()
+ self.factory.makePerson(name=name)
+ return name
+
+ def hasDatabaseBeenWrittenTo(self, test_token):
+ """Is the object made by `writeToDatabase` present in the database?
+
+ :param test_token: The return value from `writeToDatabase`.
+ :return: Has the change represented by `test_token` been made to the
+ database?
+ """
+ query = IStore(Person).find(Person, Person.name == test_token)
+ return query.one() is not None
+
+ def test_writable_permits_updates(self):
+ # Writes to the database work just fine in a non-read-only
+ # policy.
+ with DatabaseTransactionPolicy(read_only=False):
+ self.writeToDatabase()
+ # The test is that we get here without failure.
+ pass
+
+ def test_readonly_forbids_updates(self):
+ # A read-only policy forbids writes to the database.
+ def make_forbidden_update():
+ with DatabaseTransactionPolicy(read_only=True):
+ self.writeToDatabase()
+
+ self.assertRaises(InternalError, make_forbidden_update)
+
+ def test_commits_on_success(self):
+ # If the context handler exits normally (which would indicate
+ # successful completion of its contents), it commits.
+ with DatabaseTransactionPolicy(read_only=False):
+ test_token = self.writeToDatabase()
+ transaction.abort()
+ self.assertTrue(self.hasDatabaseBeenWrittenTo(test_token))
+
+ def test_aborts_on_failure(self):
+ # If the context handler exits with an exception, it aborts.
+ class CompleteFailure(Exception):
+ pass
+
+ try:
+ with DatabaseTransactionPolicy(read_only=False):
+ test_token = self.writeToDatabase()
+ raise CompleteFailure()
+ except CompleteFailure:
+ pass
+
+ self.assertFalse(self.hasDatabaseBeenWrittenTo(test_token))
+
+ def test_nested_policy_overrides_previous_policy(self):
+ # When one policy is nested in another, the nested one
+ # determines what is allowed.
+ def allows_updates(read_only=True):
+ """Does the given policy permit database updates?"""
+ try:
+ with DatabaseTransactionPolicy(read_only=read_only):
+ self.writeToDatabase()
+ return True
+ except InternalError:
+ return False
+
+ # Map (previous policy, nested policy) to whether writes to the
+ # database are allowed.
+ effects = {}
+
+ for previous_policy in [False, True]:
+ for nested_policy in [False, True]:
+ experiment = (previous_policy, nested_policy)
+ with DatabaseTransactionPolicy(read_only=previous_policy):
+ effects[experiment] = allows_updates(nested_policy)
+
+ self.assertEqual({
+ (False, False): True,
+ (False, True): False,
+ (True, False): True,
+ (True, True): False,
+ },
+ effects)
+
+ def test_policy_restores_previous_policy_on_success(self):
+ # A transaction policy, once exited, restores the previously
+ # applicable policy.
+ with DatabaseTransactionPolicy(read_only=False):
+ with DatabaseTransactionPolicy(read_only=True):
+ pass
+ self.assertTrue(
+ self.hasDatabaseBeenWrittenTo(self.writeToDatabase()))
+ self.assertTrue(
+ self.hasDatabaseBeenWrittenTo(self.writeToDatabase()))
+
+ def test_propagates_failure(self):
+ # Exceptions raised inside a transaction policy are not
+ # swallowed.
+ class Kaboom(Exception):
+ pass
+
+ def fail_within_policy():
+ with DatabaseTransactionPolicy():
+ raise Kaboom()
+
+ self.assertRaises(Kaboom, fail_within_policy)
+
+ def test_policy_restores_previous_policy_on_failure(self):
+ # A transaction policy restores the previously applicable policy
+ # even when it exits abnormally.
+ class HorribleFailure(Exception):
+ pass
+
+ try:
+ with DatabaseTransactionPolicy(read_only=True):
+ raise HorribleFailure()
+ except HorribleFailure:
+ pass
+
+ self.assertTrue(
+ self.hasDatabaseBeenWrittenTo(self.writeToDatabase()))
+
+ def test_policy_can_span_transactions(self):
+ # It's okay to commit within a policy; the policy will still
+ # apply to the next transaction inside the same policy.
+ def write_and_commit():
+ self.writeToDatabase()
+ transaction.commit()
+
+ test_token = self.writeToDatabase()
+ with DatabaseTransactionPolicy(read_only=True):
+ self.hasDatabaseBeenWrittenTo(test_token)
+ transaction.commit()
+ self.assertRaises(InternalError, write_and_commit)
+ transaction.abort()
+
+ def test_policy_survives_abort(self):
+ # Even after aborting the initial transaction, a transaction
+ # policy still applies.
+ def write_and_commit():
+ self.writeToDatabase()
+ transaction.commit()
+
+ with DatabaseTransactionPolicy(read_only=True):
+ transaction.abort()
+ self.assertRaises(InternalError, write_and_commit)
+ transaction.abort()
=== added file 'lib/lp/services/database/transaction_policy.py'
--- lib/lp/services/database/transaction_policy.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/database/transaction_policy.py 2011-09-28 10:20:33 +0000
@@ -0,0 +1,135 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Policy for database transactions."""
+
+__metaclass__ = type
+__all__ = [
+ 'DatabaseTransactionPolicy',
+ ]
+
+import transaction
+from zope.component import getUtility
+
+from canonical.database.sqlbase import quote
+from canonical.launchpad.webapp.interfaces import (
+ IStoreSelector,
+ MAIN_STORE,
+ MASTER_FLAVOR,
+ )
+
+
+class DatabaseTransactionPolicy:
+ """Context manager for read-only transaction policy.
+
+ Use this to define regions of code that explicitly allow or disallow
+ changes to the database:
+
+ # We want to be sure that inspect_data does not inadvertently
+ # make any changes in the database, but we can't run it on the
+ # slave store because it doesn't tolerate replication lag.
+ with DatabaseTransactionPolicy(read_only=True):
+ inspect_data()
+
+ The simplest way to use this is as a special transaction:
+ * Entering a policy commits the ongoing transaction.
+ * Exiting a policy normally commits its changes.
+ * Exiting by exception aborts its changes.
+
+ You can also have multiple transactions inside one policy, however; the
+ policy still applies after a commit or abort.
+
+ Policies can be nested--a nested policy overrides the one it's nested in.
+ After the nested policy has exited, the previous policy applies again:
+
+ # This code needs to control the database changes it makes very
+ # carefully. Most of it is just gathering data, with one quick
+ # database update at the end.
+ with DatabaseTransactionPolicy(read_only=True):
+ data = gather_data()
+ more_data = figure_stuff_out(data)
+
+ # This is the only part where we update the database!
+ with DatabaseTransactionPolicy(read_only=False):
+ update_model(data, more_data)
+
+ write_logs(data)
+ notify_user(more_data)
+ """
+
+ db_switch = "DEFAULT_TRANSACTION_READ_ONLY"
+
+ def __init__(self, store=None, read_only=False):
+ """Create a policy.
+
+ Merely creating a policy has no effect. Use it with "with" to affect
+ writability of database transactions.
+
+ :param store: The store to set policy on. Defaults to the main master
+ store. You don't want to use this on a slave store!
+ :param read_only: Allow database changes for the duration of this
+ policy?
+ """
+ self.policy = read_only
+ if store is None:
+ self.store = getUtility(IStoreSelector).get(
+ MAIN_STORE, MASTER_FLAVOR)
+ else:
+ self.store = store
+
+ def __enter__(self):
+ """Enter this policy.
+
+ Commits the ongoing transaction, and sets the selected default
+ read-only policy on the database.
+ """
+ self.previous_policy = self._getCurrentPolicy()
+ self._setPolicy(self.policy)
+ # Commit should include the policy itself. If this breaks
+ # because the transaction was already in a failed state before
+ # we got here, too bad.
+ transaction.commit()
+
+ def __exit__(self, exc_type, *args):
+ """Exit this policy.
+
+ Commits or aborts, depending on mode of exit, and restores the
+ previous default read-only policy.
+ """
+ if exc_type is None:
+ # Exiting normally.
+ transaction.commit()
+ else:
+ # Exiting with an exception.
+ transaction.abort()
+
+ self._setPolicy(self.previous_policy)
+ transaction.commit()
+
+ # Continue processing the exception as normal.
+ return False
+
+ def _getCurrentPolicy(self):
+ """Read the database session's default transaction read-only policy.
+
+ The information is retrieved from the database, so this will give a
+ sensible answer even when no DatabaseTransactionPolicy is in effect.
+
+ :return: True for read-only policy, False for read-write policy.
+ """
+ db_switch_value_to_policy = {
+ 'on': True,
+ 'off': False,
+ }
+ show_command = "SHOW %s" % self.db_switch
+ db_switch_value, = self.store.execute(show_command).get_one()
+ return db_switch_value_to_policy[db_switch_value]
+
+ def _setPolicy(self, read_only=True):
+ """Set the database session's default transaction read-only policy.
+
+ :param read_only: True for read-only policy, False for read-write
+ policy.
+ """
+ self.store.execute(
+ "SET %s TO %s" % (self.db_switch, quote(read_only)))
